bluetooth: Add disconnect function

bluetooth: Add disconnect function

Adds bings for BluetoothRemoteGATTServer.disconnect. Note that disconnection
is not implemented on Chrome OS.

Tested using an android phone and the peripheral simulator app.

BUG=569730, 539643
Committed: https://crrev.com/7af25d6d244ef3e7e3005c4f6aece351a00c5ed8
Cr-Commit-Position: refs/heads/master@{#369911}

[ortuno, 2015-12-15 00:04:07]

Description was changed from

==========
bluetooth: Add disconnect function

Adds bings for BluetoothRemoteGATTServer.disconnect. Note that disconnection
is not implemented on Chrome OS.

Tested using an android phone and the peripheral simulator app.

BUG=569730
==========

to

==========
bluetooth: Add disconnect function

Adds bings for BluetoothRemoteGATTServer.disconnect. Note that disconnection
is not implemented on Chrome OS.

Tested using an android phone and the peripheral simulator app.

BUG=569730, 539643
==========

[ortuno, 2016-01-14 03:05:06]

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

[ortuno, 2016-01-14 03:05:06]

jyasskin: PTAL

[Jeffrey Yasskin, 2016-01-14 22:13:19]

https://codereview.chromium.org/1527853002/diff/60001/content/browser/bluetoo...
File content/browser/bluetooth/bluetooth_allowed_devices_map.cc (right):

https://codereview.chromium.org/1527853002/diff/60001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_allowed_devices_map.cc:133: auto id_map_iter
= origin_to_device_id_to_address_map_.find(origin);
I think you don't need to use the origin anymore: just check device_id_set_.

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

https://codereview.chromium.org/1527853002/diff/60001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:778: if
(device_id_to_connection_map_.erase(device_id)) {
What happens if the OnDisconnect comes in after CreateGattConnection() was
called, but before it calls OnGATTConnectionCreated?

It should probably result in any connection getting disconnected.

https://codereview.chromium.org/1527853002/diff/60001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:1228: int
frame_routing_id,
It looks like you don't actually need this argument.

https://codereview.chromium.org/1527853002/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/bluetooth/disconnect.html (right):

https://codereview.chromium.org/1527853002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/disconnect.html:14: promise_test(() =>
{
Could you add a name to each of these tests that says what it's testing?

https://codereview.chromium.org/1527853002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/disconnect.html:40: return
device.connectGATT().then(gattServer => {
Could you add a test that calls "device.connectGATT(); device.disconnect()" and
checks that after both promises resolve the device isn't connected?

[ortuno, 2016-01-15 01:00:09]

https://codereview.chromium.org/1527853002/diff/60001/content/browser/bluetoo...
File content/browser/bluetooth/bluetooth_allowed_devices_map.cc (right):

https://codereview.chromium.org/1527853002/diff/60001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_allowed_devices_map.cc:133: auto id_map_iter
= origin_to_device_id_to_address_map_.find(origin);
On 2016/01/14 at 22:13:18, Jeffrey Yasskin wrote:
> I think you don't need to use the origin anymore: just check device_id_set_.

Done.

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

https://codereview.chromium.org/1527853002/diff/60001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:778: if
(device_id_to_connection_map_.erase(device_id)) {
On 2016/01/14 at 22:13:18, Jeffrey Yasskin wrote:
> What happens if the OnDisconnect comes in after CreateGattConnection() was
called, but before it calls OnGATTConnectionCreated?
> 
> It should probably result in any connection getting disconnected.

As discussed, we should do this once we can cancel pending connection attempts
since it will make it much easier.

https://codereview.chromium.org/1527853002/diff/60001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:1228: int
frame_routing_id,
On 2016/01/14 at 22:13:18, Jeffrey Yasskin wrote:
> It looks like you don't actually need this argument.

Done.

https://codereview.chromium.org/1527853002/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/bluetooth/disconnect.html (right):

https://codereview.chromium.org/1527853002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/disconnect.html:14: promise_test(() =>
{
On 2016/01/14 at 22:13:18, Jeffrey Yasskin wrote:
> Could you add a name to each of these tests that says what it's testing?

Done.

https://codereview.chromium.org/1527853002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/disconnect.html:40: return
device.connectGATT().then(gattServer => {
On 2016/01/14 at 22:13:18, Jeffrey Yasskin wrote:
> Could you add a test that calls "device.connectGATT(); device.disconnect()"
and checks that after both promises resolve the device isn't connected?

Hmm there is no device.disconnect.

[Jeffrey Yasskin, 2016-01-15 01:12:19]

LGTM with the unused origin parameter removed.

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

https://codereview.chromium.org/1527853002/diff/60001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:778: if
(device_id_to_connection_map_.erase(device_id)) {
On 2016/01/15 01:00:08, ortuno wrote:
> On 2016/01/14 at 22:13:18, Jeffrey Yasskin wrote:
> > What happens if the OnDisconnect comes in after CreateGattConnection() was
> called, but before it calls OnGATTConnectionCreated?
> > 
> > It should probably result in any connection getting disconnected.
> 
> As discussed, we should do this once we can cancel pending connection attempts
> since it will make it much easier.

Yep.

https://codereview.chromium.org/1527853002/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/bluetooth/disconnect.html (right):

https://codereview.chromium.org/1527853002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/disconnect.html:40: return
device.connectGATT().then(gattServer => {
On 2016/01/15 01:00:08, ortuno wrote:
> On 2016/01/14 at 22:13:18, Jeffrey Yasskin wrote:
> > Could you add a test that calls "device.connectGATT(); device.disconnect()"
> and checks that after both promises resolve the device isn't connected?
> 
> Hmm there is no device.disconnect.

True. It'd have to be:
device.connectGATT().then(gattServer => {
  gattServer.disconnect();
  device.connectGATT();
  gattServer.disconnect();
})

But we can wait on this until disconnection is better implemented in the device
layer.

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

https://codereview.chromium.org/1527853002/diff/80001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_allowed_devices_map.cc:131: const
url::Origin& origin) {
Now that you're not using it, remove |origin| from the parameter list.

https://codereview.chromium.org/1527853002/diff/80001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/bluetooth/disconnect.html (right):

https://codereview.chromium.org/1527853002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/disconnect.html:50: }, 'Disconnect
twice still results in \'connected\' being false.');
This is "Connect + disconnect twice", right?

[ortuno, 2016-01-15 01:33:14]

Thanks!

https://codereview.chromium.org/1527853002/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/bluetooth/disconnect.html (right):

https://codereview.chromium.org/1527853002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/disconnect.html:40: return
device.connectGATT().then(gattServer => {
On 2016/01/15 at 01:12:19, Jeffrey Yasskin wrote:
> On 2016/01/15 01:00:08, ortuno wrote:
> > On 2016/01/14 at 22:13:18, Jeffrey Yasskin wrote:
> > > Could you add a test that calls "device.connectGATT();
device.disconnect()"
> > and checks that after both promises resolve the device isn't connected?
> > 
> > Hmm there is no device.disconnect.
> 
> True. It'd have to be:
> device.connectGATT().then(gattServer => {
>   gattServer.disconnect();
>   device.connectGATT();
>   gattServer.disconnect();
> })
> 
> But we can wait on this until disconnection is better implemented in the
device layer.

Ack.

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

https://codereview.chromium.org/1527853002/diff/80001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_allowed_devices_map.cc:131: const
url::Origin& origin) {
On 2016/01/15 at 01:12:19, Jeffrey Yasskin wrote:
> Now that you're not using it, remove |origin| from the parameter list.

Done.

https://codereview.chromium.org/1527853002/diff/80001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/bluetooth/disconnect.html (right):

https://codereview.chromium.org/1527853002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/disconnect.html:50: }, 'Disconnect
twice still results in \'connected\' being false.');
On 2016/01/15 at 01:12:19, Jeffrey Yasskin wrote:
> This is "Connect + disconnect twice", right?

Done.

[ortuno, 2016-01-15 01:34:11]

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

[ortuno, 2016-01-15 01:34:12]

asvitkine: PTAL at histograms

[Alexei Svitkine (slow), 2016-01-15 14:12:12]

lgtm

[ortuno, 2016-01-15 16:20:54]

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

[ortuno, 2016-01-15 16:20:54]

palmer: PTAL

[ortuno, 2016-01-15 16:21:04]

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

[commit-bot: I haz the power, 2016-01-15 16:21:14]

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

[commit-bot: I haz the power, 2016-01-15 16:40:40]

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

[commit-bot: I haz the power, 2016-01-15 16:40:41]

Dry run: Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)

[Jeffrey Yasskin, 2016-01-15 19:05:44]

https://codereview.chromium.org/1527853002/diff/130001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_allowed_devices_map.h (right):

https://codereview.chromium.org/1527853002/diff/130001/content/browser/blueto...
content/browser/bluetooth/bluetooth_allowed_devices_map.h:76:
origin_to_device_id_to_address_map_;
Now that device IDs are unique, you could remove the Origin level of this and
the next map, fixing at least part of Chris's concern from the previous CL. You
might need |device_id_set_| to become a map from the device ID to is Origin, in
order to check that an origin hasn't guessed another origin's ID, but the fact
that IDs are 128 random bits mitigates that fairly well.

The origin->address->id map could probably become pair<origin,address>->id.

[ortuno, 2016-01-15 23:49:11]

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

[ortuno, 2016-01-15 23:49:11]

Adding tsepez for content/common review since palmer is busy today.

[Tom Sepez, 2016-01-16 00:03:57]

lgtm

[ortuno, 2016-01-16 00:59:22]

The CQ bit was checked by ortuno@chromium.org

[ortuno, 2016-01-16 00:59:23]

The patchset sent to the CQ was uploaded after l-g-t-m from
jyasskin@chromium.org, asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/1527853002/#ps190001
(title: "Fix test that fails after upstream change.")

[commit-bot: I haz the power, 2016-01-16 00:59:52]

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

[commit-bot: I haz the power, 2016-01-16 01:11:04]

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

[commit-bot: I haz the power, 2016-01-16 01:11:05]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[ortuno, 2016-01-16 01:35:42]

The CQ bit was checked by ortuno@chromium.org

[commit-bot: I haz the power, 2016-01-16 01:36:43]

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

[commit-bot: I haz the power, 2016-01-16 01:42:56]

Committed patchset #11 (id:190001)

[commit-bot: I haz the power, 2016-01-16 01:43:44]

Description was changed from

==========
bluetooth: Add disconnect function

Adds bings for BluetoothRemoteGATTServer.disconnect. Note that disconnection
is not implemented on Chrome OS.

Tested using an android phone and the peripheral simulator app.

BUG=569730, 539643
==========

to

==========
bluetooth: Add disconnect function

Adds bings for BluetoothRemoteGATTServer.disconnect. Note that disconnection
is not implemented on Chrome OS.

Tested using an android phone and the peripheral simulator app.

BUG=569730, 539643

Committed: https://crrev.com/7af25d6d244ef3e7e3005c4f6aece351a00c5ed8
Cr-Commit-Position: refs/heads/master@{#369911}
==========

[commit-bot: I haz the power, 2016-01-16 01:43:44]

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