bluetooth: Stop firing value changed events after gatt disconnects

bluetooth: Stop firing value changed events after gatt disconnects

Checks gatt.connected before firing an event.

BUG=681436
Review-Url: https://codereview.chromium.org/2648583004
Cr-Commit-Position: refs/heads/master@{#446809}
Committed: https://chromium.googlesource.com/chromium/src/+/c4ac85eec321202ca4ecd56f0364dd49d9fb8236

[ortuno, 2017-01-20 02:59:51]

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

[commit-bot: I haz the power, 2017-01-20 03:00:14]

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

[ortuno, 2017-01-20 04:08:43]

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

[ortuno, 2017-01-20 04:08:43]

jyasskin: PTAL :)

[commit-bot: I haz the power, 2017-01-20 04:46:10]

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

[commit-bot: I haz the power, 2017-01-20 04:46:11]

Dry run: This issue passed the CQ dry run.

[Jeffrey Yasskin, 2017-01-20 21:29:41]

LGTM with nits.

https://codereview.chromium.org/2648583004/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/bluetooth/characteristic/notifications/notification-after-disconnection.html
(right):

https://codereview.chromium.org/2648583004/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/bluetooth/characteristic/notifications/notification-after-disconnection.html:37:
let event_listener = e => {
Comment what this is waiting for.

https://codereview.chromium.org/2648583004/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/bluetooth/characteristic/notifications/notification-after-disconnection.html:47:
return assert_no_events(characteristic,
Comment why we expect the browser to still receive notifications after this
window's disconnection.

https://codereview.chromium.org/2648583004/diff/1/third_party/WebKit/Source/m...
File
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp
(right):

https://codereview.chromium.org/2648583004/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp:54:
if (!getGatt()->connected())
Did/could you double-check that we don't have any other cases where we should be
checking for disconnection?

[ortuno, 2017-01-24 02:38:25]

https://codereview.chromium.org/2648583004/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/bluetooth/characteristic/notifications/notification-after-disconnection.html
(right):

https://codereview.chromium.org/2648583004/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/bluetooth/characteristic/notifications/notification-after-disconnection.html:37:
let event_listener = e => {
On 2017/01/20 at 21:29:41, Jeffrey Yasskin wrote:
> Comment what this is waiting for.

Done.

https://codereview.chromium.org/2648583004/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/bluetooth/characteristic/notifications/notification-after-disconnection.html:47:
return assert_no_events(characteristic,
On 2017/01/20 at 21:29:41, Jeffrey Yasskin wrote:
> Comment why we expect the browser to still receive notifications after this
window's disconnection.

Added comment up top and here as well.

https://codereview.chromium.org/2648583004/diff/1/third_party/WebKit/Source/m...
File
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp
(right):

https://codereview.chromium.org/2648583004/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp:54:
if (!getGatt()->connected())
On 2017/01/20 at 21:29:41, Jeffrey Yasskin wrote:
> Did/could you double-check that we don't have any other cases where we should
be checking for disconnection?

So there are 7 instances of checking for connected in the spec:

1. GetGATTChildren [1] which is done in [2][3][4]
2. disconnect [5] which is done in [6]
3. Garbage Collect connection [7] is implemented on the browser side by the
device/ layer.
4. characteristic.readValue/writeValue/startNotifications [8][9]10] are done in
[11][12][13][14]. I'm surprised we don't check stopNotifications in the spec but
we do in chrome. Not sure if spec bug or chrome bug.
5. descriptor.readValue/writeValue [15][16]. readValue is being done in [17]
6. Responding to disconnection [18] is done in [19]
7. Responding to notifications and indications is being done in this patch.

[1]
https://webbluetoothcg.github.io/web-bluetooth/#ref-for-dom-bluetoothremotega...
[2]
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/blueto...
[3]
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/blueto...
[4]
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/blueto...
[5]
https://webbluetoothcg.github.io/web-bluetooth/#ref-for-dom-bluetoothremotega...
[6]
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/blueto...
[7]
https://webbluetoothcg.github.io/web-bluetooth/#ref-for-dom-bluetoothremotega...
[8]
https://webbluetoothcg.github.io/web-bluetooth/#ref-for-dom-bluetoothremotega...
[9]
https://webbluetoothcg.github.io/web-bluetooth/#ref-for-dom-bluetoothremotega...
[10]
https://webbluetoothcg.github.io/web-bluetooth/#ref-for-dom-bluetoothremotega...
[11]
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/blueto...
[12]
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/blueto...
[13]
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/blueto...
[14]
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/blueto...
[15]
https://webbluetoothcg.github.io/web-bluetooth/#ref-for-dom-bluetoothremotega...
[16]
https://webbluetoothcg.github.io/web-bluetooth/#ref-for-dom-bluetoothremotega...
[17]
https://codereview.chromium.org/2637343002/diff/60001/third_party/WebKit/Sour...
[18]
https://webbluetoothcg.github.io/web-bluetooth/#ref-for-dom-bluetoothremotega...
[19]
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/blueto...

[ortuno, 2017-01-27 17:29:04]

The CQ bit was checked by ortuno@chromium.org

[ortuno, 2017-01-27 17:29:05]

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/2648583004/#ps20001
(title: "Make iframe subscribe to notifications and add comments.")

[commit-bot: I haz the power, 2017-01-27 17:29:35]

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, 2017-01-27 17:37:36]

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

[commit-bot: I haz the power, 2017-01-27 17:37:37]

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

[ortuno, 2017-01-27 22:46:41]

The CQ bit was checked by ortuno@chromium.org

[commit-bot: I haz the power, 2017-01-27 22:47:25]

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, 2017-01-27 22:53:32]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1485557201180820,
"parent_rev": "cfefdd7db190a5161e39488c1f09e36635f7b8d4", "commit_rev":
"c4ac85eec321202ca4ecd56f0364dd49d9fb8236"}

[commit-bot: I haz the power, 2017-01-27 22:54:06]

Description was changed from

==========
bluetooth: Stop firing value changed events after gatt disconnects

Checks gatt.connected before firing an event.

BUG=681436
==========

to

==========
bluetooth: Stop firing value changed events after gatt disconnects

Checks gatt.connected before firing an event.

BUG=681436

Review-Url: https://codereview.chromium.org/2648583004
Cr-Commit-Position: refs/heads/master@{#446809}
Committed:
https://chromium.googlesource.com/chromium/src/+/c4ac85eec321202ca4ecd56f0364...
==========

[commit-bot: I haz the power, 2017-01-27 22:54:07]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/c4ac85eec321202ca4ecd56f0364...