Implement the Blink side of RequestDeviceOptions.

Implement the Blink side of RequestDeviceOptions.

Except for the tests that require an update to the fake bluetooth adapters.

This is the first of three patches to implement RequestDeviceOptions:

  [1] This patch.
  [2] https://crrev.com/1172853004 (Chromium side)
  [3] https://crrev.com/1174843002 (Blink tests)

BUG=440594
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=197221

[Jeffrey Yasskin, 2015-06-13 01:50:17]

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

[commit-bot: I haz the power, 2015-06-13 01:50:37]

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

[commit-bot: I haz the power, 2015-06-13 03:17:47]

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

[commit-bot: I haz the power, 2015-06-13 03:17:49]

Dry run: This issue passed the CQ dry run.

[Jeffrey Yasskin, 2015-06-15 17:50:49]

Patchset #2 (id:20001) has been deleted

[Jeffrey Yasskin, 2015-06-15 17:51:35]

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

[Jeffrey Yasskin, 2015-06-15 17:51:35]

PTAL. I'm still double-checking the other patches in the series, but this one
should be good to go.

[scheib, 2015-06-15 18:08:11]

LGTM

https://codereview.chromium.org/1182973002/diff/40001/LayoutTests/bluetooth/i...
File LayoutTests/bluetooth/idl-BluetoothDevice.html (right):

https://codereview.chromium.org/1182973002/diff/40001/LayoutTests/bluetooth/i...
LayoutTests/bluetooth/idl-BluetoothDevice.html:35:
assert_equals(device.name.slice(0, 17), "Empty Mock Device");
Why slice?

https://codereview.chromium.org/1182973002/diff/40001/public/platform/modules...
File public/platform/modules/bluetooth/WebBluetooth.h (right):

https://codereview.chromium.org/1182973002/diff/40001/public/platform/modules...
public/platform/modules/bluetooth/WebBluetooth.h:49: // TODO(jyasskin): Remove
this overload and make the other abstract after
It's OK to leave the default implementations. I used to push more on this but
other Blink members convinced me the patch churn is silly until we merge repos.

[ortuno, 2015-06-15 19:13:37]

LGTM

https://codereview.chromium.org/1182973002/diff/40001/Source/modules/bluetoot...
File Source/modules/bluetooth/Bluetooth.cpp (right):

https://codereview.chromium.org/1182973002/diff/40001/Source/modules/bluetoot...
Source/modules/bluetooth/Bluetooth.cpp:36: webFilter.services.assign(services);
why do you use assign instead of the constructor?

[Jeffrey Yasskin, 2015-06-15 19:19:19]

https://codereview.chromium.org/1182973002/diff/40001/LayoutTests/bluetooth/i...
File LayoutTests/bluetooth/idl-BluetoothDevice.html (right):

https://codereview.chromium.org/1182973002/diff/40001/LayoutTests/bluetooth/i...
LayoutTests/bluetooth/idl-BluetoothDevice.html:35:
assert_equals(device.name.slice(0, 17), "Empty Mock Device");
On 2015/06/15 18:08:11, scheib wrote:
> Why slice?

https://codereview.chromium.org/1172853004/diff/40001/content/shell/browser/l...
changes the name of this device so that the 2-device requestDevice test in
https://codereview.chromium.org/1174843002/diff/20001/LayoutTests/bluetooth/r...
can more easily assert that it got the right device. Slicing makes the test work
both before and after.

https://codereview.chromium.org/1182973002/diff/40001/Source/modules/bluetoot...
File Source/modules/bluetooth/Bluetooth.cpp (right):

https://codereview.chromium.org/1182973002/diff/40001/Source/modules/bluetoot...
Source/modules/bluetooth/Bluetooth.cpp:36: webFilter.services.assign(services);
On 2015/06/15 19:13:37, ortuno wrote:
> why do you use assign instead of the constructor?

I hadn't added the constructor when I wrote this code. Fixing.

https://codereview.chromium.org/1182973002/diff/40001/public/platform/modules...
File public/platform/modules/bluetooth/WebBluetooth.h (right):

https://codereview.chromium.org/1182973002/diff/40001/public/platform/modules...
public/platform/modules/bluetooth/WebBluetooth.h:49: // TODO(jyasskin): Remove
this overload and make the other abstract after
On 2015/06/15 18:08:11, scheib wrote:
> It's OK to leave the default implementations. I used to push more on this but
> other Blink members convinced me the patch churn is silly until we merge
repos.

I definitely want to remove the second overload, and if I'm doing that anyway, I
may as well make it abstract. But I can also leave it defaulted in
https://codereview.chromium.org/1174843002/diff/20001/public/platform/modules...
if you prefer that.

[Jeffrey Yasskin, 2015-06-15 19:19:29]

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

[Jeffrey Yasskin, 2015-06-15 19:19:30]

