bluetooth: Implement known_service_uuids for Peripherals.

bluetooth: Implement known_service_uuids for Peripherals.

Platforms expose methods to retrieve devices that have connected outside the
user agent and their UUIDs:

Android: BluetoothManager#getConnectedDevices[1] and getUuids()[2]
macOS: CBCentralManager:retrieveConnectedPeripheralsWithServices:[3]
Windows: DeviceInformation::FindAllAsync[4] with GattDeviceService::GetDeviceSelectorFromUuid()[5]

Allows tests to provide known_service_uuids that would cause the returned
devices from the platform functions to change.

[1] https://developer.android.com/reference/android/bluetooth/BluetoothManager.html#getConnectedDevices(int)
[2] https://developer.android.com/reference/android/bluetooth/BluetoothDevice.html#getUuids()
[3] https://developer.apple.com/reference/corebluetooth/cbcentralmanager/1518924-retrieveconnectedperipheralswith?language=objc
[4] https://docs.microsoft.com/en-us/uwp/api/windows.devices.enumeration.deviceinformation#Windows_Devices_Enumeration_DeviceInformation_FindAllAsync_System_String_
[5] https://docs.microsoft.com/en-us/uwp/api/windows.devices.bluetooth.genericattributeprofile.gattdeviceservice#Windows_Devices_Bluetooth_GenericAttributeProfile_GattDeviceService_GetDeviceSelectorFromUuid_System_Guid_

BUG=719813
Review-Url: https://codereview.chromium.org/2867713008
Cr-Commit-Position: refs/heads/master@{#471197}
Committed: https://chromium.googlesource.com/chromium/src/+/46a4de44f2d04ac212c55fbc08fe17fd2da63845

[ortuno, 2017-05-09 05:09:57]

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

[commit-bot: I haz the power, 2017-05-09 05:10:44]

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, 2017-05-09 07:04:04]

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

[commit-bot: I haz the power, 2017-05-09 07:04:05]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[ortuno, 2017-05-09 07:23:42]

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

[commit-bot: I haz the power, 2017-05-09 07:24:11]

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, 2017-05-09 09:51:47]

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

[commit-bot: I haz the power, 2017-05-09 09:51:48]

Dry run: This issue passed the CQ dry run.

[ortuno, 2017-05-09 10:09:40]

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

[ortuno, 2017-05-09 10:09:40]

scheib: PTAL

[scheib, 2017-05-09 23:33:54]

https://codereview.chromium.org/2867713008/diff/40001/device/bluetooth/test/f...
File device/bluetooth/test/fake_peripheral.h (right):

https://codereview.chromium.org/2867713008/diff/40001/device/bluetooth/test/f...
device/bluetooth/test/fake_peripheral.h:30: // Updates the peripheral's UUIDs.
... UUIDs that are returned by GetUUIDs.

https://codereview.chromium.org/2867713008/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/resources/bluetooth/bluetooth-helpers.js
(right):

https://codereview.chromium.org/2867713008/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/resources/bluetooth/bluetooth-helpers.js:16:
const ADDRESS1 = '00:00:00:00:00:00';
Will any code other than setup methods use these values? If not, then may as
well keep the literal values inline in the setup methods. If it's good enough
for device names, why not addresses?

Or we will need to reference them? If so, seems silly to have address1 == 00.
Maybe something more meaningful?:
LE_DEVICE_ADDRESS = "01:01:01:01:01:01"
HEALTH_THERMOMETER_ADDRESS = "02:02:02:02:02:02"
HEART_RATE_ADDRESS = "03:03:03:03:03:03"

