bluetooth: android: Fix a couple of crashes when adapter is turned on/off.

bluetooth: android: Fix a couple of crashes when adapter is turned on/off.

These changes fixes the crashes found when toggling the adapter on/off
when device chooser dialog is used in web-bluetooth.

BUG=570610
Committed: https://crrev.com/28e8a498f4348d26ddcc07a341591b0183c4bdd6
Cr-Commit-Position: refs/heads/master@{#374145}

[perja, 2016-01-20 14:05:27]

Description was changed from

==========
bluetooth: android: Fix a couple of crashes when adapter is turned on/off.

These changes fixes the crashes found when toggling the adapter on/off
when device chooser dialog is used in web-bluetooth.

BUG=570610
==========

to

==========
bluetooth: android: Fix a couple of crashes when adapter is turned on/off.

These changes fixes the crashes found when toggling the adapter on/off
when device chooser dialog is used in web-bluetooth.

BUG=570610
==========

[perja, 2016-01-20 14:05:27]

perja@opera.com changed reviewers:
+ scheib@chromium.org

[perja, 2016-01-20 14:22:52]

perja@opera.com changed reviewers:
+ ortuno@chromium.org

[perja, 2016-01-20 14:22:53]

@scheib: please take a look and see if it is ok. I incorporated some of your
changes.

There are no unit tests and the reason for that is that I can't see an easy way
to add those for these crashers (see
https://code.google.com/p/chromium/issues/detail?id=570610#c12).

[scheib, 2016-01-21 01:47:40]

The CQ bit was checked by scheib@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-21 01:49:04]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1610053005/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1610053005/1

[commit-bot: I haz the power, 2016-01-21 02:12:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-01-21 02:12:13]

Dry run: This issue passed the CQ dry run.

[scheib, 2016-01-21 05:33:51]

Thanks for taking this on.

https://codereview.chromium.org/1610053005/diff/1/device/bluetooth/android/ja...
File
device/bluetooth/android/java/src/org/chromium/device/bluetooth/ChromeBluetoothAdapter.java
(right):

https://codereview.chromium.org/1610053005/diff/1/device/bluetooth/android/ja...
device/bluetooth/android/java/src/org/chromium/device/bluetooth/ChromeBluetoothAdapter.java:185:
Wrappers.BluetoothLeScannerWrapper scanner = mAdapter.getBluetoothLeScanner();
scanner may be null.

https://codereview.chromium.org/1610053005/diff/1/device/bluetooth/android/ja...
device/bluetooth/android/java/src/org/chromium/device/bluetooth/ChromeBluetoothAdapter.java:203:
} catch (IllegalStateException e) {
Add a test:
- In Fakes.java have FakeBluetoothLeScanner's startScan() check a member
variable that indicates if an exception should be thrown. 
- Set that member variable from some new @CalledByNative method you add to
FakeBluetoothAdapter. 
- Call that method from the test fixture BluetoothTestAndroid, where you also
add a method so that tests can set this behavior.
- Different than existing patterns: because this is very Android implementation
specific, I think only add the method to BluetoothTestAndroid, not to
BluetoothTestBase.
- Add a test to bluetooth_adapter_unittest.cc in the general proximity of
'TEST_F(BluetoothTest, DiscoverySession)'. Comment is clearly as an Android
specific test.

https://codereview.chromium.org/1610053005/diff/1/device/bluetooth/android/ja...
File
device/bluetooth/android/java/src/org/chromium/device/bluetooth/Wrappers.java
(right):

https://codereview.chromium.org/1610053005/diff/1/device/bluetooth/android/ja...
device/bluetooth/android/java/src/org/chromium/device/bluetooth/Wrappers.java:99:
BluetoothLeScanner scanner = adapter.getBluetoothLeScanner();
In my patch you'll see that I moved the code for obtaining the scanner to the
point where it is attempted to be used. I did this because there was a scenario
where it is null the first time - I hadn't carefully tracked it down, but it
would seem logical to be:
- Close all browser activities (recents, swipe off all)
- Turn off Bluetooth.
- Launch browser and initiate Bluetooth scan.
- Enable Bluetooth.
- Return to web page and retry scan.

Similar to how I suggest adding a test for scan exceptions, please add a test
for the adapter being present but powered off before the first scan, then
powering on, then scanning. In this case I think it is reasonable for all
platforms, so please add methods all the way to BluetoothTestBase.

Also a test for being powered on, then off, then scanning.

Fake adapter's 'getBluetoothLeScanner' should return null based on the adapter
powered state.

https://codereview.chromium.org/1610053005/diff/1/device/bluetooth/android/ja...
device/bluetooth/android/java/src/org/chromium/device/bluetooth/Wrappers.java:285:
if (mDevice == null || mDevice.getBluetoothClass() == null) {
OK, no test needed here because we don't Fake getBluetoothClass and always
return unspecified anyway.

[perja, 2016-01-21 15:33:06]

@scheib: Thanks for the detailed feedback. If it is ok with you I would like to
add SetPowered, OnAdapterChanged and the related unit tests in another review
(for https://code.google.com/p/chromium/issues/detail?id=543060). For this
review I will make a new patch set that fixes the other issues you mentioned
including the Android specific method to force an exception to be thrown.

[scheib, 2016-01-21 16:05:01]

On 2016/01/21 15:33:06, perja wrote:
> @scheib: Thanks for the detailed feedback. If it is ok with you I would like
to
> add SetPowered, OnAdapterChanged and the related unit tests in another review
> (for https://code.google.com/p/chromium/issues/detail?id=543060). For this
> review I will make a new patch set that fixes the other issues you mentioned
> including the Android specific method to force an exception to be thrown.

Generally this isn't done in chromium -- often we split out patches so that
they're easier to review, but the 'deferred' work is visible in a dependent
patch. Deferring testing isn't typically done. I think this patch is small
enough that it should contain the associated tests. Or, it could come after
another patch that introduces the testing fixture bits.  

OnAdapterChanged does seem like something for another patch unless I'm
overlooking something.

[perja, 2016-02-04 14:50:16]

Patchset #5 (id:80001) has been deleted

[perja, 2016-02-05 07:52:31]

Fixed the issue that caused assertion error on one of the trybots.

Please take a look again.

[scheib, 2016-02-05 18:06:19]

Glad to see it sorted out! LGTM with some small changes:

https://codereview.chromium.org/1610053005/diff/100001/device/bluetooth/andro...
File
device/bluetooth/android/java/src/org/chromium/device/bluetooth/Wrappers.java
(right):

https://codereview.chromium.org/1610053005/diff/100001/device/bluetooth/andro...
device/bluetooth/android/java/src/org/chromium/device/bluetooth/Wrappers.java:143:
public boolean setPowered(boolean powered) {
The wrapper should mimic the Android API as closely as possible, such that one
could replace the wrapper instance in the code that uses it with the system
Adapter type and have it compile and work. Please expose wrapper methods for
enable/disable instead of 'setPowered'.

https://codereview.chromium.org/1610053005/diff/100001/device/bluetooth/andro...
device/bluetooth/android/java/src/org/chromium/device/bluetooth/Wrappers.java:188:
mScanner.startScan(filters, settings, callbackForwarder);
Leave the blank line.

[perja, 2016-02-08 14:48:31]

@scheib: fixed the two remaining issues you mentioned. And also rebased.

https://codereview.chromium.org/1610053005/diff/100001/device/bluetooth/andro...
File
device/bluetooth/android/java/src/org/chromium/device/bluetooth/Wrappers.java
(right):

https://codereview.chromium.org/1610053005/diff/100001/device/bluetooth/andro...
device/bluetooth/android/java/src/org/chromium/device/bluetooth/Wrappers.java:143:
public boolean setPowered(boolean powered) {
On 2016/02/05 18:06:19, scheib wrote:
> The wrapper should mimic the Android API as closely as possible, such that one
> could replace the wrapper instance in the code that uses it with the system
> Adapter type and have it compile and work. Please expose wrapper methods for
> enable/disable instead of 'setPowered'. 

Done.

https://codereview.chromium.org/1610053005/diff/100001/device/bluetooth/andro...
device/bluetooth/android/java/src/org/chromium/device/bluetooth/Wrappers.java:188:
mScanner.startScan(filters, settings, callbackForwarder);
On 2016/02/05 18:06:19, scheib wrote:
> Leave the blank line.

Done.

[scheib, 2016-02-08 18:27:50]

The CQ bit was checked by scheib@chromium.org

[scheib, 2016-02-08 18:27:51]

The patchset sent to the CQ was uploaded after l-g-t-m from scheib@chromium.org
Link to the patchset: https://codereview.chromium.org/1610053005/#ps140001
(title: "Rebased on latest master")

[commit-bot: I haz the power, 2016-02-08 18:28:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1610053005/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1610053005/140001

[commit-bot: I haz the power, 2016-02-08 18:39:25]

Description was changed from

==========
bluetooth: android: Fix a couple of crashes when adapter is turned on/off.

These changes fixes the crashes found when toggling the adapter on/off
when device chooser dialog is used in web-bluetooth.

BUG=570610
==========

to

==========
bluetooth: android: Fix a couple of crashes when adapter is turned on/off.

These changes fixes the crashes found when toggling the adapter on/off
when device chooser dialog is used in web-bluetooth.

BUG=570610
==========

[commit-bot: I haz the power, 2016-02-08 18:39:26]

Committed patchset #7 (id:140001)

[commit-bot: I haz the power, 2016-02-08 18:40:42]

Description was changed from

==========
bluetooth: android: Fix a couple of crashes when adapter is turned on/off.

These changes fixes the crashes found when toggling the adapter on/off
when device chooser dialog is used in web-bluetooth.

BUG=570610
==========

to

==========
bluetooth: android: Fix a couple of crashes when adapter is turned on/off.

These changes fixes the crashes found when toggling the adapter on/off
when device chooser dialog is used in web-bluetooth.

BUG=570610

Committed: https://crrev.com/28e8a498f4348d26ddcc07a341591b0183c4bdd6
Cr-Commit-Position: refs/heads/master@{#374145}
==========

[commit-bot: I haz the power, 2016-02-08 18:40:43]

Patchset 7 (id:??) landed as
https://crrev.com/28e8a498f4348d26ddcc07a341591b0183c4bdd6
Cr-Commit-Position: refs/heads/master@{#374145}