Chromium side of RequestDeviceOptions implementation.

Chromium side of RequestDeviceOptions implementation.

The Chrome Implementation Notes refer to this patch: https://github.com/WebBluetoothChrome/web-bluetooth/pull/10

This is the second of three patches to implement RequestDeviceOptions:

  [1] https://crrev.com/1182973002 (Main Blink implementation)
  [2] This patch.
  [3] https://crrev.com/1174843002 (Blink tests)

BUG=440594
Committed: https://crrev.com/3821e851e5d9496fa2a151d0e7707b0575e140d3
Cr-Commit-Position: refs/heads/master@{#336039}

[Jeffrey Yasskin, 2015-06-15 21:10:37]

Patchset #4 (id:60001) has been deleted

[Jeffrey Yasskin, 2015-06-15 21:12:10]

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

[Jeffrey Yasskin, 2015-06-15 21:12:10]

Vince || Gio for *bluetooth*; Arman for device/bluetooth and the new DEPS on it.

[ortuno, 2015-06-15 22:49:27]

LGTM. There are going to be a couple of merge conflicts with the error messages
patch that landed recently.

https://codereview.chromium.org/1172853004/diff/80001/content/browser/bluetoo...
File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right):

https://codereview.chromium.org/1172853004/diff/80001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:100: static
scoped_ptr<device::BluetoothDiscoveryFilter> ComputeScanFilter(
Shouldn't this be in an anonymous namespace?

[Jeffrey Yasskin, 2015-06-15 23:47:08]

Patchset #6 (id:120001) has been deleted

[scheib, 2015-06-15 23:47:21]

https://codereview.chromium.org/1172853004/diff/80001/content/browser/bluetoo...
File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right):

https://codereview.chromium.org/1172853004/diff/80001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:347: //
https://webbluetoothcg.github.io/web-bluetooth/#dfn-matches-a-filter
Please update https://github.com/WebBluetoothChrome/web-bluetooth noting this
cross reference and add a link to the pull request for it in ths change list
description.

https://codereview.chromium.org/1172853004/diff/80001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:371: }
Follow with blank line.

https://codereview.chromium.org/1172853004/diff/80001/content/shell/browser/l...
File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc
(right):

https://codereview.chromium.org/1172853004/diff/80001/content/shell/browser/l...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:130:
static std::set<BluetoothUUID> GetUUIDs(
I have a style preference of keeping these methods at top of file in the
anonymous namespace, and keeping all class member definitions in order and
contiguous below.

https://codereview.chromium.org/1172853004/diff/80001/content/shell/browser/l...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:161:
ON_CALL(*adapter, GetDevices())
Let's return something so that in JS the promise can be fullfilled demonstrating
that the session filter check was successful. Can ON_CALL...GetDevices() be set
in the anonymous function above as well, and in that case returning a device?
This location of setting the WillByDefault seems unnecessary as the mock by
default returns an empty list
https://code.google.com/p/chromium/codesearch#chromium/src/device/bluetooth/t...
But I'm OK with the redundancy to make it clear what is expected.

https://codereview.chromium.org/1172853004/diff/80001/content/shell/browser/l...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:260:
std::string value_str("Empty Mock Device name");
Use 'device_name' instead of string literal here?

https://codereview.chromium.org/1172853004/diff/80001/content/shell/browser/l...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:261:
std::vector<uint8_t> value(value_str.begin(), value_str.end());
While here, rename to 'device_name_value'.

https://codereview.chromium.org/1172853004/diff/80001/content/shell/browser/l...
File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h
(right):

https://codereview.chromium.org/1172853004/diff/80001/content/shell/browser/l...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:38:
//  - |StartDiscoverySession| runs the first argument with |DiscoverySession|
Let's clarify language pattern used on these: "runs the first argument" -> "runs
the success callback"

[Jeffrey Yasskin, 2015-06-15 23:48:22]

jyasskin@chromium.org changed reviewers:
+ tsepez@chromium.org

[Jeffrey Yasskin, 2015-06-15 23:48:23]

Tom, please review content/common/bluetooth/bluetooth_messages.h and the new
ParamTraits in bluetooth_uuid.cc.

https://codereview.chromium.org/1172853004/diff/80001/content/browser/bluetoo...
File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right):

