Remove outdated devices from Android device chooser

On Android there is no notification when a device is lost. This change keeps track of when a device was last seen and removes outdated devices.

This was already implemented for OSX and the code has been moved to make it accessible to Android as well.

BUG=581544
Committed: https://crrev.com/849f95f6082cd71789e966de3b5a31ba87d1084e
Cr-Commit-Position: refs/heads/master@{#401940}

[perja, 2016-04-05 13:12:12]

Patchset #6 (id:100001) has been deleted

[perja, 2016-04-05 13:14:47]

Patchset #6 (id:120001) has been deleted

[perja, 2016-04-06 14:46:25]

Description was changed from

==========
Re-populate the device chooser with known devices.

Always add the known devices from the adapter to the device chooser so that the
list is up-to-date.

BUG=581544
==========

to

==========
On Android there is no notification when a device is lost. This change keeps
track of when a device was last seen and removes outdated devices. 

This was already implemented for OSX and the code has been moved to make it
accessible to Android as well.

BUG=581544
==========

[perja, 2016-04-06 14:52:06]

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

[perja, 2016-04-06 14:52:07]

@scheib:
Here is my suggestion for how to solve the problem with outdated devices in the
device chooser on Android. 

NOTE: there are some changes to the OSX code here as well, but I don't have a
Mac to test it on (but unit tests passes).

[scheib, 2016-04-07 22:05:02]

https://codereview.chromium.org/1842223003/diff/160001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java
(right):

https://codereview.chromium.org/1842223003/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:395:
@VisibleForTesting
Is this needed for this patch?

https://codereview.chromium.org/1842223003/diff/160001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right):

https://codereview.chromium.org/1842223003/diff/160001/content/browser/blueto...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:1708: #if
defined(OS_ANDROID)
I'm concerned about this being platform specific.

https://codereview.chromium.org/1842223003/diff/160001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter.cc (right):

https://codereview.chromium.org/1842223003/diff/160001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter.cc:398: base::TimeDelta::FromSeconds(30);
Why 30? Mac was using 180.

https://codereview.chromium.org/1842223003/diff/160001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter.h (right):

https://codereview.chromium.org/1842223003/diff/160001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter.h:430: void RemoveTimedOutDevices();
This seems odd to me to be called externally. Instead, should a recurring posted
task be run inside the implementation? Similar to how Mac was?

[perja, 2016-04-08 07:47:35]

https://codereview.chromium.org/1842223003/diff/160001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java
(right):

https://codereview.chromium.org/1842223003/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:395:
@VisibleForTesting
On 2016/04/07 22:05:02, scheib wrote:
> Is this needed for this patch? 

I'm not 100% sure about this one. Seems that only some of trybots fails for a
junit test if I don't have this - could be that either javac or proguard removes
the method since it finds that it isn't used. I will investigate.

https://codereview.chromium.org/1842223003/diff/160001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter.cc (right):

https://codereview.chromium.org/1842223003/diff/160001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter.cc:398: base::TimeDelta::FromSeconds(30);
On 2016/04/07 22:05:02, scheib wrote:
> Why 30? Mac was using 180.

Only reason for this was that we thought 180 seconds was a too long timeout when
you turn off a device or if it is out of range. But maybe it isn't a use case we
should consider too much (I tested with 3 Mio Playbulbs - turning them on/off).
After all the purpose of the cache is to make it faster to populate the device
chooser. What do you think?

https://codereview.chromium.org/1842223003/diff/160001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter.h (right):

https://codereview.chromium.org/1842223003/diff/160001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter.h:430: void RemoveTimedOutDevices();
On 2016/04/07 22:05:02, scheib wrote:
> This seems odd to me to be called externally. Instead, should a recurring
posted
> task be run inside the implementation? Similar to how Mac was?

Yeah, we could do that instead I guess. The recurring task seemed a bit overkill
for this since we only need to purge the cache before the device chooser is
populated, but maybe it isn't that bad. I will look at it again.

[scheib, 2016-04-12 05:01:57]

https://codereview.chromium.org/1842223003/diff/160001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter.h (right):

https://codereview.chromium.org/1842223003/diff/160001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter.h:430: void RemoveTimedOutDevices();
On 2016/04/08 07:47:35, perja wrote:
> On 2016/04/07 22:05:02, scheib wrote:
> > This seems odd to me to be called externally. Instead, should a recurring
> posted
> > task be run inside the implementation? Similar to how Mac was?
> 
> Yeah, we could do that instead I guess. The recurring task seemed a bit
overkill
> for this since we only need to purge the cache before the device chooser is
> populated, but maybe it isn't that bad. I will look at it again.

Tricky. Because it doesn't affect only the cache. It also sends notifications
that the device is removed. So, e.g. code that responds to devices being removed
would behave differently if other code calls RemoveTimedOutDevices explicitly or
not.

Also, because on Mac there's a timer, but clearly we want only one timer for all
timer based operations such as this. So perhaps it simply becomes each platform
implementation's job to setup a timer if necessary for that platform and to call
'PoolAdapter' or equiv.

[perja, 2016-05-03 09:23:42]

@scheib:please take a look again. 

It is using a timer to purge the devices now. Also I changed the timeout back to
180. That seems to be the default for Bluez as well.

[scheib, 2016-05-06 00:41:50]

Sorry I've been taking a while on this. It's a problem I'm unsure if we are
solving in the best way.

I know that I suggested using a repeating timer. That was probably a poor
suggestion, which occurs to me more strongly when seeing the "every 5 seconds
poll all devices" logic in android (every 5 sec seems too often if we do end up
as this as the only option).

