bluetooth: Browser-side implementation of readValue

bluetooth: Browser-side implementation of readValue

This is the third of three patches to implement readValue:

  [1] http://crrev.com/1168113008
  [2] This patch.
  [3] http://crrev.com/1147243004

BUG=496379
Committed: https://crrev.com/006cb4a39d52c747a95b996c19b6ed1906c53bb6
Cr-Commit-Position: refs/heads/master@{#334189}

[ortuno, 2015-06-09 01:31:43]

Patchset #1 (id:1) has been deleted

[ortuno, 2015-06-09 01:31:48]

Patchset #1 (id:20001) has been deleted

[ortuno, 2015-06-09 20:01:18]

Patchset #1 (id:40001) has been deleted

[ortuno, 2015-06-09 20:01:24]

Patchset #1 (id:60001) has been deleted

[ortuno, 2015-06-09 20:16:02]

Patchset #1 (id:80001) has been deleted

[ortuno, 2015-06-09 20:16:59]

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

[ortuno, 2015-06-09 20:17:01]

jyasskin: PTAL

[Jeffrey Yasskin, 2015-06-10 21:03:45]

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

https://codereview.chromium.org/1149883011/diff/120001/content/browser/blueto...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:260: thread_id,
request_id, BluetoothError::NETWORK_ERROR));
Hm, I'm not sure this is notionally a network error. The device can drop some
services, or change the characteristics in a service, while a connection is
still there. We'd report that to the webpage with a serviceremoved or
servicechanged event, but we should also give a good error if the webpage misses
that event. I've filed
https://github.com/WebBluetoothCG/web-bluetooth/issues/130 to specify this.

This is also a place where we could really use the elaboration error string.

https://codereview.chromium.org/1149883011/diff/120001/content/browser/blueto...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:416: thread_id,
request_id, BluetoothError::NETWORK_ERROR));
https://webbluetoothcg.github.io/web-bluetooth/#error-handling says how to map
GATT errors to Javascript errors, and they're not all NetworkErrors.

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

https://codereview.chromium.org/1149883011/diff/120001/content/browser/blueto...
content/browser/bluetooth/bluetooth_dispatcher_host.h:106: // Callbacks for
BluetoothGattCharacteristic::ReadRemoteCharacteristic
Please end comments with punctuation.

https://codereview.chromium.org/1149883011/diff/120001/content/child/bluetoot...
File content/child/bluetooth/bluetooth_dispatcher.cc (left):

https://codereview.chromium.org/1149883011/diff/120001/content/child/bluetoot...
content/child/bluetooth/bluetooth_dispatcher.cc:303: 
I kinda liked these blank lines.

https://codereview.chromium.org/1149883011/diff/120001/content/common/bluetoo...
File content/common/bluetooth/bluetooth_messages.h (right):

https://codereview.chromium.org/1149883011/diff/120001/content/common/bluetoo...
content/common/bluetooth/bluetooth_messages.h:166: content::BluetoothError /*
result */)
Can we add the "description" enum or string to this IPC now?

[ortuno, 2015-06-10 22:03:01]

@jyasskin: Thanks!

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

https://codereview.chromium.org/1149883011/diff/120001/content/browser/blueto...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:260: thread_id,
request_id, BluetoothError::NETWORK_ERROR));
On 2015/06/10 at 21:03:45, Jeffrey Yasskin wrote:
> Hm, I'm not sure this is notionally a network error. The device can drop some
services, or change the characteristics in a service, while a connection is
still there. We'd report that to the webpage with a serviceremoved or
servicechanged event, but we should also give a good error if the webpage misses
that event. I've filed
https://github.com/WebBluetoothCG/web-bluetooth/issues/130 to specify this.
> 

Added a TODO for when the error is implemented.

> This is also a place where we could really use the elaboration error string.

Yes! That's happening in the next CL: http://crrev.com/1177613002 (not ready for
review).

https://codereview.chromium.org/1149883011/diff/120001/content/browser/blueto...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:416: thread_id,
request_id, BluetoothError::NETWORK_ERROR));
On 2015/06/10 at 21:03:45, Jeffrey Yasskin wrote:
> https://webbluetoothcg.github.io/web-bluetooth/#error-handling says how to map
GATT errors to Javascript errors, and they're not all NetworkErrors.

