bluetooth: {start,stop}Notifications() return the characteristic

bluetooth: {start,stop}Notifications() return the characteristic

See Spec PR at https://github.com/WebBluetoothCG/web-bluetooth/pull/264

BUG=529560
Committed: https://crrev.com/e1ce2f1c64c95ca852601407c15906b19ca26d03
Cr-Commit-Position: refs/heads/master@{#410201}

[fbeaufortchromium, 2016-08-04 12:36:37]

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

[commit-bot: I haz the power, 2016-08-04 12:36:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-04 14:15:59]

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

[commit-bot: I haz the power, 2016-08-04 14:16:00]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[François Beaufort, 2016-08-04 14:25:12]

Patchset #1 (id:1) has been deleted

[François Beaufort, 2016-08-04 14:26:11]

beaufort.francois@gmail.com changed reviewers:
+ ortuno@chromium.org

[François Beaufort, 2016-08-04 14:26:12]

Hey Gio,

Here's the patch for https://github.com/WebBluetoothCG/web-bluetooth/pull/264.
Let me know what you think.

[ortuno, 2016-08-05 01:43:44]

https://codereview.chromium.org/2216623002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/bluetooth/notifications/stop-after-start-succeeds.html
(right):

https://codereview.chromium.org/2216623002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/notifications/stop-after-start-succeeds.html:19:
'Result should be the same as characteristic.');
You could do:

  .then(characteristic => {
    return characteristic.startNotifications()
      .then(start_characteristic => {
          assert_equals(start_characteristic, characteristic, ...);
          return characteristic.stopNotifications()
            .then(stop_characteristic => { 
               // assert the same as characteristic and start_characteristic
            });
       });
   });

https://codereview.chromium.org/2216623002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp
(right):

https://codereview.chromium.org/2216623002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp:218:
WeakPersistent<BluetoothRemoteGATTCharacteristic> m_webCharacteristic;
This needs to be Persistent. Otherwise you might resolve the promise with null.

https://codereview.chromium.org/2216623002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.idl
(right):

https://codereview.chromium.org/2216623002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.idl:19:
[CallWith=ScriptState] Promise<DataView>             readValue();
nit: Line this up, I think.

[haraken, 2016-08-05 04:27:57]

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

[haraken, 2016-08-05 04:27:58]

https://codereview.chromium.org/2216623002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp
(right):

https://codereview.chromium.org/2216623002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp:218:
WeakPersistent<BluetoothRemoteGATTCharacteristic> m_webCharacteristic;
On 2016/08/05 01:43:44, ortuno wrote:
> This needs to be Persistent. Otherwise you might resolve the promise with
null.

Are you sure that the persistents don't create a cycle? i.e., is it guaranteed
that onSuccess or onError is called in a finite time period?

[François Beaufort, 2016-08-05 06:41:16]

https://codereview.chromium.org/2216623002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/bluetooth/notifications/stop-after-start-succeeds.html
(right):

https://codereview.chromium.org/2216623002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/notifications/stop-after-start-succeeds.html:19:
'Result should be the same as characteristic.');
On 2016/08/05 01:43:44, ortuno wrote:
> You could do:
> 
>   .then(characteristic => {
>     return characteristic.startNotifications()
>       .then(start_characteristic => {
>           assert_equals(start_characteristic, characteristic, ...);
>           return characteristic.stopNotifications()
>             .then(stop_characteristic => { 
>                // assert the same as characteristic and start_characteristic
>             });
>        });
>    });

Done.

https://codereview.chromium.org/2216623002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.idl
(right):

https://codereview.chromium.org/2216623002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.idl:19:
[CallWith=ScriptState] Promise<DataView>             readValue();
On 2016/08/05 01:43:44, ortuno wrote:
> nit: Line this up, I think.

Done.

[fbeaufortchromium, 2016-08-05 07:14:59]

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

[commit-bot: I haz the power, 2016-08-05 07:15:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-05 08:16:36]

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

[commit-bot: I haz the power, 2016-08-05 08:16:37]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[ortuno, 2016-08-05 14:35:38]

https://codereview.chromium.org/2216623002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp
(right):

https://codereview.chromium.org/2216623002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp:218:
WeakPersistent<BluetoothRemoteGATTCharacteristic> m_webCharacteristic;
On 2016/08/05 at 04:27:58, haraken wrote:
> On 2016/08/05 01:43:44, ortuno wrote:
> > This needs to be Persistent. Otherwise you might resolve the promise with
null.
> 
> Are you sure that the persistents don't create a cycle? i.e., is it guaranteed
that onSuccess or onError is called in a finite time period?

Yes, it's guaranteed. If we don't ever call onSuccess or onError then it's a bug
on our mojo service.

[François Beaufort, 2016-08-05 20:41:56]

On 2016/08/05 14:35:38, ortuno wrote:
>
https://codereview.chromium.org/2216623002/diff/20001/third_party/WebKit/Sour...
> File
>
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp
> (right):
> 
>
https://codereview.chromium.org/2216623002/diff/20001/third_party/WebKit/Sour...
>
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp:218:
> WeakPersistent<BluetoothRemoteGATTCharacteristic> m_webCharacteristic;
> On 2016/08/05 at 04:27:58, haraken wrote:
> > On 2016/08/05 01:43:44, ortuno wrote:
> > > This needs to be Persistent. Otherwise you might resolve the promise with
> null.
> > 
> > Are you sure that the persistents don't create a cycle? i.e., is it
guaranteed
> that onSuccess or onError is called in a finite time period?
> 
> Yes, it's guaranteed. If we don't ever call onSuccess or onError then it's a
bug
> on our mojo service.

Done.

[ortuno, 2016-08-05 20:47:58]

lgtm

[François Beaufort, 2016-08-05 20:51:59]

The CQ bit was checked by beaufort.francois@gmail.com

[commit-bot: I haz the power, 2016-08-05 20:52:17]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-05 22:38:03]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-08-05 22:40:56]

Description was changed from

==========
bluetooth: {start,stop}Notifications() return the characteristic

See Spec PR at https://github.com/WebBluetoothCG/web-bluetooth/pull/264

BUG=529560
==========

to

==========
bluetooth: {start,stop}Notifications() return the characteristic

See Spec PR at https://github.com/WebBluetoothCG/web-bluetooth/pull/264

BUG=529560

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

[commit-bot: I haz the power, 2016-08-05 22:40:57]

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

[haraken, 2016-08-08 00:49:06]

On 2016/08/05 14:35:38, ortuno wrote:
>
https://codereview.chromium.org/2216623002/diff/20001/third_party/WebKit/Sour...
> File
>
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp
> (right):
> 
>
https://codereview.chromium.org/2216623002/diff/20001/third_party/WebKit/Sour...
>
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp:218:
> WeakPersistent<BluetoothRemoteGATTCharacteristic> m_webCharacteristic;
> On 2016/08/05 at 04:27:58, haraken wrote:
> > On 2016/08/05 01:43:44, ortuno wrote:
> > > This needs to be Persistent. Otherwise you might resolve the promise with
> null.
> > 
> > Are you sure that the persistents don't create a cycle? i.e., is it
guaranteed
> that onSuccess or onError is called in a finite time period?
> 
> Yes, it's guaranteed. If we don't ever call onSuccess or onError then it's a
bug
> on our mojo service.

Thanks, LGTM