I need to research this more, but I see that the logic you're using is !paired
!connected. We have code now in Android that is notified when the connected
state changes. We could add code to detect pairing state changing with
ACTION_BOND_STATE_CHANGED. I think that would enable us to purge devices only at
the time of a change. 

Mac's .5 second polling interval is a little insane to me -- I didn't think it
was that frequent, and looking at it now I'm thinking "wwaaaaaa!?".

https://codereview.chromium.org/1842223003/diff/220001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right):

https://codereview.chromium.org/1842223003/diff/220001/content/browser/blueto...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:1014: // Populate the
initial list of devices.
This comment not needed now that the code block is in a well named method.

https://codereview.chromium.org/1842223003/diff/220001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter.cc (right):

https://codereview.chromium.org/1842223003/diff/220001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter.cc:383: removed_devices.insert(it->first);
This line is duplicated above.

[ortuno, 2016-05-06 16:09:53]

On 2016/05/06 at 00:41:50, scheib wrote:
> Sorry I've been taking a while on this. It's a problem I'm unsure if we are
solving in the best way.
> 
> I know that I suggested using a repeating timer. That was probably a poor
suggestion, which occurs to me more strongly when seeing the "every 5 seconds
poll all devices" logic in android (every 5 sec seems too often if we do end up
as this as the only option).
> 
> I need to research this more, but I see that the logic you're using is !paired
!connected. We have code now in Android that is notified when the connected
state changes. We could add code to detect pairing state changing with
ACTION_BOND_STATE_CHANGED. I think that would enable us to purge devices only at
the time of a change. 

That only takes care of devices that we've connected to. We also want to be able
to clean up devices that we've seen while scanning but are no longer there.

> 
> Mac's .5 second polling interval is a little insane to me -- I didn't think it
was that frequent, and looking at it now I'm thinking "wwaaaaaa!?".
> 
>
https://codereview.chromium.org/1842223003/diff/220001/content/browser/blueto...
> File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right):
> 
>
https://codereview.chromium.org/1842223003/diff/220001/content/browser/blueto...
> content/browser/bluetooth/bluetooth_dispatcher_host.cc:1014: // Populate the
initial list of devices.
> This comment not needed now that the code block is in a well named method.
> 
>
https://codereview.chromium.org/1842223003/diff/220001/device/bluetooth/bluet...
> File device/bluetooth/bluetooth_adapter.cc (right):
> 
>
https://codereview.chromium.org/1842223003/diff/220001/device/bluetooth/bluet...
> device/bluetooth/bluetooth_adapter.cc:383: removed_devices.insert(it->first);
> This line is duplicated above.

[ortuno, 2016-05-06 16:15:11]