The patchset sent to the CQ was uploaded after l-g-t-m from scheib@chromium.org,
ortuno@chromium.org
Link to the patchset: https://codereview.chromium.org/1182973002/#ps60001
(title: "Fix review comments.")

[commit-bot: I haz the power, 2015-06-15 19:19:57]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1182973002/60001

[scheib, 2015-06-15 20:15:15]

LGTM still

https://codereview.chromium.org/1182973002/diff/40001/LayoutTests/bluetooth/i...
File LayoutTests/bluetooth/idl-BluetoothDevice.html (right):

https://codereview.chromium.org/1182973002/diff/40001/LayoutTests/bluetooth/i...
LayoutTests/bluetooth/idl-BluetoothDevice.html:35:
assert_equals(device.name.slice(0, 17), "Empty Mock Device");
On 2015/06/15 19:19:18, Jeffrey Yasskin wrote:
> On 2015/06/15 18:08:11, scheib wrote:
> > Why slice?
> 
>
https://codereview.chromium.org/1172853004/diff/40001/content/shell/browser/l...
> changes the name of this device so that the 2-device requestDevice test in
>
https://codereview.chromium.org/1174843002/diff/20001/LayoutTests/bluetooth/r...
> can more easily assert that it got the right device. Slicing makes the test
work
> both before and after.

I see, it's changing from "Empty Mock Device name" to "Empty Mock Device" and
that you've removed the slice in patch 3. LGTM though a TODO here would be ultra
pedantic and I would have understood in the single patch review. ;)

https://codereview.chromium.org/1182973002/diff/40001/public/platform/modules...
File public/platform/modules/bluetooth/WebBluetooth.h (right):

https://codereview.chromium.org/1182973002/diff/40001/public/platform/modules...
public/platform/modules/bluetooth/WebBluetooth.h:49: // TODO(jyasskin): Remove
this overload and make the other abstract after
On 2015/06/15 19:19:19, Jeffrey Yasskin wrote:
> On 2015/06/15 18:08:11, scheib wrote:
> > It's OK to leave the default implementations. I used to push more on this
but
> > other Blink members convinced me the patch churn is silly until we merge
> repos.
> 
> I definitely want to remove the second overload, and if I'm doing that anyway,
I
> may as well make it abstract. But I can also leave it defaulted in
>
https://codereview.chromium.org/1174843002/diff/20001/public/platform/modules...
> if you prefer that.

Not a strong preference, but as you can see the newly added code goes for
convenience, leaving default implementations.

[commit-bot: I haz the power, 2015-06-15 20:34:56]

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

[commit-bot: I haz the power, 2015-06-15 20:34:57]

Dry run: This issue passed the CQ dry run.

[Jeffrey Yasskin, 2015-06-16 23:58:46]

The CQ bit was checked by jyasskin@chromium.org

[Jeffrey Yasskin, 2015-06-16 23:58:46]

The patchset sent to the CQ was uploaded after l-g-t-m from scheib@chromium.org,
ortuno@chromium.org
Link to the patchset: https://codereview.chromium.org/1182973002/#ps80001
(title: "Prepare readValue.html for a mock data change")

[commit-bot: I haz the power, 2015-06-16 23:59:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1182973002/80001

[commit-bot: I haz the power, 2015-06-17 01:09:43]

Committed patchset #4 (id:80001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=197221

[ortuno, 2015-06-24 19:35:51]

https://codereview.chromium.org/1182973002/diff/80001/Source/modules/bluetoot...
File Source/modules/bluetooth/RequestDeviceOptions.idl (right):

https://codereview.chromium.org/1182973002/diff/80001/Source/modules/bluetoot...
Source/modules/bluetooth/RequestDeviceOptions.idl:9:
sequence<BluetoothServiceUUID> optionalServices/* = []*/;
Do you know if a dictionary can have sequence of unions? I tried to replace
BluetoothServiceUUID with 'String or unsigned long' and it's not generating the
right code.

[Jeffrey Yasskin, 2015-06-24 22:33:31]

https://codereview.chromium.org/1182973002/diff/80001/Source/modules/bluetoot...
File Source/modules/bluetooth/RequestDeviceOptions.idl (right):

https://codereview.chromium.org/1182973002/diff/80001/Source/modules/bluetoot...
Source/modules/bluetooth/RequestDeviceOptions.idl:9:
sequence<BluetoothServiceUUID> optionalServices/* = []*/;
On 2015/06/24 19:35:51, ortuno wrote:
> Do you know if a dictionary can have sequence of unions? I tried to replace
> BluetoothServiceUUID with 'String or unsigned long' and it's not generating
the
> right code.

WebIDL doesn't say anything about it not working, but Blink's IDL compiler could
be broken. I think you would need to write "sequence<(String or unsigned
long)>".