bluetooth: mac: add connected LE devices to chooser

bluetooth: mac: add connected LE devices to chooser

On a mac, when showing list of available low energy devices, low energy devices which are connected are not shown.

To get the list of devices, -[CBCentralManager scanForPeripheralsWithServices:options:] is used. This method reports only advertising devices.

For devices that are connected but doesn't advertise, -[CBCentralManager retrieveConnectedPeripheralsWithServices:] needs to be called when starting a discovery session.

BUG=630581
Committed: https://crrev.com/ff8c749c2d358b34184d156fb0939a14e264438f
Cr-Commit-Position: refs/heads/master@{#430229}

[jlebel, 2016-09-14 18:47:03]

Description was changed from

==========
First implementation

BUG=2242833002
==========

to

==========
On a mac, when showing list of available low energy devices, low energy devices
which are connected are not shown.

To get the list of devices, -[CBCentralManager
scanForPeripheralsWithServices:options:] is used. This method reports only
advertising devices.

For devices that are connected but doesn't advertise, -[CBCentralManager
retrieveConnectedPeripheralsWithServices:] needs to be called when starting a
discovery session.

BUG=2242833002
==========

[jlebel, 2016-09-14 18:47:15]

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

[commit-bot: I haz the power, 2016-09-14 18:48:10]

Dry run: 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-09-14 20:11:33]

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

[commit-bot: I haz the power, 2016-09-14 20:11:34]

Dry run: This issue passed the CQ dry run.

[François Beaufort, 2016-09-15 07:38:15]

beaufort.francois@gmail.com changed reviewers:
+ beaufort.francois@gmail.com

[François Beaufort, 2016-09-15 07:38:16]

Here are my 2 cents.
You might want to update BUG ID as well ;)

https://codereview.chromium.org/2339253002/diff/1/device/bluetooth/bluetooth_...
File device/bluetooth/bluetooth_low_energy_discovery_manager_mac.mm (right):

https://codereview.chromium.org/2339253002/diff/1/device/bluetooth/bluetooth_...
device/bluetooth/bluetooth_low_energy_discovery_manager_mac.mm:80: CBUUID*
defaultService = [CBUUID UUIDWithString:@"1800"];
You may want to add a comment there to explain why we've picked "Generic Access"
service UUID:
https://www.bluetooth.com/specifications/gatt/viewer?attributeXmlFile=org.blu...

In the future, we should consider adding previously connected devices as well.

https://codereview.chromium.org/2339253002/diff/1/device/bluetooth/bluetooth_...
device/bluetooth/bluetooth_low_energy_discovery_manager_mac.mm:84: #pragma clang
diagnostic ignored "-Wpartial-availability"
According to
https://developer.apple.com/reference/corebluetooth/cbcentralmanager/1518924-...,
retrieveConnectedPeripheralsWithServices has been introduced in macOS 10.9.
Shouldn't we update src/base/mac/sdk_forward_declarations.h?

https://codereview.chromium.org/2339253002/diff/1/device/bluetooth/bluetooth_...
device/bluetooth/bluetooth_low_energy_discovery_manager_mac.mm:89:
observer_->LowEnergyDeviceUpdated(peripheral, nil, 0);
Nit:  observer_->LowEnergyDeviceUpdated(peripheral, nil /* advertisementData */,
0 /* rssi */);

[jlebel, 2016-09-21 09:45:55]

Patchset #2 (id:20001) has been deleted

[jlebel, 2016-09-21 13:24:36]

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

[commit-bot: I haz the power, 2016-09-21 13:24:54]

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

[jlebel, 2016-09-21 13:26:08]

Description was changed from

==========
On a mac, when showing list of available low energy devices, low energy devices
which are connected are not shown.

To get the list of devices, -[CBCentralManager
scanForPeripheralsWithServices:options:] is used. This method reports only
advertising devices.

For devices that are connected but doesn't advertise, -[CBCentralManager
retrieveConnectedPeripheralsWithServices:] needs to be called when starting a
discovery session.

BUG=2242833002
==========

to

==========
On a mac, when showing list of available low energy devices, low energy devices
which are connected are not shown.

To get the list of devices, -[CBCentralManager
scanForPeripheralsWithServices:options:] is used. This method reports only
advertising devices.

For devices that are connected but doesn't advertise, -[CBCentralManager
retrieveConnectedPeripheralsWithServices:] needs to be called when starting a
discovery session.

BUG=630581
==========

[jlebel, 2016-09-21 13:49:27]

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

[jlebel, 2016-09-21 13:49:28]

Hello Giovanni ,

Can you review my patch about the adding connected peripherals on the mac while
discovering devices.

Thanks,

https://codereview.chromium.org/2339253002/diff/1/device/bluetooth/bluetooth_...
File device/bluetooth/bluetooth_low_energy_discovery_manager_mac.mm (right):

https://codereview.chromium.org/2339253002/diff/1/device/bluetooth/bluetooth_...
device/bluetooth/bluetooth_low_energy_discovery_manager_mac.mm:80: CBUUID*
defaultService = [CBUUID UUIDWithString:@"1800"];
On 2016/09/15 07:38:16, François Beaufort wrote:
> You may want to add a comment there to explain why we've picked "Generic
Access"
> service UUID:
>
https://www.bluetooth.com/specifications/gatt/viewer?attributeXmlFile=org.blu...
> 
> In the future, we should consider adding previously connected devices as well.


Done.

https://codereview.chromium.org/2339253002/diff/1/device/bluetooth/bluetooth_...
device/bluetooth/bluetooth_low_energy_discovery_manager_mac.mm:84: #pragma clang
diagnostic ignored "-Wpartial-availability"
On 2016/09/15 07:38:16, François Beaufort wrote:
> According to
>
https://developer.apple.com/reference/corebluetooth/cbcentralmanager/1518924-...,
> retrieveConnectedPeripheralsWithServices has been introduced in macOS 10.9.
> Shouldn't we update src/base/mac/sdk_forward_declarations.h?

I think it is a terrible idea to create a fake declaration to avoid a compiler
warning. I prefer to be explicit: ask the compiler to not generate the warning.

https://codereview.chromium.org/2339253002/diff/1/device/bluetooth/bluetooth_...
device/bluetooth/bluetooth_low_energy_discovery_manager_mac.mm:89:
observer_->LowEnergyDeviceUpdated(peripheral, nil, 0);
On 2016/09/15 07:38:16, François Beaufort wrote:
> Nit:  observer_->LowEnergyDeviceUpdated(peripheral, nil /* advertisementData
*/,
> 0 /* rssi */);

Done.

[commit-bot: I haz the power, 2016-09-21 14:32:58]

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

[commit-bot: I haz the power, 2016-09-21 14:32:59]

Dry run: This issue passed the CQ dry run.

[ortuno, 2016-09-21 23:17:20]

I don't think we should do this during discovery. They are two separate concepts
and should remain separate. Please add a new function to BluetoothAdapter called
RetrievedConnectedPeripherals. We will have to do the same on Android.

[jlebel, 2016-09-22 13:58:37]

On 2016/09/21 23:17:20, ortuno wrote:
> I don't think we should do this during discovery. They are two separate
concepts
> and should remain separate. Please add a new function to BluetoothAdapter
called
> RetrievedConnectedPeripherals. We will have to do the same on Android.

Why do we want to keep track of peripherals that we don't use? When will you
want to call this method? I'm guessing those devices will expire quickly if we
don't do anything with them.

[jracle (use Gerrit), 2016-09-22 14:28:49]

On 2016/09/22 13:58:37, jlebel wrote:
> On 2016/09/21 23:17:20, ortuno wrote:
> > I don't think we should do this during discovery. They are two separate
> concepts
> > and should remain separate. Please add a new function to BluetoothAdapter
> called
> > RetrievedConnectedPeripherals. We will have to do the same on Android.
> 
> Why do we want to keep track of peripherals that we don't use? When will you
> want to call this method? I'm guessing those devices will expire quickly if we
> don't do anything with them.

Jérôme, as we discussed 2 weeks ago I got your point with optimizing code and
merging data discovery and connected devices, but those are 2 separate things.

I think we'd better-off implementing the way Giovanni says, knowing that in a 
near future we'll have to separate requestDevice() from requestLEScan() in 
web-bluetooth API.

It's also a cleaner implentation, easier to read, cause you'd be in line with
current designs of CoreBluetooth, Android and other OSes.

[ortuno, 2016-09-23 00:56:48]

On 2016/09/22 at 13:58:37, jlebel wrote:
> On 2016/09/21 23:17:20, ortuno wrote:
> > I don't think we should do this during discovery. They are two separate
concepts
> > and should remain separate. Please add a new function to BluetoothAdapter
called
> > RetrievedConnectedPeripherals. We will have to do the same on Android.
> 
> Why do we want to keep track of peripherals that we don't use?

I'm not sure what you mean? This would be just another way for clients to
retrieve known devices.

> When will you want to call this method?

We would call that method here:
https://cs.chromium.org/chromium/src/content/browser/bluetooth/bluetooth_devi...

Which is also a step in the Web Bluetooth spec:
https://webbluetoothcg.github.io/web-bluetooth/#scan-for-devices but not part of
the scanning spec.

> I'm guessing those devices will expire quickly if we don't do anything with
them.

Not necessarily since we don't purge devices that are connected
https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_adapter.cc?sq...

[jlebel, 2016-09-28 12:24:15]

On 2016/09/23 00:56:48, ortuno wrote:
> On 2016/09/22 at 13:58:37, jlebel wrote:
> > On 2016/09/21 23:17:20, ortuno wrote:
> > > I don't think we should do this during discovery. They are two separate
> concepts
> > > and should remain separate. Please add a new function to BluetoothAdapter
> called
> > > RetrievedConnectedPeripherals. We will have to do the same on Android.
> > 
> > Why do we want to keep track of peripherals that we don't use?
> 
> I'm not sure what you mean? This would be just another way for clients to
> retrieve known devices.
> 
> > When will you want to call this method?
> 
> We would call that method here:
>
https://cs.chromium.org/chromium/src/content/browser/bluetooth/bluetooth_devi...
> 
> Which is also a step in the Web Bluetooth spec:
> https://webbluetoothcg.github.io/web-bluetooth/#scan-for-devices but not part
of
> the scanning spec.
> 
> > I'm guessing those devices will expire quickly if we don't do anything with
> them.
> 
> Not necessarily since we don't purge devices that are connected
>
https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_adapter.cc?sq...

The state of GATT for a device is from the point of view of the application, not
the system. This means, that devices given by -[CBCentralManager
retrieveConnectedPeripheralsWithServices:]  will be seen as not connected (using
-[CBPeripheral state]), unless the current application (Chromium) started the
GATT connection. We also have no way to know when the GATT connection is really
closed. If Chromium closes the GATT connection for a device, an other
application might need it, therefore for Chromium the GATT will be closed, but
macOS will keep it alive for another application.

[ortuno, 2016-09-28 23:15:52]

Ah right. Are you worried about the peripherals mac sent expiring or us expiring
the devices? I think there is always a risk that the mac peripherals expired
even when doing a scan. Regarding us expiring the devices. I think it's OK. If
clients really wanted to interact with the device they would have connected to
it.

https://codereview.chromium.org/2339253002/diff/60001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_adapter.h (right):

https://codereview.chromium.org/2339253002/diff/60001/device/bluetooth/blueto...
device/bluetooth/bluetooth_adapter.h:323: // This function get add all the
connected peripherals into |devices_|.
Could you open an issue and add a TODO for implementing this on all platforms?

https://codereview.chromium.org/2339253002/diff/60001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_adapter_mac.mm (right):

https://codereview.chromium.org/2339253002/diff/60001/device/bluetooth/blueto...
device/bluetooth/bluetooth_adapter_mac.mm:198: // Can remove ignore
-Wpartial-availability when 10.8 will not be supported
Is there an issue you could include with this comment?