On 2016/05/06 at 16:09:53, ortuno wrote:
> On 2016/05/06 at 00:41:50, scheib wrote:
> > Sorry I've been taking a while on this. It's a problem I'm unsure if we are
solving in the best way.
> > 
> > I know that I suggested using a repeating timer. That was probably a poor
suggestion, which occurs to me more strongly when seeing the "every 5 seconds
poll all devices" logic in android (every 5 sec seems too often if we do end up
as this as the only option).
> > 
> > I need to research this more, but I see that the logic you're using is
!paired !connected. We have code now in Android that is notified when the
connected state changes. We could add code to detect pairing state changing with
ACTION_BOND_STATE_CHANGED. I think that would enable us to purge devices only at
the time of a change. 
> 
> That only takes care of devices that we've connected to. We also want to be
able to clean up devices that we've seen while scanning but are no longer there.
> 
> > 
> > Mac's .5 second polling interval is a little insane to me -- I didn't think
it was that frequent, and looking at it now I'm thinking "wwaaaaaa!?".
> > 
> >
https://codereview.chromium.org/1842223003/diff/220001/content/browser/blueto...
> > File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right):
> > 
> >
https://codereview.chromium.org/1842223003/diff/220001/content/browser/blueto...
> > content/browser/bluetooth/bluetooth_dispatcher_host.cc:1014: // Populate the
initial list of devices.
> > This comment not needed now that the code block is in a well named method.
> > 
> >
https://codereview.chromium.org/1842223003/diff/220001/device/bluetooth/bluet...
> > File device/bluetooth/bluetooth_adapter.cc (right):
> > 
> >
https://codereview.chromium.org/1842223003/diff/220001/device/bluetooth/bluet...
> > device/bluetooth/bluetooth_adapter.cc:383:
removed_devices.insert(it->first);
> > This line is duplicated above.

Also FWIW 4.4.2.2 Advertising Interval of the spec indicates that the max
allowed advertising interval for a connectable device is 10.24 seconds so we
could use that value to purge devices.

[scheib, 2016-05-06 18:01:05]

On 2016/05/06 16:15:11, ortuno wrote:
> On 2016/05/06 at 16:09:53, ortuno wrote:
> > On 2016/05/06 at 00:41:50, scheib wrote:
> > > Sorry I've been taking a while on this. It's a problem I'm unsure if we
are
> solving in the best way.
> > > 
> > > I know that I suggested using a repeating timer. That was probably a poor
> suggestion, which occurs to me more strongly when seeing the "every 5 seconds
> poll all devices" logic in android (every 5 sec seems too often if we do end
up
> as this as the only option).
> > > 
> > > I need to research this more, but I see that the logic you're using is
> !paired !connected. We have code now in Android that is notified when the
> connected state changes. We could add code to detect pairing state changing
with
> ACTION_BOND_STATE_CHANGED. I think that would enable us to purge devices only
at
> the time of a change. 
> > 
> > That only takes care of devices that we've connected to. We also want to be
> able to clean up devices that we've seen while scanning but are no longer
there.
> > 
> > > 
> > > Mac's .5 second polling interval is a little insane to me -- I didn't
think
> it was that frequent, and looking at it now I'm thinking "wwaaaaaa!?".
> > > 
> > >
>
https://codereview.chromium.org/1842223003/diff/220001/content/browser/blueto...
> > > File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right):
> > > 
> > >
>
https://codereview.chromium.org/1842223003/diff/220001/content/browser/blueto...
> > > content/browser/bluetooth/bluetooth_dispatcher_host.cc:1014: // Populate
the
> initial list of devices.
> > > This comment not needed now that the code block is in a well named method.
> > > 
> > >
>
https://codereview.chromium.org/1842223003/diff/220001/device/bluetooth/bluet...
> > > File device/bluetooth/bluetooth_adapter.cc (right):
> > > 
> > >
>
https://codereview.chromium.org/1842223003/diff/220001/device/bluetooth/bluet...
> > > device/bluetooth/bluetooth_adapter.cc:383:
> removed_devices.insert(it->first);
> > > This line is duplicated above.
> 
> Also FWIW 4.4.2.2 Advertising Interval of the spec indicates that the max
> allowed advertising interval for a connectable device is 10.24 seconds so we
> could use that value to purge devices.

ortuno's point that the bonding state change isn't sufficient is good. We can
ignore that for now I think.

In chat we came to the suggestion that we run the polling loop (even at say, 1
Purge per second) while there is an active Discovery Session and for a fixed
time period after that (e.g. 11 seconds as per the Advertising Interval cited
above).

[perja, 2016-06-13 07:30:34]

Change the code now so that it scans every second when there is an active
discovery session and 11 seconds otherwise.

[ortuno, 2016-06-14 15:29:31]

ortuno@chromium.org changed reviewers:
+ ortuno@chromium.org

[ortuno, 2016-06-14 15:29:33]

https://codereview.chromium.org/1842223003/diff/300001/chrome/browser/ui/andr...
File chrome/browser/ui/android/bluetooth_chooser_android.cc (left):