https://codereview.chromium.org/2867713008/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/resources/bluetooth/bluetooth-helpers.js:440:
function setUpHealthThermometerHeartRate() {
setUpHealthThermometerAndHeartRateDevices
                      ^^^         ^^^^^^^

https://codereview.chromium.org/2867713008/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/resources/bluetooth/bluetooth-helpers.js:441:
return setUpPreconnectedDevice({
Calling setUpPreconnectedDevice multiple times, resulting in multiple calls to
simulateCentral, seems odd. I think I'd rather see a duplicate call to
simulateCentral, and then multiple .simulatePreconnectedPeripheral calls.

See note in simulateCentral...

https://codereview.chromium.org/2867713008/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/resources/bluetooth/web-bluetooth-test.js
(right):

https://codereview.chromium.org/2867713008/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/resources/bluetooth/web-bluetooth-test.js:93: if
(this.fake_central_ === undefined) {
... continued from previous comment. This logic seems complicated and odd that
we have to handle simulateCentral being called more than once. Perhaps if we
want to update the state of the central later we should just call setState.

Also, code like this seems to be having too many nested awaits. Perhaps we
should have a private method that we can call at top of all FakeBluetooth
methods that ensures mojo is loaded? If code here stayed exactly the same I'd
like to remove the need for getFakeBluetoothInterface and change
toMojoCentralState to be synchronous.

https://codereview.chromium.org/2867713008/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/resources/bluetooth/web-bluetooth-test.js:128: //
Simulates a peripheral with |address|, |name| and |known_service_uuids|
Describe that array members of known_service_uuids must be of
BluetoothServiceUUID type.

[ortuno, 2017-05-10 06:32:51]

https://codereview.chromium.org/2867713008/diff/40001/device/bluetooth/test/f...
File device/bluetooth/test/fake_peripheral.h (right):

https://codereview.chromium.org/2867713008/diff/40001/device/bluetooth/test/f...
device/bluetooth/test/fake_peripheral.h:30: // Updates the peripheral's UUIDs.
On 2017/05/09 at 23:33:54, scheib wrote:
> ... UUIDs that are returned by GetUUIDs.

Done.

https://codereview.chromium.org/2867713008/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/resources/bluetooth/bluetooth-helpers.js
(right):

https://codereview.chromium.org/2867713008/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/resources/bluetooth/bluetooth-helpers.js:16:
const ADDRESS1 = '00:00:00:00:00:00';
On 2017/05/09 at 23:33:54, scheib wrote:
> Will any code other than setup methods use these values? If not, then may as
well keep the literal values inline in the setup methods. If it's good enough
for device names, why not addresses?
> 
> Or we will need to reference them? If so, seems silly to have address1 == 00.
Maybe something more meaningful?:
> LE_DEVICE_ADDRESS = "01:01:01:01:01:01"
> HEALTH_THERMOMETER_ADDRESS = "02:02:02:02:02:02"
> HEART_RATE_ADDRESS = "03:03:03:03:03:03"

We might when we add advertisements but I'm OK with inlining in the mean time.
The use case, which I experienced when building the C++ fakes framework, I had
in mind was the following:

In a test:
1. Use one of the setup methods.
2. Try to add your own device to central.
3. Be confused as to why the device is not showing up.
4. Realize that your new device was using an address already used.

To avoid ADDRESS*, I changed to keep the strings inline and made the addresses
less common to avoid collisions.

https://codereview.chromium.org/2867713008/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/resources/bluetooth/bluetooth-helpers.js:440:
function setUpHealthThermometerHeartRate() {
On 2017/05/09 at 23:33:54, scheib wrote:
> setUpHealthThermometerAndHeartRateDevices
>                       ^^^         ^^^^^^^

Done. I was trying to keep our names as short as possible.

https://codereview.chromium.org/2867713008/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/resources/bluetooth/bluetooth-helpers.js:441:
return setUpPreconnectedDevice({
On 2017/05/09 at 23:33:54, scheib wrote:
> Calling setUpPreconnectedDevice multiple times, resulting in multiple calls to
simulateCentral, seems odd. I think I'd rather see a duplicate call to
simulateCentral, and then multiple .simulatePreconnectedPeripheral calls.
> 
> See note in simulateCentral...

I was hoping to make it so that a test could call setUpPreconnectedDevice
multiple times.

return setUpPreconnectedDevice({address: makeMACAddress(0x01), name: 'dev1'})
  .then(() => setUpPreconnectedDevice({address: makeMACAddress(0x02), name:
'dev2'})
  .then(() => setUpPreconnectedDevice({address: makeMACAddress(0x03), name:
'dev3'})

vs.

return navigator.bluetooth.test.simulateCentral({state: 'powered-on'})
  .then(fake_central => {
    return fake_central.simulatePreconnectedPeripheral({
      address: makeMACAddress(0x01), name: 'dev1'})
      .then(() => fake_central.simulatePreconnectedPeripheral({
        address: makeMACAddress(0x02), name: 'dev2'})
      .then(() => fake_central.simulatePreconnectedPerupheral({
        address: makeMACAddress(0x03), name: 'dev3'})
  });

But I guess we can revisit if/when tests like that pop up.

Done.

https://codereview.chromium.org/2867713008/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/resources/bluetooth/web-bluetooth-test.js
(right):

https://codereview.chromium.org/2867713008/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/resources/bluetooth/web-bluetooth-test.js:93: if
(this.fake_central_ === undefined) {
On 2017/05/09 at 23:33:54, scheib wrote:
> ... continued from previous comment. This logic seems complicated and odd that
we have to handle simulateCentral being called more than once. Perhaps if we
want to update the state of the central later we should just call setState.
> 

Yeah, I agree. It was a trade-off between convenience e.g. being able to call
setUpPreconnectedDevice and complexity. I guess we can avoid the complexity for
now and introduce it when/if we need it. 

> Also, code like this seems to be having too many nested awaits. Perhaps we
should have a private method that we can call at top of all FakeBluetooth
methods that ensures mojo is loaded? If code here stayed exactly the same I'd
like to remove the need for getFakeBluetoothInterface and change
toMojoCentralState to be synchronous.

I shuffled the code a bit. I'm slightly worried that toMojoCentralState doesn't
explicitly get the mojo interfaces but then again, why would we use
toMojoCentralState if we haven't initialized mojo_ yet?

https://codereview.chromium.org/2867713008/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/resources/bluetooth/web-bluetooth-test.js:128: //
Simulates a peripheral with |address|, |name| and |known_service_uuids|
On 2017/05/09 at 23:33:54, scheib wrote:
> Describe that array members of known_service_uuids must be of
BluetoothServiceUUID type.

Done.

[ortuno, 2017-05-10 06:32:55]

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

[commit-bot: I haz the power, 2017-05-10 06:33:29]

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, 2017-05-10 06:37:26]

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

[commit-bot: I haz the power, 2017-05-10 06:37:28]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[ortuno, 2017-05-10 07:16:23]

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

[commit-bot: I haz the power, 2017-05-10 07:17:02]

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, 2017-05-10 08:34:10]

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

[commit-bot: I haz the power, 2017-05-10 08:34:11]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[scheib, 2017-05-10 17:51:14]

lgtm

https://codereview.chromium.org/2867713008/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/resources/bluetooth/bluetooth-helpers.js
(right):

https://codereview.chromium.org/2867713008/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/resources/bluetooth/bluetooth-helpers.js:16:
const ADDRESS1 = '00:00:00:00:00:00';
On 2017/05/10 06:32:50, ortuno wrote:
> On 2017/05/09 at 23:33:54, scheib wrote:
> > Will any code other than setup methods use these values? If not, then may as
> well keep the literal values inline in the setup methods. If it's good enough
> for device names, why not addresses?
> > 
> > Or we will need to reference them? If so, seems silly to have address1 ==
00.
> Maybe something more meaningful?:
> > LE_DEVICE_ADDRESS = "01:01:01:01:01:01"
> > HEALTH_THERMOMETER_ADDRESS = "02:02:02:02:02:02"
> > HEART_RATE_ADDRESS = "03:03:03:03:03:03"
> 
> We might when we add advertisements but I'm OK with inlining in the mean time.
> The use case, which I experienced when building the C++ fakes framework, I had
> in mind was the following:
> 
> In a test:
> 1. Use one of the setup methods.
> 2. Try to add your own device to central.
> 3. Be confused as to why the device is not showing up.
> 4. Realize that your new device was using an address already used.
> 
> To avoid ADDRESS*, I changed to keep the strings inline and made the addresses
> less common to avoid collisions.

OK, yeah we could just use a random number literal each time (I think my pref).
Or, could have a test helper method that vends out unique addresses.

https://codereview.chromium.org/2867713008/diff/80001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/resources/bluetooth/web-bluetooth-test.js
(right):

https://codereview.chromium.org/2867713008/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/resources/bluetooth/web-bluetooth-test.js:30: if
(mojo_ === undefined) {
OK to leave it, but I think the console error message that can't find
CentralState on undefined would be enough to give us the clue we forgot to init
this class.

[scheib, 2017-05-10 17:52:23]

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

[scheib, 2017-05-10 17:52:24]

dcheng: PTAL mojom

[ortuno, 2017-05-10 23:34:23]

https://codereview.chromium.org/2867713008/diff/80001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/resources/bluetooth/web-bluetooth-test.js
(right):

https://codereview.chromium.org/2867713008/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/resources/bluetooth/web-bluetooth-test.js:30: if
(mojo_ === undefined) {
On 2017/05/10 at 17:51:14, scheib wrote:
> OK to leave it, but I think the console error message that can't find
CentralState on undefined would be enough to give us the clue we forgot to init
this class.

True. Done.

[dcheng, 2017-05-11 05:44:21]

ipc lgtm

https://codereview.chromium.org/2867713008/diff/100001/device/bluetooth/publi...
File device/bluetooth/public/interfaces/test/fake_bluetooth.mojom (right):

https://codereview.chromium.org/2867713008/diff/100001/device/bluetooth/publi...
device/bluetooth/public/interfaces/test/fake_bluetooth.mojom:53: array<UUID>
known_service_uuids) => ();
Not a blocker for this CL, but my previous comment about not just wrapping a
string as the mojo type and then enforcing structure on it applies here. That
being said, I'm not sure how annoying it would be to convert from a GUID string
(in BluetoothUUID) to a 128bit binary representation back to a GUID string: I
get the sense that probably it's not /too/ bad.

[ortuno, 2017-05-11 06:44:59]

The CQ bit was checked by ortuno@chromium.org

[ortuno, 2017-05-11 06:44:59]

The patchset sent to the CQ was uploaded after l-g-t-m from scheib@chromium.org
Link to the patchset: https://codereview.chromium.org/2867713008/#ps100001
(title: "Remove unnecessary mojo_ check")

[ortuno, 2017-05-11 06:45:08]

Thanks!

https://codereview.chromium.org/2867713008/diff/100001/device/bluetooth/publi...
File device/bluetooth/public/interfaces/test/fake_bluetooth.mojom (right):

https://codereview.chromium.org/2867713008/diff/100001/device/bluetooth/publi...
device/bluetooth/public/interfaces/test/fake_bluetooth.mojom:53: array<UUID>
known_service_uuids) => ();
On 2017/05/11 at 05:44:21, dcheng wrote:
> Not a blocker for this CL, but my previous comment about not just wrapping a
string as the mojo type and then enforcing structure on it applies here. That
being said, I'm not sure how annoying it would be to convert from a GUID string
(in BluetoothUUID) to a 128bit binary representation back to a GUID string: I
get the sense that probably it's not /too/ bad.

I'm unsure what the benefits of sending binary data over a string are here. All
parts of the Bluetooth stack, from Web APIs down to the platform APIs use the
string representation of the UUIDs. It seems a bit unnecessary to convert them
to binary data just to send them over.

[commit-bot: I haz the power, 2017-05-11 06:45:44]

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

[commit-bot: I haz the power, 2017-05-11 06:45:45]

This CL has an open dependency (Issue 2858803003 Patch 220001). Please resolve
the dependency and try again.
If you are sure that there is no real dependency, please use one of the options
listed in https://goo.gl/9Es4OR to land the CL.

[ortuno, 2017-05-12 00:02:37]

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

[commit-bot: I haz the power, 2017-05-12 00:04:25]

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, 2017-05-12 02:28:23]

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

[commit-bot: I haz the power, 2017-05-12 02:28:24]

Dry run: This issue passed the CQ dry run.

[ortuno, 2017-05-12 02:48:18]

The CQ bit was checked by ortuno@chromium.org

[ortuno, 2017-05-12 02:48:19]

The patchset sent to the CQ was uploaded after l-g-t-m from scheib@chromium.org,
dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2867713008/#ps120001
(title: "rebase")

[commit-bot: I haz the power, 2017-05-12 02:48:40]

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, 2017-05-12 02:54:19]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1494557298921830,
"parent_rev": "8628a9e95249d1900319f645beb2bbe3b055ff75", "commit_rev":
"46a4de44f2d04ac212c55fbc08fe17fd2da63845"}

[commit-bot: I haz the power, 2017-05-12 02:54:31]

Description was changed from

==========
bluetooth: Implement known_service_uuids for Peripherals.

Platforms expose methods to retrieve devices that have connected outside the
user agent and their UUIDs:

Android: BluetoothManager#getConnectedDevices[1] and getUuids()[2]
macOS: CBCentralManager:retrieveConnectedPeripheralsWithServices:[3]
Windows: DeviceInformation::FindAllAsync[4] with
GattDeviceService::GetDeviceSelectorFromUuid()[5]

Allows tests to provide known_service_uuids that would cause the returned
devices from the platform functions to change.

[1]
https://developer.android.com/reference/android/bluetooth/BluetoothManager.ht...
[2]
https://developer.android.com/reference/android/bluetooth/BluetoothDevice.htm...
[3]
https://developer.apple.com/reference/corebluetooth/cbcentralmanager/1518924-...
[4]
https://docs.microsoft.com/en-us/uwp/api/windows.devices.enumeration.devicein...
[5]
https://docs.microsoft.com/en-us/uwp/api/windows.devices.bluetooth.genericatt...

BUG=719813
==========

to

==========
bluetooth: Implement known_service_uuids for Peripherals.

Platforms expose methods to retrieve devices that have connected outside the
user agent and their UUIDs:

Android: BluetoothManager#getConnectedDevices[1] and getUuids()[2]
macOS: CBCentralManager:retrieveConnectedPeripheralsWithServices:[3]
Windows: DeviceInformation::FindAllAsync[4] with
GattDeviceService::GetDeviceSelectorFromUuid()[5]

Allows tests to provide known_service_uuids that would cause the returned
devices from the platform functions to change.

[1]
https://developer.android.com/reference/android/bluetooth/BluetoothManager.ht...
[2]
https://developer.android.com/reference/android/bluetooth/BluetoothDevice.htm...
[3]
https://developer.apple.com/reference/corebluetooth/cbcentralmanager/1518924-...
[4]
https://docs.microsoft.com/en-us/uwp/api/windows.devices.enumeration.devicein...
[5]
https://docs.microsoft.com/en-us/uwp/api/windows.devices.bluetooth.genericatt...

BUG=719813

Review-Url: https://codereview.chromium.org/2867713008
Cr-Commit-Position: refs/heads/master@{#471197}
Committed:
https://chromium.googlesource.com/chromium/src/+/46a4de44f2d04ac212c55fbc08fe...
==========

[commit-bot: I haz the power, 2017-05-12 02:54:32]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/46a4de44f2d04ac212c55fbc08fe...