https://codereview.chromium.org/2339253002/diff/60001/device/bluetooth/blueto...
device/bluetooth/bluetooth_adapter_mac.mm:204:
LowEnergyDeviceUpdated(peripheral, nil /* advertisementData */,
LowEnergyDeviceUpdated is meant to be used when receiving an advertisement. I
think you want a specific function for constructing devices from connected
peripherals.

https://codereview.chromium.org/2339253002/diff/60001/device/bluetooth/blueto...
device/bluetooth/bluetooth_adapter_mac.mm:411: NSArray* services =
CBUUIDArrayWithUUIDList(BluetoothDevice::UUIDList());
This seems unrelated to this change.

https://codereview.chromium.org/2339253002/diff/60001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_low_energy_discovery_manager_mac.mm (right):

https://codereview.chromium.org/2339253002/diff/60001/device/bluetooth/blueto...
device/bluetooth/bluetooth_low_energy_discovery_manager_mac.mm:59:
scanForPeripheralsWithServices:services_uuids_
This seems unrelated. Please do in another patch.

Also fwiw I tried implementing this in
https://codereview.chromium.org/2253223002 but there is quite a bit of
refactoring that needs to be done at the cross platform layer.

https://codereview.chromium.org/2339253002/diff/60001/device/bluetooth/test/m...
File device/bluetooth/test/mock_bluetooth_central_manager_mac.mm (right):

https://codereview.chromium.org/2339253002/diff/60001/device/bluetooth/test/m...
device/bluetooth/test/mock_bluetooth_central_manager_mac.mm:23:
_connectedMockPeripheral = [[NSMutableArray alloc] init];
What do you think of the following ideas:

Instead of addConnectedMockPeripherals, would a
setConnectedMockPeripherals(NSArray*) make more sense? It would allow tests to
explicitly set all the devices rather than adding and removing each manually.

Would it make sense to have a "connected_peripherals_" scoped_nsobject<NSArray>
member? We would just return it in retrieveConnectedPeripherals.

[jlebel, 2016-10-05 14:09:41]

https://codereview.chromium.org/2339253002/diff/60001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_adapter.h (right):

https://codereview.chromium.org/2339253002/diff/60001/device/bluetooth/blueto...
device/bluetooth/bluetooth_adapter.h:323: // This function get add all the
connected peripherals into |devices_|.
On 2016/09/28 23:15:52, ortuno wrote:
> Could you open an issue and add a TODO for implementing this on all platforms?

Done.

https://codereview.chromium.org/2339253002/diff/60001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_adapter_mac.mm (right):

https://codereview.chromium.org/2339253002/diff/60001/device/bluetooth/blueto...
device/bluetooth/bluetooth_adapter_mac.mm:198: // Can remove ignore
-Wpartial-availability when 10.8 will not be supported
On 2016/09/28 23:15:52, ortuno wrote:
> Is there an issue you could include with this comment?

Done.

https://codereview.chromium.org/2339253002/diff/60001/device/bluetooth/blueto...
device/bluetooth/bluetooth_adapter_mac.mm:204:
LowEnergyDeviceUpdated(peripheral, nil /* advertisementData */,
On 2016/09/28 23:15:52, ortuno wrote:
> LowEnergyDeviceUpdated is meant to be used when receiving an advertisement. I
> think you want a specific function for constructing devices from connected
> peripherals.

I don't know how to use the first part and the third part of
LowEnergyDeviceUpdated() without using the middle part, and without duplicating
the code.

I was thinking of creating FindOrCreateLowEnergyDeviceUpdated() to put the first
part of the method, but there is also an ownership problem, because the instance
returned in case of a new device would have to be release if not used, and not
for the case of a known device. Plus a flag would have to be returned from this
new method in order to know if the device should be added and to know which
notification should be sent (DeviceAdded or DeviceChanged).

https://codereview.chromium.org/2339253002/diff/60001/device/bluetooth/blueto...
device/bluetooth/bluetooth_adapter_mac.mm:411: NSArray* services =
CBUUIDArrayWithUUIDList(BluetoothDevice::UUIDList());
On 2016/09/28 23:15:52, ortuno wrote:
> This seems unrelated to this change.

Done.

https://codereview.chromium.org/2339253002/diff/60001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_low_energy_discovery_manager_mac.mm (right):

https://codereview.chromium.org/2339253002/diff/60001/device/bluetooth/blueto...
device/bluetooth/bluetooth_low_energy_discovery_manager_mac.mm:59:
scanForPeripheralsWithServices:services_uuids_
On 2016/09/28 23:15:52, ortuno wrote:
> This seems unrelated. Please do in another patch.
> 
> Also fwiw I tried implementing this in
> https://codereview.chromium.org/2253223002 but there is quite a bit of
> refactoring that needs to be done at the cross platform layer.

Done.

https://codereview.chromium.org/2339253002/diff/60001/device/bluetooth/test/m...
File device/bluetooth/test/mock_bluetooth_central_manager_mac.mm (right):

https://codereview.chromium.org/2339253002/diff/60001/device/bluetooth/test/m...
device/bluetooth/test/mock_bluetooth_central_manager_mac.mm:23:
_connectedMockPeripheral = [[NSMutableArray alloc] init];
On 2016/09/28 23:15:52, ortuno wrote:
> What do you think of the following ideas:
> 
> Instead of addConnectedMockPeripherals, would a
> setConnectedMockPeripherals(NSArray*) make more sense? It would allow tests to
> explicitly set all the devices rather than adding and removing each manually.
> 
> Would it make sense to have a "connected_peripherals_"
scoped_nsobject<NSArray>
> member? We would just return it in retrieveConnectedPeripherals.

Done.

[jlebel, 2016-10-05 14:53:03]

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

[commit-bot: I haz the power, 2016-10-05 14:53:23]

Dry run: 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-10-05 16:32:37]

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

[commit-bot: I haz the power, 2016-10-05 16:32:38]

Dry run: This issue passed the CQ dry run.

[ortuno, 2016-10-06 02:22:07]

https://codereview.chromium.org/2339253002/diff/80001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_adapter_mac.mm (right):

https://codereview.chromium.org/2339253002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_adapter_mac.mm:173: void
BluetoothAdapterMac::RetrievedConnectedPeripherals() {
+scheib for thoughts. Some context: We are adding a new function to retrieve
connected devices. These devices are probably not advertising and platforms
require us to manually query for these.

I don't think this function need to do the lower half of LowEnergyDeviceUpdated:

const std::vector<BluetoothDevice*> RetrieveGattConnectedDevices()

1. Initialize connected_devices to be an empty std::vector<BluetoothDevice*>
2. Query the platform for GATT connected devices
3. For each device in Step 2.:
  1. If we don't know of the device already, initialize a new BluetoothDevice
and add it to the list a known devices. Otherwise, retrieve the BluetoothDevice
representing device.
  2. Add the BluetoothDevice from step 1 to connected_devices.
4. Return connected_devices.

Note that we won't be calling DeviceAdded for new devices. I think calling
DeviceAdded in the middle of a synchronous function will be confusing and cause
bugs for clients. Furthermore DeviceAdded is only useful for OS clients that
need to know everything about all devices. In the next iteration of the API we
will probably not have such an event.

[ortuno, 2016-10-06 02:22:52]

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

[ortuno, 2016-10-06 02:22:52]

https://codereview.chromium.org/2339253002/diff/80001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_adapter_mac.mm (right):

https://codereview.chromium.org/2339253002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_adapter_mac.mm:173: void
BluetoothAdapterMac::RetrievedConnectedPeripherals() {
On 2016/10/06 at 02:22:07, ortuno wrote:
> +scheib for thoughts. Some context: We are adding a new function to retrieve
connected devices. These devices are probably not advertising and platforms
require us to manually query for these.
> 
> I don't think this function need to do the lower half of
LowEnergyDeviceUpdated:
> 
> const std::vector<BluetoothDevice*> RetrieveGattConnectedDevices()
> 
> 1. Initialize connected_devices to be an empty std::vector<BluetoothDevice*>
> 2. Query the platform for GATT connected devices
> 3. For each device in Step 2.:
>   1. If we don't know of the device already, initialize a new BluetoothDevice
and add it to the list a known devices. Otherwise, retrieve the BluetoothDevice
representing device.
>   2. Add the BluetoothDevice from step 1 to connected_devices.
> 4. Return connected_devices.
> 
> Note that we won't be calling DeviceAdded for new devices. I think calling
DeviceAdded in the middle of a synchronous function will be confusing and cause
bugs for clients. Furthermore DeviceAdded is only useful for OS clients that
need to know everything about all devices. In the next iteration of the API we
will probably not have such an event.

+scheib for realz

[scheib, 2016-10-06 20:39:13]

Description was changed from

==========
On a mac, when showing list of available low energy devices, low energy devices
which are connected are not shown.

To get the list of devices, -[CBCentralManager
scanForPeripheralsWithServices:options:] is used. This method reports only
advertising devices.

For devices that are connected but doesn't advertise, -[CBCentralManager
retrieveConnectedPeripheralsWithServices:] needs to be called when starting a
discovery session.

BUG=630581
==========

to

==========
bluetooth: mac: chooser doesn't show a low energy device

On a mac, when showing list of available low energy devices, low energy devices
which are connected are not shown.

To get the list of devices, -[CBCentralManager
scanForPeripheralsWithServices:options:] is used. This method reports only
advertising devices.

For devices that are connected but doesn't advertise, -[CBCentralManager
retrieveConnectedPeripheralsWithServices:] needs to be called when starting a
discovery session.

BUG=630581
==========

[scheib, 2016-10-06 20:40:54]

Description was changed from

==========
bluetooth: mac: chooser doesn't show a low energy device

On a mac, when showing list of available low energy devices, low energy devices
which are connected are not shown.

To get the list of devices, -[CBCentralManager
scanForPeripheralsWithServices:options:] is used. This method reports only
advertising devices.

For devices that are connected but doesn't advertise, -[CBCentralManager
retrieveConnectedPeripheralsWithServices:] needs to be called when starting a
discovery session.

BUG=630581
==========

to

==========
bluetooth: mac: add connected LE devices to chooser

On a mac, when showing list of available low energy devices, low energy devices
which are connected are not shown.

To get the list of devices, -[CBCentralManager
scanForPeripheralsWithServices:options:] is used. This method reports only
advertising devices.

For devices that are connected but doesn't advertise, -[CBCentralManager
retrieveConnectedPeripheralsWithServices:] needs to be called when starting a
discovery session.

BUG=630581
==========

[scheib, 2016-10-06 21:01:34]

https://codereview.chromium.org/2339253002/diff/80001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_adapter_mac.mm (right):