https://codereview.chromium.org/1842223003/diff/300001/chrome/browser/ui/andr...
chrome/browser/ui/android/bluetooth_chooser_android.cc:106: void
BluetoothChooserAndroid::RemoveDevice(const std::string& device_id) {
I would separate this chooser clean up from the rest of this patch.

https://codereview.chromium.org/1842223003/diff/300001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter.cc (right):

https://codereview.chromium.org/1842223003/diff/300001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter.cc:374: if (device->IsPaired() ||
device->IsConnected())
Also need device->IsGattConnected()

https://codereview.chromium.org/1842223003/diff/300001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter_android.cc (right):

https://codereview.chromium.org/1842223003/diff/300001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_android.cc:44:
adapter->PurgeTimedOutDevices();
I don't think we want to be purging devices all the time. This should only be
called after we start a scan. To do this we will have move the start scan logic
from ChromeBluetoothAdapter.addDiscoverySession to
BluetoothAdapterAndroid::AddDiscoverySession.

https://codereview.chromium.org/1842223003/diff/300001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_android.cc:210: int pollIntervalMs = 11000;
Make these enums:

namespace {
  // TODO add a comment explaining the values.
  enum { kPassivePollInterval = 11000 };
  enum { kActivePollInterval = 1000 };
}