https://codereview.chromium.org/1172853004/diff/80001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:100: static
scoped_ptr<device::BluetoothDiscoveryFilter> ComputeScanFilter(
On 2015/06/15 22:49:27, ortuno wrote:
> Shouldn't this be in an anonymous namespace?

It's static, which has the same effect. Making it static makes it easier to
define it close to where it's used, but I could put it in an anonymous namespace
at the top of the file instead, if folks prefer.

https://codereview.chromium.org/1172853004/diff/140001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_uuid.cc (right):

https://codereview.chromium.org/1172853004/diff/140001/device/bluetooth/bluet...
device/bluetooth/bluetooth_uuid.cc:114: return r->format() ==
device::BluetoothUUID::kFormat128Bit;
Tom, is this the right way to validate IPCs that are supposed to have a
particular format? When I force an invalid UUID across the wire, this appears to
cause the browser to hit
https://code.google.com/p/chromium/codesearch/#chromium/src/ipc/ipc_message_u...
instead of killing the renderer, which is a little sad.

[Jeffrey Yasskin, 2015-06-16 00:48:10]

https://codereview.chromium.org/1172853004/diff/80001/content/browser/bluetoo...
File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right):

https://codereview.chromium.org/1172853004/diff/80001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:100: static
scoped_ptr<device::BluetoothDiscoveryFilter> ComputeScanFilter(
On 2015/06/15 23:48:22, Jeffrey Yasskin wrote:
> On 2015/06/15 22:49:27, ortuno wrote:
> > Shouldn't this be in an anonymous namespace?
> 
> It's static, which has the same effect. Making it static makes it easier to
> define it close to where it's used, but I could put it in an anonymous
namespace
> at the top of the file instead, if folks prefer.

Done per Vince's comment elsewhere.

https://codereview.chromium.org/1172853004/diff/80001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:347: //
https://webbluetoothcg.github.io/web-bluetooth/#dfn-matches-a-filter
On 2015/06/15 23:47:21, scheib wrote:
> Please update https://github.com/WebBluetoothChrome/web-bluetooth noting this
> cross reference and add a link to the pull request for it in ths change list
> description. 

Done: https://github.com/WebBluetoothChrome/web-bluetooth/pull/10.

https://codereview.chromium.org/1172853004/diff/80001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:371: }
On 2015/06/15 23:47:21, scheib wrote:
> Follow with blank line.

Done.

https://codereview.chromium.org/1172853004/diff/80001/content/shell/browser/l...
File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc
(right):

https://codereview.chromium.org/1172853004/diff/80001/content/shell/browser/l...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:130:
static std::set<BluetoothUUID> GetUUIDs(
On 2015/06/15 23:47:21, scheib wrote:
> I have a style preference of keeping these methods at top of file in the
> anonymous namespace, and keeping all class member definitions in order and
> contiguous below.

Ick, but done, and for the definitions of MatchesFilter*(). I would have made
this a lambda, but ResultOf expects C++98 member typedefs.

https://codereview.chromium.org/1172853004/diff/80001/content/shell/browser/l...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:161:
ON_CALL(*adapter, GetDevices())
On 2015/06/15 23:47:21, scheib wrote:
> Let's return something so that in JS the promise can be fullfilled
demonstrating
> that the session filter check was successful.

'k.

> Can ON_CALL...GetDevices() be set
> in the anonymous function above as well, and in that case returning a device?

It makes more sense, I think, to set it here. The
StartDiscoverySessionWithFilterRaw(_, _, _) case already rejects the promise by
failing to start discovery.

> This location of setting the WillByDefault seems unnecessary as the mock by
> default returns an empty list
>
https://code.google.com/p/chromium/codesearch#chromium/src/device/bluetooth/t...
> But I'm OK with the redundancy to make it clear what is expected.

There's no default: "BluetoothAdapter::ConstDeviceList()" is just the type of
the function.

https://codereview.chromium.org/1172853004/diff/80001/content/shell/browser/l...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:260:
std::string value_str("Empty Mock Device name");
On 2015/06/15 23:47:21, scheib wrote:
> Use 'device_name' instead of string literal here?

I just replaced this whole string with 'device_name'.

https://codereview.chromium.org/1172853004/diff/80001/content/shell/browser/l...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:261:
std::vector<uint8_t> value(value_str.begin(), value_str.end());
On 2015/06/15 23:47:21, scheib wrote:
> While here, rename to 'device_name_value'.

Done.

https://codereview.chromium.org/1172853004/diff/80001/content/shell/browser/l...
File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h
(right):

https://codereview.chromium.org/1172853004/diff/80001/content/shell/browser/l...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:38:
//  - |StartDiscoverySession| runs the first argument with |DiscoverySession|
On 2015/06/15 23:47:21, scheib wrote:
> Let's clarify language pattern used on these: "runs the first argument" ->
"runs
> the success callback"

Done.

[scheib, 2015-06-16 03:12:07]

LGTM

https://codereview.chromium.org/1172853004/diff/180001/content/shell/browser/...
File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h
(right):

https://codereview.chromium.org/1172853004/diff/180001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:42:
//  - |GetDevices| returns an empty list of devices.
returns a "Battery Device"

[Jeffrey Yasskin, 2015-06-16 19:11:40]

https://codereview.chromium.org/1172853004/diff/180001/content/shell/browser/...
File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h
(right):

https://codereview.chromium.org/1172853004/diff/180001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:42:
//  - |GetDevices| returns an empty list of devices.
On 2015/06/16 03:12:06, scheib wrote:
> returns a "Battery Device"

Done.

[scheib, 2015-06-16 19:44:00]

LGTM

[Tom Sepez, 2015-06-16 19:48:18]

lgtm

[Jeffrey Yasskin, 2015-06-17 00:15:31]

jyasskin@chromium.org changed reviewers:
+ keybuk@chromium.org

[Jeffrey Yasskin, 2015-06-17 00:15:32]

Hey Scott, Arman's OOO until Friday, so I wonder if you could look at the
device/bluetooth/bluetooth_uuid change and the new DEPS from content/child to
device/bluetooth before then. (content/common already depends on
device/bluetooth.)

If not, no problem.

Thanks,
Jeffrey

[armansito, 2015-06-23 18:24:28]

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

https://codereview.chromium.org/1172853004/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_uuid.cc:114: return r->format() ==
device::BluetoothUUID::kFormat128Bit;
I'm not fully understanding the necessity for this check for readers who aren't
familiar with the IPC code? Why not always construct and return a BluetoothUUID
in 128-bit format?

https://codereview.chromium.org/1172853004/diff/200001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_uuid.h (right):

https://codereview.chromium.org/1172853004/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_uuid.h:109: template <>
Can you document the purpose of this structure?

[scheib, 2015-06-23 18:33:27]

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

https://codereview.chromium.org/1172853004/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_uuid.cc:114: return r->format() ==
device::BluetoothUUID::kFormat128Bit;
On 2015/06/23 18:24:28, armansito wrote:
> I'm not fully understanding the necessity for this check for readers who
aren't
> familiar with the IPC code? Why not always construct and return a
BluetoothUUID
> in 128-bit format?

A correctly running renderer will construct a 128bit format above. However, a
compromised renderer can send whatever data it pleases. This check verifies that
an attacking renderer is not intentionally sending another format to confuse the
browser.

[Jeffrey Yasskin, 2015-06-24 21:04:26]

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

https://codereview.chromium.org/1172853004/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_uuid.cc:114: return r->format() ==
device::BluetoothUUID::kFormat128Bit;
On 2015/06/23 18:33:27, scheib wrote:
> On 2015/06/23 18:24:28, armansito wrote:
> > I'm not fully understanding the necessity for this check for readers who
> aren't
> > familiar with the IPC code? Why not always construct and return a
> BluetoothUUID
> > in 128-bit format?
> 
> A correctly running renderer will construct a 128bit format above. However, a
> compromised renderer can send whatever data it pleases. This check verifies
that
> an attacking renderer is not intentionally sending another format to confuse
the
> browser.

Yeah, we could recover from a non-128-bit message by writing "*r =
device::BluetoothUUID(r->canonical_value())", but since only a broken renderer
would send that, it doesn't seem worth the extra code. 

Even if we didn't check for the 128-bit format, we have to check for
r->IsValid() somewhere, and it may as well be while receiving the IPC instead of
every place in the browser that tries to use the data.

https://codereview.chromium.org/1172853004/diff/200001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_uuid.h (right):

https://codereview.chromium.org/1172853004/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_uuid.h:109: template <>
On 2015/06/23 18:24:28, armansito wrote:
> Can you document the purpose of this structure?

Is the new comment here what you were going for, or do you want more?
ParamTraits itself is very badly documented, so I don't have a good place to
link to. See
https://code.google.com/p/chromium/codesearch/#chromium/src/ipc/ipc_param_tra...,
http://www.chromium.org/developers/design-documents/inter-process-communicati...,
and
https://code.google.com/p/chromium/codesearch/#chromium/src/ipc/ipc_message_u....

[armansito, 2015-06-24 21:26:43]

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

https://codereview.chromium.org/1172853004/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_uuid.cc:114: return r->format() ==
device::BluetoothUUID::kFormat128Bit;
On 2015/06/24 21:04:25, jyasskin - OOO until June 24 wrote:
> On 2015/06/23 18:33:27, scheib wrote:
> > On 2015/06/23 18:24:28, armansito wrote:
> > > I'm not fully understanding the necessity for this check for readers who
> > aren't
> > > familiar with the IPC code? Why not always construct and return a
> > BluetoothUUID
> > > in 128-bit format?
> > 
> > A correctly running renderer will construct a 128bit format above. However,
a
> > compromised renderer can send whatever data it pleases. This check verifies
> that
> > an attacking renderer is not intentionally sending another format to confuse
> the
> > browser.
> 
> Yeah, we could recover from a non-128-bit message by writing "*r =
> device::BluetoothUUID(r->canonical_value())", but since only a broken renderer
> would send that, it doesn't seem worth the extra code. 
> 
> Even if we didn't check for the 128-bit format, we have to check for
> r->IsValid() somewhere, and it may as well be while receiving the IPC instead
of
> every place in the browser that tries to use the data.

Got it. If that's the consensus on what the IPC behavior should be, then sgtm.

https://codereview.chromium.org/1172853004/diff/200001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_uuid.h (right):

https://codereview.chromium.org/1172853004/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_uuid.h:109: template <>
On 2015/06/24 21:04:26, jyasskin - OOO until June 24 wrote:
> On 2015/06/23 18:24:28, armansito wrote:
> > Can you document the purpose of this structure?
> 
> Is the new comment here what you were going for, or do you want more?
> ParamTraits itself is very badly documented, so I don't have a good place to
> link to. See
>
https://code.google.com/p/chromium/codesearch/#chromium/src/ipc/ipc_param_tra...,
>
http://www.chromium.org/developers/design-documents/inter-process-communicati...,
> and
>
https://code.google.com/p/chromium/codesearch/#chromium/src/ipc/ipc_message_u....

Yep, the new comment was what I was going for.

https://codereview.chromium.org/1172853004/diff/220001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_uuid.h (right):

https://codereview.chromium.org/1172853004/diff/220001/device/bluetooth/bluet...
device/bluetooth/bluetooth_uuid.h:15: }
nit "}  // namespace base"

[Jeffrey Yasskin, 2015-06-24 22:05:10]

jyasskin@chromium.org changed reviewers:
- keybuk@chromium.org

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

https://codereview.chromium.org/1172853004/diff/220001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_uuid.h (right):

https://codereview.chromium.org/1172853004/diff/220001/device/bluetooth/bluet...
device/bluetooth/bluetooth_uuid.h:15: }
On 2015/06/24 21:26:43, armansito wrote:
> nit "}  // namespace base"

Done.

[armansito, 2015-06-24 22:41:45]

lgtm

[Jeffrey Yasskin, 2015-06-24 22:52:07]

The CQ bit was checked by jyasskin@chromium.org

[Jeffrey Yasskin, 2015-06-24 22:52:08]

The patchset sent to the CQ was uploaded after l-g-t-m from ortuno@chromium.org,
tsepez@chromium.org, scheib@chromium.org
Link to the patchset: https://codereview.chromium.org/1172853004/#ps240001
(title: "Add an end-namespace comment")

[commit-bot: I haz the power, 2015-06-24 22:55:08]

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

[commit-bot: I haz the power, 2015-06-24 23:54:03]

Committed patchset #11 (id:240001)

[commit-bot: I haz the power, 2015-06-24 23:54:51]

Patchset 11 (id:??) landed as
https://crrev.com/3821e851e5d9496fa2a151d0e7707b0575e140d3
Cr-Commit-Position: refs/heads/master@{#336039}