Put the RenderFrame's ID into a per-frame WebBluetooth, and plumb that back to the browser.

Put the RenderFrame's ID into a per-frame WebBluetooth, and plumb that back to the browser.

This lets us look up the right RenderFrameHost, so we can find the site's origin
and attach the requestDevice dialog to the right tab.

This is the third of a 3-patch sequence:
1. Attach a frame-specific WebBluetooth in Blink. (https://codereview.chromium.org/1228283003)
2. Move Web Bluetooth from content/child/ to content/renderer/ (https://codereview.chromium.org/1234413006/)
3. Put the RenderFrame's ID in that WebBluetooth and plumb it back to the browser. (This patch)

BUG=500989
Committed: https://crrev.com/7aa22257bc748edd0dfab2e04df87fa251492f6e
Cr-Commit-Position: refs/heads/master@{#339600}

[Jeffrey Yasskin, 2015-07-15 07:36:12]

Patchset #1 (id:1) has been deleted

[Jeffrey Yasskin, 2015-07-15 07:40:44]

jyasskin@chromium.org changed reviewers:
+ jochen@chromium.org, scheib@chromium.org, tsepez@chromium.org

[Jeffrey Yasskin, 2015-07-15 07:40:45]

PTAL. No rush, as I won't be able to get back to this until Thursday.

Tom for bluetooth_messages.h; Jochen for render_{process,frame} changes.

[jochen (gone - plz use gerrit), 2015-07-15 13:40:55]

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

https://codereview.chromium.org/1228113004/diff/20001/content/child/bluetooth...
content/child/bluetooth/web_bluetooth_impl.cc:20:
frame_routing_id_(frame_routing_id) {
stuff in child/ shouldn't know about frames.

why is bluetooth in child/ anyway, does anything but renderer use it?

[Jeffrey Yasskin, 2015-07-15 15:47:17]

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

https://codereview.chromium.org/1228113004/diff/20001/content/child/bluetooth...
content/child/bluetooth/web_bluetooth_impl.cc:20:
frame_routing_id_(frame_routing_id) {
On 2015/07/15 13:40:55, jochen wrote:
> stuff in child/ shouldn't know about frames.

Would this be better if I called it a "routing_id" instead of a
"frame_routing_id"? The code in child/ isn't using frames in any way; it's just
propagating an ID back to the browser. There's a bunch of instances of code in
content/child and content/common using routing_ids.

> why is bluetooth in child/ anyway, does anything but renderer use it?

Right now, Web Bluetooth is only exposed on window.navigator, and not in
Workers, but we anticipate most of the API eventually being accessible to
Workers and wanted to architect the code to make sure that's straightforward.
The one part that *won't* be accessible is requestDevice(), since that needs to
be called from a Window in order to show its chooser dialog. But it seemed like
overkill to add a whole extra interface in content/renderer to handle that one
function.

[jochen (gone - plz use gerrit), 2015-07-15 15:53:16]

ok, then call it routing id, so it doesn't imply that it corresponds to a frame

[jochen (gone - plz use gerrit), 2015-07-15 15:53:37]

On 2015/07/15 at 15:53:16, jochen wrote:
> ok, then call it routing id, so it doesn't imply that it corresponds to a
frame

or why not get your own routing id anyway?

[Jeffrey Yasskin, 2015-07-15 16:23:21]

On 2015/07/15 15:53:37, jochen wrote:
> On 2015/07/15 at 15:53:16, jochen wrote:
> > ok, then call it routing id, so it doesn't imply that it corresponds to a
> frame
> 
> or why not get your own routing id anyway?

I'm not sure I understand: how would creating a new routing ID for the
WebBluetoothImpl (which I think we'd have to send over from the browser
process?) help the code in
content/browser/bluetooth/bluetooth_dispatcher_host.cc look up the RenderFrame
that made the call, if such a frame exists? The code in child/ doesn't use the
frame (or other thing-with-a-routing-id), but the code in browser does.

[Tom Sepez, 2015-07-15 16:57:27]

LGTM on the messages (once you figure out how the ID works).

[Jeffrey Yasskin, 2015-07-15 17:16:19]

On 2015/07/15 15:53:16, jochen wrote:
> ok, then call it routing id, so it doesn't imply that it corresponds to a
frame

This is done.

[ortuno, 2015-07-15 23:00:45]

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

[ortuno, 2015-07-15 23:00:46]

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

https://codereview.chromium.org/1228113004/diff/40001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:192:
RequestDeviceSession(int frame_routing_id,
This is called frame_routing_id but you call it routing_id.

https://codereview.chromium.org/1228113004/diff/40001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:241: DLOG(WARNING)
Should we add a histogram for this?

[scheib, 2015-07-16 01:11:46]

LGTM, but again understand-ability (especially without looking up this
particular patch set that introduces the changes) is poor for the construction
of WebBluetooth and the routing ID establishment. I think document the "with a
frame" and "without a frame" construction and access steps somewhere and then
refer to it from other places.

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

https://codereview.chromium.org/1228113004/diff/40001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:199: int
frame_routing_id;
Perhaps add it when we need it? Or if you have a follow up patch that uses it
OK, but would be good to see that patch draft.

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

https://codereview.chromium.org/1228113004/diff/40001/content/common/bluetoot...
content/common/bluetooth/bluetooth_messages.h:192: int /* routing_id */,
Yes, adding frame_routing_id here would make this clearer.

[jochen (gone - plz use gerrit), 2015-07-16 10:51:55]

you can get a routing ID via RenderThreadImpl::GenerateRoutingID()

since you're interacting with the frame on the browser side, I really think the
bluetooth stuff should be on renderer, not in child.

once you add support to workers, you can either extract a shared core to child,
or have a worker specific implementation in worker.

[Jeffrey Yasskin, 2015-07-16 17:15:11]

On 2015/07/16 10:51:55, jochen wrote:
> since you're interacting with the frame on the browser side, I really think
the
> bluetooth stuff should be on renderer, not in child.

'k, we'll move everything over to renderer/ until we need worker support.

[jochen (gone - plz use gerrit), 2015-07-16 17:16:41]

lgtm then

[Jeffrey Yasskin, 2015-07-17 01:32:44]

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

https://codereview.chromium.org/1228113004/diff/40001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:192:
RequestDeviceSession(int frame_routing_id,
On 2015/07/15 23:00:46, ortuno wrote:
> This is called frame_routing_id but you call it routing_id.

Yep, this is undone now that it's no longer coming from content/child.

https://codereview.chromium.org/1228113004/diff/40001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:199: int
frame_routing_id;
On 2015/07/16 01:11:46, scheib wrote:
> Perhaps add it when we need it? Or if you have a follow up patch that uses it
> OK, but would be good to see that patch draft.

I've undone this storage. We'll see how it needs to be stored when I wire up the
dialog.

https://codereview.chromium.org/1228113004/diff/40001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:241: DLOG(WARNING)
On 2015/07/15 23:00:46, ortuno wrote:
> Should we add a histogram for this?

Will do, but not today. I'll ping Monday when it's done.

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

https://codereview.chromium.org/1228113004/diff/40001/content/common/bluetoot...
content/common/bluetooth/bluetooth_messages.h:192: int /* routing_id */,
On 2015/07/16 01:11:46, scheib wrote:
> Yes, adding frame_routing_id here would make this clearer.

Done.

[scheib, 2015-07-17 19:19:17]

LGTM still

[Jeffrey Yasskin, 2015-07-21 00:47:05]

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

https://codereview.chromium.org/1228113004/diff/40001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:241: DLOG(WARNING)
On 2015/07/17 01:32:44, Jeffrey Yasskin wrote:
> On 2015/07/15 23:00:46, ortuno wrote:
> > Should we add a histogram for this?
> 
> Will do, but not today. I'll ping Monday when it's done.

Done in https://codereview.chromium.org/1248573002.

[ortuno, 2015-07-21 00:49:33]

lgtm

[Jeffrey Yasskin, 2015-07-21 01:21:32]

The CQ bit was checked by jyasskin@chromium.org

[Jeffrey Yasskin, 2015-07-21 01:21:32]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
jochen@chromium.org, scheib@chromium.org
Link to the patchset: https://codereview.chromium.org/1228113004/#ps100001
(title: "Rebase")

[commit-bot: I haz the power, 2015-07-21 01:22:09]

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

[commit-bot: I haz the power, 2015-07-21 02:40:36]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2015-07-21 02:41:26]

Patchset 5 (id:??) landed as
https://crrev.com/7aa22257bc748edd0dfab2e04df87fa251492f6e
Cr-Commit-Position: refs/heads/master@{#339600}