bluetooth: Impl uses new WebBluetoothDevice in content/browser.

bluetooth: Impl uses new WebBluetoothDevice in content/browser.

Across 5 patches due to content/blink repository split:

Create a blink::WebBluetoothDevice interface and migrate the
blink::WebBluetooth::requestDevice interface to use it.

Also, Move the mock implementation from content/child to
content/browser.

http://crrev.com/718503002 Define new requestDevice and WebBluetoothDevice.
http://crrev.com/699843003 Impl uses new WebBluetoothDevice in content/browser.
http://crrev.com/715613005 Request and receive WebBluetoothDevice from embedder.
http://crrev.com/712153002 Remove old WebBluetoothImpl::requestDevice impl.
http://crrev.com/713043005 Remove old WebBluetooth::requestDevice signature.

BUG=421668
Committed: https://crrev.com/566d082ea134d42ecf0b697ed966c0c2634e2687
Cr-Commit-Position: refs/heads/master@{#305837}

[scheib, 2014-11-10 22:18:45]

Patchset #1 (id:1) has been deleted

[scheib, 2014-11-22 21:07:25]

Patchset #1 (id:20001) has been deleted

[scheib, 2014-11-22 21:17:01]

Patchset #1 (id:40001) has been deleted

[scheib, 2014-11-22 21:59:51]

Patchset #1 (id:60001) has been deleted

[scheib, 2014-11-22 22:14:30]

Patchset #1 (id:80001) has been deleted

[scheib, 2014-11-22 22:18:00]

Patchset #1 (id:100001) has been deleted

[scheib, 2014-11-22 22:27:09]

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

[scheib, 2014-11-24 16:19:45]

scheib@chromium.org changed reviewers:
+ nasko@chromium.org
- mek@chromium.org

[scheib, 2014-11-24 16:19:45]

nasko@chromium.org: PTAL

[Jeffrey Yasskin, 2014-11-24 23:30:10]

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

[Jeffrey Yasskin, 2014-11-24 23:30:12]

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

https://codereview.chromium.org/699843003/diff/120001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:61: } else if (name ==
"ResolveRequestDevice_Empty" ||  // TODO Remove.
TODOs generally have a '(username):' after them:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#TODO_Comments

https://codereview.chromium.org/699843003/diff/120001/content/browser/bluetoo...
File content/browser/bluetooth/bluetooth_dispatcher_host.h (right):

https://codereview.chromium.org/699843003/diff/120001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.h:33:
content::BluetoothError bluetoothRequestDeviceRejectType_;
You don't need the 'content::' qualifier inside the content namespace.

https://codereview.chromium.org/699843003/diff/120001/content/browser/rendere...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/699843003/diff/120001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:878: AddFilter(new
BluetoothDispatcherHost());
Dumb question: on the browser side, where's the right place to block all
Bluetooth access unless the --experimental-web-platform-features flag is set?

https://codereview.chromium.org/699843003/diff/120001/content/child/bluetooth...
File content/child/bluetooth/bluetooth_dispatcher.h (right):

https://codereview.chromium.org/699843003/diff/120001/content/child/bluetooth...
content/child/bluetooth/bluetooth_dispatcher.h:55: const std::string&
device_id);
We should be consistent between instance_id and device_id here and in the
_messages file. I kind of prefer device_id, since instance_id is used for
services, characteristics, and descriptors too.

https://codereview.chromium.org/699843003/diff/120001/content/common/bluetoot...
File content/common/bluetooth/bluetooth_error.h (right):

https://codereview.chromium.org/699843003/diff/120001/content/common/bluetoot...
content/common/bluetooth/bluetooth_error.h:10: enum class BluetoothError {
NOT_FOUND, SECURITY, ENUM_MAX_VALUE = SECURITY };
These probably deserve a comment that they will produce a DOMException in the
renderer named NotFoundError, SecurityError, etc.

Is there any need to pass the 'message' string back with the error name?

https://codereview.chromium.org/699843003/diff/120001/content/common/bluetoot...
File content/common/bluetooth/bluetooth_messages.h (right):

