bluetooth: Add characteristicvaluechanged event

bluetooth: Add characteristicvaluechanged event

https://webbluetoothcg.github.io/web-bluetooth/#dom-bluetoothgattcharacteristic-characteristicvaluechanged

NOPRESUBMIT=true

BUG=483344
Committed: https://crrev.com/601997adf6e30112109528ec4221c0917523d9ee
Cr-Commit-Position: refs/heads/master@{#355166}

[ortuno, 2015-10-15 00:14:46]

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

[ortuno, 2015-10-15 00:14:46]

PTAL

[ortuno, 2015-10-15 00:14:50]

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

[commit-bot: I haz the power, 2015-10-15 00:15:53]

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

[commit-bot: I haz the power, 2015-10-15 00:27:42]

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

[commit-bot: I haz the power, 2015-10-15 00:27:43]

Dry run: 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...)

[Jeffrey Yasskin, 2015-10-15 23:00:52]

https://codereview.chromium.org/1382743002/diff/120001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right):

https://codereview.chromium.org/1382743002/diff/120001/content/browser/blueto...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:387: // Yield to the
event loop so that the event gets dispatched after the
I've filed https://github.com/WebBluetoothCG/web-bluetooth/issues/175 to make
sure the spec matches this behavior, or clearly specifies the opposite. No need
to change your code for now, I think.

https://codereview.chromium.org/1382743002/diff/120001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_dispatcher_host.h (right):

https://codereview.chromium.org/1382743002/diff/120001/content/browser/blueto...
content/browser/bluetooth/bluetooth_dispatcher_host.h:85: void
NotifyActiveCharacteristic(int thread_id,
Comment what this function does, since it's not a BluetoothAdapter::Observer
member.

https://codereview.chromium.org/1382743002/diff/120001/content/browser/blueto...
content/browser/bluetooth/bluetooth_dispatcher_host.h:87: const
std::vector<uint8> value);
Pass these two arguments by const&. Eventually we'll pass this sort of thing by
non-const value, but we need standard library move support for that to work
well.

https://codereview.chromium.org/1382743002/diff/120001/content/renderer/bluet...
File content/renderer/bluetooth/bluetooth_dispatcher.cc (right):

https://codereview.chromium.org/1382743002/diff/120001/content/renderer/bluet...
content/renderer/bluetooth/bluetooth_dispatcher.cc:306: // TODO(ortuno): We
should only unregister a characteristic once
"should only unregister" sounds like we're unregistering in too many cases, but
the bug here looks like it's actually too _few_ cases.

You could also link to the bug about supporting multiple frames, since fixing
that will probably involve passing |characteristic| to
UnregisterCharacteristicObject.

https://codereview.chromium.org/1382743002/diff/120001/content/renderer/bluet...
content/renderer/bluetooth/bluetooth_dispatcher.cc:308: //
http://crbug.com/541390
Nit: Prefer https links.

https://codereview.chromium.org/1382743002/diff/120001/content/shell/browser/...
File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h
(right):

https://codereview.chromium.org/1382743002/diff/120001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:178:
//                 GattCharacteristicValueChanged every 10ms and calls success
Good hack. Can you file a bug to change this to an event-based notification, so
we don't delay by 10ms?

https://codereview.chromium.org/1382743002/diff/120001/device/bluetooth/test/...
File device/bluetooth/test/mock_bluetooth_gatt_notify_session.cc (right):

https://codereview.chromium.org/1382743002/diff/120001/device/bluetooth/test/...
device/bluetooth/test/mock_bluetooth_gatt_notify_session.cc:40: void
MockBluetoothGattNotifySession::TestNotify(
Maybe "DoNotify" since this actually notifies the adapter's observers.

https://codereview.chromium.org/1382743002/diff/120001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/bluetooth/characteristicvaluechanged.html
(right):

https://codereview.chromium.org/1382743002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/characteristicvaluechanged.html:20:
'characteristicvaluechanged');
Add another test that registers 2 'characteristicvaluechanged' listeners, and
check that both fire.

https://codereview.chromium.org/1382743002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/characteristicvaluechanged.html:24: //
TODO(ortuno): The Arraybuffer used to resolve the promise
s/Arraybuffer/ArrayBuffer/

https://codereview.chromium.org/1382743002/diff/120001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/bluetooth/resources/bluetooth-helpers.js
(right):