https://codereview.chromium.org/1842223003/diff/300001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_android.cc:214:
ui_task_runner_->PostDelayedTask(
I think what Vince was suggesting was that we run RemoveTimedOutDevices every
second as long as there is an active discovery session but once the discovery
session is finished we run RemoveTimedOutDevices once after 11 seconds. This can
be done with the following code:

if (IsDiscovering()) {
  ui_task_runner_->PostDelayedTask(
      FROM_HERE, base::Bind(&BluetoothAdapterAndroid::PurgeTimedOutDevices,
                            weak_ptr_factory_.GetWeakPtr()),
      base::TimeDelta::FromMilliseconds(kActivePollInterval));
} else {
  ui_task_runner_->PostDelayedTask(
      FROM_HERE, base::Bind(&BluetoothAdapterAndroid::RemoveTimedOutDevices,
                            weak_ptr_factory_.GetWeakPtr()),
      base::TimeDelta::FromMilliseconds(kPassivePollInterval));
}

https://codereview.chromium.org/1842223003/diff/300001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter_unittest.cc (right):

https://codereview.chromium.org/1842223003/diff/300001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_unittest.cc:820: BluetoothDevice* device2 =
SimulateLowEnergyDevice(4);
Please add a test to make sure gatt connected devices, classic connected devices
and paired devices are not purged.

[perja, 2016-06-15 13:03:40]

Addressed all the comments except the test case for classic devices which I'm
not sure how to support.

https://codereview.chromium.org/1842223003/diff/300001/chrome/browser/ui/andr...
File chrome/browser/ui/android/bluetooth_chooser_android.cc (left):

https://codereview.chromium.org/1842223003/diff/300001/chrome/browser/ui/andr...
chrome/browser/ui/android/bluetooth_chooser_android.cc:106: void
BluetoothChooserAndroid::RemoveDevice(const std::string& device_id) {
On 2016/06/14 15:29:33, ortuno wrote:
> I would separate this chooser clean up from the rest of this patch.

Acknowledged.

https://codereview.chromium.org/1842223003/diff/300001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter.cc (right):

https://codereview.chromium.org/1842223003/diff/300001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter.cc:374: if (device->IsPaired() ||
device->IsConnected())
On 2016/06/14 15:29:33, ortuno wrote:
> Also need device->IsGattConnected()

Acknowledged.

https://codereview.chromium.org/1842223003/diff/300001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter_android.cc (right):

https://codereview.chromium.org/1842223003/diff/300001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_android.cc:44:
adapter->PurgeTimedOutDevices();
On 2016/06/14 15:29:33, ortuno wrote:
> I don't think we want to be purging devices all the time. This should only be
> called after we start a scan. To do this we will have move the start scan
logic
> from ChromeBluetoothAdapter.addDiscoverySession to
> BluetoothAdapterAndroid::AddDiscoverySession.

Acknowledged.

https://codereview.chromium.org/1842223003/diff/300001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_android.cc:210: int pollIntervalMs = 11000;
On 2016/06/14 15:29:33, ortuno wrote:
> Make these enums:
> 
> namespace {
>   // TODO add a comment explaining the values.
>   enum { kPassivePollInterval = 11000 };
>   enum { kActivePollInterval = 1000 };
> }

Acknowledged.

https://codereview.chromium.org/1842223003/diff/300001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_android.cc:214:
ui_task_runner_->PostDelayedTask(
On 2016/06/14 15:29:33, ortuno wrote:
> I think what Vince was suggesting was that we run RemoveTimedOutDevices every
> second as long as there is an active discovery session but once the discovery
> session is finished we run RemoveTimedOutDevices once after 11 seconds. This
can
> be done with the following code:
> 
> if (IsDiscovering()) {
>   ui_task_runner_->PostDelayedTask(
>       FROM_HERE, base::Bind(&BluetoothAdapterAndroid::PurgeTimedOutDevices,
>                             weak_ptr_factory_.GetWeakPtr()),
>       base::TimeDelta::FromMilliseconds(kActivePollInterval));
> } else {
>   ui_task_runner_->PostDelayedTask(
>       FROM_HERE, base::Bind(&BluetoothAdapterAndroid::RemoveTimedOutDevices,
>                             weak_ptr_factory_.GetWeakPtr()),
>       base::TimeDelta::FromMilliseconds(kPassivePollInterval));
> }

Acknowledged.

https://codereview.chromium.org/1842223003/diff/300001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter_unittest.cc (right):

https://codereview.chromium.org/1842223003/diff/300001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_unittest.cc:820: BluetoothDevice* device2 =
SimulateLowEnergyDevice(4);
On 2016/06/14 15:29:33, ortuno wrote:
> Please add a test to make sure gatt connected devices, classic connected
devices
> and paired devices are not purged.

I'm not sure how to test for classic connected devices. Are there any examples
in the unit tests for this? 

Also I made an attempt at testing paired devices, but it looks like this isn't
implemented.

[ortuno, 2016-06-15 17:20:16]

Thanks for doing this! It's looking good!

https://codereview.chromium.org/1842223003/diff/300001/chrome/browser/ui/andr...
File chrome/browser/ui/android/bluetooth_chooser_android.cc (left):

https://codereview.chromium.org/1842223003/diff/300001/chrome/browser/ui/andr...
chrome/browser/ui/android/bluetooth_chooser_android.cc:106: void
BluetoothChooserAndroid::RemoveDevice(const std::string& device_id) {
On 2016/06/15 at 13:03:40, perja wrote:
> On 2016/06/14 15:29:33, ortuno wrote:
> > I would separate this chooser clean up from the rest of this patch.
> 
> Acknowledged.

We usually say: "Done" :)

https://codereview.chromium.org/1842223003/diff/300001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter_unittest.cc (right):

https://codereview.chromium.org/1842223003/diff/300001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_unittest.cc:820: BluetoothDevice* device2 =
SimulateLowEnergyDevice(4);
On 2016/06/15 at 13:03:40, perja wrote:
> On 2016/06/14 15:29:33, ortuno wrote:
> > Please add a test to make sure gatt connected devices, classic connected
devices
> > and paired devices are not purged.
> 
> I'm not sure how to test for classic connected devices. Are there any examples
in the unit tests for this? 
> 
> Also I made an attempt at testing paired devices, but it looks like this isn't
implemented.

I would add two new functions to bluetooth_test.h i.e. SimulatePairedDevice and
SimulateConnectedDevice, and implement them only on mac since we don't support
BT Classic on Android. The tests that use these two functions should only be
enabled on mac.

https://codereview.chromium.org/1842223003/diff/340001/device/bluetooth/andro...
File
device/bluetooth/android/java/src/org/chromium/device/bluetooth/ChromeBluetoothAdapter.java
(right):

https://codereview.chromium.org/1842223003/diff/340001/device/bluetooth/andro...
device/bluetooth/android/java/src/org/chromium/device/bluetooth/ChromeBluetoothAdapter.java:145:
private boolean addDiscoverySession() {
This function seems less useful now. Make startScan public and call it from
BluetoothAdapterAndroid directly. Same with stopScan().

https://codereview.chromium.org/1842223003/diff/340001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter_android.cc (right):

https://codereview.chromium.org/1842223003/diff/340001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_android.cc:217: void
BluetoothAdapterAndroid::PurgeTimedOutDevices() {
I'm wondering if we should post a delayed task to RemoveTimedOutDevices after a
device disconnects. What do you think?

https://codereview.chromium.org/1842223003/diff/340001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_android.cc:245: bool
BluetoothAdapterAndroid::AddDiscoverySessionInternal() {
optional: I think this could be moved to AddDiscoverySession. Same with
StopDiscoverySessionInternal.

https://codereview.chromium.org/1842223003/diff/340001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_android.cc:263: PurgeTimedOutDevices();
This results in mAdapter.isDiscovering() being called right after
scanner.startScan(), which is kind of an async function. Are we sure
isDiscovering will return true? If not, we might want to post a delayed task to
give it time to settle.

https://codereview.chromium.org/1842223003/diff/340001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter_unittest.cc (right):

https://codereview.chromium.org/1842223003/diff/340001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_unittest.cc:795: observer.Reset();
Move this above StartLowEnergyDiscoverySession().

https://codereview.chromium.org/1842223003/diff/340001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_unittest.cc:829: // Check that outdated
devices are removed.
Also add a check to make sure DeviceRemoved was called.

https://codereview.chromium.org/1842223003/diff/340001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_unittest.cc:852: EXPECT_EQ(1u,
adapter_->GetDevices().size());
Set last_update_time_  to be bigger than timeoutSec, otherwise the device
wouldn't get removed even if not connected.

https://codereview.chromium.org/1842223003/diff/340001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_unittest.cc:861: #if defined(OS_ANDROID) ||
defined(OS_MACOSX)
Only OS_MACOSX, since we don't support classic bluetooth in android.

https://codereview.chromium.org/1842223003/diff/340001/device/bluetooth/test/...
File device/bluetooth/test/bluetooth_test_android.h (right):

https://codereview.chromium.org/1842223003/diff/340001/device/bluetooth/test/...
device/bluetooth/test/bluetooth_test_android.h:33: void
SetBonded(BluetoothDevice* device) override;
As mentioned above. I don't think we need to do this, since we don't support
classic BT on Android.

[perja, 2016-06-17 13:39:27]

ortuno: I have address most of the issues you pointed out, but still a bit
unsure about the tests for osx (see comments inline).

https://codereview.chromium.org/1842223003/diff/300001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter_unittest.cc (right):

https://codereview.chromium.org/1842223003/diff/300001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_unittest.cc:820: BluetoothDevice* device2 =
SimulateLowEnergyDevice(4);
On 2016/06/15 17:20:16, ortuno wrote:
> On 2016/06/15 at 13:03:40, perja wrote:
> > On 2016/06/14 15:29:33, ortuno wrote:
> > > Please add a test to make sure gatt connected devices, classic connected
> devices
> > > and paired devices are not purged.
> > 
> > I'm not sure how to test for classic connected devices. Are there any
examples
> in the unit tests for this? 
> > 
> > Also I made an attempt at testing paired devices, but it looks like this
isn't
> implemented.
> 
> I would add two new functions to bluetooth_test.h i.e. SimulatePairedDevice
and
> SimulateConnectedDevice, and implement them only on mac since we don't support
> BT Classic on Android. The tests that use these two functions should only be
> enabled on mac.

OK, do you know what class should be used for a fake BT classic device? Maybe
create a new device/bluetooth/bluetooth_classic_device_mac_fake.h?

https://codereview.chromium.org/1842223003/diff/340001/device/bluetooth/andro...
File
device/bluetooth/android/java/src/org/chromium/device/bluetooth/ChromeBluetoothAdapter.java
(right):

https://codereview.chromium.org/1842223003/diff/340001/device/bluetooth/andro...
device/bluetooth/android/java/src/org/chromium/device/bluetooth/ChromeBluetoothAdapter.java:145:
private boolean addDiscoverySession() {
On 2016/06/15 17:20:16, ortuno wrote:
> This function seems less useful now. Make startScan public and call it from
> BluetoothAdapterAndroid directly. Same with stopScan().

Done.

https://codereview.chromium.org/1842223003/diff/340001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter_android.cc (right):

https://codereview.chromium.org/1842223003/diff/340001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_android.cc:217: void
BluetoothAdapterAndroid::PurgeTimedOutDevices() {
On 2016/06/15 17:20:16, ortuno wrote:
> I'm wondering if we should post a delayed task to RemoveTimedOutDevices after
a
> device disconnects. What do you think?

Not sure I see why. It will be kept or purged during the next discover session
like other devices.

https://codereview.chromium.org/1842223003/diff/340001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_android.cc:245: bool
BluetoothAdapterAndroid::AddDiscoverySessionInternal() {
On 2016/06/15 17:20:16, ortuno wrote:
> optional: I think this could be moved to AddDiscoverySession. Same with
> StopDiscoverySessionInternal.

Done.

https://codereview.chromium.org/1842223003/diff/340001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_android.cc:263: PurgeTimedOutDevices();
On 2016/06/15 17:20:16, ortuno wrote:
> This results in mAdapter.isDiscovering() being called right after
> scanner.startScan(), which is kind of an async function. Are we sure
> isDiscovering will return true? If not, we might want to post a delayed task
to
> give it time to settle.

Yeah, that sounds like a good idea. I guess we can't be sure that isDiscovering
will return true right away. Also I tested with the ACTION_DISCOVERY_STARTED
broadcast on Android, but this is only fired for classic BT discovery.

https://codereview.chromium.org/1842223003/diff/340001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter_unittest.cc (right):

https://codereview.chromium.org/1842223003/diff/340001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_unittest.cc:829: // Check that outdated
devices are removed.
On 2016/06/15 17:20:16, ortuno wrote:
> Also add a check to make sure DeviceRemoved was called.

Done.

https://codereview.chromium.org/1842223003/diff/340001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_unittest.cc:852: EXPECT_EQ(1u,
adapter_->GetDevices().size());
On 2016/06/15 17:20:16, ortuno wrote:
> Set last_update_time_  to be bigger than timeoutSec, otherwise the device
> wouldn't get removed even if not connected.

Good catch.

[ortuno, 2016-06-17 19:31:41]

I thought we had some support for testing BT Classic code but it seems there is
none :( This is obviously something that we have to build but we can do so after
landing this patch.

https://codereview.chromium.org/1842223003/diff/360001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter_android.cc (right):

https://codereview.chromium.org/1842223003/diff/360001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_android.cc:239: bool sessionAdded = false;
session_added

https://codereview.chromium.org/1842223003/diff/360001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_android.cc:273: bool sessionRemoved = false;
session_removed

https://codereview.chromium.org/1842223003/diff/360001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_android.cc:275: DCHECK(false);
Move this below the following line and change it to NOTREACHED()

https://codereview.chromium.org/1842223003/diff/360001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter_android.h (right):

https://codereview.chromium.org/1842223003/diff/360001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_android.h:139: uint8_t numDiscoverySessions =
0;
Sorry for not noticing earlier. This should follow the C++ style guide:

size_t num_discovery_sessions_

[perja, 2016-06-22 07:41:52]

Fixed the remaining review issues.

https://codereview.chromium.org/1842223003/diff/360001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter_android.cc (right):

https://codereview.chromium.org/1842223003/diff/360001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_android.cc:239: bool sessionAdded = false;
On 2016/06/17 19:31:41, ortuno wrote:
> session_added

Done.

https://codereview.chromium.org/1842223003/diff/360001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_android.cc:273: bool sessionRemoved = false;
On 2016/06/17 19:31:41, ortuno wrote:
> session_removed

Done.

https://codereview.chromium.org/1842223003/diff/360001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_android.cc:275: DCHECK(false);
On 2016/06/17 19:31:41, ortuno wrote:
> Move this below the following line and change it to NOTREACHED()

Done.

https://codereview.chromium.org/1842223003/diff/360001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter_android.h (right):

https://codereview.chromium.org/1842223003/diff/360001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_android.h:139: uint8_t numDiscoverySessions =
0;
On 2016/06/17 19:31:41, ortuno wrote:
> Sorry for not noticing earlier. This should follow the C++ style guide:
> 
> size_t num_discovery_sessions_

Done.

[ortuno, 2016-06-22 17:23:11]

lgtm bar two nits

https://codereview.chromium.org/1842223003/diff/380001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter.cc (right):

https://codereview.chromium.org/1842223003/diff/380001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter.cc:388: removed_devices.insert(it->first);
nit: I would change removed_devices to be a std::vector<std::unique_ptr> and
call 

removed_devices.push_back(devices_.take_and_erase(it));

This way if someone calls GetDevices inside DeviceRemoved the removed device is
not included.

https://codereview.chromium.org/1842223003/diff/380001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter_android.cc (right):

https://codereview.chromium.org/1842223003/diff/380001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_android.cc:221: RemoveTimedOutDevices();
nit: We always want to purge timed out devices, so move this outside of the if
statement.

[perja, 2016-06-23 14:31:26]

https://codereview.chromium.org/1842223003/diff/380001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter.cc (right):

https://codereview.chromium.org/1842223003/diff/380001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter.cc:388: removed_devices.insert(it->first);
On 2016/06/22 17:23:11, ortuno wrote:
> nit: I would change removed_devices to be a std::vector<std::unique_ptr> and
> call 
> 
> removed_devices.push_back(devices_.take_and_erase(it));
> 
> This way if someone calls GetDevices inside DeviceRemoved the removed device
is
> not included.

I used a slightly different approach. Please take a look.

https://codereview.chromium.org/1842223003/diff/380001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter_android.cc (right):

https://codereview.chromium.org/1842223003/diff/380001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_android.cc:221: RemoveTimedOutDevices();
On 2016/06/22 17:23:11, ortuno wrote:
> nit: We always want to purge timed out devices, so move this outside of the if
> statement.

Done.

[ortuno, 2016-06-23 15:43:00]

lgtm with one small question/suggestion

https://codereview.chromium.org/1842223003/diff/400001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter.cc (right):

https://codereview.chromium.org/1842223003/diff/400001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter.cc:393: it = next;
Any special reason why you manually copy and increase the iterator? We just put
the ++it in the for loop in a couple of places[1][2][3] and I would like to know
if this is a pattern we should follow there as well. If not then I would prefer
the existing pattern already to be consistent throughout the code base.

[1]
https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_adapter_win.c...
[2]
https://cs.chromium.org/chromium/src/device/bluetooth/bluez/bluetooth_adapter...
[3]
https://cs.chromium.org/chromium/src/device/bluetooth/bluez/bluetooth_adapter...

[ortuno, 2016-06-23 17:57:34]

https://codereview.chromium.org/1842223003/diff/400001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter.cc (right):

https://codereview.chromium.org/1842223003/diff/400001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter.cc:393: it = next;
On 2016/06/23 at 15:42:59, ortuno wrote:
> Any special reason why you manually copy and increase the iterator? We just
put the ++it in the for loop in a couple of places[1][2][3] and I would like to
know if this is a pattern we should follow there as well. If not then I would
prefer the existing pattern already to be consistent throughout the code base.
> 
> [1]
https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_adapter_win.c...
> [2]
https://cs.chromium.org/chromium/src/device/bluetooth/bluez/bluetooth_adapter...
> [3]
https://cs.chromium.org/chromium/src/device/bluetooth/bluez/bluetooth_adapter...

Discussed this with scheib. I see why this is the correct approach and we should
fix other instances of the broken pattern. No need for you to do anything.

[perja, 2016-06-24 14:15:29]

ortuno: I've rebased on master and reverted two small changes. So if I haven't
missed something this CL should be good to go? Is there a way to check that all
code changes have been reviewed? End result of this CL is that only files in
device/bluetooth have been changed.

https://codereview.chromium.org/1842223003/diff/160001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java
(right):

https://codereview.chromium.org/1842223003/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ItemChooserDialog.java:395:
@VisibleForTesting
On 2016/04/08 07:47:35, perja wrote:
> On 2016/04/07 22:05:02, scheib wrote:
> > Is this needed for this patch? 
> 
> I'm not 100% sure about this one. Seems that only some of trybots fails for a
> junit test if I don't have this - could be that either javac or proguard
removes
> the method since it finds that it isn't used. I will investigate.
>  

I have reverted the @VisibleForTesting line now.

[ortuno, 2016-06-24 16:25:40]

Still lgtm

[perja, 2016-06-24 17:40:16]

The CQ bit was checked by perja@opera.com

[commit-bot: I haz the power, 2016-06-24 17:40:57]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-24 19:43:47]

Description was changed from

==========
On Android there is no notification when a device is lost. This change keeps
track of when a device was last seen and removes outdated devices. 

This was already implemented for OSX and the code has been moved to make it
accessible to Android as well.

BUG=581544
==========

to

==========
On Android there is no notification when a device is lost. This change keeps
track of when a device was last seen and removes outdated devices. 

This was already implemented for OSX and the code has been moved to make it
accessible to Android as well.

BUG=581544
==========

[commit-bot: I haz the power, 2016-06-24 19:43:49]

Committed patchset #21 (id:440001)

[commit-bot: I haz the power, 2016-06-24 19:46:04]

Description was changed from

==========
On Android there is no notification when a device is lost. This change keeps
track of when a device was last seen and removes outdated devices. 

This was already implemented for OSX and the code has been moved to make it
accessible to Android as well.

BUG=581544
==========

to

==========
On Android there is no notification when a device is lost. This change keeps
track of when a device was last seen and removes outdated devices.

This was already implemented for OSX and the code has been moved to make it
accessible to Android as well.

BUG=581544

Committed: https://crrev.com/849f95f6082cd71789e966de3b5a31ba87d1084e
Cr-Commit-Position: refs/heads/master@{#401940}
==========

[commit-bot: I haz the power, 2016-06-24 19:46:05]

Patchset 21 (id:??) landed as
https://crrev.com/849f95f6082cd71789e966de3b5a31ba87d1084e
Cr-Commit-Position: refs/heads/master@{#401940}

[scheib, 2016-06-30 23:03:15]

LGTM after being distracted and post land.