https://codereview.chromium.org/699843003/diff/120001/content/common/bluetoot...
content/common/bluetooth/bluetooth_messages.h:33:
IPC_MESSAGE_CONTROL2(BluetoothHostMsg_RequestDevice,
This file is probably the place to explain the security contracts across the IPC
boundary. For example, that RequestDevice is expected to open a chooser to let
the user pair a device with an origin. RequestDeviceSuccess indicates that
|instance_id| represents a device the origin has permission to use.

Is SetBluetoothMockDataSetForTesting blocked in any way outside of tests?

How does the browser figure out which origin is requesting access?

In terms of CONTROL vs ROUTED, the browser needs to be able to attach the
RequestDevice call to a particular RenderFrameHost, in order to attribute the
dialog to the right tab (or block it for iframes). How does that mapping get
made?

[nasko, 2014-11-25 00:57:23]

Started looking at the CL, but it will take a bit more time. Sending some
preliminary comments, mostly on style.

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

https://codereview.chromium.org/699843003/diff/120001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:37: return;
No need to return here, just plain break would work. This will allow you to drop
the {} for each case statement.

https://codereview.chromium.org/699843003/diff/120001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:50: NOTREACHED();
This belongs in a default switch statement.

https://codereview.chromium.org/699843003/diff/120001/content/browser/bluetoo...
File content/browser/bluetooth/bluetooth_dispatcher_host.h (right):

https://codereview.chromium.org/699843003/diff/120001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.h:15: // process
BluetoothDispatcher from/to the main browser process.
It will be useful to mention lifetime of the object and any expectations on
which thread it lives/executes on.

https://codereview.chromium.org/699843003/diff/120001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.h:32: MockData
bluetoothMockDataSet_;
style: variables use hacker_case_ in chromium

[Jeffrey Yasskin, 2014-11-25 01:08:08]

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

https://codereview.chromium.org/699843003/diff/120001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:37: return;
On 2014/11/25 00:57:23, nasko wrote:
> No need to return here, just plain break would work. This will allow you to
drop
> the {} for each case statement.

Using return here, and the NOTREACHED after the switch, allows Vince to take
advantage of the -Wswitch check that he's listed all possible values of the
enum. default: would turn off that checking. 'course it's up to the code owner
whether you want that check.

The braces are optional either way.

[scheib, 2014-11-25 16:11:06]

Thanks, PTAL

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

https://codereview.chromium.org/699843003/diff/120001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:37: return;
On 2014/11/25 01:08:07, Jeffrey Yasskin wrote:
> On 2014/11/25 00:57:23, nasko wrote:
> > No need to return here, just plain break would work. This will allow you to
> drop
> > the {} for each case statement.
> 
> Using return here, and the NOTREACHED after the switch, allows Vince to take
> advantage of the -Wswitch check that he's listed all possible values of the
> enum. default: would turn off that checking. 'course it's up to the code owner
> whether you want that check.
> 
> The braces are optional either way.

Yes, as Jeffrey said, I prefer compile failure when enumerations are added so
that all uses are examined and handled.

https://codereview.chromium.org/699843003/diff/120001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:50: NOTREACHED();
On 2014/11/25 00:57:23, nasko wrote:
> This belongs in a default switch statement.

Intended, see above re compile checking all enum cases.

https://codereview.chromium.org/699843003/diff/120001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:61: } else if (name ==
"ResolveRequestDevice_Empty" ||  // TODO Remove.
On 2014/11/24 23:30:11, Jeffrey Yasskin wrote:
> TODOs generally have a '(username):' after them:
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#TODO_Comments

Done.

https://codereview.chromium.org/699843003/diff/120001/content/browser/bluetoo...
File content/browser/bluetooth/bluetooth_dispatcher_host.h (right):

https://codereview.chromium.org/699843003/diff/120001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.h:15: // process
BluetoothDispatcher from/to the main browser process.
On 2014/11/25 00:57:23, nasko wrote:
> It will be useful to mention lifetime of the object and any expectations on
> which thread it lives/executes on.

Done.

https://codereview.chromium.org/699843003/diff/120001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.h:32: MockData
bluetoothMockDataSet_;
On 2014/11/25 00:57:23, nasko wrote:
> style: variables use hacker_case_ in chromium

Done.

https://codereview.chromium.org/699843003/diff/120001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.h:33:
content::BluetoothError bluetoothRequestDeviceRejectType_;
On 2014/11/24 23:30:11, Jeffrey Yasskin wrote:
> You don't need the 'content::' qualifier inside the content namespace.

Done.

https://codereview.chromium.org/699843003/diff/120001/content/browser/rendere...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/699843003/diff/120001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:878: AddFilter(new
BluetoothDispatcherHost());
On 2014/11/24 23:30:11, Jeffrey Yasskin wrote:
> Dumb question: on the browser side, where's the right place to block all
> Bluetooth access unless the --experimental-web-platform-features flag is set?

Here is a good place. I've added a switch check.

https://codereview.chromium.org/699843003/diff/120001/content/child/bluetooth...
File content/child/bluetooth/bluetooth_dispatcher.h (right):

https://codereview.chromium.org/699843003/diff/120001/content/child/bluetooth...
content/child/bluetooth/bluetooth_dispatcher.h:55: const std::string&
device_id);
On 2014/11/24 23:30:11, Jeffrey Yasskin wrote:
> We should be consistent between instance_id and device_id here and in the
> _messages file. I kind of prefer device_id, since instance_id is used for
> services, characteristics, and descriptors too.