https://codereview.chromium.org/1382743002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/resources/bluetooth-helpers.js:137:
function event_after_promise(object, func, event) {
Comment what this does.

A better name might be assert_event_fires_after_promise()?

https://codereview.chromium.org/1382743002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/resources/bluetooth-helpers.js:165:
object.addEventListener(event_name, event => {
You probably need to remove this listener, so that the object can be re-used in
a subsequent test.

https://codereview.chromium.org/1382743002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/resources/bluetooth-helpers.js:166:
assert_unreached('Object should not throw an event');
s/throw/fire/, matching https://dom.spec.whatwg.org/#concept-event-fire

https://codereview.chromium.org/1382743002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/bluetooth/BluetoothGATTCharacteristic.cpp
(right):

https://codereview.chromium.org/1382743002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothGATTCharacteristic.cpp:80:
bool BluetoothGATTCharacteristic::addEventListener(const AtomicString&
eventType, PassRefPtrWillBeRawPtr<EventListener> listener, bool useCapture)
Presumably we'll also need to unregister on removeEventListener? Link to
https://crbug.com/541390 if that's the right bug for this.

https://codereview.chromium.org/1382743002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/bluetooth/BluetoothGATTCharacteristic.h
(right):

https://codereview.chromium.org/1382743002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothGATTCharacteristic.h:49: //
Interface required by WebBluetoothGATTCharacteristic
This is just the implementation of WebBluetoothGATTCharacteristic, so say
"WebBluetoothGATTCharacteristic methods:" or "WebBluetoothGATTCharacteristic
interface:".

https://codereview.chromium.org/1382743002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothGATTCharacteristic.h:63: //
Interface required by EventTarget.
Similarly here, these are just EventTarget methods.

https://codereview.chromium.org/1382743002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothGATTCharacteristic.h:73:
PassRefPtr<DOMArrayBuffer> value() const {return m_value;}
Style: Be consistent about spaces within braces. Probably add them to this line.

https://codereview.chromium.org/1382743002/diff/120001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/modules/bluetooth/WebBluetooth.h
(right):

https://codereview.chromium.org/1382743002/diff/120001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/bluetooth/WebBluetooth.h:89: //
Called when addEventListener is called on a characteristic.
Add a blank line since this ends the BluetoothGATTCharacteristic methods
section.

[ortuno, 2015-10-16 01:24:22]

https://codereview.chromium.org/1382743002/diff/120001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right):

https://codereview.chromium.org/1382743002/diff/120001/content/browser/blueto...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:387: // Yield to the
event loop so that the event gets dispatched after the
On 2015/10/15 at 23:00:51, jyasskin-OOO until Oct 30 wrote:
> I've filed https://github.com/WebBluetoothCG/web-bluetooth/issues/175 to make
sure the spec matches this behavior, or clearly specifies the opposite. No need
to change your code for now, I think.

It's possible to miss a notification if the characteristic doesn't have any
listeners attached:

1. startNotifications().
2. Renderer sends IPC to start notifications.
3. Browser sends IPC after notifications have successfully started.
4. CharacteristicValueChanged is called on the observer. Since no
characteristics are registered no IPCs are sent to the renderer.
5. Renderer resolves the startNotifications promise
6. char.addEventListener()
7. Renderer sends an IPC to register the characteristic but by now the
notification has already been missed.

I added a TODO to make sure we remember to check that the implementation matches
the spec. I have a couple of ideas on how we can avoid this issue, but I think
we should land them in a different CL.

https://codereview.chromium.org/1382743002/diff/120001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_dispatcher_host.h (right):

https://codereview.chromium.org/1382743002/diff/120001/content/browser/blueto...
content/browser/bluetooth/bluetooth_dispatcher_host.h:85: void
NotifyActiveCharacteristic(int thread_id,
On 2015/10/15 at 23:00:51, jyasskin-OOO until Oct 30 wrote:
> Comment what this function does, since it's not a BluetoothAdapter::Observer
member.

Done.

https://codereview.chromium.org/1382743002/diff/120001/content/browser/blueto...
content/browser/bluetooth/bluetooth_dispatcher_host.h:87: const
std::vector<uint8> value);
On 2015/10/15 at 23:00:51, jyasskin-OOO until Oct 30 wrote:
> Pass these two arguments by const&. Eventually we'll pass this sort of thing
by non-const value, but we need standard library move support for that to work
well.

Done.

https://codereview.chromium.org/1382743002/diff/120001/content/renderer/bluet...
File content/renderer/bluetooth/bluetooth_dispatcher.cc (right):

https://codereview.chromium.org/1382743002/diff/120001/content/renderer/bluet...
content/renderer/bluetooth/bluetooth_dispatcher.cc:306: // TODO(ortuno): We
should only unregister a characteristic once
On 2015/10/15 at 23:00:51, jyasskin-OOO until Oct 30 wrote:
> "should only unregister" sounds like we're unregistering in too many cases,
but the bug here looks like it's actually too _few_ cases.
> 
> You could also link to the bug about supporting multiple frames, since fixing
that will probably involve passing |characteristic| to
UnregisterCharacteristicObject.

Linked to multiple frames bug. Done.

https://codereview.chromium.org/1382743002/diff/120001/content/renderer/bluet...
content/renderer/bluetooth/bluetooth_dispatcher.cc:308: //
http://crbug.com/541390
On 2015/10/15 at 23:00:51, jyasskin-OOO until Oct 30 wrote:
> Nit: Prefer https links.

Done.

https://codereview.chromium.org/1382743002/diff/120001/content/shell/browser/...
File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h
(right):

https://codereview.chromium.org/1382743002/diff/120001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:178:
//                 GattCharacteristicValueChanged every 10ms and calls success
On 2015/10/15 at 23:00:51, jyasskin-OOO until Oct 30 wrote:
> Good hack. Can you file a bug to change this to an event-based notification,
so we don't delay by 10ms?

Done.

https://codereview.chromium.org/1382743002/diff/120001/device/bluetooth/test/...
File device/bluetooth/test/mock_bluetooth_gatt_notify_session.cc (right):

https://codereview.chromium.org/1382743002/diff/120001/device/bluetooth/test/...
device/bluetooth/test/mock_bluetooth_gatt_notify_session.cc:40: void
MockBluetoothGattNotifySession::TestNotify(
On 2015/10/15 at 23:00:51, jyasskin-OOO until Oct 30 wrote:
> Maybe "DoNotify" since this actually notifies the adapter's observers.

Done.

https://codereview.chromium.org/1382743002/diff/120001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/bluetooth/characteristicvaluechanged.html
(right):

https://codereview.chromium.org/1382743002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/characteristicvaluechanged.html:20:
'characteristicvaluechanged');
On 2015/10/15 at 23:00:51, jyasskin-OOO until Oct 30 wrote:
> Add another test that registers 2 'characteristicvaluechanged' listeners, and
check that both fire.

Done. It adds 3!

https://codereview.chromium.org/1382743002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/characteristicvaluechanged.html:24: //
TODO(ortuno): The Arraybuffer used to resolve the promise
On 2015/10/15 at 23:00:51, jyasskin-OOO until Oct 30 wrote:
> s/Arraybuffer/ArrayBuffer/

Done.

https://codereview.chromium.org/1382743002/diff/120001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/bluetooth/resources/bluetooth-helpers.js
(right):

https://codereview.chromium.org/1382743002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/resources/bluetooth-helpers.js:137:
function event_after_promise(object, func, event) {
On 2015/10/15 at 23:00:51, jyasskin-OOO until Oct 30 wrote:
> Comment what this does.
> 
> A better name might be assert_event_fires_after_promise()?

Yeah I thought about that but I wasn't sure if I async functions should be
called assert_*.
Done though.

https://codereview.chromium.org/1382743002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/resources/bluetooth-helpers.js:165:
object.addEventListener(event_name, event => {
On 2015/10/15 at 23:00:51, jyasskin-OOO until Oct 30 wrote:
> You probably need to remove this listener, so that the object can be re-used
in a subsequent test.

Done.

https://codereview.chromium.org/1382743002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/resources/bluetooth-helpers.js:166:
assert_unreached('Object should not throw an event');
On 2015/10/15 at 23:00:51, jyasskin-OOO until Oct 30 wrote:
> s/throw/fire/, matching https://dom.spec.whatwg.org/#concept-event-fire

Ah I was wondering what was the correct term.
Done.

https://codereview.chromium.org/1382743002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/bluetooth/BluetoothGATTCharacteristic.cpp
(right):

https://codereview.chromium.org/1382743002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothGATTCharacteristic.cpp:80:
bool BluetoothGATTCharacteristic::addEventListener(const AtomicString&
eventType, PassRefPtrWillBeRawPtr<EventListener> listener, bool useCapture)
On 2015/10/15 at 23:00:51, jyasskin-OOO until Oct 30 wrote:
> Presumably we'll also need to unregister on removeEventListener? Link to
https://crbug.com/541390 if that's the right bug for this.

Done.

https://codereview.chromium.org/1382743002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/bluetooth/BluetoothGATTCharacteristic.h
(right):

https://codereview.chromium.org/1382743002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothGATTCharacteristic.h:49: //
Interface required by WebBluetoothGATTCharacteristic
On 2015/10/15 at 23:00:51, jyasskin-OOO until Oct 30 wrote:
> This is just the implementation of WebBluetoothGATTCharacteristic, so say
"WebBluetoothGATTCharacteristic methods:" or "WebBluetoothGATTCharacteristic
interface:".

Done.

https://codereview.chromium.org/1382743002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothGATTCharacteristic.h:63: //
Interface required by EventTarget.
On 2015/10/15 at 23:00:51, jyasskin-OOO until Oct 30 wrote:
> Similarly here, these are just EventTarget methods.

Done.

https://codereview.chromium.org/1382743002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothGATTCharacteristic.h:73:
PassRefPtr<DOMArrayBuffer> value() const {return m_value;}
On 2015/10/15 at 23:00:51, jyasskin-OOO until Oct 30 wrote:
> Style: Be consistent about spaces within braces. Probably add them to this
line.

Done.

https://codereview.chromium.org/1382743002/diff/120001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/modules/bluetooth/WebBluetooth.h
(right):

https://codereview.chromium.org/1382743002/diff/120001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/bluetooth/WebBluetooth.h:89: //
Called when addEventListener is called on a characteristic.
On 2015/10/15 at 23:00:51, jyasskin-OOO until Oct 30 wrote:
> Add a blank line since this ends the BluetoothGATTCharacteristic methods
section.

Done.

[Jeffrey Yasskin, 2015-10-16 01:40:18]

Other than the nits below, lgtm.

https://codereview.chromium.org/1382743002/diff/120001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right):

https://codereview.chromium.org/1382743002/diff/120001/content/browser/blueto...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:387: // Yield to the
event loop so that the event gets dispatched after the
On 2015/10/16 01:24:21, ortuno wrote:
> On 2015/10/15 at 23:00:51, jyasskin-OOO until Oct 30 wrote:
> > I've filed https://github.com/WebBluetoothCG/web-bluetooth/issues/175 to
make
> sure the spec matches this behavior, or clearly specifies the opposite. No
need
> to change your code for now, I think.
> 
> It's possible to miss a notification if the characteristic doesn't have any
> listeners attached:
> 
> 1. startNotifications().
> 2. Renderer sends IPC to start notifications.
> 3. Browser sends IPC after notifications have successfully started.
> 4. CharacteristicValueChanged is called on the observer. Since no
> characteristics are registered no IPCs are sent to the renderer.
> 5. Renderer resolves the startNotifications promise
> 6. char.addEventListener()
> 7. Renderer sends an IPC to register the characteristic but by now the
> notification has already been missed.
> 
> I added a TODO to make sure we remember to check that the implementation
matches
> the spec. I have a couple of ideas on how we can avoid this issue, but I think
> we should land them in a different CL.

Heh, argh. I'm suspicious we'll need to send events as long as the
characteristic is alive, regardless of whether a listener is attached, but
definitely fix this in a different CL.

https://codereview.chromium.org/1382743002/diff/140001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/bluetooth/characteristicvaluechanged.html
(right):

https://codereview.chromium.org/1382743002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/characteristicvaluechanged.html:19:
'readValue',
Check the indentation.

https://codereview.chromium.org/1382743002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/characteristicvaluechanged.html:34: let
char;
It looks like you never use this variable. (And above)

https://codereview.chromium.org/1382743002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/characteristicvaluechanged.html:42:
'readValue',
Indentation

https://codereview.chromium.org/1382743002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/characteristicvaluechanged.html:47: let
event_values = Array.prototype.map.call(results.slice(1), v => new
Uint8Array(v));
Why not results.slice(1).map(...)?

https://codereview.chromium.org/1382743002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/characteristicvaluechanged.html:49: //
TODO(ortuno): The ArrayBuffer used to resolve the promise
Indent this to the body of the for loop.

https://codereview.chromium.org/1382743002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/characteristicvaluechanged.html:50: //
should be the same ArrayBuffer than the one saved in the
s/than/as/

https://codereview.chromium.org/1382743002/diff/140001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/bluetooth/resources/bluetooth-helpers.js
(right):

https://codereview.chromium.org/1382743002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/resources/bluetooth-helpers.js:165:
return Promise.all([Promise.resolve(result),
You can omit the "Promise.resolve" here. Arguments to Promise.all get auto-boxed
into promises.

https://codereview.chromium.org/1382743002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/resources/bluetooth-helpers.js:180:
setTimeout(resolve, 100);
And remove the listener after the timeout.

[ortuno, 2015-10-16 19:37:10]

Patchset #9 (id:160001) has been deleted

[ortuno, 2015-10-16 19:41:00]

Thanks!

https://codereview.chromium.org/1382743002/diff/120001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right):

https://codereview.chromium.org/1382743002/diff/120001/content/browser/blueto...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:387: // Yield to the
event loop so that the event gets dispatched after the
On 2015/10/16 at 01:40:17, jyasskin-OOO until Oct 30 wrote:
> On 2015/10/16 01:24:21, ortuno wrote:
> > On 2015/10/15 at 23:00:51, jyasskin-OOO until Oct 30 wrote:
> > > I've filed https://github.com/WebBluetoothCG/web-bluetooth/issues/175 to
make
> > sure the spec matches this behavior, or clearly specifies the opposite. No
need
> > to change your code for now, I think.
> > 
> > It's possible to miss a notification if the characteristic doesn't have any
> > listeners attached:
> > 
> > 1. startNotifications().
> > 2. Renderer sends IPC to start notifications.
> > 3. Browser sends IPC after notifications have successfully started.
> > 4. CharacteristicValueChanged is called on the observer. Since no
> > characteristics are registered no IPCs are sent to the renderer.
> > 5. Renderer resolves the startNotifications promise
> > 6. char.addEventListener()
> > 7. Renderer sends an IPC to register the characteristic but by now the
> > notification has already been missed.
> > 
> > I added a TODO to make sure we remember to check that the implementation
matches
> > the spec. I have a couple of ideas on how we can avoid this issue, but I
think
> > we should land them in a different CL.
> 
> Heh, argh. I'm suspicious we'll need to send events as long as the
characteristic is alive, regardless of whether a listener is attached, but
definitely fix this in a different CL.

Yup. That's probably going to be the solution.

https://codereview.chromium.org/1382743002/diff/140001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/bluetooth/characteristicvaluechanged.html
(right):

https://codereview.chromium.org/1382743002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/characteristicvaluechanged.html:19:
'readValue',
On 2015/10/16 at 01:40:18, jyasskin-OOO until Oct 30 wrote:
> Check the indentation.

Done.

https://codereview.chromium.org/1382743002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/characteristicvaluechanged.html:34: let
char;
On 2015/10/16 at 01:40:18, jyasskin-OOO until Oct 30 wrote:
> It looks like you never use this variable. (And above)

Done.

https://codereview.chromium.org/1382743002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/characteristicvaluechanged.html:42:
'readValue',
On 2015/10/16 at 01:40:18, jyasskin-OOO until Oct 30 wrote:
> Indentation

Done.

https://codereview.chromium.org/1382743002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/characteristicvaluechanged.html:47: let
event_values = Array.prototype.map.call(results.slice(1), v => new
Uint8Array(v));
On 2015/10/16 at 01:40:18, jyasskin-OOO until Oct 30 wrote:
> Why not results.slice(1).map(...)?

Done.

https://codereview.chromium.org/1382743002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/characteristicvaluechanged.html:49: //
TODO(ortuno): The ArrayBuffer used to resolve the promise
On 2015/10/16 at 01:40:18, jyasskin-OOO until Oct 30 wrote:
> Indent this to the body of the for loop.

Done.

https://codereview.chromium.org/1382743002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/characteristicvaluechanged.html:50: //
should be the same ArrayBuffer than the one saved in the
On 2015/10/16 at 01:40:17, jyasskin-OOO until Oct 30 wrote:
> s/than/as/

Done.

https://codereview.chromium.org/1382743002/diff/140001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/bluetooth/resources/bluetooth-helpers.js
(right):

https://codereview.chromium.org/1382743002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/resources/bluetooth-helpers.js:165:
return Promise.all([Promise.resolve(result),
On 2015/10/16 at 01:40:18, jyasskin-OOO until Oct 30 wrote:
> You can omit the "Promise.resolve" here. Arguments to Promise.all get
auto-boxed into promises.

Done.

https://codereview.chromium.org/1382743002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/resources/bluetooth-helpers.js:180:
setTimeout(resolve, 100);
On 2015/10/16 at 01:40:18, jyasskin-OOO until Oct 30 wrote:
> And remove the listener after the timeout.

Done.

[ortuno, 2015-10-16 19:41:37]

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

[ortuno, 2015-10-16 19:41:38]

scheib: PTAL

[scheib, 2015-10-16 20:39:25]

LGTM, 

Some comments warranted. 

Please add more details to the change description.

The use of timer is decidedly technical debt that should be paid down no later
than just after getting the feature to work.

https://codereview.chromium.org/1382743002/diff/180001/content/shell/browser/...
File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc
(right):

https://codereview.chromium.org/1382743002/diff/180001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:384:
.WillByDefault(RunCallbackWithResult<0>([adapter_ptr, location_ptr]() {
"0 /* success_callback */" just for consistency.

https://codereview.chromium.org/1382743002/diff/180001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:387:
FOR_EACH_OBSERVER(BluetoothAdapter::Observer,
Comment, E.G.:
// Reading a characteristic has a side effect of ValueChanged being called.

https://codereview.chromium.org/1382743002/diff/180001/device/bluetooth/test/...
File device/bluetooth/test/mock_bluetooth_gatt_notify_session.h (right):

https://codereview.chromium.org/1382743002/diff/180001/device/bluetooth/test/...
device/bluetooth/test/mock_bluetooth_gatt_notify_session.h:30: // Starts
notifying the adapter's observers that the characteristic's value
Timer concept is leaking outside of only Web Bluetooth code. Mark this code
clearly as needing fixup before others use it. https://crbug.com/543884.

https://codereview.chromium.org/1382743002/diff/180001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/bluetooth/characteristicvaluechanged.html
(right):

https://codereview.chromium.org/1382743002/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/characteristicvaluechanged.html:30: },
'Reading a characteristic should fire an event.');
I think we can remove the duplicated tests for single and multiple listeners. If
multiple work, then we're good.

https://codereview.chromium.org/1382743002/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/characteristicvaluechanged.html:71: },
'Starting notifications should fire an events.');
"should fire an event"

https://codereview.chromium.org/1382743002/diff/180001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/bluetooth/resources/bluetooth-helpers.js
(right):

https://codereview.chromium.org/1382743002/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/resources/bluetooth-helpers.js:169: //
Returns a promise that resolves after 100ms unless
Timers in tests are super bad, causing flake or tests that appear to offer
coverage but don't, and tests that take longer than they should. 

OK, in this test if the bot is running too slowly the 100ms will trigger and
resolve with the test passing. The test may pass when it otherwise shouldn't.
This is not so good, but at least it won't cause flake.

Probably explain this and copy the comment and issue number to not use timers.

[ortuno, 2015-10-16 21:55:49]

Thanks!

https://codereview.chromium.org/1382743002/diff/180001/content/shell/browser/...
File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc
(right):

https://codereview.chromium.org/1382743002/diff/180001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:384:
.WillByDefault(RunCallbackWithResult<0>([adapter_ptr, location_ptr]() {
On 2015/10/16 20:39:25, scheib wrote:
> "0 /* success_callback */" just for consistency.

Done.

https://codereview.chromium.org/1382743002/diff/180001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:387:
FOR_EACH_OBSERVER(BluetoothAdapter::Observer,
On 2015/10/16 20:39:25, scheib wrote:
> Comment, E.G.:
> // Reading a characteristic has a side effect of ValueChanged being called.

Done.

https://codereview.chromium.org/1382743002/diff/180001/device/bluetooth/test/...
File device/bluetooth/test/mock_bluetooth_gatt_notify_session.h (right):

https://codereview.chromium.org/1382743002/diff/180001/device/bluetooth/test/...
device/bluetooth/test/mock_bluetooth_gatt_notify_session.h:30: // Starts
notifying the adapter's observers that the characteristic's value
On 2015/10/16 20:39:25, scheib wrote:
> Timer concept is leaking outside of only Web Bluetooth code. Mark this code
> clearly as needing fixup before others use it. https://crbug.com/543884. 

Done.

https://codereview.chromium.org/1382743002/diff/180001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/bluetooth/characteristicvaluechanged.html
(right):

https://codereview.chromium.org/1382743002/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/characteristicvaluechanged.html:30: },
'Reading a characteristic should fire an event.');
On 2015/10/16 20:39:25, scheib wrote:
> I think we can remove the duplicated tests for single and multiple listeners.
If
> multiple work, then we're good. 

I thinks it's good to have a test for the most basic functionality, specially
when doing test driven development. We still need to iterate over the listeners
code so I think this test will be important.

https://codereview.chromium.org/1382743002/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/characteristicvaluechanged.html:71: },
'Starting notifications should fire an events.');
On 2015/10/16 20:39:25, scheib wrote:
> "should fire an event"

Done.

https://codereview.chromium.org/1382743002/diff/180001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/bluetooth/resources/bluetooth-helpers.js
(right):

https://codereview.chromium.org/1382743002/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/resources/bluetooth-helpers.js:169: //
Returns a promise that resolves after 100ms unless
On 2015/10/16 20:39:25, scheib wrote:
> Timers in tests are super bad, causing flake or tests that appear to offer
> coverage but don't, and tests that take longer than they should. 
> 
> OK, in this test if the bot is running too slowly the 100ms will trigger and
> resolve with the test passing. The test may pass when it otherwise shouldn't.
> This is not so good, but at least it won't cause flake.
> 
> Probably explain this and copy the comment and issue number to not use timers.

The idea behind the setTimeout is to add an event to the end of the event queue
after any notification events are fired. But setting the timeout to 0 might miss
events being fired if the renderer or the browser are busy so I set it to 100ms
just in case. I can set it to 0 but then chances of missing events being
incorrectly fired increase.

Added a comment explaining this.

[ortuno, 2015-10-16 21:56:22]

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

[ortuno, 2015-10-16 21:56:22]

PTAL at third_part/WebKit

[haraken, 2015-10-17 03:17:12]

On 2015/10/16 21:56:22, ortuno wrote:
> PTAL at third_part/WebKit

WebKit/ LGTM

[scheib, 2015-10-17 19:33:44]

https://codereview.chromium.org/1382743002/diff/180001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/bluetooth/characteristicvaluechanged.html
(right):

https://codereview.chromium.org/1382743002/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/characteristicvaluechanged.html:30: },
'Reading a characteristic should fire an event.');
On 2015/10/16 21:55:49, ortuno wrote:
> On 2015/10/16 20:39:25, scheib wrote:
> > I think we can remove the duplicated tests for single and multiple
listeners.
> If
> > multiple work, then we're good. 
> 
> I thinks it's good to have a test for the most basic functionality, specially
> when doing test driven development. We still need to iterate over the
listeners
> code so I think this test will be important.

Either way I suppose, but if you leave the two tests you may as well pull out
the code to a method that takes listeners as an argument and only have 1 copy of
the code.

https://codereview.chromium.org/1382743002/diff/180001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/bluetooth/resources/bluetooth-helpers.js
(right):

https://codereview.chromium.org/1382743002/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/resources/bluetooth-helpers.js:169: //
Returns a promise that resolves after 100ms unless
On 2015/10/16 21:55:49, ortuno wrote:
> On 2015/10/16 20:39:25, scheib wrote:
> > Timers in tests are super bad, causing flake or tests that appear to offer
> > coverage but don't, and tests that take longer than they should. 
> > 
> > OK, in this test if the bot is running too slowly the 100ms will trigger and
> > resolve with the test passing. The test may pass when it otherwise
shouldn't.
> > This is not so good, but at least it won't cause flake.
> > 
> > Probably explain this and copy the comment and issue number to not use
timers.
> 
> The idea behind the setTimeout is to add an event to the end of the event
queue
> after any notification events are fired. But setting the timeout to 0 might
miss
> events being fired if the renderer or the browser are busy so I set it to
100ms
> just in case. I can set it to 0 but then chances of missing events being
> incorrectly fired increase.
> 
> Added a comment explaining this.

Either:

The task just needs to be placed in the queue to have deterministic ordering so
that it doesn't process until after e.g. this execution turn completes. That
would be OK.

Or, there's some amount of time needed for some reason, e.g. waiting on the
browser process. That is not OK. We can never be certain about the amount of
time. The test won't be clean until the delay is expressed in events, and not
duration of time. So, e.g. we can send a signal to the browser, or we can have
the browser send another signal to us that it has completed whatever may be in
process that would send an event.

The solution that comes to mind for me is to not have the browser send
notifications based on a timer. Instead have them sent when a test API call
causes them - and have that test API have a promise/callback that informs the
test that the event would have been sent and received by the time the result
comes in.

[ortuno, 2015-10-19 18:23:47]

https://codereview.chromium.org/1382743002/diff/180001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/bluetooth/characteristicvaluechanged.html
(right):

https://codereview.chromium.org/1382743002/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/characteristicvaluechanged.html:30: },
'Reading a characteristic should fire an event.');
On 2015/10/17 at 19:33:43, scheib wrote:
> On 2015/10/16 21:55:49, ortuno wrote:
> > On 2015/10/16 20:39:25, scheib wrote:
> > > I think we can remove the duplicated tests for single and multiple
listeners.
> > If
> > > multiple work, then we're good. 
> > 
> > I thinks it's good to have a test for the most basic functionality,
specially
> > when doing test driven development. We still need to iterate over the
listeners
> > code so I think this test will be important.
> 
> Either way I suppose, but if you leave the two tests you may as well pull out
the code to a method that takes listeners as an argument and only have 1 copy of
the code.

I agree. We use this pattern in all of our tests and we should refactor our
tests to avoid it. I opened an issue about it: http://crbug.com/545089 Is it OK
to leave this for later? I didn't add the issue in the code as it applies to all
tests.

https://codereview.chromium.org/1382743002/diff/180001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/bluetooth/resources/bluetooth-helpers.js
(right):

https://codereview.chromium.org/1382743002/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/resources/bluetooth-helpers.js:169: //
Returns a promise that resolves after 100ms unless
On 2015/10/17 at 19:33:43, scheib wrote:
> On 2015/10/16 21:55:49, ortuno wrote:
> > On 2015/10/16 20:39:25, scheib wrote:
> > > Timers in tests are super bad, causing flake or tests that appear to offer
> > > coverage but don't, and tests that take longer than they should. 
> > > 
> > > OK, in this test if the bot is running too slowly the 100ms will trigger
and
> > > resolve with the test passing. The test may pass when it otherwise
shouldn't.
> > > This is not so good, but at least it won't cause flake.
> > > 
> > > Probably explain this and copy the comment and issue number to not use
timers.
> > 
> > The idea behind the setTimeout is to add an event to the end of the event
queue
> > after any notification events are fired. But setting the timeout to 0 might
miss
> > events being fired if the renderer or the browser are busy so I set it to
100ms
> > just in case. I can set it to 0 but then chances of missing events being
> > incorrectly fired increase.
> > 
> > Added a comment explaining this.
> 
> Either:
> 
> The task just needs to be placed in the queue to have deterministic ordering
so that it doesn't process until after e.g. this execution turn completes. That
would be OK.
> 
> Or, there's some amount of time needed for some reason, e.g. waiting on the
browser process. That is not OK. We can never be certain about the amount of
time. The test won't be clean until the delay is expressed in events, and not
duration of time. So, e.g. we can send a signal to the browser, or we can have
the browser send another signal to us that it has completed whatever may be in
process that would send an event.
> 
> The solution that comes to mind for me is to not have the browser send
notifications based on a timer. Instead have them sent when a test API call
causes them - and have that test API have a promise/callback that informs the
test that the event would have been sent and received by the time the result
comes in.

Added TODO that points to http://crbug.com/543884

[ortuno, 2015-10-19 18:27:02]

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

[ortuno, 2015-10-19 18:27:03]

Hi tkent. As mentioned before, eventually Web Bluetooth events will bubble
through the Web Bluetooth hierarchy. This patch starts by implementing a basic
event, no bubbling yet. PTAL

[ortuno, 2015-10-19 18:28:05]

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

[ortuno, 2015-10-19 18:28:05]

palmer: PTAL at content/common/bluetooth/

[palmer, 2015-10-19 19:23:33]

https://codereview.chromium.org/1382743002/diff/220001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right):

https://codereview.chromium.org/1382743002/diff/220001/content/browser/blueto...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:852: if (active_iter !=
active_characteristic_threads_.end()) {
Nit: Could save a level of indentation here by returning if (active_iter| ==
active_characteristic_threads_.end());.

https://codereview.chromium.org/1382743002/diff/220001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_dispatcher_host.h (right):

https://codereview.chromium.org/1382743002/diff/220001/content/browser/blueto...
content/browser/bluetooth/bluetooth_dispatcher_host.h:227: std::map<std::string,
std::set<int>> active_characteristic_threads_;
Is int the exact right type for a thread ID?

https://codereview.chromium.org/1382743002/diff/220001/content/renderer/bluet...
File content/renderer/bluetooth/bluetooth_dispatcher.h (right):

https://codereview.chromium.org/1382743002/diff/220001/content/renderer/bluet...
content/renderer/bluetooth/bluetooth_dispatcher.h:146: // Tells
BluetoothDispatcherHost that we are no longer interested in
Nit: If this comment is 1 paragraph, fill it; otherwise, separate paragraphs
with blank lines:

// Foo...
//
// Bar...

https://codereview.chromium.org/1382743002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/bluetooth/BluetoothGATTCharacteristic.cpp
(right):

https://codereview.chromium.org/1382743002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothGATTCharacteristic.cpp:44:
static_assert(sizeof(*value.data()) == 1, "uint8_t should be a single byte");
This doesn't necessarily mean what you think it means. See
http://en.cppreference.com/w/cpp/language/sizeof:

"""Depending on the computer architecture, a byte may consist of 8 or more bits,
the exact number being recorded in CHAR_BIT."""

[ortuno, 2015-10-19 20:09:18]

https://codereview.chromium.org/1382743002/diff/220001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right):

https://codereview.chromium.org/1382743002/diff/220001/content/browser/blueto...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:852: if (active_iter !=
active_characteristic_threads_.end()) {
On 2015/10/19 at 19:23:33, palmer wrote:
> Nit: Could save a level of indentation here by returning if (active_iter| ==
active_characteristic_threads_.end());.

Done.

https://codereview.chromium.org/1382743002/diff/220001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_dispatcher_host.h (right):

https://codereview.chromium.org/1382743002/diff/220001/content/browser/blueto...
content/browser/bluetooth/bluetooth_dispatcher_host.h:227: std::map<std::string,
std::set<int>> active_characteristic_threads_;
On 2015/10/19 at 19:23:33, palmer wrote:
> Is int the exact right type for a thread ID?

This is a Worker's thread ID which is an int.

https://code.google.com/p/chromium/codesearch#chromium/src/content/public/chi...

https://codereview.chromium.org/1382743002/diff/220001/content/renderer/bluet...
File content/renderer/bluetooth/bluetooth_dispatcher.h (right):

https://codereview.chromium.org/1382743002/diff/220001/content/renderer/bluet...
content/renderer/bluetooth/bluetooth_dispatcher.h:146: // Tells
BluetoothDispatcherHost that we are no longer interested in
On 2015/10/19 at 19:23:33, palmer wrote:
> Nit: If this comment is 1 paragraph, fill it; otherwise, separate paragraphs
with blank lines:
> 
> // Foo...
> //
> // Bar...

Done.

https://codereview.chromium.org/1382743002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/bluetooth/BluetoothGATTCharacteristic.cpp
(right):

https://codereview.chromium.org/1382743002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothGATTCharacteristic.cpp:44:
static_assert(sizeof(*value.data()) == 1, "uint8_t should be a single byte");
On 2015/10/19 at 19:23:33, palmer wrote:
> This doesn't necessarily mean what you think it means. See
http://en.cppreference.com/w/cpp/language/sizeof:
> 
> """Depending on the computer architecture, a byte may consist of 8 or more
bits, the exact number being recorded in CHAR_BIT."""

That assert is there to make sure that the byteLength of the vector's data is
equal to the number of elements or value.size(). This is because we pass
value.size() as byteLength to DOMArrayBuffer::create():

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[palmer, 2015-10-19 20:56:44]

LGTM, thanks.

[ortuno, 2015-10-19 21:03:51]

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

[commit-bot: I haz the power, 2015-10-19 21:06:03]

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

[commit-bot: I haz the power, 2015-10-19 21:15:49]

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

[commit-bot: I haz the power, 2015-10-19 21:15:50]

Dry run: 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...)

[tkent, 2015-10-19 23:00:59]

third_party/WebKit lgtm.  Some cosmetic comments.

https://codereview.chromium.org/1382743002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/bluetooth/BluetoothGATTCharacteristic.h
(right):

https://codereview.chromium.org/1382743002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothGATTCharacteristic.h:50:
void dispatchCharacteristicValueChanged(const WebVector<uint8_t>& value);
Add |override|

https://codereview.chromium.org/1382743002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothGATTCharacteristic.h:64:
const WTF::AtomicString& interfaceName() const override;
Drop |WTF::|

https://codereview.chromium.org/1382743002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/bluetooth/BluetoothGATTCharacteristic.idl
(right):

https://codereview.chromium.org/1382743002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothGATTCharacteristic.idl:27:
// TODO(ortuno): Move this to CharacteristicEventHandlers.
indentation error

https://codereview.chromium.org/1382743002/diff/240001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/modules/bluetooth/WebBluetooth.h
(right):

https://codereview.chromium.org/1382743002/diff/240001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/bluetooth/WebBluetooth.h:93:
WebBluetoothGATTCharacteristic*) {}
|{}| -> |= 0|.

We don't need to provide an empty implementation in public headers any longer
because we merged the repositories.

https://codereview.chromium.org/1382743002/diff/240001/third_party/WebKit/pub...
File
third_party/WebKit/public/platform/modules/bluetooth/WebBluetoothGATTCharacteristic.h
(right):

https://codereview.chromium.org/1382743002/diff/240001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/bluetooth/WebBluetoothGATTCharacteristic.h:17:
virtual void dispatchCharacteristicValueChanged(const WebVector<uint8_t>&) { };
|{ }| -> |= 0|.

[ortuno, 2015-10-20 01:12:11]

The CQ bit was checked by ortuno@chromium.org

[ortuno, 2015-10-20 01:12:12]

Thanks!

https://codereview.chromium.org/1382743002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/bluetooth/BluetoothGATTCharacteristic.h
(right):

https://codereview.chromium.org/1382743002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothGATTCharacteristic.h:50:
void dispatchCharacteristicValueChanged(const WebVector<uint8_t>& value);
On 2015/10/19 at 23:00:59, tkent wrote:
> Add |override|

Done.

https://codereview.chromium.org/1382743002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothGATTCharacteristic.h:64:
const WTF::AtomicString& interfaceName() const override;
On 2015/10/19 at 23:00:59, tkent wrote:
> Drop |WTF::|

Done.

https://codereview.chromium.org/1382743002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/bluetooth/BluetoothGATTCharacteristic.idl
(right):

https://codereview.chromium.org/1382743002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothGATTCharacteristic.idl:27:
// TODO(ortuno): Move this to CharacteristicEventHandlers.
On 2015/10/19 at 23:00:59, tkent wrote:
> indentation error

Done.

https://codereview.chromium.org/1382743002/diff/240001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/modules/bluetooth/WebBluetooth.h
(right):

https://codereview.chromium.org/1382743002/diff/240001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/bluetooth/WebBluetooth.h:93:
WebBluetoothGATTCharacteristic*) {}
On 2015/10/19 at 23:00:59, tkent wrote:
> |{}| -> |= 0|.
> 
> We don't need to provide an empty implementation in public headers any longer
because we merged the repositories.

Done.

https://codereview.chromium.org/1382743002/diff/240001/third_party/WebKit/pub...
File
third_party/WebKit/public/platform/modules/bluetooth/WebBluetoothGATTCharacteristic.h
(right):

https://codereview.chromium.org/1382743002/diff/240001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/bluetooth/WebBluetoothGATTCharacteristic.h:17:
virtual void dispatchCharacteristicValueChanged(const WebVector<uint8_t>&) { };
On 2015/10/19 at 23:00:59, tkent wrote:
> |{ }| -> |= 0|.

Done.

[ortuno, 2015-10-20 01:12:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
jyasskin@chromium.org, scheib@chromium.org, haraken@chromium.org,
tkent@chromium.org, palmer@chromium.org
Link to the patchset: https://codereview.chromium.org/1382743002/#ps260001
(title: "Address tkent's comments")

[commit-bot: I haz the power, 2015-10-20 01:13:23]

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

[commit-bot: I haz the power, 2015-10-20 01:22:34]

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

[commit-bot: I haz the power, 2015-10-20 01:22:35]

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, 2015-10-20 21:56:29]

Description was changed from

==========
bluetooth: Add characteristicvaluechanged event

https://webbluetoothcg.github.io/web-bluetooth/#dom-bluetoothgattcharacterist...

BUG=483344
==========

to

==========
bluetooth: Add characteristicvaluechanged event

https://webbluetoothcg.github.io/web-bluetooth/#dom-bluetoothgattcharacterist...

NOPRESUBMIT=true

BUG=483344
==========

[ortuno, 2015-10-20 21:56:35]

The CQ bit was checked by ortuno@chromium.org

[commit-bot: I haz the power, 2015-10-20 21:57:23]

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

[commit-bot: I haz the power, 2015-10-20 21:59:35]

Committed patchset #13 (id:260001)

[commit-bot: I haz the power, 2015-10-20 22:00:34]

Patchset 13 (id:??) landed as
https://crrev.com/601997adf6e30112109528ec4221c0917523d9ee
Cr-Commit-Position: refs/heads/master@{#355166}