https://codereview.chromium.org/2339253002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_adapter_mac.mm:173: void
BluetoothAdapterMac::RetrievedConnectedPeripherals() {
On 2016/10/06 02:22:07, ortuno wrote:
> +scheib for thoughts. Some context: We are adding a new function to retrieve
> connected devices. These devices are probably not advertising and platforms
> require us to manually query for these.
> 
> I don't think this function need to do the lower half of
LowEnergyDeviceUpdated:
> 
> const std::vector<BluetoothDevice*> RetrieveGattConnectedDevices()
> 
> 1. Initialize connected_devices to be an empty std::vector<BluetoothDevice*>
> 2. Query the platform for GATT connected devices
> 3. For each device in Step 2.:
>   1. If we don't know of the device already, initialize a new BluetoothDevice
> and add it to the list a known devices. Otherwise, retrieve the
BluetoothDevice
> representing device.
>   2. Add the BluetoothDevice from step 1 to connected_devices.
> 4. Return connected_devices.
> 
> Note that we won't be calling DeviceAdded for new devices. I think calling
> DeviceAdded in the middle of a synchronous function will be confusing and
cause
> bugs for clients. Furthermore DeviceAdded is only useful for OS clients that
> need to know everything about all devices. In the next iteration of the API we
> will probably not have such an event.

RetrievedConnectedPeripherals seems to be incorrectly named, should be
RetrieveConnectedPeripherals.

Even with a changing API, we need to be consistent with the current style and
API.

The best way to avoid bugs with re-entrant calls to ::Observers is to PostTask
instead of calling the observers directly. We could consider changing
LowEnergyDeviceUpdated now to use PostTask (but this will likely require test
changes as well - and should be split to another patch).

If a new device is identified, I think that DeviceAdded needs to be called.
Clients need to be able to have the full list of devices known trusting that
DeviceAdded will offer them, as specified by that method [1]

Ditto for DeviceChanged I think, but it would be best if we only sent those
updates for devices that actually did change. I could see that as not practical.
I'd be happy enough with RetrieveConnectedPeripherals calling
LowEnergyDeviceUpdated in a way that only generated DeviceAdded calls.


[1]
https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_adapter.h?typ...

[ortuno, 2016-10-06 23:43:12]

https://codereview.chromium.org/2339253002/diff/80001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_adapter_mac.mm (right):

https://codereview.chromium.org/2339253002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_adapter_mac.mm:173: void
BluetoothAdapterMac::RetrievedConnectedPeripherals() {
On 2016/10/06 at 21:01:34, scheib wrote:
> On 2016/10/06 02:22:07, ortuno wrote:
> > +scheib for thoughts. Some context: We are adding a new function to retrieve
> > connected devices. These devices are probably not advertising and platforms
> > require us to manually query for these.
> > 
> > I don't think this function need to do the lower half of
LowEnergyDeviceUpdated:
> > 
> > const std::vector<BluetoothDevice*> RetrieveGattConnectedDevices()
> > 
> > 1. Initialize connected_devices to be an empty std::vector<BluetoothDevice*>
> > 2. Query the platform for GATT connected devices
> > 3. For each device in Step 2.:
> >   1. If we don't know of the device already, initialize a new
BluetoothDevice
> > and add it to the list a known devices. Otherwise, retrieve the
BluetoothDevice
> > representing device.
> >   2. Add the BluetoothDevice from step 1 to connected_devices.
> > 4. Return connected_devices.
> > 
> > Note that we won't be calling DeviceAdded for new devices. I think calling
> > DeviceAdded in the middle of a synchronous function will be confusing and
cause
> > bugs for clients. Furthermore DeviceAdded is only useful for OS clients that
> > need to know everything about all devices. In the next iteration of the API
we
> > will probably not have such an event.
> 
> RetrievedConnectedPeripherals seems to be incorrectly named, should be
> RetrieveConnectedPeripherals.
> 
> Even with a changing API, we need to be consistent with the current style and
API.
> 
> The best way to avoid bugs with re-entrant calls to ::Observers is to PostTask
instead of calling the observers directly. We could consider changing
LowEnergyDeviceUpdated now to use PostTask (but this will likely require test
changes as well - and should be split to another patch).
> 

Right, but we still have the problem of providing the same device to clients in
two different ways (through the return value of this function and through
DeviceAdded) and of returning devices for which we haven't called DeviceAdded.
Both of these will lead to very subtle bugs since this will only happen on macOS
and Android. If we really wanted to keep the style of the API, we wouldn't have
this function but rather we would poll like we do for the adapter name and
paired devices. The introduction of this function is trying to avoid that.

> If a new device is identified, I think that DeviceAdded needs to be called.
Clients need to be able to have the full list of devices known trusting that
DeviceAdded will offer them, as specified by that method [1]
> 

I would like to push clients of the API to use DeviceAdded as a signal that the
device was discovered during a scanning procedure. Having this function not call
DeviceAdded would fit into that model.

> Ditto for DeviceChanged I think, but it would be best if we only sent those
updates for devices that actually did change. I could see that as not practical.
I'd be happy enough with RetrieveConnectedPeripherals calling
LowEnergyDeviceUpdated in a way that only generated DeviceAdded calls.
> 
> 
> [1]
https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_adapter.h?typ...

[scheib, 2016-10-07 17:08:01]

https://codereview.chromium.org/2339253002/diff/80001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_adapter_mac.mm (right):

https://codereview.chromium.org/2339253002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_adapter_mac.mm:173: void
BluetoothAdapterMac::RetrievedConnectedPeripherals() {
On 2016/10/06 23:43:12, ortuno wrote:
> On 2016/10/06 at 21:01:34, scheib wrote:
> > On 2016/10/06 02:22:07, ortuno wrote:
> > > +scheib for thoughts. Some context: We are adding a new function to
retrieve
> > > connected devices. These devices are probably not advertising and
platforms
> > > require us to manually query for these.
> > > 
> > > I don't think this function need to do the lower half of
> LowEnergyDeviceUpdated:
> > > 
> > > const std::vector<BluetoothDevice*> RetrieveGattConnectedDevices()
> > > 
> > > 1. Initialize connected_devices to be an empty
std::vector<BluetoothDevice*>
> > > 2. Query the platform for GATT connected devices
> > > 3. For each device in Step 2.:
> > >   1. If we don't know of the device already, initialize a new
> BluetoothDevice
> > > and add it to the list a known devices. Otherwise, retrieve the
> BluetoothDevice
> > > representing device.
> > >   2. Add the BluetoothDevice from step 1 to connected_devices.
> > > 4. Return connected_devices.
> > > 
> > > Note that we won't be calling DeviceAdded for new devices. I think calling
> > > DeviceAdded in the middle of a synchronous function will be confusing and
> cause
> > > bugs for clients. Furthermore DeviceAdded is only useful for OS clients
that
> Right, but we still have the problem of providing the same device to clients
in
> two different ways (through the return value of this function and through
> DeviceAdded)

This method doesn't return devices. It causes connected devices to be
discovered. Perhaps we can think about it as another kind of scan? DeviceAdded
is the only way clients find the new devices.

I think I'm OK with that.



As an alternative, should GetDevices implementation discover these devices? And
the Web Bluetooth for example would start by GetDevices, filter out only the
ones that are connected, and add those to the chooser, then start a scan and any
DeviceAdded devices get added as well.

[ortuno, 2016-10-10 02:57:22]

https://codereview.chromium.org/2339253002/diff/80001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_adapter_mac.mm (right):

https://codereview.chromium.org/2339253002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_adapter_mac.mm:173: void
BluetoothAdapterMac::RetrievedConnectedPeripherals() {
On 2016/10/07 at 17:08:00, scheib wrote:
> On 2016/10/06 23:43:12, ortuno wrote:
> > On 2016/10/06 at 21:01:34, scheib wrote:
> > > On 2016/10/06 02:22:07, ortuno wrote:
> > > > +scheib for thoughts. Some context: We are adding a new function to
retrieve
> > > > connected devices. These devices are probably not advertising and
platforms
> > > > require us to manually query for these.
> > > > 
> > > > I don't think this function need to do the lower half of
> > LowEnergyDeviceUpdated:
> > > > 
> > > > const std::vector<BluetoothDevice*> RetrieveGattConnectedDevices()
> > > > 
> > > > 1. Initialize connected_devices to be an empty
std::vector<BluetoothDevice*>
> > > > 2. Query the platform for GATT connected devices
> > > > 3. For each device in Step 2.:
> > > >   1. If we don't know of the device already, initialize a new
> > BluetoothDevice
> > > > and add it to the list a known devices. Otherwise, retrieve the
> > BluetoothDevice
> > > > representing device.
> > > >   2. Add the BluetoothDevice from step 1 to connected_devices.
> > > > 4. Return connected_devices.
> > > > 
> > > > Note that we won't be calling DeviceAdded for new devices. I think
calling
> > > > DeviceAdded in the middle of a synchronous function will be confusing
and
> > cause
> > > > bugs for clients. Furthermore DeviceAdded is only useful for OS clients
that
> > Right, but we still have the problem of providing the same device to clients
in
> > two different ways (through the return value of this function and through
> > DeviceAdded)
> 
> This method doesn't return devices. It causes connected devices to be
discovered. Perhaps we can think about it as another kind of scan? DeviceAdded
is the only way clients find the new devices.
> 
> I think I'm OK with that.
> 
> 
> 
> As an alternative, should GetDevices implementation discover these devices?
And the Web Bluetooth for example would start by GetDevices, filter out only the
ones that are connected, and add those to the chooser, then start a scan and any
DeviceAdded devices get added as well.


The current method as is does not return devices but when I proposed the method
I thought it should and I still think it should. Having a synchronous method is
the easiest to use and implement:

1. Android: We would use BluetoothManager.getConnectedDevices() and create the
new devices if we haven't seen them before.
2. macOS: We would use retrieveConnectedPeripheralsWithServices and create the
new devices if we haven't seen them before.
3. BlueZ: We would iterate over the currently known devices and return the ones
that are connected.
4. Windows: Enumerate all LE devices and return the connected ones. (In fact I
think the current implementation of tying this to a Discovery session is wrong.)

Clients will simply need to do:

for (BluetoothDevice* device : adapter->GetConnectedDevices()) {
  // Do something with the device.
}

Having methods that abstract bluetooth concepts or build on top of them is very
annoying e.g. all the Device/Service/Characteristic/Descriptor Added/Removed
events, GetUUIDs, DeviceChanged, etc. add more complexity to the implementation,
means clients need to understand yet another concept and all its subtleties, and
sometimes result in clients not being able to implement features e.g. we had
many problems with the chooser because of how DeviceChanged worked and Arc++ is
still not able to implement advertisements because of DeviceChanged.

Although I agree that we should keep the style of the API, I also think that
whatever work we do to move clients to use the API in a way that is compatible
with the eventual mojo API will save future refactoring work.

Overall I think exposing low level APIs reduces the API complexity, reduces the
burden on clients, allows clients to implement more powerful features and will
be easier to refactor.

[scheib, 2016-10-10 18:00:27]

On 2016/10/10 02:57:22, ortuno wrote:
>
https://codereview.chromium.org/2339253002/diff/80001/device/bluetooth/blueto...
> File device/bluetooth/bluetooth_adapter_mac.mm (right):
> 
>
https://codereview.chromium.org/2339253002/diff/80001/device/bluetooth/blueto...
> device/bluetooth/bluetooth_adapter_mac.mm:173: void
> BluetoothAdapterMac::RetrievedConnectedPeripherals() {
> On 2016/10/07 at 17:08:00, scheib wrote:
> > On 2016/10/06 23:43:12, ortuno wrote:
> > > On 2016/10/06 at 21:01:34, scheib wrote:
> > > > On 2016/10/06 02:22:07, ortuno wrote:
> > > > > +scheib for thoughts. Some context: We are adding a new function to
> retrieve
> > > > > connected devices. These devices are probably not advertising and
> platforms
> > > > > require us to manually query for these.
> > > > > 
> > > > > I don't think this function need to do the lower half of
> > > LowEnergyDeviceUpdated:
> > > > > 
> > > > > const std::vector<BluetoothDevice*> RetrieveGattConnectedDevices()
> > > > > 
> > > > > 1. Initialize connected_devices to be an empty
> std::vector<BluetoothDevice*>
> > > > > 2. Query the platform for GATT connected devices
> > > > > 3. For each device in Step 2.:
> > > > >   1. If we don't know of the device already, initialize a new
> > > BluetoothDevice
> > > > > and add it to the list a known devices. Otherwise, retrieve the
> > > BluetoothDevice
> > > > > representing device.
> > > > >   2. Add the BluetoothDevice from step 1 to connected_devices.
> > > > > 4. Return connected_devices.
> > > > > 
> > > > > Note that we won't be calling DeviceAdded for new devices. I think
> calling
> > > > > DeviceAdded in the middle of a synchronous function will be confusing
> and
> > > cause
> > > > > bugs for clients. Furthermore DeviceAdded is only useful for OS
clients
> that
> > > Right, but we still have the problem of providing the same device to
clients
> in
> > > two different ways (through the return value of this function and through
> > > DeviceAdded)
> > 
> > This method doesn't return devices. It causes connected devices to be
> discovered. Perhaps we can think about it as another kind of scan? DeviceAdded
> is the only way clients find the new devices.
> > 
> > I think I'm OK with that.
> > 
> > 
> > 
> > As an alternative, should GetDevices implementation discover these devices?
> And the Web Bluetooth for example would start by GetDevices, filter out only
the
> ones that are connected, and add those to the chooser, then start a scan and
any
> DeviceAdded devices get added as well.
> 
> 
> The current method as is does not return devices but when I proposed the
method
> I thought it should and I still think it should. Having a synchronous method
is
> the easiest to use and implement:
> 
> 1. Android: We would use BluetoothManager.getConnectedDevices() and create the
> new devices if we haven't seen them before.
> 2. macOS: We would use retrieveConnectedPeripheralsWithServices and create the
> new devices if we haven't seen them before.
> 3. BlueZ: We would iterate over the currently known devices and return the
ones
> that are connected.
> 4. Windows: Enumerate all LE devices and return the connected ones. (In fact I
> think the current implementation of tying this to a Discovery session is
wrong.)
> 
> Clients will simply need to do:
> 
> for (BluetoothDevice* device : adapter->GetConnectedDevices()) {
>   // Do something with the device.
> }
> 
> Having methods that abstract bluetooth concepts or build on top of them is
very
> annoying e.g. all the Device/Service/Characteristic/Descriptor Added/Removed
> events, GetUUIDs, DeviceChanged, etc. add more complexity to the
implementation,
> means clients need to understand yet another concept and all its subtleties,
and
> sometimes result in clients not being able to implement features e.g. we had
> many problems with the chooser because of how DeviceChanged worked and Arc++
is
> still not able to implement advertisements because of DeviceChanged.
> 
> Although I agree that we should keep the style of the API, I also think that
> whatever work we do to move clients to use the API in a way that is compatible
> with the eventual mojo API will save future refactoring work.
> 
> Overall I think exposing low level APIs reduces the API complexity, reduces
the
> burden on clients, allows clients to implement more powerful features and will
> be easier to refactor.

I don't like the side-effect of GetConnectedDevices, which appears to just
return already known devices, causing new devices to be discovered on some
platforms. Not calling DeviceAdded seems wrong to me when we do discover a
device. When there are multiple clients all should be made aware when a new
device is discovered in any way. E.g. the bluetooth internals page should know
about connected devices that were newly discovered with GetConnectedDevices
called by another client.

I'm not sure what a long term redesign of scanning would look like that doesn't
include something similar to DeviceAdded. But I do agree with the goal to
minimize additional concepts, while keeping common client code easy. E.g. I
think we'll want to keep GetDevices.

In your previous example, calling RetrieveConnectedPeripherals and then
GetDevices isn't much more work for a client, but the API has fewer surprises.

The 4 platform implementations seem to just flip-flop in complexity. Some
platforms just no-op for RetrieveConnectedPeripherals, and the others enumerate
create devices that are connected and not already tracked. Seems about the same
overall either way.

[ortuno, 2016-10-10 23:34:09]

On 2016/10/10 at 18:00:27, scheib wrote:
> On 2016/10/10 02:57:22, ortuno wrote:
> >
https://codereview.chromium.org/2339253002/diff/80001/device/bluetooth/blueto...
> > File device/bluetooth/bluetooth_adapter_mac.mm (right):
> > 
> >
https://codereview.chromium.org/2339253002/diff/80001/device/bluetooth/blueto...
> > device/bluetooth/bluetooth_adapter_mac.mm:173: void
> > BluetoothAdapterMac::RetrievedConnectedPeripherals() {
> > On 2016/10/07 at 17:08:00, scheib wrote:
> > > On 2016/10/06 23:43:12, ortuno wrote:
> > > > On 2016/10/06 at 21:01:34, scheib wrote:
> > > > > On 2016/10/06 02:22:07, ortuno wrote:
> > > > > > +scheib for thoughts. Some context: We are adding a new function to
> > retrieve
> > > > > > connected devices. These devices are probably not advertising and
> > platforms
> > > > > > require us to manually query for these.
> > > > > > 
> > > > > > I don't think this function need to do the lower half of
> > > > LowEnergyDeviceUpdated:
> > > > > > 
> > > > > > const std::vector<BluetoothDevice*> RetrieveGattConnectedDevices()
> > > > > > 
> > > > > > 1. Initialize connected_devices to be an empty
> > std::vector<BluetoothDevice*>
> > > > > > 2. Query the platform for GATT connected devices
> > > > > > 3. For each device in Step 2.:
> > > > > >   1. If we don't know of the device already, initialize a new
> > > > BluetoothDevice
> > > > > > and add it to the list a known devices. Otherwise, retrieve the
> > > > BluetoothDevice
> > > > > > representing device.
> > > > > >   2. Add the BluetoothDevice from step 1 to connected_devices.
> > > > > > 4. Return connected_devices.
> > > > > > 
> > > > > > Note that we won't be calling DeviceAdded for new devices. I think
> > calling
> > > > > > DeviceAdded in the middle of a synchronous function will be
confusing
> > and
> > > > cause
> > > > > > bugs for clients. Furthermore DeviceAdded is only useful for OS
clients
> > that
> > > > Right, but we still have the problem of providing the same device to
clients
> > in
> > > > two different ways (through the return value of this function and
through
> > > > DeviceAdded)
> > > 
> > > This method doesn't return devices. It causes connected devices to be
> > discovered. Perhaps we can think about it as another kind of scan?
DeviceAdded
> > is the only way clients find the new devices.
> > > 
> > > I think I'm OK with that.
> > > 
> > > 
> > > 
> > > As an alternative, should GetDevices implementation discover these
devices?
> > And the Web Bluetooth for example would start by GetDevices, filter out only
the
> > ones that are connected, and add those to the chooser, then start a scan and
any
> > DeviceAdded devices get added as well.
> > 
> > 
> > The current method as is does not return devices but when I proposed the
method
> > I thought it should and I still think it should. Having a synchronous method
is
> > the easiest to use and implement:
> > 
> > 1. Android: We would use BluetoothManager.getConnectedDevices() and create
the
> > new devices if we haven't seen them before.
> > 2. macOS: We would use retrieveConnectedPeripheralsWithServices and create
the
> > new devices if we haven't seen them before.
> > 3. BlueZ: We would iterate over the currently known devices and return the
ones
> > that are connected.
> > 4. Windows: Enumerate all LE devices and return the connected ones. (In fact
I
> > think the current implementation of tying this to a Discovery session is
wrong.)
> > 
> > Clients will simply need to do:
> > 
> > for (BluetoothDevice* device : adapter->GetConnectedDevices()) {
> >   // Do something with the device.
> > }
> > 
> > Having methods that abstract bluetooth concepts or build on top of them is
very
> > annoying e.g. all the Device/Service/Characteristic/Descriptor Added/Removed
> > events, GetUUIDs, DeviceChanged, etc. add more complexity to the
implementation,
> > means clients need to understand yet another concept and all its subtleties,
and
> > sometimes result in clients not being able to implement features e.g. we had
> > many problems with the chooser because of how DeviceChanged worked and Arc++
is
> > still not able to implement advertisements because of DeviceChanged.
> > 
> > Although I agree that we should keep the style of the API, I also think that
> > whatever work we do to move clients to use the API in a way that is
compatible
> > with the eventual mojo API will save future refactoring work.
> > 
> > Overall I think exposing low level APIs reduces the API complexity, reduces
the
> > burden on clients, allows clients to implement more powerful features and
will
> > be easier to refactor.
> 
> I don't like the side-effect of GetConnectedDevices, which appears to just
return already known devices, causing new devices to be discovered on some
platforms. Not calling DeviceAdded seems wrong to me when we do discover a
device. When there are multiple clients all should be made aware when a new
device is discovered in any way. E.g. the bluetooth internals page should know
about connected devices that were newly discovered with GetConnectedDevices
called by another client.
> 

I agree that it seems wrong to not call DeviceAdded but the alternative that
clients have to call an async function for a sync operation seems worse to me.

To me, this:

BluetoothDeviceChooserController::GetDevice() {

  // Check permissions and filters

  for (BluetoothDevice* device : RetrieveConnectedPeripherals()) {
    AddFilteredDevice(device);
  }

  // Start scan.
}

Seems much easier to follow than:

BluetoothDeviceChooserController::GetDevice() {

  // Check permissions and filters

  // Connected peripherals will be added through DeviceAdded.
  RetrieveConnectedPeripherals();

  // Star Scan.

}

Additionally it avoid abstracting yet another BT concept on top of a Chrome-only
concept e.g. DeviceAdded.

> I'm not sure what a long term redesign of scanning would look like that
doesn't include something similar to DeviceAdded. But I do agree with the goal
to minimize additional concepts, while keeping common client code easy. E.g. I
think we'll want to keep GetDevices.
> 

I hope the new design won't include either DeviceAdded or GetDevices because
neither is an actual bluetooth concept and force us to implement things like
TimedOutDevices. I would prefer if we exposed AdvertisementReceived and
RetrieveConnectedPeripherals which are simple concepts that match current APIs
and are easier to implement.

> In your previous example, calling RetrieveConnectedPeripherals and then
GetDevices isn't much more work for a client, but the API has fewer surprises.
> 
> The 4 platform implementations seem to just flip-flop in complexity. Some
platforms just no-op for RetrieveConnectedPeripherals, and the others enumerate
create devices that are connected and not already tracked. Seems about the same
overall either way.

[ortuno, 2016-10-12 02:53:09]

Chatted offline with scheib and jyasskin. The conclusion was that:

 1. We should rename the function to describe what it does more accurately. My
suggestion would be RetrieveUnknownGattConnectedDevices().
 2. The function would query the platform for gatt connected devices and it
would call DeviceAdded for each device that we haven't seen before.

// bluetooth_adapter_mac.mm

void BluetoothAdapterMac::RetrieveUnknownGattConnectedDevices() {

  NSArray* peripherals = [low_energy_central_manager_
    retrieveConnectedPeripheralsWithServices:connectedServices];

  for (CBPeripheral* peripheral : peripherals) {
    
    BluetoothLowEnergyDeviceMac* device_mac =
        GetBluetoothLowEnergyDeviceMac(peripheral);
    const bool is_new_device = device_mac == nullptr;

    if (!is_new_device) {
      DoesCollideWithKnownDevice(peripheral, device);
      return;
    }
  
    std::string device_address =
      BluetoothLowEnergyDeviceMac::GetPeripheralHashAddress(peripheral);
    devices_.add(device_address, std::unique_ptr<BluetoothDevice>(device_mac));
    FOR_EACH_OBSERVER(BluetoothAdapter::Observer, observers_,
                      DeviceAdded(this, device_mac));
  }
}

// bluetooth_device_chooser_controller.cc

void BluetoothDeviceChooserController::GetDevice() {
  // ...
  PopulateKnownConnectedDevices();
  adapter_->RetrieveUnknownGattConnectedDevices();

  // Start scan.
}

[ortuno, 2016-10-12 06:42:46]

https://codereview.chromium.org/2339253002/diff/80001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_adapter_mac.mm (right):

https://codereview.chromium.org/2339253002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_adapter_mac.mm:178: CBUUID* genericAccessServiceUUID
= [CBUUID UUIDWithString:@"1800"];
I've been thinking about this and I think we need to do more on this method. As
proposed, this function will be adding new BluetoothDevices for each new device
but those devices won't have any uuids so they will probably be filtered out.
Does [CBPeripheral services] return any services that we can use to populate the
uuids member? Otherwise developers might have to retrieve all connected devices
and force users to pick the right connected device on their own D:

[jlebel, 2016-10-13 15:29:02]

https://codereview.chromium.org/2339253002/diff/80001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_adapter_mac.mm (right):

https://codereview.chromium.org/2339253002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_adapter_mac.mm:173: void
BluetoothAdapterMac::RetrievedConnectedPeripherals() {
On 2016/10/10 02:57:22, ortuno wrote:
> On 2016/10/07 at 17:08:00, scheib wrote:
> > On 2016/10/06 23:43:12, ortuno wrote:
> > > On 2016/10/06 at 21:01:34, scheib wrote:
> > > > On 2016/10/06 02:22:07, ortuno wrote:
> > > > > +scheib for thoughts. Some context: We are adding a new function to
> retrieve
> > > > > connected devices. These devices are probably not advertising and
> platforms
> > > > > require us to manually query for these.
> > > > > 
> > > > > I don't think this function need to do the lower half of
> > > LowEnergyDeviceUpdated:
> > > > > 
> > > > > const std::vector<BluetoothDevice*> RetrieveGattConnectedDevices()
> > > > > 
> > > > > 1. Initialize connected_devices to be an empty
> std::vector<BluetoothDevice*>
> > > > > 2. Query the platform for GATT connected devices
> > > > > 3. For each device in Step 2.:
> > > > >   1. If we don't know of the device already, initialize a new
> > > BluetoothDevice
> > > > > and add it to the list a known devices. Otherwise, retrieve the
> > > BluetoothDevice
> > > > > representing device.
> > > > >   2. Add the BluetoothDevice from step 1 to connected_devices.
> > > > > 4. Return connected_devices.
> > > > > 
> > > > > Note that we won't be calling DeviceAdded for new devices. I think
> calling
> > > > > DeviceAdded in the middle of a synchronous function will be confusing
> and
> > > cause
> > > > > bugs for clients. Furthermore DeviceAdded is only useful for OS
clients
> that
> > > Right, but we still have the problem of providing the same device to
clients
> in
> > > two different ways (through the return value of this function and through
> > > DeviceAdded)
> > 
> > This method doesn't return devices. It causes connected devices to be
> discovered. Perhaps we can think about it as another kind of scan? DeviceAdded
> is the only way clients find the new devices.
> > 
> > I think I'm OK with that.
> > 
> > 
> > 
> > As an alternative, should GetDevices implementation discover these devices?
> And the Web Bluetooth for example would start by GetDevices, filter out only
the
> ones that are connected, and add those to the chooser, then start a scan and
any
> DeviceAdded devices get added as well.
> 
> 
> The current method as is does not return devices but when I proposed the
method
> I thought it should and I still think it should. Having a synchronous method
is
> the easiest to use and implement:
> 
> 1. Android: We would use BluetoothManager.getConnectedDevices() and create the
> new devices if we haven't seen them before.
> 2. macOS: We would use retrieveConnectedPeripheralsWithServices and create the
> new devices if we haven't seen them before.
> 3. BlueZ: We would iterate over the currently known devices and return the
ones
> that are connected.
> 4. Windows: Enumerate all LE devices and return the connected ones. (In fact I
> think the current implementation of tying this to a Discovery session is
wrong.)
> 
> Clients will simply need to do:
> 
> for (BluetoothDevice* device : adapter->GetConnectedDevices()) {
>   // Do something with the device.
> }
> 
> Having methods that abstract bluetooth concepts or build on top of them is
very
> annoying e.g. all the Device/Service/Characteristic/Descriptor Added/Removed
> events, GetUUIDs, DeviceChanged, etc. add more complexity to the
implementation,
> means clients need to understand yet another concept and all its subtleties,
and
> sometimes result in clients not being able to implement features e.g. we had
> many problems with the chooser because of how DeviceChanged worked and Arc++
is
> still not able to implement advertisements because of DeviceChanged.
> 
> Although I agree that we should keep the style of the API, I also think that
> whatever work we do to move clients to use the API in a way that is compatible
> with the eventual mojo API will save future refactoring work.
> 
> Overall I think exposing low level APIs reduces the API complexity, reduces
the
> burden on clients, allows clients to implement more powerful features and will
> be easier to refactor.

Acknowledged.

https://codereview.chromium.org/2339253002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_adapter_mac.mm:178: CBUUID* genericAccessServiceUUID
= [CBUUID UUIDWithString:@"1800"];
On 2016/10/12 06:42:46, ortuno wrote:
> I've been thinking about this and I think we need to do more on this method.
As
> proposed, this function will be adding new BluetoothDevices for each new
device
> but those devices won't have any uuids so they will probably be filtered out.
> Does [CBPeripheral services] return any services that we can use to populate
the
> uuids member? Otherwise developers might have to retrieve all connected
devices
> and force users to pick the right connected device on their own D:

I've tried, and yes, the devices are filtered out because CBPeripheral are not
connected, and therefore, we have no informations on their services.

I guess to solve that problem we would need to give the service used to filter
to this method, and add the service to the device.

[ortuno, 2016-10-14 03:03:04]

https://codereview.chromium.org/2339253002/diff/80001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_adapter_mac.mm (right):

https://codereview.chromium.org/2339253002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_adapter_mac.mm:178: CBUUID* genericAccessServiceUUID
= [CBUUID UUIDWithString:@"1800"];
On 2016/10/13 at 15:29:01, jlebel wrote:
> On 2016/10/12 06:42:46, ortuno wrote:
> > I've been thinking about this and I think we need to do more on this method.
As
> > proposed, this function will be adding new BluetoothDevices for each new
device
> > but those devices won't have any uuids so they will probably be filtered
out.
> > Does [CBPeripheral services] return any services that we can use to populate
the
> > uuids member? Otherwise developers might have to retrieve all connected
devices
> > and force users to pick the right connected device on their own D:
> 
> I've tried, and yes, the devices are filtered out because CBPeripheral are not
connected, and therefore, we have no informations on their services.
> 
> I guess to solve that problem we would need to give the service used to filter
to this method, and add the service to the device.

To summarize the problem: Peripherals returned by retrieveConnectedPeripherals
do not include any information about their services. We need to find a way to
expose that information.

I think passing the filter to only get a set of devices that match the filters
makes sense but there are a couple of subtleties we need to consider:
  - How does retrieveConnectedPeripheralsWithServices's filters work? If a
device only has a Heart Rate Service and the filter has Glucose and Heart Rate
would the function find the device? (I might be missing something but I don't
think the documentation is clear here)
  - How do we integrate the services with the current GetUUIDs model?
  - How long do we persist these devices and their UUIDs?

I thought of the following approaches:

  1. We would call DeviceAdded for each new device, set the device's UUIDs to
the filter's UUIDs if IsGattDiscoveryComplete is false, call DeviceChanged() and
clear the UUIDs.

Pros:
 - Ensures the UUIDs are always up to date.
 - Doesn't affect old connected devices.
 - DeviceAdded and DeviceChanged allow other clients to stay up to date
regarding peripherals.
Cons:
 - Yet another concept that we pile up on GetUUIDs() D:
 - The fact that the uuids are cleared after DeviceChanged runs is very subtle.

2. Another approach would be to call DeviceAdded for each new device and if the
filter has UUIDs not in the device add them and call DeviceChanged.

Pros:
  - GetUUIDs is more stable
  - DeviceAdded and DeviceChanged allow other clients to stay up to date
regarding peripherals.
Cons:
  - Yet another concept that we pile up on GetUUIDs() D:
  - GetUUIDs could return uuids that are no longer present in the device or we
would have to implement some sort of time out for these uuids.

3. A third option would be to call DeviceAdded for each new device and then just
return all the connected devices that match the filters.

Pros:
  - Easy to implement: No need to mess with GetUUIDs() or DeviceChanged.
Cons: 
  - For some devices, we would be calling DeviceAdded and then returning the
device.
  - We won't be exposing known UUIDs through GetUUIDs() or DeviceChanged meaning
bluetooth-internals won't get notified of the device's UUIDs.

I don't think any option is ideal D: but I'm leaning towards the last approach.
Adding more things to GetUUIDs() would be very unfortunate.

jlebel, scheib: wdyt?

https://codereview.chromium.org/2339253002/diff/100001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter_mac.mm (right):

https://codereview.chromium.org/2339253002/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_mac.mm:534: if (advertisement_data) {
I might be missing something but I don't think you need this anymore?

[scheib, 2016-10-14 17:16:14]

https://codereview.chromium.org/2339253002/diff/80001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_adapter_mac.mm (right):

https://codereview.chromium.org/2339253002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_adapter_mac.mm:178: CBUUID* genericAccessServiceUUID
= [CBUUID UUIDWithString:@"1800"];
On 2016/10/14 03:03:03, ortuno wrote:
> On 2016/10/13 at 15:29:01, jlebel wrote:
> > On 2016/10/12 06:42:46, ortuno wrote:
> > > I've been thinking about this and I think we need to do more on this
method.
> As
> > > proposed, this function will be adding new BluetoothDevices for each new
> device
> > > but those devices won't have any uuids so they will probably be filtered
> out.
> > > Does [CBPeripheral services] return any services that we can use to
populate
> the
> > > uuids member? Otherwise developers might have to retrieve all connected
> devices
> > > and force users to pick the right connected device on their own D:
> > 
> > I've tried, and yes, the devices are filtered out because CBPeripheral are
not
> connected, and therefore, we have no informations on their services.
> > 
> > I guess to solve that problem we would need to give the service used to
filter
> to this method, and add the service to the device.
> 
> To summarize the problem: Peripherals returned by retrieveConnectedPeripherals
> do not include any information about their services. We need to find a way to
> expose that information.
> 
> I think passing the filter to only get a set of devices that match the filters
> makes sense but there are a couple of subtleties we need to consider:
>   - How does retrieveConnectedPeripheralsWithServices's filters work? If a
> device only has a Heart Rate Service and the filter has Glucose and Heart Rate
> would the function find the device? (I might be missing something but I don't
> think the documentation is clear here)
>   - How do we integrate the services with the current GetUUIDs model?
>   - How long do we persist these devices and their UUIDs?
> 
> I thought of the following approaches:
> 
>   1. We would call DeviceAdded for each new device, set the device's UUIDs to
> the filter's UUIDs if IsGattDiscoveryComplete is false, call DeviceChanged()
and
> clear the UUIDs.
> 
> Pros:
>  - Ensures the UUIDs are always up to date.
>  - Doesn't affect old connected devices.
>  - DeviceAdded and DeviceChanged allow other clients to stay up to date
> regarding peripherals.
> Cons:
>  - Yet another concept that we pile up on GetUUIDs() D:
>  - The fact that the uuids are cleared after DeviceChanged runs is very
subtle.
> 
> 2. Another approach would be to call DeviceAdded for each new device and if
the
> filter has UUIDs not in the device add them and call DeviceChanged.
> 
> Pros:
>   - GetUUIDs is more stable
>   - DeviceAdded and DeviceChanged allow other clients to stay up to date
> regarding peripherals.
> Cons:
>   - Yet another concept that we pile up on GetUUIDs() D:
>   - GetUUIDs could return uuids that are no longer present in the device or we
> would have to implement some sort of time out for these uuids.
> 
> 3. A third option would be to call DeviceAdded for each new device and then
just
> return all the connected devices that match the filters.
> 
> Pros:
>   - Easy to implement: No need to mess with GetUUIDs() or DeviceChanged.
> Cons: 
>   - For some devices, we would be calling DeviceAdded and then returning the
> device.
>   - We won't be exposing known UUIDs through GetUUIDs() or DeviceChanged
meaning
> bluetooth-internals won't get notified of the device's UUIDs.
> 
> I don't think any option is ideal D: but I'm leaning towards the last
approach.
> Adding more things to GetUUIDs() would be very unfortunate.
> 
> jlebel, scheib: wdyt?

:( 

I'm a tad confused on if we have the UUIDs to filter for. If we have multiple
UUIDs in a call to retrieveConnectedPeripheralsWithServices, does it return all
devices with any UUID that matches, or only those with every UUID? Either way I
don't think it helps -- we need to find devices that match any of multiple UUID
filters, but then when Web Bluetooth filters them for the chooser we need to
know more specifically which UUIDs are associated with each device.

If we can call retrieveConnectedPeripheralsWithServices with a useful set of
UUIDs, I'd suggest:

call retrieveConnectedPeripheralsWithServices
for each peripheral
 call LowEnergyDeviceUpdated with synthetic advertisementData that we construct
including the UUIDs associated with that device.
A DeviceAdded should result that includes the correct UUIDs

But, as mentioned above, perhaps that's not possible. In that case:

I wonder if it is possible to cause the services to be discovered for these
connected devices? Calling discoverServices on each one. Then that process
should complete and we can get a DeviceUpdated that has known UUIDs?

[ortuno, 2016-10-17 02:43:49]

On 2016/10/14 at 17:16:14, scheib wrote:
>
https://codereview.chromium.org/2339253002/diff/80001/device/bluetooth/blueto...
> File device/bluetooth/bluetooth_adapter_mac.mm (right):
> 
>
https://codereview.chromium.org/2339253002/diff/80001/device/bluetooth/blueto...
> device/bluetooth/bluetooth_adapter_mac.mm:178: CBUUID*
genericAccessServiceUUID = [CBUUID UUIDWithString:@"1800"];
> On 2016/10/14 03:03:03, ortuno wrote:
> > On 2016/10/13 at 15:29:01, jlebel wrote:
> > > On 2016/10/12 06:42:46, ortuno wrote:
> > > > I've been thinking about this and I think we need to do more on this
method.
> > As
> > > > proposed, this function will be adding new BluetoothDevices for each new
> > device
> > > > but those devices won't have any uuids so they will probably be filtered
> > out.
> > > > Does [CBPeripheral services] return any services that we can use to
populate
> > the
> > > > uuids member? Otherwise developers might have to retrieve all connected
> > devices
> > > > and force users to pick the right connected device on their own D:
> > > 
> > > I've tried, and yes, the devices are filtered out because CBPeripheral are
not
> > connected, and therefore, we have no informations on their services.
> > > 
> > > I guess to solve that problem we would need to give the service used to
filter
> > to this method, and add the service to the device.
> > 
> > To summarize the problem: Peripherals returned by
retrieveConnectedPeripherals
> > do not include any information about their services. We need to find a way
to
> > expose that information.
> > 
> > I think passing the filter to only get a set of devices that match the
filters
> > makes sense but there are a couple of subtleties we need to consider:
> >   - How does retrieveConnectedPeripheralsWithServices's filters work? If a
> > device only has a Heart Rate Service and the filter has Glucose and Heart
Rate
> > would the function find the device? (I might be missing something but I
don't
> > think the documentation is clear here)
> >   - How do we integrate the services with the current GetUUIDs model?
> >   - How long do we persist these devices and their UUIDs?
> > 
> > I thought of the following approaches:
> > 
> >   1. We would call DeviceAdded for each new device, set the device's UUIDs
to
> > the filter's UUIDs if IsGattDiscoveryComplete is false, call DeviceChanged()
and
> > clear the UUIDs.
> > 
> > Pros:
> >  - Ensures the UUIDs are always up to date.
> >  - Doesn't affect old connected devices.
> >  - DeviceAdded and DeviceChanged allow other clients to stay up to date
> > regarding peripherals.
> > Cons:
> >  - Yet another concept that we pile up on GetUUIDs() D:
> >  - The fact that the uuids are cleared after DeviceChanged runs is very
subtle.
> > 
> > 2. Another approach would be to call DeviceAdded for each new device and if
the
> > filter has UUIDs not in the device add them and call DeviceChanged.
> > 
> > Pros:
> >   - GetUUIDs is more stable
> >   - DeviceAdded and DeviceChanged allow other clients to stay up to date
> > regarding peripherals.
> > Cons:
> >   - Yet another concept that we pile up on GetUUIDs() D:
> >   - GetUUIDs could return uuids that are no longer present in the device or
we
> > would have to implement some sort of time out for these uuids.
> > 
> > 3. A third option would be to call DeviceAdded for each new device and then
just
> > return all the connected devices that match the filters.
> > 
> > Pros:
> >   - Easy to implement: No need to mess with GetUUIDs() or DeviceChanged.
> > Cons: 
> >   - For some devices, we would be calling DeviceAdded and then returning the
> > device.
> >   - We won't be exposing known UUIDs through GetUUIDs() or DeviceChanged
meaning
> > bluetooth-internals won't get notified of the device's UUIDs.
> > 
> > I don't think any option is ideal D: but I'm leaning towards the last
approach.
> > Adding more things to GetUUIDs() would be very unfortunate.
> > 
> > jlebel, scheib: wdyt?
> 
> :( 
> 
> I'm a tad confused on if we have the UUIDs to filter for. If we have multiple
UUIDs in a call to retrieveConnectedPeripheralsWithServices, does it return all
devices with any UUID that matches, or only those with every UUID? 

So I did some experimentation. A device needs to contain all the services passed
to retrieveConnectedPeripheralsWithServices to be returned.

> Either way I don't think it helps -- we need to find devices that match any of
multiple UUID filters, but then when Web Bluetooth filters them for the chooser
we need to know more specifically which UUIDs are associated with each device.
> 
Ah right. Please see pseudo-code below.

> If we can call retrieveConnectedPeripheralsWithServices with a useful set of
UUIDs, I'd suggest:
> 
> call retrieveConnectedPeripheralsWithServices
> for each peripheral
>  call LowEnergyDeviceUpdated with synthetic advertisementData that we
construct including the UUIDs associated with that device.
> A DeviceAdded should result that includes the correct UUIDs

If I understand correctly this would be equivalent to option 2 and has the same
drawbacks. I am particularly opposed to pilling up more things on top of
GetUUIDs. Right now the definition of what GetUUIDs returns is: Advertised UUIDs
+ Service UUIDs. Advertised UUIDs gets updated for each advertisement and get
cleared when scan is stopped. Service UUIDs are updated when services are
discovered and cleared when disconnecting. It's already a confusing concept and
I would rather we don't keep pilling up on it. There is also a lot of
maintenance burden regarding GetUUIDs() because we have to test each transition
and combination e.g. Connected with services discovered and advertisement
received, Connected with no services discovered and advertisement received, etc.

> But, as mentioned above, perhaps that's not possible. In that case:
> 
> I wonder if it is possible to cause the services to be discovered for these
connected devices? Calling discoverServices on each one. Then that process
should complete and we can get a DeviceUpdated that has known UUIDs?

It certainly is. We would have to connect and then walk down the attributes'
tree. The question then is: Who owns that connection? How long should we keep
that connection open for? We would have to add a mechanism to time out those
connections.

I still prefer Option 3 (with a slight modification) due to its simpler
implementation even though it results in an uglier API:

// bluetooth_adapter_mac.mm
std::unordered_map<BluetoothDevice*, std::unordered_set<BluetoothUUID>>
RetrieveGattConnectedDevicesWithServices(std::unique_ptr<BluetoothDiscoveryFilter>
discovery_filter) {
  std::unordered_set<BluetoothDevice*> connected_devices;

  for (const BluetoothUUID& uuid : discovery_filter->GetUUIDs()) {
    NSArray* peripherals = [low_energy_central_manager_
      retrieveConnectedPeripheralsWithServices:@[uuid]];
    for (auto* peripheral : peripherals) {
      BluetoothDevice* device = ... // Get or construct device, call DeviceAdded
if new.
      connected_devices[device].insert(uuid);
    }
  }
  return connected_devices;
}


// bluetooth_device_chooser_controller.cc
void GetDevice() {
  // Check permissions, etc.
  // ...

  for (const std::pair<BluetoothDevice*, std::unordered_set<BluetoothUUID>>&
pair : adapter_->RetrieveGattConnectedDevicesWithServices(discovery_filter)) {
    AddFilteredDevice(pair.first, pair.second);
  }

  // Start scan.
  // ...
}

[ortuno, 2016-10-17 02:46:36]

On 2016/10/17 at 02:43:49, ortuno wrote:
> On 2016/10/14 at 17:16:14, scheib wrote:
> >
https://codereview.chromium.org/2339253002/diff/80001/device/bluetooth/blueto...
> > File device/bluetooth/bluetooth_adapter_mac.mm (right):
> > 
> >
https://codereview.chromium.org/2339253002/diff/80001/device/bluetooth/blueto...
> > device/bluetooth/bluetooth_adapter_mac.mm:178: CBUUID*
genericAccessServiceUUID = [CBUUID UUIDWithString:@"1800"];
> > On 2016/10/14 03:03:03, ortuno wrote:
> > > On 2016/10/13 at 15:29:01, jlebel wrote:
> > > > On 2016/10/12 06:42:46, ortuno wrote:
> > > > > I've been thinking about this and I think we need to do more on this
method.
> > > As
> > > > > proposed, this function will be adding new BluetoothDevices for each
new
> > > device
> > > > > but those devices won't have any uuids so they will probably be
filtered
> > > out.
> > > > > Does [CBPeripheral services] return any services that we can use to
populate
> > > the
> > > > > uuids member? Otherwise developers might have to retrieve all
connected
> > > devices
> > > > > and force users to pick the right connected device on their own D:
> > > > 
> > > > I've tried, and yes, the devices are filtered out because CBPeripheral
are not
> > > connected, and therefore, we have no informations on their services.
> > > > 
> > > > I guess to solve that problem we would need to give the service used to
filter
> > > to this method, and add the service to the device.
> > > 
> > > To summarize the problem: Peripherals returned by
retrieveConnectedPeripherals
> > > do not include any information about their services. We need to find a way
to
> > > expose that information.
> > > 
> > > I think passing the filter to only get a set of devices that match the
filters
> > > makes sense but there are a couple of subtleties we need to consider:
> > >   - How does retrieveConnectedPeripheralsWithServices's filters work? If a
> > > device only has a Heart Rate Service and the filter has Glucose and Heart
Rate
> > > would the function find the device? (I might be missing something but I
don't
> > > think the documentation is clear here)
> > >   - How do we integrate the services with the current GetUUIDs model?
> > >   - How long do we persist these devices and their UUIDs?
> > > 
> > > I thought of the following approaches:
> > > 
> > >   1. We would call DeviceAdded for each new device, set the device's UUIDs
to
> > > the filter's UUIDs if IsGattDiscoveryComplete is false, call
DeviceChanged() and
> > > clear the UUIDs.
> > > 
> > > Pros:
> > >  - Ensures the UUIDs are always up to date.
> > >  - Doesn't affect old connected devices.
> > >  - DeviceAdded and DeviceChanged allow other clients to stay up to date
> > > regarding peripherals.
> > > Cons:
> > >  - Yet another concept that we pile up on GetUUIDs() D:
> > >  - The fact that the uuids are cleared after DeviceChanged runs is very
subtle.
> > > 
> > > 2. Another approach would be to call DeviceAdded for each new device and
if the
> > > filter has UUIDs not in the device add them and call DeviceChanged.
> > > 
> > > Pros:
> > >   - GetUUIDs is more stable
> > >   - DeviceAdded and DeviceChanged allow other clients to stay up to date
> > > regarding peripherals.
> > > Cons:
> > >   - Yet another concept that we pile up on GetUUIDs() D:
> > >   - GetUUIDs could return uuids that are no longer present in the device
or we
> > > would have to implement some sort of time out for these uuids.
> > > 
> > > 3. A third option would be to call DeviceAdded for each new device and
then just
> > > return all the connected devices that match the filters.
> > > 
> > > Pros:
> > >   - Easy to implement: No need to mess with GetUUIDs() or DeviceChanged.
> > > Cons: 
> > >   - For some devices, we would be calling DeviceAdded and then returning
the
> > > device.
> > >   - We won't be exposing known UUIDs through GetUUIDs() or DeviceChanged
meaning
> > > bluetooth-internals won't get notified of the device's UUIDs.
> > > 
> > > I don't think any option is ideal D: but I'm leaning towards the last
approach.
> > > Adding more things to GetUUIDs() would be very unfortunate.
> > > 
> > > jlebel, scheib: wdyt?
> > 
> > :( 
> > 
> > I'm a tad confused on if we have the UUIDs to filter for. If we have
multiple UUIDs in a call to retrieveConnectedPeripheralsWithServices, does it
return all devices with any UUID that matches, or only those with every UUID? 
> 
> So I did some experimentation. A device needs to contain all the services
passed to retrieveConnectedPeripheralsWithServices to be returned.
> 
> > Either way I don't think it helps -- we need to find devices that match any
of multiple UUID filters, but then when Web Bluetooth filters them for the
chooser we need to know more specifically which UUIDs are associated with each
device.
> > 
> Ah right. Please see pseudo-code below.
> 
> > If we can call retrieveConnectedPeripheralsWithServices with a useful set of
UUIDs, I'd suggest:
> > 
> > call retrieveConnectedPeripheralsWithServices
> > for each peripheral
> >  call LowEnergyDeviceUpdated with synthetic advertisementData that we
construct including the UUIDs associated with that device.
> > A DeviceAdded should result that includes the correct UUIDs
> 
> If I understand correctly this would be equivalent to option 2 and has the
same drawbacks. I am particularly opposed to pilling up more things on top of
GetUUIDs. Right now the definition of what GetUUIDs returns is: Advertised UUIDs
+ Service UUIDs. Advertised UUIDs gets updated for each advertisement and get
cleared when scan is stopped. Service UUIDs are updated when services are
discovered and cleared when disconnecting. It's already a confusing concept and
I would rather we don't keep pilling up on it. There is also a lot of
maintenance burden regarding GetUUIDs() because we have to test each transition
and combination e.g. Connected with services discovered and advertisement
received, Connected with no services discovered and advertisement received, etc.
> 
> > But, as mentioned above, perhaps that's not possible. In that case:
> > 
> > I wonder if it is possible to cause the services to be discovered for these
connected devices? Calling discoverServices on each one. Then that process
should complete and we can get a DeviceUpdated that has known UUIDs?
> 
> It certainly is. We would have to connect and then walk down the attributes'
tree. The question then is: Who owns that connection? How long should we keep
that connection open for? We would have to add a mechanism to time out those
connections.
> 
> I still prefer Option 3 (with a slight modification) due to its simpler
implementation even though it results in an uglier API:
> 
> // bluetooth_adapter_mac.mm
> std::unordered_map<BluetoothDevice*, std::unordered_set<BluetoothUUID>>
RetrieveGattConnectedDevicesWithServices(std::unique_ptr<BluetoothDiscoveryFilter>
discovery_filter) {
>   std::unordered_set<BluetoothDevice*> connected_devices;
> 
>   for (const BluetoothUUID& uuid : discovery_filter->GetUUIDs()) {
>     NSArray* peripherals = [low_energy_central_manager_
>       retrieveConnectedPeripheralsWithServices:@[uuid]];
>     for (auto* peripheral : peripherals) {
>       BluetoothDevice* device = ... // Get or construct device, call
DeviceAdded if new.
>       connected_devices[device].insert(uuid);
>     }
>   }
>   return connected_devices;
> }
> 
> 
> // bluetooth_device_chooser_controller.cc
> void GetDevice() {
>   // Check permissions, etc.
>   // ...
> 
>   for (const std::pair<BluetoothDevice*, std::unordered_set<BluetoothUUID>>&
pair : adapter_->RetrieveGattConnectedDevicesWithServices(discovery_filter)) {
>     AddFilteredDevice(pair.first, pair.second);
>   }
> 
>   // Start scan.
>   // ...
> }

Small correction

// bluetooth_adapter_mac.mm
std::unordered_map<BluetoothDevice*, std::unordered_set<BluetoothUUID>>
RetrieveGattConnectedDevicesWithServices(std::unique_ptr<BluetoothDiscoveryFilter>
discovery_filter) {
  std::unordered_map<BluetoothDevice*, std::unordered_set<BluetoothUUID>>
connected_devices;

  for (const BluetoothUUID& uuid : discovery_filter->GetUUIDs()) {
    NSArray* peripherals = [low_energy_central_manager_
      retrieveConnectedPeripheralsWithServices:@[uuid]];
    for (auto* peripheral : peripherals) {
      BluetoothDevice* device = ... // Get or construct device, call DeviceAdded
if new.
      connected_devices[device].insert(uuid);
    }
  }
  return connected_devices;
}


// bluetooth_device_chooser_controller.cc
void GetDevice() {
  // Check permissions, etc.
  // ...

  for (const std::pair<BluetoothDevice*, std::unordered_set<BluetoothUUID>>&
pair : adapter_->RetrieveGattConnectedDevicesWithServices(discovery_filter)) {
    AddFilteredDevice(pair.first, pair.second);
  }

  // Start scan.
  // ...
}

[scheib, 2016-10-18 04:56:16]

On 2016/10/17 02:46:36, ortuno wrote:
> On 2016/10/17 at 02:43:49, ortuno wrote:
> > On 2016/10/14 at 17:16:14, scheib wrote:
> > >
>
https://codereview.chromium.org/2339253002/diff/80001/device/bluetooth/blueto...
> > > File device/bluetooth/bluetooth_adapter_mac.mm (right):
> > > 
> > >
>
https://codereview.chromium.org/2339253002/diff/80001/device/bluetooth/blueto...
> > > device/bluetooth/bluetooth_adapter_mac.mm:178: CBUUID*
> genericAccessServiceUUID = [CBUUID UUIDWithString:@"1800"];
> > > On 2016/10/14 03:03:03, ortuno wrote:
> > > > On 2016/10/13 at 15:29:01, jlebel wrote:
> > > > > On 2016/10/12 06:42:46, ortuno wrote:
> > > > > > I've been thinking about this and I think we need to do more on this
> method.
> > > > As
> > > > > > proposed, this function will be adding new BluetoothDevices for each
> new
> > > > device
> > > > > > but those devices won't have any uuids so they will probably be
> filtered
> > > > out.
> > > > > > Does [CBPeripheral services] return any services that we can use to
> populate
> > > > the
> > > > > > uuids member? Otherwise developers might have to retrieve all
> connected
> > > > devices
> > > > > > and force users to pick the right connected device on their own D:
> > > > > 
> > > > > I've tried, and yes, the devices are filtered out because CBPeripheral
> are not
> > > > connected, and therefore, we have no informations on their services.
> > > > > 
> > > > > I guess to solve that problem we would need to give the service used
to
> filter
> > > > to this method, and add the service to the device.
> > > > 
> > > > To summarize the problem: Peripherals returned by
> retrieveConnectedPeripherals
> > > > do not include any information about their services. We need to find a
way
> to
> > > > expose that information.
> > > > 
> > > > I think passing the filter to only get a set of devices that match the
> filters
> > > > makes sense but there are a couple of subtleties we need to consider:
> > > >   - How does retrieveConnectedPeripheralsWithServices's filters work? If
a
> > > > device only has a Heart Rate Service and the filter has Glucose and
Heart
> Rate
> > > > would the function find the device? (I might be missing something but I
> don't
> > > > think the documentation is clear here)
> > > >   - How do we integrate the services with the current GetUUIDs model?
> > > >   - How long do we persist these devices and their UUIDs?
> > > > 
> > > > I thought of the following approaches:
> > > > 
> > > >   1. We would call DeviceAdded for each new device, set the device's
UUIDs
> to
> > > > the filter's UUIDs if IsGattDiscoveryComplete is false, call
> DeviceChanged() and
> > > > clear the UUIDs.
> > > > 
> > > > Pros:
> > > >  - Ensures the UUIDs are always up to date.
> > > >  - Doesn't affect old connected devices.
> > > >  - DeviceAdded and DeviceChanged allow other clients to stay up to date
> > > > regarding peripherals.
> > > > Cons:
> > > >  - Yet another concept that we pile up on GetUUIDs() D:
> > > >  - The fact that the uuids are cleared after DeviceChanged runs is very
> subtle.
> > > > 
> > > > 2. Another approach would be to call DeviceAdded for each new device and
> if the
> > > > filter has UUIDs not in the device add them and call DeviceChanged.
> > > > 
> > > > Pros:
> > > >   - GetUUIDs is more stable
> > > >   - DeviceAdded and DeviceChanged allow other clients to stay up to date
> > > > regarding peripherals.
> > > > Cons:
> > > >   - Yet another concept that we pile up on GetUUIDs() D:
> > > >   - GetUUIDs could return uuids that are no longer present in the device
> or we
> > > > would have to implement some sort of time out for these uuids.
> > > > 
> > > > 3. A third option would be to call DeviceAdded for each new device and
> then just
> > > > return all the connected devices that match the filters.
> > > > 
> > > > Pros:
> > > >   - Easy to implement: No need to mess with GetUUIDs() or DeviceChanged.
> > > > Cons: 
> > > >   - For some devices, we would be calling DeviceAdded and then returning
> the
> > > > device.
> > > >   - We won't be exposing known UUIDs through GetUUIDs() or DeviceChanged
> meaning
> > > > bluetooth-internals won't get notified of the device's UUIDs.
> > > > 
> > > > I don't think any option is ideal D: but I'm leaning towards the last
> approach.
> > > > Adding more things to GetUUIDs() would be very unfortunate.
> > > > 
> > > > jlebel, scheib: wdyt?
> > > 
> > > :( 
> > > 
> > > I'm a tad confused on if we have the UUIDs to filter for. If we have
> multiple UUIDs in a call to retrieveConnectedPeripheralsWithServices, does it
> return all devices with any UUID that matches, or only those with every UUID? 
> > 
> > So I did some experimentation. A device needs to contain all the services
> passed to retrieveConnectedPeripheralsWithServices to be returned.
> > 
> > > Either way I don't think it helps -- we need to find devices that match
any
> of multiple UUID filters, but then when Web Bluetooth filters them for the
> chooser we need to know more specifically which UUIDs are associated with each
> device.
> > > 
> > Ah right. Please see pseudo-code below.
> > 
> > > If we can call retrieveConnectedPeripheralsWithServices with a useful set
of
> UUIDs, I'd suggest:
> > > 
> > > call retrieveConnectedPeripheralsWithServices
> > > for each peripheral
> > >  call LowEnergyDeviceUpdated with synthetic advertisementData that we
> construct including the UUIDs associated with that device.
> > > A DeviceAdded should result that includes the correct UUIDs
> > 
> > If I understand correctly this would be equivalent to option 2 and has the
> same drawbacks. I am particularly opposed to pilling up more things on top of
> GetUUIDs. Right now the definition of what GetUUIDs returns is: Advertised
UUIDs
> + Service UUIDs. Advertised UUIDs gets updated for each advertisement and get
> cleared when scan is stopped. Service UUIDs are updated when services are
> discovered and cleared when disconnecting. It's already a confusing concept
and
> I would rather we don't keep pilling up on it. There is also a lot of
> maintenance burden regarding GetUUIDs() because we have to test each
transition
> and combination e.g. Connected with services discovered and advertisement
> received, Connected with no services discovered and advertisement received,
etc.
> > 
> > > But, as mentioned above, perhaps that's not possible. In that case:
> > > 
> > > I wonder if it is possible to cause the services to be discovered for
these
> connected devices? Calling discoverServices on each one. Then that process
> should complete and we can get a DeviceUpdated that has known UUIDs?
> > 
> > It certainly is. We would have to connect and then walk down the attributes'
> tree. The question then is: Who owns that connection? How long should we keep
> that connection open for? We would have to add a mechanism to time out those
> connections.
> > 
> > I still prefer Option 3 (with a slight modification) due to its simpler
> implementation even though it results in an uglier API:
> > 
> > // bluetooth_adapter_mac.mm
> > std::unordered_map<BluetoothDevice*, std::unordered_set<BluetoothUUID>>
>
RetrieveGattConnectedDevicesWithServices(std::unique_ptr<BluetoothDiscoveryFilter>
> discovery_filter) {
> >   std::unordered_set<BluetoothDevice*> connected_devices;
> > 
> >   for (const BluetoothUUID& uuid : discovery_filter->GetUUIDs()) {
> >     NSArray* peripherals = [low_energy_central_manager_
> >       retrieveConnectedPeripheralsWithServices:@[uuid]];
> >     for (auto* peripheral : peripherals) {
> >       BluetoothDevice* device = ... // Get or construct device, call
> DeviceAdded if new.
> >       connected_devices[device].insert(uuid);
> >     }
> >   }
> >   return connected_devices;
> > }
> > 
> > 
> > // bluetooth_device_chooser_controller.cc
> > void GetDevice() {
> >   // Check permissions, etc.
> >   // ...
> > 
> >   for (const std::pair<BluetoothDevice*, std::unordered_set<BluetoothUUID>>&
> pair : adapter_->RetrieveGattConnectedDevicesWithServices(discovery_filter)) {
> >     AddFilteredDevice(pair.first, pair.second);
> >   }
> > 
> >   // Start scan.
> >   // ...
> > }
> 
> Small correction
> 
> // bluetooth_adapter_mac.mm
> std::unordered_map<BluetoothDevice*, std::unordered_set<BluetoothUUID>>
>
RetrieveGattConnectedDevicesWithServices(std::unique_ptr<BluetoothDiscoveryFilter>
> discovery_filter) {
>   std::unordered_map<BluetoothDevice*, std::unordered_set<BluetoothUUID>>
> connected_devices;
> 
>   for (const BluetoothUUID& uuid : discovery_filter->GetUUIDs()) {
>     NSArray* peripherals = [low_energy_central_manager_
>       retrieveConnectedPeripheralsWithServices:@[uuid]];
>     for (auto* peripheral : peripherals) {
>       BluetoothDevice* device = ... // Get or construct device, call
DeviceAdded
> if new.
>       connected_devices[device].insert(uuid);
>     }
>   }
>   return connected_devices;
> }
> 
> 
> // bluetooth_device_chooser_controller.cc
> void GetDevice() {
>   // Check permissions, etc.
>   // ...
> 
>   for (const std::pair<BluetoothDevice*, std::unordered_set<BluetoothUUID>>&
> pair : adapter_->RetrieveGattConnectedDevicesWithServices(discovery_filter)) {
>     AddFilteredDevice(pair.first, pair.second);
>   }
> 
>   // Start scan.
>   // ...
> }

I'm OK with option 3, and the sketch ortuno just laid out. It certainly is a
pragmatic solution to getting what we need as directly as possible.

I've redacted here me discussing more about option 2. Let's skip that and just
go with this sketch.

[jlebel, 2016-10-19 20:25:22]

Hello Giovani,

I think there is a mistake:
BluetoothDeviceChooserController::AddFilteredDevice() takes only one parameter.
Is there a point std::unordered_set<BluetoothUUID>> to give it
AddFilteredDevice()?

Also I'm not able to compile that because of this error:
error: implicit instantiation of undefined template
'std::__1::hash<device::BluetoothUUID>'
This is related to:
std::unordered_map<BluetoothDevice*, std::unordered_set<BluetoothUUID>>

Thanks,

[ortuno, 2016-10-19 23:24:29]

On 2016/10/19 at 20:25:22, jlebel wrote:
> Hello Giovani,
> 
> I think there is a mistake:
BluetoothDeviceChooserController::AddFilteredDevice() takes only one parameter.
Is there a point std::unordered_set<BluetoothUUID>> to give it
AddFilteredDevice()?
> 

Right, we need to change AddFilteredDevice to take another parameter. I think we
should do it in another patch since we will need to add tests and modify other
tests.

We need to pass the information about the device's UUIDs to filtered device and
that parameter is how we can do it.

> Also I'm not able to compile that because of this error:
> error: implicit instantiation of undefined template
'std::__1::hash<device::BluetoothUUID>'
> This is related to:
> std::unordered_map<BluetoothDevice*, std::unordered_set<BluetoothUUID>>
> 

You can use UUIDSet:
https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_device.h?q=UU...

Also please make sure we have thorough tests for this :)

> Thanks,

[jlebel, 2016-10-26 13:48:32]

Patchset #6 (id:120001) has been deleted

[jlebel, 2016-10-26 17:01:19]

Patchset #6 (id:140001) has been deleted

[jlebel, 2016-10-26 17:11:03]

Thanks for your help, Giovanni!

Let me know what you think about my implementation and if you think I need more
tests.

Jérôme,

[scheib, 2016-10-28 20:35:50]

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

https://codereview.chromium.org/2339253002/diff/160001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter.h:321: // This function get all the connected
peripherals into |devices_|.
Let's fix up comments some. First, GetDevices() is currently documented as
returning all connected and paired devices. We need to explain on that method
why that may not be true and that this method is needed.

This method should then explain that it returns connected devices and makes them
known to GetDevices. e.g. here something like:

Returns currently connected peripherals and adds them to known devices returned
by GetDevices().

[ortuno, 2016-10-31 04:30:51]

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

https://codereview.chromium.org/2339253002/diff/160001/content/browser/blueto...
content/browser/bluetooth/bluetooth_device_chooser_controller.cc:339:
std::unique_ptr<device::BluetoothDiscoveryFilter> discovery_filter =
Let's not change web bluetooth yet. A change in web bluetooth requires new
layout tests and mocks which I think is too much for this patch.

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

https://codereview.chromium.org/2339253002/diff/160001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_mac.h:189: // Returns true if a new device
collide with an existing device.
s/collide/collides/

https://codereview.chromium.org/2339253002/diff/160001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter_mac.mm (right):

https://codereview.chromium.org/2339253002/diff/160001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_mac.mm:192: DCHECK(discovery_filter);
Any reason why this is a pointer rather than a const reference? If you are
DCHECKing might as well make it a const reference.

https://codereview.chromium.org/2339253002/diff/160001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_mac.mm:197: if (uuids.empty()) {
Assuming you change RetrieveGattConnectedDevicesWithService to just return
devices.

if (uuids.empty()) {
  for (BluetoothDevice* device :
RetrieveGattConnectedDevicesWithService(nullptr)) {
    connected_devices[device] = BluetoothDevice::UUIDSet();
  }
  return connected_devices;
}

https://codereview.chromium.org/2339253002/diff/160001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_mac.mm:204: for (auto iterator =
devices.begin(); iterator != devices.end();
Assuming you change RetrieveGattConnectedDevicesWithService to just return
devices:


for (const BluetoothUUID& uuid : uuids) {
  for (BluetoothDevice* device : RetrieveGattConnectedDevicesWithService(&uuid))
{
    connected_devices[device].insert(uuid);
  }
}

https://codereview.chromium.org/2339253002/diff/160001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_mac.mm:536: if (advertisement_data) {
I don't think you need this if statement anymore.

https://codereview.chromium.org/2339253002/diff/160001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_mac.mm:599:
std::unordered_map<BluetoothDevice*, BluetoothDevice::UUIDSet>
Why does this method return a map of devices to the UUID that we passed to this
method? I don't think you need to return the UUID. An array of devices should be
enough.

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

https://codereview.chromium.org/2339253002/diff/160001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_unittest.cc:890: EXPECT_EQ(2u,
adapter_->GetDevices().size());
Also check that the return value is correct:

std::unordered_map<BluetoothDevice*, BluetoothDevice::UUIDSet> result =
adapter_->RetrieveGattConnectedDevicesWithDiscoveryFilter(...);

EXPECT_EQ(2u, result.size());
for (const auto& pair : result) {
  EXPECT_TRUE(adapter->GetDevice(pair->first->GetDeviceAddress()));
  EXPECT_TRUE(pair->second.empty());
}

https://codereview.chromium.org/2339253002/diff/160001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_unittest.cc:915: EXPECT_EQ(1u,
adapter_->GetDevices().size());
Same here, please test the return value is correct:

EXPECT_EQ(1u, result.size());
for (const auto& pair : result) {
  EXPECT_EQ("...", pair->first->GetDeviceAddress());
  EXPECT_TRUE(adapter->GetDevice(pair->first->GetDeviceAddress());
 
EXPECT_EQ(BluetoothDevice::UUIDSet({device::BluetoothUUID(kTestHeartRateService)}),
pair->second);
}

https://codereview.chromium.org/2339253002/diff/160001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_unittest.cc:937:
adapter_->RetrieveGattConnectedDevicesWithDiscoveryFilter(&discovery_filter);
Add:

EXPECT_TRUE(result.empty());

https://codereview.chromium.org/2339253002/diff/160001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_unittest.cc:942: #endif  //
defined(OS_MACOSX)
Please add the following tests:

Filter with more than one UUID -> Test 1. A device that has only one of the
UUIDs is found and 2. that a device with both of the uuids is found and the
result has both the UUIDs.
RetrieveGattConnectedDevicesWithDiscoveryFilter called twice -> Tests we don't
create a new device.

https://codereview.chromium.org/2339253002/diff/160001/device/bluetooth/test/...
File device/bluetooth/test/bluetooth_test_mac.h (right):

https://codereview.chromium.org/2339253002/diff/160001/device/bluetooth/test/...
device/bluetooth/test/bluetooth_test_mac.h:81: void
SimulateConnectedLowEnergyDevice(
Please add this function to bluetooth_test.h and add a more detailed comment to
it. Similar to SimulateLowEnergyDevice.

https://codereview.chromium.org/2339253002/diff/160001/device/bluetooth/test/...
device/bluetooth/test/bluetooth_test_mac.h:83: const BluetoothDevice::UUIDSet&
service_uuids);
I like the current pattern of just specifying a single value rather than having
to specify name, identifier, uuids, etc. but I dislike that we pass in a number.
Could you please use an enum class instead and just take the enum.

enum class ConnectedDeviceType {
  GENERIC_DEVICE,
  HEART_RATE_DEVICE
}

SimulateLowEnergyDevice(ConnectedDeviceType type);

https://codereview.chromium.org/2339253002/diff/160001/device/bluetooth/test/...
File device/bluetooth/test/mock_bluetooth_central_manager_mac.h (right):

https://codereview.chromium.org/2339253002/diff/160001/device/bluetooth/test/...
device/bluetooth/test/mock_bluetooth_central_manager_mac.h:35: -
(void)setConnectedMockPeripheral:(CBPeripheral*)peripheral
Comment please.

https://codereview.chromium.org/2339253002/diff/160001/device/bluetooth/test/...
File device/bluetooth/test/mock_bluetooth_central_manager_mac.mm (right):

https://codereview.chromium.org/2339253002/diff/160001/device/bluetooth/test/...
device/bluetooth/test/mock_bluetooth_central_manager_mac.mm:70: -
(NSArray*)retrieveConnectedPeripheralsWithServices:(NSArray*)services {
Save the services used in this function. That way you can test that this
function is called for all services in the filter.

[jlebel, 2016-11-07 01:40:30]

Patchset #7 (id:180001) has been deleted

[jlebel, 2016-11-07 01:43:18]

Thanks for your input and your help!

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

https://codereview.chromium.org/2339253002/diff/160001/content/browser/blueto...
content/browser/bluetooth/bluetooth_device_chooser_controller.cc:339:
std::unique_ptr<device::BluetoothDiscoveryFilter> discovery_filter =
On 2016/10/31 04:30:51, ortuno wrote:
> Let's not change web bluetooth yet. A change in web bluetooth requires new
> layout tests and mocks which I think is too much for this patch.

Are you suggesting to add this code in a next patch?

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

https://codereview.chromium.org/2339253002/diff/160001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter.h:321: // This function get all the connected
peripherals into |devices_|.
On 2016/10/28 20:35:50, scheib wrote:
> Let's fix up comments some. First, GetDevices() is currently documented as
> returning all connected and paired devices. We need to explain on that method
> why that may not be true and that this method is needed.
> 
> This method should then explain that it returns connected devices and makes
them
> known to GetDevices. e.g. here something like:
> 
> Returns currently connected peripherals and adds them to known devices
returned
> by GetDevices().

Done.

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

https://codereview.chromium.org/2339253002/diff/160001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_mac.h:189: // Returns true if a new device
collide with an existing device.
On 2016/10/31 04:30:51, ortuno wrote:
> s/collide/collides/

Done.

https://codereview.chromium.org/2339253002/diff/160001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter_mac.mm (right):

https://codereview.chromium.org/2339253002/diff/160001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_mac.mm:192: DCHECK(discovery_filter);
On 2016/10/31 04:30:51, ortuno wrote:
> Any reason why this is a pointer rather than a const reference? If you are
> DCHECKing might as well make it a const reference.

Done.

https://codereview.chromium.org/2339253002/diff/160001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_mac.mm:197: if (uuids.empty()) {
On 2016/10/31 04:30:51, ortuno wrote:
> Assuming you change RetrieveGattConnectedDevicesWithService to just return
> devices.
> 
> if (uuids.empty()) {
>   for (BluetoothDevice* device :
> RetrieveGattConnectedDevicesWithService(nullptr)) {
>     connected_devices[device] = BluetoothDevice::UUIDSet();
>   }
>   return connected_devices;
> }

Done.

https://codereview.chromium.org/2339253002/diff/160001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_mac.mm:204: for (auto iterator =
devices.begin(); iterator != devices.end();
On 2016/10/31 04:30:51, ortuno wrote:
> Assuming you change RetrieveGattConnectedDevicesWithService to just return
> devices:
> 
> 
> for (const BluetoothUUID& uuid : uuids) {
>   for (BluetoothDevice* device :
RetrieveGattConnectedDevicesWithService(&uuid))
> {
>     connected_devices[device].insert(uuid);
>   }
> }

Done.

https://codereview.chromium.org/2339253002/diff/160001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_mac.mm:536: if (advertisement_data) {
On 2016/10/31 04:30:51, ortuno wrote:
> I don't think you need this if statement anymore.

Done.

https://codereview.chromium.org/2339253002/diff/160001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_mac.mm:599:
std::unordered_map<BluetoothDevice*, BluetoothDevice::UUIDSet>
On 2016/10/31 04:30:51, ortuno wrote:
> Why does this method return a map of devices to the UUID that we passed to
this
> method? I don't think you need to return the UUID. An array of devices should
be
> enough.

Done.

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

https://codereview.chromium.org/2339253002/diff/160001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_unittest.cc:890: EXPECT_EQ(2u,
adapter_->GetDevices().size());
On 2016/10/31 04:30:51, ortuno wrote:
> Also check that the return value is correct:
> 
> std::unordered_map<BluetoothDevice*, BluetoothDevice::UUIDSet> result =
> adapter_->RetrieveGattConnectedDevicesWithDiscoveryFilter(...);
> 
> EXPECT_EQ(2u, result.size());
> for (const auto& pair : result) {
>   EXPECT_TRUE(adapter->GetDevice(pair->first->GetDeviceAddress()));
>   EXPECT_TRUE(pair->second.empty());
> }

Done.

https://codereview.chromium.org/2339253002/diff/160001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_unittest.cc:915: EXPECT_EQ(1u,
adapter_->GetDevices().size());
On 2016/10/31 04:30:51, ortuno wrote:
> Same here, please test the return value is correct:
> 
> EXPECT_EQ(1u, result.size());
> for (const auto& pair : result) {
>   EXPECT_EQ("...", pair->first->GetDeviceAddress());
>   EXPECT_TRUE(adapter->GetDevice(pair->first->GetDeviceAddress());
>  
>
EXPECT_EQ(BluetoothDevice::UUIDSet({device::BluetoothUUID(kTestHeartRateService)}),
> pair->second);
> }

Done.

https://codereview.chromium.org/2339253002/diff/160001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_unittest.cc:937:
adapter_->RetrieveGattConnectedDevicesWithDiscoveryFilter(&discovery_filter);
On 2016/10/31 04:30:51, ortuno wrote:
> Add:
> 
> EXPECT_TRUE(result.empty());

Done.

https://codereview.chromium.org/2339253002/diff/160001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_unittest.cc:942: #endif  //
defined(OS_MACOSX)
On 2016/10/31 04:30:51, ortuno wrote:
> Please add the following tests:
> 
> Filter with more than one UUID -> Test 1. A device that has only one of the
> UUIDs is found and 2. that a device with both of the uuids is found and the
> result has both the UUIDs.
> RetrieveGattConnectedDevicesWithDiscoveryFilter called twice -> Tests we don't
> create a new device.

Done.

https://codereview.chromium.org/2339253002/diff/160001/device/bluetooth/test/...
File device/bluetooth/test/bluetooth_test_mac.h (right):

https://codereview.chromium.org/2339253002/diff/160001/device/bluetooth/test/...
device/bluetooth/test/bluetooth_test_mac.h:81: void
SimulateConnectedLowEnergyDevice(
On 2016/10/31 04:30:51, ortuno wrote:
> Please add this function to bluetooth_test.h and add a more detailed comment
to
> it. Similar to SimulateLowEnergyDevice.

Done.

https://codereview.chromium.org/2339253002/diff/160001/device/bluetooth/test/...
device/bluetooth/test/bluetooth_test_mac.h:83: const BluetoothDevice::UUIDSet&
service_uuids);
On 2016/10/31 04:30:51, ortuno wrote:
> I like the current pattern of just specifying a single value rather than
having
> to specify name, identifier, uuids, etc. but I dislike that we pass in a
number.
> Could you please use an enum class instead and just take the enum.
> 
> enum class ConnectedDeviceType {
>   GENERIC_DEVICE,
>   HEART_RATE_DEVICE
> }
> 
> SimulateLowEnergyDevice(ConnectedDeviceType type);

Done.

https://codereview.chromium.org/2339253002/diff/160001/device/bluetooth/test/...
File device/bluetooth/test/mock_bluetooth_central_manager_mac.h (right):

https://codereview.chromium.org/2339253002/diff/160001/device/bluetooth/test/...
device/bluetooth/test/mock_bluetooth_central_manager_mac.h:35: -
(void)setConnectedMockPeripheral:(CBPeripheral*)peripheral
On 2016/10/31 04:30:51, ortuno wrote:
> Comment please.

Done.

https://codereview.chromium.org/2339253002/diff/160001/device/bluetooth/test/...
File device/bluetooth/test/mock_bluetooth_central_manager_mac.mm (right):

https://codereview.chromium.org/2339253002/diff/160001/device/bluetooth/test/...
device/bluetooth/test/mock_bluetooth_central_manager_mac.mm:70: -
(NSArray*)retrieveConnectedPeripheralsWithServices:(NSArray*)services {
On 2016/10/31 04:30:51, ortuno wrote:
> Save the services used in this function. That way you can test that this
> function is called for all services in the filter.

Done.

[ortuno, 2016-11-07 03:12:34]

Almost there! Thanks for sticking with it!

https://codereview.chromium.org/2339253002/diff/200001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_device_chooser_controller.cc (right):

https://codereview.chromium.org/2339253002/diff/200001/content/browser/blueto...
content/browser/bluetooth/bluetooth_device_chooser_controller.cc:339:
std::unique_ptr<device::BluetoothDiscoveryFilter> discovery_filter =
yes, please remove this code. We will do it in a separate patch.

https://codereview.chromium.org/2339253002/diff/200001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter_mac.mm (right):

https://codereview.chromium.org/2339253002/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_mac.mm:609: #pragma clang diagnostic ignored
"-Wpartial-availability"
nit: I think we can remove this now.

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

https://codereview.chromium.org/2339253002/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_unittest.cc:891: BluetoothDevice::UUIDSet
service_uuids;
nit: you don't need insert anymore. You can do:

BluetoothDevice::UUIDSet service_uuids =
{BluetoothUUID(kTestUUIDGenericAccess)};

or just:

EXPECT_EQ(BluetoothDevice::UUIDSet({BluetoothUUID(kTestUUIDGenericAccess)}),
          RetrieveConnectedPeripheralServiceUUIDs());

Note that the expected value comes first in EXPECT_EQ.

https://codereview.chromium.org/2339253002/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_unittest.cc:926:
EXPECT_EQ(RetrieveConnectedPeripheralServiceUUIDs(), service_uuids);
nit: Same as above

BluetoothDevice::UUIDSet service_uuids =
{BluetoothUUID(kTestUUIDGenericAccess)};

or just:

EXPECT_EQ(BluetoothDevice::UUIDSet({BluetoothUUID(kTestUUIDGenericAccess)}),
          RetrieveConnectedPeripheralServiceUUIDs());

Note that the expected value comes first in EXPECT_EQ.

https://codereview.chromium.org/2339253002/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_unittest.cc:944:
discovery_filter.AddUUID(device::BluetoothUUID("1002"));
nit: let's use one of the existing test uuids like kTestUUIDLinkLoss.
https://cs.chromium.org/chromium/src/device/bluetooth/test/bluetooth_test.h?q...

https://codereview.chromium.org/2339253002/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_unittest.cc:952:
EXPECT_EQ(RetrieveConnectedPeripheralServiceUUIDs(), service_uuids);
nit: same as above: Use initializer lists

https://codereview.chromium.org/2339253002/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_unittest.cc:983: if (pair.first->GetAddress()
== "02:00:00:8B:74:63") {
Use the constants in:
https://cs.chromium.org/chromium/src/device/bluetooth/test/bluetooth_test.cc?...

https://codereview.chromium.org/2339253002/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_unittest.cc:987: EXPECT_EQ(service_uuids,
pair.second);
nit: Either:

BluetoothDevice::UUIDSet = service_uuids = {heart_service_uuid,
generic_service_uuid};

or

EXPECT_EQ(BluetoothDevice::UUIDSet({heart_service_uuid, generic_service_uuid}),
          pair.second);

https://codereview.chromium.org/2339253002/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_unittest.cc:988: } else if
(pair.first->GetAddress() == "01:00:00:90:1E:BE") {
Same here, use constants in:
https://cs.chromium.org/chromium/src/device/bluetooth/test/bluetooth_test.cc?...

https://codereview.chromium.org/2339253002/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_unittest.cc:1000:
EXPECT_EQ(RetrieveConnectedPeripheralServiceUUIDs(), service_uuids);
Same here, use initializer lists.

https://codereview.chromium.org/2339253002/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_unittest.cc:1005: // Simulate two devices
being connected before starting discovery session with
nit: fix description.

https://codereview.chromium.org/2339253002/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_unittest.cc:1027:
EXPECT_EQ("02:00:00:8B:74:63", pair.first->GetAddress());
nit: use constants
https://cs.chromium.org/chromium/src/device/bluetooth/test/bluetooth_test.cc?...

https://codereview.chromium.org/2339253002/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_unittest.cc:1033:
EXPECT_EQ(RetrieveConnectedPeripheralServiceUUIDs(), service_uuids);
nit: expected value comes first and use initializer lists.

https://codereview.chromium.org/2339253002/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_unittest.cc:1041:
EXPECT_EQ("02:00:00:8B:74:63", pair.first->GetAddress());
Same here, use already existing constants.

https://codereview.chromium.org/2339253002/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_unittest.cc:1045:
EXPECT_EQ(RetrieveConnectedPeripheralServiceUUIDs(), service_uuids);
nit: expected value comes first.

https://codereview.chromium.org/2339253002/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_unittest.cc:1047: EXPECT_EQ(1,
observer.device_added_count());
Test this after the first call to RetrieveConnectedPeripherals then before the
second call clear the counters and test that the device_added_count is 0 after
the second call.

https://codereview.chromium.org/2339253002/diff/200001/device/bluetooth/test/...
File device/bluetooth/test/bluetooth_test_mac.mm (right):

https://codereview.chromium.org/2339253002/diff/200001/device/bluetooth/test/...
device/bluetooth/test/bluetooth_test_mac.mm:260:
service_uuids.insert(device::BluetoothUUID(kTestUUIDGenericAccess));
q: To avoid having to loop over this set, could you do:

NSSet* service_uuids;

service_uuids = [
  NSSet setWithObjects:@[
    [CBUUID UUIDWithString:@(kTestUUIDGenericAccess.c_str())]
  ]];

?

[jlebel, 2016-11-07 05:53:10]

Patchset #8 (id:220001) has been deleted

[jlebel, 2016-11-07 05:54:25]

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

[commit-bot: I haz the power, 2016-11-07 05:54:37]

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

[jlebel, 2016-11-07 05:56:39]

Thanks for your time.

https://codereview.chromium.org/2339253002/diff/200001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_device_chooser_controller.cc (right):

https://codereview.chromium.org/2339253002/diff/200001/content/browser/blueto...
content/browser/bluetooth/bluetooth_device_chooser_controller.cc:339:
std::unique_ptr<device::BluetoothDiscoveryFilter> discovery_filter =
On 2016/11/07 03:12:33, ortuno wrote:
> yes, please remove this code. We will do it in a separate patch.

Done.

https://codereview.chromium.org/2339253002/diff/200001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter_mac.mm (right):

https://codereview.chromium.org/2339253002/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_mac.mm:609: #pragma clang diagnostic ignored
"-Wpartial-availability"
On 2016/11/07 03:12:33, ortuno wrote:
> nit: I think we can remove this now.

Ah yes!

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

https://codereview.chromium.org/2339253002/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_unittest.cc:891: BluetoothDevice::UUIDSet
service_uuids;
On 2016/11/07 03:12:33, ortuno wrote:
> nit: you don't need insert anymore. You can do:
> 
> BluetoothDevice::UUIDSet service_uuids =
> {BluetoothUUID(kTestUUIDGenericAccess)};
> 
> or just:
> 
> EXPECT_EQ(BluetoothDevice::UUIDSet({BluetoothUUID(kTestUUIDGenericAccess)}),
>           RetrieveConnectedPeripheralServiceUUIDs());
> 
> Note that the expected value comes first in EXPECT_EQ.

Done.

https://codereview.chromium.org/2339253002/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_unittest.cc:926:
EXPECT_EQ(RetrieveConnectedPeripheralServiceUUIDs(), service_uuids);
On 2016/11/07 03:12:34, ortuno wrote:
> nit: Same as above
> 
> BluetoothDevice::UUIDSet service_uuids =
> {BluetoothUUID(kTestUUIDGenericAccess)};
> 
> or just:
> 
> EXPECT_EQ(BluetoothDevice::UUIDSet({BluetoothUUID(kTestUUIDGenericAccess)}),
>           RetrieveConnectedPeripheralServiceUUIDs());
> 
> Note that the expected value comes first in EXPECT_EQ.

Done.

https://codereview.chromium.org/2339253002/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_unittest.cc:944:
discovery_filter.AddUUID(device::BluetoothUUID("1002"));
On 2016/11/07 03:12:33, ortuno wrote:
> nit: let's use one of the existing test uuids like kTestUUIDLinkLoss.
>
https://cs.chromium.org/chromium/src/device/bluetooth/test/bluetooth_test.h?q...

Done.

https://codereview.chromium.org/2339253002/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_unittest.cc:952:
EXPECT_EQ(RetrieveConnectedPeripheralServiceUUIDs(), service_uuids);
On 2016/11/07 03:12:33, ortuno wrote:
> nit: same as above: Use initializer lists

Done.

https://codereview.chromium.org/2339253002/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_unittest.cc:983: if (pair.first->GetAddress()
== "02:00:00:8B:74:63") {
On 2016/11/07 03:12:34, ortuno wrote:
> Use the constants in:
>
https://cs.chromium.org/chromium/src/device/bluetooth/test/bluetooth_test.cc?...

Done.

https://codereview.chromium.org/2339253002/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_unittest.cc:987: EXPECT_EQ(service_uuids,
pair.second);
On 2016/11/07 03:12:33, ortuno wrote:
> nit: Either:
> 
> BluetoothDevice::UUIDSet = service_uuids = {heart_service_uuid,
> generic_service_uuid};
> 
> or
> 
> EXPECT_EQ(BluetoothDevice::UUIDSet({heart_service_uuid,
generic_service_uuid}),
>           pair.second);

Done.

https://codereview.chromium.org/2339253002/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_unittest.cc:988: } else if
(pair.first->GetAddress() == "01:00:00:90:1E:BE") {
On 2016/11/07 03:12:33, ortuno wrote:
> Same here, use constants in:
>
https://cs.chromium.org/chromium/src/device/bluetooth/test/bluetooth_test.cc?...

Done.

https://codereview.chromium.org/2339253002/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_unittest.cc:1000:
EXPECT_EQ(RetrieveConnectedPeripheralServiceUUIDs(), service_uuids);
On 2016/11/07 03:12:33, ortuno wrote:
> Same here, use initializer lists.

Done.

https://codereview.chromium.org/2339253002/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_unittest.cc:1005: // Simulate two devices
being connected before starting discovery session with
On 2016/11/07 03:12:34, ortuno wrote:
> nit: fix description.

Done.

https://codereview.chromium.org/2339253002/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_unittest.cc:1027:
EXPECT_EQ("02:00:00:8B:74:63", pair.first->GetAddress());
On 2016/11/07 03:12:34, ortuno wrote:
> nit: use constants
>
https://cs.chromium.org/chromium/src/device/bluetooth/test/bluetooth_test.cc?...

Done.

https://codereview.chromium.org/2339253002/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_unittest.cc:1033:
EXPECT_EQ(RetrieveConnectedPeripheralServiceUUIDs(), service_uuids);
On 2016/11/07 03:12:34, ortuno wrote:
> nit: expected value comes first and use initializer lists.

Done.

https://codereview.chromium.org/2339253002/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_unittest.cc:1041:
EXPECT_EQ("02:00:00:8B:74:63", pair.first->GetAddress());
On 2016/11/07 03:12:34, ortuno wrote:
> Same here, use already existing constants.

Done.

https://codereview.chromium.org/2339253002/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_unittest.cc:1045:
EXPECT_EQ(RetrieveConnectedPeripheralServiceUUIDs(), service_uuids);
On 2016/11/07 03:12:34, ortuno wrote:
> nit: expected value comes first.

Done.

https://codereview.chromium.org/2339253002/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_unittest.cc:1047: EXPECT_EQ(1,
observer.device_added_count());
On 2016/11/07 03:12:33, ortuno wrote:
> Test this after the first call to RetrieveConnectedPeripherals then before the
> second call clear the counters and test that the device_added_count is 0 after
> the second call.

Done.

https://codereview.chromium.org/2339253002/diff/200001/device/bluetooth/test/...
File device/bluetooth/test/bluetooth_test_mac.mm (right):

https://codereview.chromium.org/2339253002/diff/200001/device/bluetooth/test/...
device/bluetooth/test/bluetooth_test_mac.mm:260:
service_uuids.insert(device::BluetoothUUID(kTestUUIDGenericAccess));
On 2016/11/07 03:12:34, ortuno wrote:
> q: To avoid having to loop over this set, could you do:
> 
> NSSet* service_uuids;
> 
> service_uuids = [
>   NSSet setWithObjects:@[
>     [CBUUID UUIDWithString:@(kTestUUIDGenericAccess.c_str())]
>   ]];
> 
> ?

Yes, of course!

[ortuno, 2016-11-07 06:03:00]

lgtm! Thanks!!

[commit-bot: I haz the power, 2016-11-07 07:13:29]

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

[commit-bot: I haz the power, 2016-11-07 07:13:30]

Dry run: This issue passed the CQ dry run.

[jlebel, 2016-11-07 07:14:23]

The CQ bit was checked by jlebel@chromium.org

[commit-bot: I haz the power, 2016-11-07 07:14:38]

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-11-07 07:18:54]

Description was changed from

==========
bluetooth: mac: add connected LE devices to chooser

On a mac, when showing list of available low energy devices, low energy devices
which are connected are not shown.

To get the list of devices, -[CBCentralManager
scanForPeripheralsWithServices:options:] is used. This method reports only
advertising devices.

For devices that are connected but doesn't advertise, -[CBCentralManager
retrieveConnectedPeripheralsWithServices:] needs to be called when starting a
discovery session.

BUG=630581
==========

to

==========
bluetooth: mac: add connected LE devices to chooser

On a mac, when showing list of available low energy devices, low energy devices
which are connected are not shown.

To get the list of devices, -[CBCentralManager
scanForPeripheralsWithServices:options:] is used. This method reports only
advertising devices.

For devices that are connected but doesn't advertise, -[CBCentralManager
retrieveConnectedPeripheralsWithServices:] needs to be called when starting a
discovery session.

BUG=630581
==========

[commit-bot: I haz the power, 2016-11-07 07:18:57]

Committed patchset #8 (id:240001)

[commit-bot: I haz the power, 2016-11-07 07:20:30]

Description was changed from

==========
bluetooth: mac: add connected LE devices to chooser

On a mac, when showing list of available low energy devices, low energy devices
which are connected are not shown.

To get the list of devices, -[CBCentralManager
scanForPeripheralsWithServices:options:] is used. This method reports only
advertising devices.

For devices that are connected but doesn't advertise, -[CBCentralManager
retrieveConnectedPeripheralsWithServices:] needs to be called when starting a
discovery session.

BUG=630581
==========

to

==========
bluetooth: mac: add connected LE devices to chooser

On a mac, when showing list of available low energy devices, low energy devices
which are connected are not shown.

To get the list of devices, -[CBCentralManager
scanForPeripheralsWithServices:options:] is used. This method reports only
advertising devices.

For devices that are connected but doesn't advertise, -[CBCentralManager
retrieveConnectedPeripheralsWithServices:] needs to be called when starting a
discovery session.

BUG=630581

Committed: https://crrev.com/ff8c749c2d358b34184d156fb0939a14e264438f
Cr-Commit-Position: refs/heads/master@{#430229}
==========

[commit-bot: I haz the power, 2016-11-07 07:20:32]

Patchset 8 (id:??) landed as
https://crrev.com/ff8c749c2d358b34184d156fb0939a14e264438f
Cr-Commit-Position: refs/heads/master@{#430229}

[jracle (use Gerrit), 2016-11-07 09:20:18]

On 2016/11/07 07:20:32, commit-bot: I haz the power wrote:
> Patchset 8 (id:??) landed as
> https://crrev.com/ff8c749c2d358b34184d156fb0939a14e264438f
> Cr-Commit-Position: refs/heads/master@{#430229}

Thanks a lot guys! I'll give a try soon.