I added a TODO here as well. The CL that introduces more detailed error messages
 (http://crrev.com/1177613002) will have a function to get the message and the
error based on the
GattError code. Sounds good?

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

https://codereview.chromium.org/1149883011/diff/120001/content/browser/blueto...
content/browser/bluetooth/bluetooth_dispatcher_host.h:106: // Callbacks for
BluetoothGattCharacteristic::ReadRemoteCharacteristic
On 2015/06/10 at 21:03:45, Jeffrey Yasskin wrote:
> Please end comments with punctuation.

Done.

https://codereview.chromium.org/1149883011/diff/120001/content/child/bluetoot...
File content/child/bluetooth/bluetooth_dispatcher.cc (left):

https://codereview.chromium.org/1149883011/diff/120001/content/child/bluetoot...
content/child/bluetooth/bluetooth_dispatcher.cc:303: 
On 2015/06/10 at 21:03:45, Jeffrey Yasskin wrote:
> I kinda liked these blank lines.


Done.

https://codereview.chromium.org/1149883011/diff/120001/content/common/bluetoo...
File content/common/bluetooth/bluetooth_messages.h (right):

https://codereview.chromium.org/1149883011/diff/120001/content/common/bluetoo...
content/common/bluetooth/bluetooth_messages.h:166: content::BluetoothError /*
result */)
On 2015/06/10 at 21:03:45, Jeffrey Yasskin wrote:
> Can we add the "description" enum or string to this IPC now?

That's on this CL: http://crrev.com/1177613002

[Jeffrey Yasskin, 2015-06-10 22:55:50]

LGTM

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