Done.

https://codereview.chromium.org/699843003/diff/120001/content/common/bluetoot...
File content/common/bluetooth/bluetooth_error.h (right):

https://codereview.chromium.org/699843003/diff/120001/content/common/bluetoot...
content/common/bluetooth/bluetooth_error.h:10: enum class BluetoothError {
NOT_FOUND, SECURITY, ENUM_MAX_VALUE = SECURITY };
On 2014/11/24 23:30:11, Jeffrey Yasskin wrote:
> These probably deserve a comment that they will produce a DOMException in the
> renderer named NotFoundError, SecurityError, etc.

Done.

> Is there any need to pass the 'message' string back with the error name?

Currently, no messages are needed. Later the implementation will likely have
useful strings to send to developers. Then I plan to add variations to these
enumerations, but only associate with the string in the child process. The
exception will be if a procedural generated message is warranted, and I'll
expand the IPC message at that time if needed.

https://codereview.chromium.org/699843003/diff/120001/content/common/bluetoot...
File content/common/bluetooth/bluetooth_messages.h (right):

https://codereview.chromium.org/699843003/diff/120001/content/common/bluetoot...
content/common/bluetooth/bluetooth_messages.h:33:
IPC_MESSAGE_CONTROL2(BluetoothHostMsg_RequestDevice,
On 2014/11/24 23:30:12, Jeffrey Yasskin wrote:
> This file is probably the place to explain the security contracts across the
IPC
> boundary. For example, that RequestDevice is expected to open a chooser to let
> the user pair a device with an origin. RequestDeviceSuccess indicates that
> |instance_id| represents a device the origin has permission to use.

Done.

> 
> Is SetBluetoothMockDataSetForTesting blocked in any way outside of tests?

Not yet, issue created and commented. This shouldn't be harmful though.

> 
> How does the browser figure out which origin is requesting access?
> 
> In terms of CONTROL vs ROUTED, the browser needs to be able to attach the
> RequestDevice call to a particular RenderFrameHost, in order to attribute the
> dialog to the right tab (or block it for iframes). How does that mapping get
> made?

This is deferred until later, I've now commented in code as such.

[nasko, 2014-11-25 23:27:01]

LGTM with a few nits to be fixed.

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

https://codereview.chromium.org/699843003/diff/120001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:37: return;
On 2014/11/25 16:11:06, scheib wrote:
> On 2014/11/25 01:08:07, Jeffrey Yasskin wrote:
> > On 2014/11/25 00:57:23, nasko wrote:
> > > No need to return here, just plain break would work. This will allow you
to
> > drop
> > > the {} for each case statement.
> > 
> > Using return here, and the NOTREACHED after the switch, allows Vince to take
> > advantage of the -Wswitch check that he's listed all possible values of the
> > enum. default: would turn off that checking. 'course it's up to the code
owner
> > whether you want that check.
> > 
> > The braces are optional either way.
> 
> Yes, as Jeffrey said, I prefer compile failure when enumerations are added so
> that all uses are examined and handled.

Heh, I learn something new every day. Thanks for the details!

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

https://codereview.chromium.org/699843003/diff/140001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:65: } else if (name ==
"ResolveRequestDevice_Empty" ||  // TODO(scheib) Remove.
nit: The TODO still needs ":" after ")".

https://codereview.chromium.org/699843003/diff/140001/content/browser/bluetoo...
File content/browser/bluetooth/bluetooth_dispatcher_host.h (right):

https://codereview.chromium.org/699843003/diff/140001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.h:37: BluetoothError
bluetoothRequestDeviceRejectType_;
You still need to convert this variable to hacker_case.

https://codereview.chromium.org/699843003/diff/140001/content/child/bluetooth...
File content/child/bluetooth/bluetooth_dispatcher.h (right):

https://codereview.chromium.org/699843003/diff/140001/content/child/bluetooth...
content/child/bluetooth/bluetooth_dispatcher.h:36: // IPC Send and Receiving
interface, see IPC::Sender and IPC::Listener.
You don't inherit from IPC::Sender and Listener in the class. Is this
intentional?

https://codereview.chromium.org/699843003/diff/140001/content/child/bluetooth...
File content/child/bluetooth/web_bluetooth_impl.cc (right):

https://codereview.chromium.org/699843003/diff/140001/content/child/bluetooth...
content/child/bluetooth/web_bluetooth_impl.cc:19: // TODO: Remove old void
version after crrev.com/715613005 lands.
nit: TODO(username):

https://codereview.chromium.org/699843003/diff/140001/content/child/bluetooth...
File content/child/bluetooth/web_bluetooth_impl.h (right):

https://codereview.chromium.org/699843003/diff/140001/content/child/bluetooth...
content/child/bluetooth/web_bluetooth_impl.h:29: // TODO: Remove void version
after crrev.com/715613005 lands.
nit: TODO(username):

[scheib, 2014-11-26 00:46:35]

Thanks

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

https://codereview.chromium.org/699843003/diff/140001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:65: } else if (name ==
"ResolveRequestDevice_Empty" ||  // TODO(scheib) Remove.
On 2014/11/25 23:27:01, nasko (Out until Dec 08) wrote:
> nit: The TODO still needs ":" after ")".

Done.

https://codereview.chromium.org/699843003/diff/140001/content/browser/bluetoo...
File content/browser/bluetooth/bluetooth_dispatcher_host.h (right):

https://codereview.chromium.org/699843003/diff/140001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.h:37: BluetoothError
bluetoothRequestDeviceRejectType_;
On 2014/11/25 23:27:01, nasko (Out until Dec 08) wrote:
> You still need to convert this variable to hacker_case.

Done.

https://codereview.chromium.org/699843003/diff/140001/content/child/bluetooth...
File content/child/bluetooth/bluetooth_dispatcher.h (right):

https://codereview.chromium.org/699843003/diff/140001/content/child/bluetooth...
content/child/bluetooth/bluetooth_dispatcher.h:36: // IPC Send and Receiving
interface, see IPC::Sender and IPC::Listener.
On 2014/11/25 23:27:01, nasko (Out until Dec 08) wrote:
> You don't inherit from IPC::Sender and Listener in the class. Is this
> intentional?

Yes. I've now explained this better above in class comment.

https://codereview.chromium.org/699843003/diff/140001/content/child/bluetooth...
File content/child/bluetooth/web_bluetooth_impl.cc (right):

https://codereview.chromium.org/699843003/diff/140001/content/child/bluetooth...
content/child/bluetooth/web_bluetooth_impl.cc:19: // TODO: Remove old void
version after crrev.com/715613005 lands.
On 2014/11/25 23:27:01, nasko (Out until Dec 08) wrote:
> nit: TODO(username):

Done.

https://codereview.chromium.org/699843003/diff/140001/content/child/bluetooth...
File content/child/bluetooth/web_bluetooth_impl.h (right):

https://codereview.chromium.org/699843003/diff/140001/content/child/bluetooth...
content/child/bluetooth/web_bluetooth_impl.h:29: // TODO: Remove void version
after crrev.com/715613005 lands.
On 2014/11/25 23:27:01, nasko (Out until Dec 08) wrote:
> nit: TODO(username):

Done.

[scheib, 2014-11-26 16:46:36]

The CQ bit was checked by scheib@chromium.org

[commit-bot: I haz the power, 2014-11-26 16:47:00]

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

[commit-bot: I haz the power, 2014-11-26 16:59:14]

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

[commit-bot: I haz the power, 2014-11-26 16:59:15]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[scheib, 2014-11-26 17:21:52]

The CQ bit was checked by scheib@chromium.org

[commit-bot: I haz the power, 2014-11-26 17:23:00]

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

[commit-bot: I haz the power, 2014-11-26 18:21:27]

Committed patchset #4 (id:180001)

[commit-bot: I haz the power, 2014-11-26 18:22:34]

Patchset 4 (id:??) landed as
https://crrev.com/566d082ea134d42ecf0b697ed966c0c2634e2687
Cr-Commit-Position: refs/heads/master@{#305837}

[ortuno, 2015-04-01 19:45:29]

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

[ortuno, 2015-04-01 19:45:30]

https://codereview.chromium.org/699843003/diff/180001/content/child/bluetooth...
File content/child/bluetooth/bluetooth_dispatcher.cc (right):

https://codereview.chromium.org/699843003/diff/180001/content/child/bluetooth...
content/child/bluetooth/bluetooth_dispatcher.cc:69: if
(WorkerTaskRunner::Instance()->CurrentWorkerId())
Why isn't the function "CurrentWorkerId()", defined above, used here?

[scheib, 2015-04-01 19:47:25]

https://codereview.chromium.org/699843003/diff/180001/content/child/bluetooth...
File content/child/bluetooth/bluetooth_dispatcher.cc (right):

https://codereview.chromium.org/699843003/diff/180001/content/child/bluetooth...
content/child/bluetooth/bluetooth_dispatcher.cc:69: if
(WorkerTaskRunner::Instance()->CurrentWorkerId())
On 2015/04/01 19:45:30, ortuno wrote:
> Why isn't the function "CurrentWorkerId()", defined above, used here?

Oversight, this could be cleaned up.