bluetooth: Move connect/disconnect to mojo

bluetooth: Move connect/disconnect to mojo

This change also adds a new BluetoothConnectedDevicesMap class to keep track of
connected devices. This class was abstracted from BluetoothDispatcherHost.

BUG=508771
Committed: https://crrev.com/b6c45d4f0ad3d8ec7fd89b87b0f53fdf12cda55d
Cr-Commit-Position: refs/heads/master@{#392244}

[ortuno, 2016-04-28 22:44:55]

Description was changed from

==========
bluetooth: (WIP) Move connect/disconnect to mojo

BUG=
==========

to

==========
bluetooth: Move connect/disconnect to mojo

BUG=508771
==========

[ortuno, 2016-04-28 23:02:14]

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

[ortuno, 2016-04-28 23:02:15]

jyasskin: PTAL

[ortuno, 2016-04-28 23:31:54]

Description was changed from

==========
bluetooth: Move connect/disconnect to mojo

BUG=508771
==========

to

==========
bluetooth: Move connect/disconnect to mojo

This change also adds a new BluetoothConnectedDevicesMap class to keep track of
connected devices. This class was abstracted from BluetoothDispatcherHost.

BUG=508771
==========

[Jeffrey Yasskin, 2016-05-03 01:15:57]

https://codereview.chromium.org/1902153003/diff/140001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_connected_devices_map.cc (right):

https://codereview.chromium.org/1902153003/diff/140001/content/browser/blueto...
content/browser/bluetooth/bluetooth_connected_devices_map.cc:58: // A frame
should never try to connect to an already connected device.
I'm not sure whether we should detect duplicate connection attempts on the
browser or renderer side. This comment implies that you're now convinced it
should happen on the renderer side. How come? It looks like notifications are
moving to the browser side.

https://codereview.chromium.org/1902153003/diff/140001/content/browser/blueto...
content/browser/bluetooth/bluetooth_connected_devices_map.cc:77:
DCHECK(device_address_to_id_map_.erase(
Oops, the contents of DCHECK won't run in release mode. To DCHECK things that
should run in released Chrome, we have to assign them to local variables
unconditionally and then DCHECK the variables.

https://codereview.chromium.org/1902153003/diff/140001/content/browser/blueto...
content/browser/bluetooth/bluetooth_connected_devices_map.cc:79:
DCHECK(device_id_to_connection_map_.erase(device_id));
May as well erase(connection_iter) here, to avoid a redundant lookup.

https://codereview.chromium.org/1902153003/diff/140001/content/browser/blueto...
content/browser/bluetooth/bluetooth_connected_devices_map.cc:87: return
base::EmptyString();
The main reason to use EmptyString() is if you're returning a string&. When
you're returning by value, it's simpler to return std::string().

https://codereview.chromium.org/1902153003/diff/140001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_connected_devices_map.h (right):

https://codereview.chromium.org/1902153003/diff/140001/content/browser/blueto...
content/browser/bluetooth/bluetooth_connected_devices_map.h:25: // when new
connections are made or connections closed. WebContents should
s/should/must/

https://codereview.chromium.org/1902153003/diff/140001/content/browser/blueto...
content/browser/bluetooth/bluetooth_connected_devices_map.h:30: class
CONTENT_EXPORT BluetoothConnectedDevicesMap final {
I think I'd name this "FrameConnectedBluetoothDevices" or similar, to emphasize
that this records the devices that are connected to a particular frame, and not
any wider scope.

https://codereview.chromium.org/1902153003/diff/140001/content/browser/blueto...
content/browser/bluetooth/bluetooth_connected_devices_map.h:32: explicit
BluetoothConnectedDevicesMap(WebContents* web_contents);
If this argument flows directly into this->web_contents_impl_, you should have a
comment here that matches the one on web_contents_impl_.

https://codereview.chromium.org/1902153003/diff/140001/content/browser/blueto...
content/browser/bluetooth/bluetooth_connected_devices_map.h:38: // If a
connection doesn't exist already for |device_id| adds a connection to
Add a comma before "adds".

https://codereview.chromium.org/1902153003/diff/140001/content/browser/blueto...
content/browser/bluetooth/bluetooth_connected_devices_map.h:64: // get closed..
s/.././

https://codereview.chromium.org/1902153003/diff/140001/content/renderer/bluet...
File content/renderer/bluetooth/web_bluetooth_impl.cc (right):

https://codereview.chromium.org/1902153003/diff/140001/content/renderer/bluet...
content/renderer/bluetooth/web_bluetooth_impl.cc:50: void
WebBluetoothImpl::disconnect(const blink::WebString& device_id) {
We need this to cancel every in-progress operation for |device_id|, and for all
operations to check that their device is logically connected to this frame:
https://crbug.com/608538

https://codereview.chromium.org/1902153003/diff/140001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom
(right):

https://codereview.chromium.org/1902153003/diff/140001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom:102: //
already then this function increases the ref count to keep that connection
If the connection already existed, this should also allow the current realm to
call functions that require a connection: https://crbug.com/608538

[ortuno, 2016-05-03 16:11:54]

https://codereview.chromium.org/1902153003/diff/140001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_connected_devices_map.cc (right):

https://codereview.chromium.org/1902153003/diff/140001/content/browser/blueto...
content/browser/bluetooth/bluetooth_connected_devices_map.cc:58: // A frame
should never try to connect to an already connected device.
On 2016/05/03 at 01:15:57, Jeffrey Yasskin wrote:
> I'm not sure whether we should detect duplicate connection attempts on the
browser or renderer side. This comment implies that you're now convinced it
should happen on the renderer side. How come? It looks like notifications are
moving to the browser side.

This comment shouldn't imply that. The case I had in mind was
WebBluetoothServiceImpl getting two successive connection requests. Once
http://crbug.com/583544 is fixed, WebBluetoothServiceImpl can check if there is
a pending connection and if so queue the second connection or reject with In
Progress. But for now WebBluetoothServiceImpl would just try to connect twice
and we would get two callbacks for the same connection.

I changed the comment to clarify and also added this comment to
WebBluetoothServiceImpl::RemoteServerConnect.

https://codereview.chromium.org/1902153003/diff/140001/content/browser/blueto...
content/browser/bluetooth/bluetooth_connected_devices_map.cc:77:
DCHECK(device_address_to_id_map_.erase(
On 2016/05/03 at 01:15:57, Jeffrey Yasskin wrote:
> Oops, the contents of DCHECK won't run in release mode. To DCHECK things that
should run in released Chrome, we have to assign them to local variables
unconditionally and then DCHECK the variables.

Ah, I thought we did the same in BluetoothAllowedDevicesMap but turns out we
used CHECK there. Changed to CHECK. Let me know if you prefer the variable
pattern.

https://codereview.chromium.org/1902153003/diff/140001/content/browser/blueto...
content/browser/bluetooth/bluetooth_connected_devices_map.cc:79:
DCHECK(device_id_to_connection_map_.erase(device_id));
On 2016/05/03 at 01:15:57, Jeffrey Yasskin wrote:
> May as well erase(connection_iter) here, to avoid a redundant lookup.

I did it this way because otherwise I have to compare the result with end(). Do
you think the optimization is worth the extra code?

CHECK(device_id_to_connection_map_.erase(connection_iter) !=
device_id_to_connection_map_.end());

https://codereview.chromium.org/1902153003/diff/140001/content/browser/blueto...
content/browser/bluetooth/bluetooth_connected_devices_map.cc:87: return
base::EmptyString();
On 2016/05/03 at 01:15:57, Jeffrey Yasskin wrote:
> The main reason to use EmptyString() is if you're returning a string&. When
you're returning by value, it's simpler to return std::string().

Done.

https://codereview.chromium.org/1902153003/diff/140001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_connected_devices_map.h (right):

https://codereview.chromium.org/1902153003/diff/140001/content/browser/blueto...
content/browser/bluetooth/bluetooth_connected_devices_map.h:25: // when new
connections are made or connections closed. WebContents should
On 2016/05/03 at 01:15:57, Jeffrey Yasskin wrote:
> s/should/must/

Done.

https://codereview.chromium.org/1902153003/diff/140001/content/browser/blueto...
content/browser/bluetooth/bluetooth_connected_devices_map.h:30: class
CONTENT_EXPORT BluetoothConnectedDevicesMap final {
On 2016/05/03 at 01:15:57, Jeffrey Yasskin wrote:
> I think I'd name this "FrameConnectedBluetoothDevices" or similar, to
emphasize that this records the devices that are connected to a particular
frame, and not any wider scope.

Done.

https://codereview.chromium.org/1902153003/diff/140001/content/browser/blueto...
content/browser/bluetooth/bluetooth_connected_devices_map.h:32: explicit
BluetoothConnectedDevicesMap(WebContents* web_contents);
On 2016/05/03 at 01:15:57, Jeffrey Yasskin wrote:
> If this argument flows directly into this->web_contents_impl_, you should have
a comment here that matches the one on web_contents_impl_.

Done.

https://codereview.chromium.org/1902153003/diff/140001/content/browser/blueto...
content/browser/bluetooth/bluetooth_connected_devices_map.h:38: // If a
connection doesn't exist already for |device_id| adds a connection to
On 2016/05/03 at 01:15:57, Jeffrey Yasskin wrote:
> Add a comma before "adds".

Done.

https://codereview.chromium.org/1902153003/diff/140001/content/browser/blueto...
content/browser/bluetooth/bluetooth_connected_devices_map.h:64: // get closed..
On 2016/05/03 at 01:15:57, Jeffrey Yasskin wrote:
> s/.././

Done.

https://codereview.chromium.org/1902153003/diff/140001/content/renderer/bluet...
File content/renderer/bluetooth/web_bluetooth_impl.cc (right):

https://codereview.chromium.org/1902153003/diff/140001/content/renderer/bluet...
content/renderer/bluetooth/web_bluetooth_impl.cc:50: void
WebBluetoothImpl::disconnect(const blink::WebString& device_id) {
On 2016/05/03 at 01:15:57, Jeffrey Yasskin wrote:
> We need this to cancel every in-progress operation for |device_id|, and for
all operations to check that their device is logically connected to this frame:
https://crbug.com/608538

Acknowledged. Thanks for opening the issue!

https://codereview.chromium.org/1902153003/diff/140001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom
(right):

https://codereview.chromium.org/1902153003/diff/140001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom:102: //
already then this function increases the ref count to keep that connection
On 2016/05/03 at 01:15:57, Jeffrey Yasskin wrote:
> If the connection already existed, this should also allow the current realm to
call functions that require a connection: https://crbug.com/608538

Should I add a TODO for that issue here?

[Jeffrey Yasskin, 2016-05-03 16:31:33]

lgtm

https://codereview.chromium.org/1902153003/diff/140001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_connected_devices_map.cc (right):

https://codereview.chromium.org/1902153003/diff/140001/content/browser/blueto...
content/browser/bluetooth/bluetooth_connected_devices_map.cc:58: // A frame
should never try to connect to an already connected device.
On 2016/05/03 16:11:53, ortuno wrote:
> On 2016/05/03 at 01:15:57, Jeffrey Yasskin wrote:
> > I'm not sure whether we should detect duplicate connection attempts on the
> browser or renderer side. This comment implies that you're now convinced it
> should happen on the renderer side. How come? It looks like notifications are
> moving to the browser side.
> 
> This comment shouldn't imply that. The case I had in mind was
> WebBluetoothServiceImpl getting two successive connection requests. Once
> http://crbug.com/583544 is fixed, WebBluetoothServiceImpl can check if there
is
> a pending connection and if so queue the second connection or reject with In
> Progress. But for now WebBluetoothServiceImpl would just try to connect twice
> and we would get two callbacks for the same connection.
> 
> I changed the comment to clarify and also added this comment to
> WebBluetoothServiceImpl::RemoteServerConnect.

Looks good, thanks.

https://codereview.chromium.org/1902153003/diff/140001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom
(right):

https://codereview.chromium.org/1902153003/diff/140001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom:102: //
already then this function increases the ref count to keep that connection
On 2016/05/03 16:11:54, ortuno wrote:
> On 2016/05/03 at 01:15:57, Jeffrey Yasskin wrote:
> > If the connection already existed, this should also allow the current realm
to
> call functions that require a connection: https://crbug.com/608538
> 
> Should I add a TODO for that issue here?

Oops, I meant to remove this comment: I think the disconnect enforcement has to
happen on the renderer side because otherwise you could disconnect while the
response is coming back and still get the response. So there's no need to
comment it in the mojo interface.

https://codereview.chromium.org/1902153003/diff/160001/content/browser/blueto...
File content/browser/bluetooth/frame_connected_bluetooth_devices.cc (right):

https://codereview.chromium.org/1902153003/diff/160001/content/browser/blueto...
content/browser/bluetooth/frame_connected_bluetooth_devices.cc:83:
CHECK(device_id_to_connection_map_.erase(device_id));
On 2016/05/03 16:11:53, ortuno wrote:
> On 2016/05/03 at 01:15:57, Jeffrey Yasskin wrote:
> > May as well erase(connection_iter) here, to avoid a redundant lookup.
> 
> I did it this way because otherwise I have to compare the result with end().
Do
> you think the optimization is worth the extra code?
> 
> CHECK(device_id_to_connection_map_.erase(connection_iter) !=
> device_id_to_connection_map_.end());

erase(iter) is only valid for non-end arguments, and because it returns the
iterator after the erased one, it can return end() even when it erased
something. I'd just remove the CHECK for an iterator argument.

https://codereview.chromium.org/1902153003/diff/160001/content/browser/blueto...
File content/browser/bluetooth/frame_connected_bluetooth_devices.h (right):

https://codereview.chromium.org/1902153003/diff/160001/content/browser/blueto...
content/browser/bluetooth/frame_connected_bluetooth_devices.h:29: // connection
removed from the appropriate maps.
"connection is removed"

https://codereview.chromium.org/1902153003/diff/160001/content/browser/blueto...
content/browser/bluetooth/frame_connected_bluetooth_devices.h:33: //
WebBluetoothServiceImpl that owns this map.
I think we should mention the third level of ownership: "the WebContentsImpl
that owns the RenderFrameHost that owns the WebBluetoothServiceImpl that owns
this object." It'd be possible to just pass in the RenderFrameHost, since we can
look up and cache the WebContentsImpl from there. Maybe we should do that, to
make it clearer which frame this object is attached to?

[ortuno, 2016-05-03 16:54:39]

Thanks!

https://codereview.chromium.org/1902153003/diff/160001/content/browser/blueto...
File content/browser/bluetooth/frame_connected_bluetooth_devices.cc (right):

https://codereview.chromium.org/1902153003/diff/160001/content/browser/blueto...
content/browser/bluetooth/frame_connected_bluetooth_devices.cc:83:
CHECK(device_id_to_connection_map_.erase(device_id));
On 2016/05/03 at 16:31:32, Jeffrey Yasskin wrote:
> On 2016/05/03 16:11:53, ortuno wrote:
> > On 2016/05/03 at 01:15:57, Jeffrey Yasskin wrote:
> > > May as well erase(connection_iter) here, to avoid a redundant lookup.
> > 
> > I did it this way because otherwise I have to compare the result with end().
Do
> > you think the optimization is worth the extra code?
> > 
> > CHECK(device_id_to_connection_map_.erase(connection_iter) !=
> > device_id_to_connection_map_.end());
> 
> erase(iter) is only valid for non-end arguments, and because it returns the
iterator after the erased one, it can return end() even when it erased
something. I'd just remove the CHECK for an iterator argument.

Done.

https://codereview.chromium.org/1902153003/diff/160001/content/browser/blueto...
File content/browser/bluetooth/frame_connected_bluetooth_devices.h (right):

https://codereview.chromium.org/1902153003/diff/160001/content/browser/blueto...
content/browser/bluetooth/frame_connected_bluetooth_devices.h:29: // connection
removed from the appropriate maps.
On 2016/05/03 at 16:31:33, Jeffrey Yasskin wrote:
> "connection is removed"

Done.

https://codereview.chromium.org/1902153003/diff/160001/content/browser/blueto...
content/browser/bluetooth/frame_connected_bluetooth_devices.h:33: //
WebBluetoothServiceImpl that owns this map.
On 2016/05/03 at 16:31:32, Jeffrey Yasskin wrote:
> I think we should mention the third level of ownership: "the WebContentsImpl
that owns the RenderFrameHost that owns the WebBluetoothServiceImpl that owns
this object." It'd be possible to just pass in the RenderFrameHost, since we can
look up and cache the WebContentsImpl from there. Maybe we should do that, to
make it clearer which frame this object is attached to?

Good idea. Changed to take a RenderFrameHost.

[ortuno, 2016-05-03 17:07:52]

ortuno@chromium.org changed reviewers:
+ nick@chromium.org, palmer@chromium.org

[ortuno, 2016-05-03 17:07:53]

nick: PTAL at content/test/BUILD.gn and, since you've reviewed some of my
connection patches in the past could you also take a look at
frame_connected_bluetooth_devices*?
palmer: PTAL at bluetooth_messages.h and web_bluetooth.mojom.

[ortuno, 2016-05-03 20:28:03]

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

[ortuno, 2016-05-03 20:28:05]

rockot: PTAL at mojo bits in web_bluetooth.mojom, web_bluetooth_impl.h/cc and
web_bluetooth_service_impl.h/cc

[Ken Rockot(use gerrit already), 2016-05-03 20:39:30]

lgtm

[ncarter (slow), 2016-05-03 22:20:08]

lgtm

[palmer, 2016-05-06 22:33:04]

lgtm

[ortuno, 2016-05-06 23:03:56]

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

[commit-bot: I haz the power, 2016-05-06 23:04:10]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1902153003/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1902153003/220001

[ortuno, 2016-05-07 01:06:36]

The CQ bit was unchecked by ortuno@chromium.org

[ortuno, 2016-05-07 01:06:40]

The CQ bit was checked by ortuno@chromium.org

[ortuno, 2016-05-07 01:06:40]

The patchset sent to the CQ was uploaded after l-g-t-m from
jyasskin@chromium.org
Link to the patchset: https://codereview.chromium.org/1902153003/#ps220001
(title: "Fix gypi")

[commit-bot: I haz the power, 2016-05-07 01:06:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1902153003/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1902153003/220001

[commit-bot: I haz the power, 2016-05-07 04:19:56]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2016-05-07 04:21:07]

Description was changed from

==========
bluetooth: Move connect/disconnect to mojo

This change also adds a new BluetoothConnectedDevicesMap class to keep track of
connected devices. This class was abstracted from BluetoothDispatcherHost.

BUG=508771
==========

to

==========
bluetooth: Move connect/disconnect to mojo

This change also adds a new BluetoothConnectedDevicesMap class to keep track of
connected devices. This class was abstracted from BluetoothDispatcherHost.

BUG=508771

Committed: https://crrev.com/b6c45d4f0ad3d8ec7fd89b87b0f53fdf12cda55d
Cr-Commit-Position: refs/heads/master@{#392244}
==========

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

Patchset 12 (id:??) landed as
https://crrev.com/b6c45d4f0ad3d8ec7fd89b87b0f53fdf12cda55d
Cr-Commit-Position: refs/heads/master@{#392244}