https://codereview.chromium.org/1149883011/diff/120001/content/browser/blueto...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:416: thread_id,
request_id, BluetoothError::NETWORK_ERROR));
On 2015/06/10 22:03:01, ortuno wrote:
> On 2015/06/10 at 21:03:45, Jeffrey Yasskin wrote:
> > https://webbluetoothcg.github.io/web-bluetooth/#error-handling says how to
map
> GATT errors to Javascript errors, and they're not all NetworkErrors.
> 
> I added a TODO here as well. The CL that introduces more detailed error
messages
>  (http://crrev.com/1177613002) will have a function to get the message and the
> error based on the
> GattError code. Sounds good? 

Sure.

[ortuno, 2015-06-11 00:11:46]

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

[ortuno, 2015-06-11 00:11:47]

@nick: PTAL at bad_message
@scheib: PTAL

[ncarter (slow), 2015-06-11 03:55:58]

bad_message and histograms.xml lgtm (though I am not an owner for
histograms.xml)

[ortuno, 2015-06-11 16:08:13]

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

[ortuno, 2015-06-11 16:08:14]

Thanks @nick.
@asvitkine: PTAL at histograms.xml

[Alexei Svitkine (slow), 2015-06-11 16:35:26]

lgtm

[scheib, 2015-06-11 20:20:50]

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

https://codereview.chromium.org/1149883011/diff/160001/content/browser/blueto...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:206: if
(!insert_result.second)
Reduce comment size,
if (!insert_result.second)  // If a value is already in map DCHECK it's valid.

https://codereview.chromium.org/1149883011/diff/160001/content/browser/blueto...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:240: // A
service_instance_id not in the map implies a hostile renderer
Isn't the first map check sufficient for bad renderer detection? If we found the
characteristic_iter, then the renderer is OK. If we then can't follow it through
service_to_device that's a data consistency error in this class.

What to do about it:
Probably DCHECK and handle it as a NOT_FOUND for now, with TODO to use
InvalidStateError https://github.com/WebBluetoothCG/web-bluetooth/issues/130

https://codereview.chromium.org/1149883011/diff/160001/content/browser/blueto...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:417: // TODO(ortuno):
Send a different Error based on the GattErrorCode.
Issue number for this.

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

https://codereview.chromium.org/1149883011/diff/160001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:77:
//    characteristic, with a value of "Empty Mock Device Name",
Doc the reconnection address.

[ortuno, 2015-06-11 21:17:10]

@scheib: Thanks!

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

https://codereview.chromium.org/1149883011/diff/160001/content/browser/blueto...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:206: if
(!insert_result.second)
On 2015/06/11 at 20:20:50, scheib wrote:
> Reduce comment size,
> if (!insert_result.second)  // If a value is already in map DCHECK it's valid.

Done.

https://codereview.chromium.org/1149883011/diff/160001/content/browser/blueto...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:240: // A
service_instance_id not in the map implies a hostile renderer
On 2015/06/11 at 20:20:50, scheib wrote:
> Isn't the first map check sufficient for bad renderer detection? If we found
the characteristic_iter, then the renderer is OK. If we then can't follow it
through service_to_device that's a data consistency error in this class.
> 
> What to do about it:
> Probably DCHECK and handle it as a NOT_FOUND for now, with TODO to use
InvalidStateError https://github.com/WebBluetoothCG/web-bluetooth/issues/130

Done.

https://codereview.chromium.org/1149883011/diff/160001/content/browser/blueto...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:417: // TODO(ortuno):
Send a different Error based on the GattErrorCode.
On 2015/06/11 at 20:20:49, scheib wrote:
> Issue number for this.

Done.

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

https://codereview.chromium.org/1149883011/diff/160001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:77:
//    characteristic, with a value of "Empty Mock Device Name",
On 2015/06/11 at 20:20:50, scheib wrote:
> Doc the reconnection address.

Done.

[scheib, 2015-06-11 21:51:57]

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

https://codereview.chromium.org/1149883011/diff/160001/content/browser/blueto...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:206: if
(!insert_result.second)
On 2015/06/11 21:17:09, ortuno wrote:
> On 2015/06/11 at 20:20:50, scheib wrote:
> > Reduce comment size,
> > if (!insert_result.second)  // If a value is already in map DCHECK it's
valid.
> 
> Done.

You can do it on one line, 
"""
if (!insert_result.second)  // If a value is already in map DCHECK it's valid.
"""

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

https://codereview.chromium.org/1149883011/diff/200001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:77:
//    "Empty Mock Device Name", and a "Reconnection Address" characteristic.
Point out here that a read of "Reconnection Address" will fail.

[ortuno, 2015-06-11 22:04:38]

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

https://codereview.chromium.org/1149883011/diff/160001/content/browser/blueto...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:206: if
(!insert_result.second)
On 2015/06/11 at 21:51:57, scheib wrote:
> On 2015/06/11 21:17:09, ortuno wrote:
> > On 2015/06/11 at 20:20:50, scheib wrote:
> > > Reduce comment size,
> > > if (!insert_result.second)  // If a value is already in map DCHECK it's
valid.
> > 
> > Done.
> 
> You can do it on one line, 
> """
> if (!insert_result.second)  // If a value is already in map DCHECK it's valid.
> """

That's longer that character limit.

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

https://codereview.chromium.org/1149883011/diff/200001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:77:
//    "Empty Mock Device Name", and a "Reconnection Address" characteristic.
On 2015/06/11 at 21:51:57, scheib wrote:
> Point out here that a read of "Reconnection Address" will fail.

Done.

[scheib, 2015-06-11 22:36:24]

LGTM

[ortuno, 2015-06-11 23:35:42]

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

[ortuno, 2015-06-11 23:35:43]

@armansito: PTAL at mock_bluetooth_gatt_service

[armansito, 2015-06-12 00:25:27]

lgtm

[ortuno, 2015-06-12 04:35:57]

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

[ortuno, 2015-06-12 04:35:57]

The CQ bit was checked by ortuno@chromium.org

[ortuno, 2015-06-12 04:35:58]

tsepez

[ortuno, 2015-06-12 04:35:59]

The patchset sent to the CQ was uploaded after l-g-t-m from
jyasskin@chromium.org, nick@chromium.org, asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/1149883011/#ps220001
(title: "Moar details about Reconnection address.")

[commit-bot: I haz the power, 2015-06-12 04:36:13]

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

[ortuno, 2015-06-12 04:36:13]

The CQ bit was unchecked by ortuno@chromium.org

[ortuno, 2015-06-12 04:37:37]

@tsepez: PTAL at content/common/bluetooth/bluetooth_messages.h

[Tom Sepez, 2015-06-12 17:26:57]

Messages LGTM.

[ortuno, 2015-06-12 17:27:17]

The CQ bit was checked by ortuno@chromium.org

[ortuno, 2015-06-12 17:27:18]

The patchset sent to the CQ was uploaded after l-g-t-m from nick@chromium.org,
scheib@chromium.org, armansito@chromium.org, asvitkine@chromium.org,
jyasskin@chromium.org
Link to the patchset: https://codereview.chromium.org/1149883011/#ps240001
(title: "Fix merge conflicts")

[commit-bot: I haz the power, 2015-06-12 17:28:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1149883011/240001

[commit-bot: I haz the power, 2015-06-12 17:35:55]

Committed patchset #8 (id:240001)

[commit-bot: I haz the power, 2015-06-12 17:36:53]

Patchset 8 (id:??) landed as
https://crrev.com/006cb4a39d52c747a95b996c19b6ed1906c53bb6
Cr-Commit-Position: refs/heads/master@{#334189}