Implement WebBluetooth descriptor.readValue()

Implement WebBluetooth descriptor.readValue()

A GATT Descriptor provides further information about a characteristic's
value. For each characteristic there may be many descriptors as defined by
https://webbluetoothcg.github.io/web-bluetooth/#bluetoothremotegattdescriptor.

In Bug 660699, we added support for accessing descriptors from characteristics.
In this CL, we add support for reading the value of the descriptor.

BUG=668837
R=scheib

Review-Url: https://codereview.chromium.org/2637343002
Cr-Commit-Position: refs/heads/master@{#446603}
Committed: https://chromium.googlesource.com/chromium/src/+/a2fe05521add5347b1024ca582045447cc469325

[dougt, 2017-01-18 00:59:08]

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

[dougt, 2017-01-18 00:59:09]

ortuno, ptal?

I had some questions I outlined around sharing between
BluetoothRemoteGATTDescriptor and BluetoothRemoteGATTCharacteristic that we can
address in the next draft.

[dougt, 2017-01-18 01:02:08]

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

[commit-bot: I haz the power, 2017-01-18 01:03:04]

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, 2017-01-18 01:14:19]

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

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

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

[scheib, 2017-01-19 06:16:18]

The command:
  git cl upload --similarity 
can make patches with new files that are similar to others easier to see the
small deltas of. For all these tests I think it could find the characteristic
version match and highlight just the new things the descriptor versions have had
changed.

https://codereview.chromium.org/2637343002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/bluetooth/descriptor/readValue/device-goes-out-of-range.html
(right):

https://codereview.chromium.org/2637343002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/bluetooth/descriptor/readValue/device-goes-out-of-range.html:15:
.then(characteristic => {
This is just testing characteristic, did you mean to test failing for a
descriptor readValue?

https://codereview.chromium.org/2637343002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/bluetooth/descriptor/readValue/read-fails.html
(right):

https://codereview.chromium.org/2637343002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/bluetooth/descriptor/readValue/read-fails.html:18:
characteristic.readValue(),
ditto: test for descriptor.readValue()

https://codereview.chromium.org/2637343002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/bluetooth/descriptor/readValue/service-is-removed.html
(right):

https://codereview.chromium.org/2637343002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/bluetooth/descriptor/readValue/service-is-removed.html:18:
characteristic.readValue(),
ditto

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

https://codereview.chromium.org/2637343002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTDescriptor.cpp:26:
DOMDataView* ConvertWTFVectorToDataView(const Vector<uint8_t>& wtfVector) {
Copy paste... hmm... but when you asked earlier I'd agreed with your utils file
idea.

https://codereview.chromium.org/2637343002/diff/1/third_party/WebKit/public/p...
File third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom
(right):

https://codereview.chromium.org/2637343002/diff/1/third_party/WebKit/public/p...
third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom:203: //
Reads the value for descriptor with |descriptor_instance_id|.
See comment style template change in tip of tree, made in this patch:
https://codereview.chromium.org/2635473004

[dougt, 2017-01-19 23:47:28]

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

[commit-bot: I haz the power, 2017-01-19 23:48: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, 2017-01-20 02:09:30]

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

[commit-bot: I haz the power, 2017-01-20 02:09:31]

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_...)

[dougt, 2017-01-20 03:17:24]

scheib, ptal.

https://codereview.chromium.org/2637343002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/bluetooth/descriptor/readValue/device-goes-out-of-range.html
(right):

https://codereview.chromium.org/2637343002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/bluetooth/descriptor/readValue/device-goes-out-of-range.html:15:
.then(characteristic => {
On 2017/01/19 06:16:18, scheib wrote:
> This is just testing characteristic, did you mean to test failing for a
> descriptor readValue?

Done.

https://codereview.chromium.org/2637343002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/bluetooth/descriptor/readValue/read-fails.html
(right):

https://codereview.chromium.org/2637343002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/bluetooth/descriptor/readValue/read-fails.html:18:
characteristic.readValue(),
On 2017/01/19 06:16:18, scheib wrote:
> ditto: test for descriptor.readValue()

Done.

https://codereview.chromium.org/2637343002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/bluetooth/descriptor/readValue/service-is-removed.html
(right):

https://codereview.chromium.org/2637343002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/bluetooth/descriptor/readValue/service-is-removed.html:18:
characteristic.readValue(),
On 2017/01/19 06:16:18, scheib wrote:
> ditto

Done.

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

https://codereview.chromium.org/2637343002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTDescriptor.cpp:26:
DOMDataView* ConvertWTFVectorToDataView(const Vector<uint8_t>& wtfVector) {
On 2017/01/19 06:16:18, scheib wrote:
> Copy paste... hmm... but when you asked earlier I'd agreed with your utils
file
> idea.

Done.

https://codereview.chromium.org/2637343002/diff/1/third_party/WebKit/public/p...
File third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom
(right):

https://codereview.chromium.org/2637343002/diff/1/third_party/WebKit/public/p...
third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom:203: //
Reads the value for descriptor with |descriptor_instance_id|.
On 2017/01/19 06:16:18, scheib wrote:
> See comment style template change in tip of tree, made in this patch:
> https://codereview.chromium.org/2635473004

Done.

[ortuno, 2017-01-20 04:30:53]

Mostly nits. I still need to finish reviewing the tests.

https://codereview.chromium.org/2637343002/diff/20001/content/browser/bluetoo...
File content/browser/bluetooth/web_bluetooth_service_impl.cc (right):

https://codereview.chromium.org/2637343002/diff/20001/content/browser/bluetoo...
content/browser/bluetooth/web_bluetooth_service_impl.cc:978:
DCHECK_CURRENTLY_ON(BrowserThread::UI);
Add todo for metrics.

https://codereview.chromium.org/2637343002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/bluetooth/descriptor/readValue/device-goes-out-of-range.html
(right):

https://codereview.chromium.org/2637343002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/descriptor/readValue/device-goes-out-of-range.html:1:
<!DOCTYPE html>
Any reason why we can share this test with writeValue?

https://codereview.chromium.org/2637343002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/descriptor/readValue/device-goes-out-of-range.html:14:
.then(characteristic =>
characteristic.getDescriptor('gatt.characteristic_user_description'))
nit: You used user_description.name on previous tests.

https://codereview.chromium.org/2637343002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/bluetooth/descriptor/readValue/read-updates-value.html
(right):

https://codereview.chromium.org/2637343002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/descriptor/readValue/read-updates-value.html:18:
.then(() => {
nit: align with 't'

https://codereview.chromium.org/2637343002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/descriptor/readValue/read-updates-value.html:20:
let value_str = decoder.decode(descriptor.value);
can you also assert that the value read is the same object as descriptor.value

https://codereview.chromium.org/2637343002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/bluetooth/descriptor/readValue/service-is-removed.html
(right):

https://codereview.chromium.org/2637343002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/descriptor/readValue/service-is-removed.html:1:
<!DOCTYPE html>
Any reason why we can share this test with writeValue?

https://codereview.chromium.org/2637343002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/bluetooth/script-tests/descriptor/descriptor-is-removed.js
(right):

https://codereview.chromium.org/2637343002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/script-tests/descriptor/descriptor-is-removed.js:5:
() => requestDeviceWithKeyDown(
nit: Was a new layout test style introduced? Otherwise let's keep the same style
across tests. Move this to the previous line and fix indentation. (same applies
to other tests)

https://codereview.chromium.org/2637343002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/bluetooth/script-tests/descriptor/io-op-device-disconnects-before.js
(right):

https://codereview.chromium.org/2637343002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/script-tests/descriptor/io-op-device-disconnects-before.js:5:
.then(() => requestDeviceWithKeyDown({
nit: align with 't'.

https://codereview.chromium.org/2637343002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/script-tests/descriptor/io-op-device-disconnects-before.js:11:
let measurement_interval;
I think you meant user_description here.  Otherwise you are replacing
'user_description' in bluetooth_helpers.js.

https://codereview.chromium.org/2637343002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/bluetooth/script-tests/descriptor/io-op-device-disconnects-during-error.js
(right):

https://codereview.chromium.org/2637343002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/script-tests/descriptor/io-op-device-disconnects-during-error.js:18:
.then(d => user_description = d)
I think you mean error_characteristic.

https://codereview.chromium.org/2637343002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/bluetooth/script-tests/descriptor/io-op-device-disconnects-during-success.js
(right):

https://codereview.chromium.org/2637343002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/script-tests/descriptor/io-op-device-disconnects-during-success.js:12:
let measurement_interval;
s/measurement_interval/user_description/

https://codereview.chromium.org/2637343002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/bluetooth/script-tests/descriptor/io-op-device-reconnects-during-error.js
(right):

https://codereview.chromium.org/2637343002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/script-tests/descriptor/io-op-device-reconnects-during-error.js:28:
.then(() => promise);
I don't think you need "() =>"

https://codereview.chromium.org/2637343002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/bluetooth/script-tests/descriptor/io-op-reconnect-during-error.js
(right):

https://codereview.chromium.org/2637343002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/script-tests/descriptor/io-op-reconnect-during-error.js:25:
'disconnect() and connect() called during a FUNCTION_NAME call that ' +
nit: move to previous line.

https://codereview.chromium.org/2637343002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/bluetooth/script-tests/descriptor/io-op-reconnect-during-success.js
(right):

https://codereview.chromium.org/2637343002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/script-tests/descriptor/io-op-reconnect-during-success.js:1:
promise_test(
out of curiosity: Why are these no longer called gatt-op-*?

https://codereview.chromium.org/2637343002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/script-tests/descriptor/io-op-reconnect-during-success.js:7:
() => requestDeviceWithKeyDown(
Same here. Move to previous line. Same for other tests.

https://codereview.chromium.org/2637343002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/script-tests/descriptor/io-op-reconnect-during-success.js:13:
characteristic => characteristic.getDescriptor(
move to previous line. Same for other tests.

https://codereview.chromium.org/2637343002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/script-tests/descriptor/io-op-reconnect-during-success.js:26:
'disconnect() and connect() called during a FUNCTION_NAME call that ' +
nit: Move to previous line. Same for other tests.

https://codereview.chromium.org/2637343002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/script-tests/descriptor/io-op-reconnect-during-success.js:27:
'succeeds. Reject with NetworkError.');
nit: align with ' from previous line. Same for other tests.

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

https://codereview.chromium.org/2637343002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTDescriptor.cpp:56:
setValue(domDataView);
Q: Why do we need the extra function?

https://codereview.chromium.org/2637343002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTDescriptor.cpp:65:
// We always check that the device is connected.
Unsure why we need this comment.

https://codereview.chromium.org/2637343002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTDescriptor.cpp:84:
mojom::blink::WebBluetoothService* service =
should we add a getBluetoothService() method?

https://codereview.chromium.org/2637343002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTUtils.h
(right):

https://codereview.chromium.org/2637343002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTUtils.h:18: enum
ExceptionType {
Out of curiosity, why is this not an enum class?

https://codereview.chromium.org/2637343002/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom
(right):

https://codereview.chromium.org/2637343002/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom:244: //
Reads the value for descriptor with |descriptor_instance_id|.
I think you wrote this comment twice?

https://codereview.chromium.org/2637343002/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom:254: //
corresponding error and nullptr for value
nit: Shouldn't this be nullopt.

[ortuno, 2017-01-22 22:04:05]

https://codereview.chromium.org/2637343002/diff/20001/content/shell/browser/l...
File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc
(right):

https://codereview.chromium.org/2637343002/diff/20001/content/shell/browser/l...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:744:
if (addDescriptors == true) {
nit: no need for == true.

https://codereview.chromium.org/2637343002/diff/20001/content/shell/browser/l...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:745:
std::string descriptorName = kCharacteristicUserDescription;
nit: const std::string.

https://codereview.chromium.org/2637343002/diff/20001/content/shell/browser/l...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:748:
BluetoothUUID(kUserDescriptionUUID), false,
nit: false /* is_local */

https://codereview.chromium.org/2637343002/diff/20001/content/shell/browser/l...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:755:
std::vector<uint8_t> value(descriptorName.begin(),
optional: I think you would end up with less code if you just returned a [1] or
similar.

https://codereview.chromium.org/2637343002/diff/20001/content/shell/browser/l...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:762:
BluetoothUUID(kClientConfigUUID), false,
nit: false /* is_local */

https://codereview.chromium.org/2637343002/diff/20001/content/shell/browser/l...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:774:

optional: What do you think about adding a ON_CALL(*blocklisted_descriptor,
ReadRemoteDescriptor(...)) that DCHECKS? That way we have another layer to make
sure we are not performing operations on blocklisted attributes.

https://codereview.chromium.org/2637343002/diff/20001/content/shell/browser/l...
File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h
(right):

https://codereview.chromium.org/2637343002/diff/20001/content/shell/browser/l...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:344:
//           - Descriptors (if |addDescriptor| input is true)
nit: should_add_descriptors or add_descriptors

https://codereview.chromium.org/2637343002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/bluetooth/descriptor/readValue/read-fails.html
(right):

https://codereview.chromium.org/2637343002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/descriptor/readValue/read-fails.html:1:
<!DOCTYPE html>
I think we can share this test with writeValue.

[dougt, 2017-01-23 21:35:16]

Patchset #3 (id:40001) has been deleted

[dougt, 2017-01-23 21:36:52]

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

[commit-bot: I haz the power, 2017-01-23 21:37:48]

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

[dougt, 2017-01-23 21:40:37]

ptal

https://codereview.chromium.org/2637343002/diff/20001/content/browser/bluetoo...
File content/browser/bluetooth/web_bluetooth_service_impl.cc (right):

https://codereview.chromium.org/2637343002/diff/20001/content/browser/bluetoo...
content/browser/bluetooth/web_bluetooth_service_impl.cc:978:
DCHECK_CURRENTLY_ON(BrowserThread::UI);
On 2017/01/20 04:30:52, ortuno wrote:
> Add todo for metrics.

Acknowledged.

There are probably a dozen places that I need to add metrics. I would rather not
add these TODOs in this CL and instead, just add them when I fix 667319.

https://codereview.chromium.org/2637343002/diff/20001/content/shell/browser/l...
File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc
(right):

https://codereview.chromium.org/2637343002/diff/20001/content/shell/browser/l...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:744:
if (addDescriptors == true) {
On 2017/01/22 22:04:04, ortuno wrote:
> nit: no need for == true.

Done.

https://codereview.chromium.org/2637343002/diff/20001/content/shell/browser/l...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:745:
std::string descriptorName = kCharacteristicUserDescription;
On 2017/01/22 22:04:04, ortuno wrote:
> nit: const std::string.

Done.

https://codereview.chromium.org/2637343002/diff/20001/content/shell/browser/l...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:748:
BluetoothUUID(kUserDescriptionUUID), false,
On 2017/01/22 22:04:04, ortuno wrote:
> nit: false /* is_local */

Done.

https://codereview.chromium.org/2637343002/diff/20001/content/shell/browser/l...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:755:
std::vector<uint8_t> value(descriptorName.begin(),
On 2017/01/22 22:04:04, ortuno wrote:
> optional: I think you would end up with less code if you just returned a [1]
or
> similar.

Acknowledged.

https://codereview.chromium.org/2637343002/diff/20001/content/shell/browser/l...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:762:
BluetoothUUID(kClientConfigUUID), false,
On 2017/01/22 22:04:04, ortuno wrote:
> nit: false /* is_local */

Done.

https://codereview.chromium.org/2637343002/diff/20001/content/shell/browser/l...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:774:

On 2017/01/22 22:04:04, ortuno wrote:
> optional: What do you think about adding a ON_CALL(*blocklisted_descriptor,
> ReadRemoteDescriptor(...)) that DCHECKS? That way we have another layer to
make
> sure we are not performing operations on blocklisted attributes.

I'll add a separate descriptor for blocklist testing.

https://codereview.chromium.org/2637343002/diff/20001/content/shell/browser/l...
File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h
(right):

https://codereview.chromium.org/2637343002/diff/20001/content/shell/browser/l...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:344:
//           - Descriptors (if |addDescriptor| input is true)
On 2017/01/22 22:04:04, ortuno wrote:
> nit: should_add_descriptors or add_descriptors

Acknowledged.  I am not sure I like using _'s in parameter names, but plural I
can do!

https://codereview.chromium.org/2637343002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/bluetooth/descriptor/readValue/device-goes-out-of-range.html
(right):

https://codereview.chromium.org/2637343002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/descriptor/readValue/device-goes-out-of-range.html:1:
<!DOCTYPE html>
On 2017/01/20 04:30:52, ortuno wrote:
> Any reason why we can share this test with writeValue?

Done.

https://codereview.chromium.org/2637343002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/descriptor/readValue/device-goes-out-of-range.html:14:
.then(characteristic =>
characteristic.getDescriptor('gatt.characteristic_user_description'))
On 2017/01/20 04:30:52, ortuno wrote:
> nit: You used user_description.name on previous tests.

Done.

https://codereview.chromium.org/2637343002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/bluetooth/descriptor/readValue/read-fails.html
(right):

https://codereview.chromium.org/2637343002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/descriptor/readValue/read-fails.html:1:
<!DOCTYPE html>
On 2017/01/22 22:04:04, ortuno wrote:
> I think we can share this test with writeValue.

Done.

https://codereview.chromium.org/2637343002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/bluetooth/descriptor/readValue/read-updates-value.html
(right):

https://codereview.chromium.org/2637343002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/descriptor/readValue/read-updates-value.html:18:
.then(() => {
On 2017/01/20 04:30:52, ortuno wrote:
> nit: align with 't'

we use two spaces here

https://codereview.chromium.org/2637343002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/descriptor/readValue/read-updates-value.html:20:
let value_str = decoder.decode(descriptor.value);
On 2017/01/20 04:30:52, ortuno wrote:
> can you also assert that the value read is the same object as descriptor.value

I am not sure what you mean here.

https://codereview.chromium.org/2637343002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/bluetooth/descriptor/readValue/service-is-removed.html
(right):

https://codereview.chromium.org/2637343002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/descriptor/readValue/service-is-removed.html:1:
<!DOCTYPE html>
On 2017/01/20 04:30:52, ortuno wrote:
> Any reason why we can share this test with writeValue?

Nope. I'll move this into a script test when I start on writeValue().

https://codereview.chromium.org/2637343002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/bluetooth/script-tests/descriptor/descriptor-is-removed.js
(right):

https://codereview.chromium.org/2637343002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/script-tests/descriptor/descriptor-is-removed.js:5:
() => requestDeviceWithKeyDown(
On 2017/01/20 04:30:52, ortuno wrote:
> nit: Was a new layout test style introduced? Otherwise let's keep the same
style
> across tests. Move this to the previous line and fix indentation. (same
applies
> to other tests)

Running git cl format will do this to the javascript tests.

For example, look at the output of clang-format
<src>/src/third_party/WebKit/LayoutTests/bluetooth/script-tests/characteristic/*


Maybe we should adopt this style?

https://codereview.chromium.org/2637343002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/bluetooth/script-tests/descriptor/io-op-device-disconnects-before.js
(right):

https://codereview.chromium.org/2637343002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/script-tests/descriptor/io-op-device-disconnects-before.js:5:
.then(() => requestDeviceWithKeyDown({
On 2017/01/20 04:30:52, ortuno wrote:
> nit: align with 't'.

Acknowledged.

https://codereview.chromium.org/2637343002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/script-tests/descriptor/io-op-device-disconnects-before.js:11:
let measurement_interval;
On 2017/01/20 04:30:52, ortuno wrote:
> I think you meant user_description here.  Otherwise you are replacing
> 'user_description' in bluetooth_helpers.js.

Done.

https://codereview.chromium.org/2637343002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/bluetooth/script-tests/descriptor/io-op-device-disconnects-during-error.js
(right):

https://codereview.chromium.org/2637343002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/script-tests/descriptor/io-op-device-disconnects-during-error.js:18:
.then(d => user_description = d)
On 2017/01/20 04:30:52, ortuno wrote:
> I think you mean error_characteristic.

Done.

https://codereview.chromium.org/2637343002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/bluetooth/script-tests/descriptor/io-op-device-disconnects-during-success.js
(right):

https://codereview.chromium.org/2637343002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/script-tests/descriptor/io-op-device-disconnects-during-success.js:12:
let measurement_interval;
On 2017/01/20 04:30:52, ortuno wrote:
> s/measurement_interval/user_description/

Done.

https://codereview.chromium.org/2637343002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/bluetooth/script-tests/descriptor/io-op-device-reconnects-during-error.js
(right):

https://codereview.chromium.org/2637343002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/script-tests/descriptor/io-op-device-reconnects-during-error.js:28:
.then(() => promise);
On 2017/01/20 04:30:53, ortuno wrote:
> I don't think you need "() =>"

Acknowledged.

https://codereview.chromium.org/2637343002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/bluetooth/script-tests/descriptor/io-op-reconnect-during-error.js
(right):

https://codereview.chromium.org/2637343002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/script-tests/descriptor/io-op-reconnect-during-error.js:25:
'disconnect() and connect() called during a FUNCTION_NAME call that ' +
On 2017/01/20 04:30:53, ortuno wrote:
> nit: move to previous line.

Acknowledged.

https://codereview.chromium.org/2637343002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/bluetooth/script-tests/descriptor/io-op-reconnect-during-success.js
(right):

https://codereview.chromium.org/2637343002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/script-tests/descriptor/io-op-reconnect-during-success.js:1:
promise_test(
On 2017/01/20 04:30:53, ortuno wrote:
> out of curiosity: Why are these no longer called gatt-op-*?

These are just going to be for read/write.  I figured gatt as correct for
characteristic because it does so much more than descriptor.

https://codereview.chromium.org/2637343002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/script-tests/descriptor/io-op-reconnect-during-success.js:7:
() => requestDeviceWithKeyDown(
On 2017/01/20 04:30:53, ortuno wrote:
> Same here. Move to previous line. Same for other tests.

Acknowledged.

https://codereview.chromium.org/2637343002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/script-tests/descriptor/io-op-reconnect-during-success.js:13:
characteristic => characteristic.getDescriptor(
On 2017/01/20 04:30:53, ortuno wrote:
> move to previous line. Same for other tests.

Acknowledged.

https://codereview.chromium.org/2637343002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/script-tests/descriptor/io-op-reconnect-during-success.js:26:
'disconnect() and connect() called during a FUNCTION_NAME call that ' +
On 2017/01/20 04:30:53, ortuno wrote:
> nit: Move to previous line. Same for other tests.

Acknowledged.

https://codereview.chromium.org/2637343002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/script-tests/descriptor/io-op-reconnect-during-success.js:27:
'succeeds. Reject with NetworkError.');
On 2017/01/20 04:30:53, ortuno wrote:
> nit: align with ' from previous line. Same for other tests.

Acknowledged.

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

https://codereview.chromium.org/2637343002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTDescriptor.cpp:56:
setValue(domDataView);
On 2017/01/20 04:30:53, ortuno wrote:
> Q: Why do we need the extra function?

Removed the method and just setting the member directly.

https://codereview.chromium.org/2637343002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTDescriptor.cpp:65:
// We always check that the device is connected.
On 2017/01/20 04:30:53, ortuno wrote:
> Unsure why we need this comment.

Done.

Also removed this comment in the 5 other places it appeared.

https://codereview.chromium.org/2637343002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTDescriptor.cpp:84:
mojom::blink::WebBluetoothService* service =
On 2017/01/20 04:30:53, ortuno wrote:
> should we add a getBluetoothService() method?

Maybe soon.  I suspect when we add ::writeValue() we can do this.

https://codereview.chromium.org/2637343002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTUtils.h
(right):

https://codereview.chromium.org/2637343002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTUtils.h:18: enum
ExceptionType {
On 2017/01/20 04:30:53, ortuno wrote:
> Out of curiosity, why is this not an enum class?

No reason, really. Follow up?

https://codereview.chromium.org/2637343002/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom
(right):

https://codereview.chromium.org/2637343002/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom:244: //
Reads the value for descriptor with |descriptor_instance_id|.
On 2017/01/20 04:30:53, ortuno wrote:
> I think you wrote this comment twice?

Done.

https://codereview.chromium.org/2637343002/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom:254: //
corresponding error and nullptr for value
On 2017/01/20 04:30:53, ortuno wrote:
> nit: Shouldn't this be nullopt.

Not sure.  On the C++ binding side, the callback is going to be called with an
Optional<Vector<uint8_t>>. I think when its not set (and when the result !=
SUCCESS), the value is testable with is_null.  Vince, do you know the answer
here since you just updated these docs?

[ortuno, 2017-01-23 22:20:43]

lgtm

https://codereview.chromium.org/2637343002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTUtils.h
(right):

https://codereview.chromium.org/2637343002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTUtils.h:18: enum
ExceptionType {
On 2017/01/23 at 21:40:37, dougt wrote:
> On 2017/01/20 04:30:53, ortuno wrote:
> > Out of curiosity, why is this not an enum class?
> 
> No reason, really. Follow up?

SGTM

https://codereview.chromium.org/2637343002/diff/60001/content/shell/browser/l...
File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc
(right):

https://codereview.chromium.org/2637343002/diff/60001/content/shell/browser/l...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:777:
ON_CALL(*no_read_descriptor, ReadRemoteDescriptor(_, _))
  // Crash if ReadRemoteDescriptor called. Not using GoogleMock's Expect
  // because this is used in layout tests that may not report a mock expectation
  // error correctly as a layout test failure.
  ON_CALL(*no_read_descriptor, ReadRemoteDescriptor(_, _))
      .WillByDefault(
          Invoke([](const BluetoothRemoteGattCharacteristic::ValueCallback&,
                    const BluetoothRemoteGattCharacteristic::ErrorCallback&) {
            NOTREACHED();
          }));

https://codereview.chromium.org/2637343002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/bluetooth/descriptor/readValue/read-updates-value.html
(right):

https://codereview.chromium.org/2637343002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/descriptor/readValue/read-updates-value.html:19:
let decoder = new TextDecoder('utf-8');
hmm maybe we need a separate test for this but this is what I meant:

return descriptor.readValue()
    .then(value => {
      assert_equals(value, descriptor.value);
      let decoder = ...
    })
});

https://codereview.chromium.org/2637343002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/descriptor/readValue/read-updates-value.html:22:
})
nit: Missing ';'

https://codereview.chromium.org/2637343002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/descriptor/readValue/read-updates-value.html:23:
})
nit: Missing ';'

https://codereview.chromium.org/2637343002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/bluetooth/script-tests/descriptor/descriptor-is-blocklisted.js
(right):

https://codereview.chromium.org/2637343002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/script-tests/descriptor/descriptor-is-blocklisted.js:24:
'A read request succeeds and returns the descriptor\'s value.');
Fix description. Also make sure to use 'FUNCTION_NAME' instead of 'read' so that
the right name gets generated.

https://codereview.chromium.org/2637343002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/bluetooth/script-tests/descriptor/io-op-fails.js
(right):

https://codereview.chromium.org/2637343002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/script-tests/descriptor/io-op-fails.js:26:
testSpec.testName));
So much whitespace T_T

[commit-bot: I haz the power, 2017-01-23 23:23:44]

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

[commit-bot: I haz the power, 2017-01-23 23:23:46]

Dry run: This issue passed the CQ dry run.

[dougt, 2017-01-24 00:09:45]

Patchset #4 (id:80001) has been deleted

[dougt, 2017-01-24 00:19:13]

Patchset #4 (id:100001) has been deleted

[dougt, 2017-01-24 00:26:23]

dougt@chromium.org changed reviewers:
+ dcheng@chromium.org

[dougt, 2017-01-24 00:26:24]

dcheng, ptal at:
third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom

https://codereview.chromium.org/2637343002/diff/60001/content/shell/browser/l...
File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc
(right):

https://codereview.chromium.org/2637343002/diff/60001/content/shell/browser/l...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:777:
ON_CALL(*no_read_descriptor, ReadRemoteDescriptor(_, _))
On 2017/01/23 22:20:42, ortuno wrote:
>   // Crash if ReadRemoteDescriptor called. Not using GoogleMock's Expect
>   // because this is used in layout tests that may not report a mock
expectation
>   // error correctly as a layout test failure.
>   ON_CALL(*no_read_descriptor, ReadRemoteDescriptor(_, _))
>       .WillByDefault(
>           Invoke([](const BluetoothRemoteGattCharacteristic::ValueCallback&,
>                     const BluetoothRemoteGattCharacteristic::ErrorCallback&) {
>             NOTREACHED();
>           }));

Done.

https://codereview.chromium.org/2637343002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/bluetooth/descriptor/readValue/read-updates-value.html
(right):

https://codereview.chromium.org/2637343002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/descriptor/readValue/read-updates-value.html:19:
let decoder = new TextDecoder('utf-8');
On 2017/01/23 22:20:42, ortuno wrote:
> hmm maybe we need a separate test for this but this is what I meant:
> 
> return descriptor.readValue()
>     .then(value => {
>       assert_equals(value, descriptor.value);
>       let decoder = ...
>     })
> });

Done.

https://codereview.chromium.org/2637343002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/descriptor/readValue/read-updates-value.html:22:
})
On 2017/01/23 22:20:42, ortuno wrote:
> nit: Missing ';'

Done.

https://codereview.chromium.org/2637343002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/descriptor/readValue/read-updates-value.html:23:
})
On 2017/01/23 22:20:42, ortuno wrote:
> nit: Missing ';'

Done.

https://codereview.chromium.org/2637343002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/bluetooth/script-tests/descriptor/descriptor-is-blocklisted.js
(right):

https://codereview.chromium.org/2637343002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/script-tests/descriptor/descriptor-is-blocklisted.js:24:
'A read request succeeds and returns the descriptor\'s value.');
On 2017/01/23 22:20:42, ortuno wrote:
> Fix description. Also make sure to use 'FUNCTION_NAME' instead of 'read' so
that
> the right name gets generated.

Done.

https://codereview.chromium.org/2637343002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/bluetooth/script-tests/descriptor/io-op-fails.js
(right):

https://codereview.chromium.org/2637343002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/script-tests/descriptor/io-op-fails.js:26:
testSpec.testName));
On 2017/01/23 22:20:42, ortuno wrote:
> So much whitespace T_T

clang formatter

[dougt, 2017-01-24 00:33:57]

Patchset #4 (id:120001) has been deleted

[scheib, 2017-01-24 01:23:33]

LGTM

https://codereview.chromium.org/2637343002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/bluetooth/script-tests/descriptor/io-op-fails.js
(right):

https://codereview.chromium.org/2637343002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/script-tests/descriptor/io-op-fails.js:26:
testSpec.testName));
On 2017/01/24 00:26:24, dougt wrote:
> On 2017/01/23 22:20:42, ortuno wrote:
> > So much whitespace T_T
> 
> clang formatter

I've filed an issue to see if this can be improved.
https://bugs.chromium.org/p/chromium/issues/detail?id=684123

[dcheng, 2017-01-24 10:21:03]

(The mock comment is just random musing and not necessary to address in this CL)

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

https://codereview.chromium.org/2637343002/diff/140001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:791:
base::MakeUnique<NiceMockBluetoothGattDescriptor>(
FWIW, I think this would be more clearly written with explicit test fakes,
rather than using the magic boilerplate gmock generates, especially since we
don't use any of the EXPECT_CALL magic.

P.S. I'm not too sure what the current thinking on gmock is. One previous thread
is
https://groups.google.com/a/chromium.org/forum/#!msg/chromium-dev/-KH_IP0rIWQ....

https://codereview.chromium.org/2637343002/diff/140001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTDescriptor.cpp
(right):

https://codereview.chromium.org/2637343002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTDescriptor.cpp:41:
// disconnected so we reject.
Out of curiosity, aren't the checks on line 36/37 sufficient for checking frame
disconnection?

[dougt, 2017-01-24 16:14:29]

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

https://codereview.chromium.org/2637343002/diff/140001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:791:
base::MakeUnique<NiceMockBluetoothGattDescriptor>(
On 2017/01/24 10:21:02, dcheng wrote:
> FWIW, I think this would be more clearly written with explicit test fakes,
> rather than using the magic boilerplate gmock generates, especially since we
> don't use any of the EXPECT_CALL magic.
> 
> P.S. I'm not too sure what the current thinking on gmock is. One previous
thread
> is
>
https://groups.google.com/a/chromium.org/forum/#!msg/chromium-dev/-KH_IP0rIWQ....


I agree. There is a plan to move these tests to Web Platform tests. Until that
works starts, we're going to continue using this framework.

https://codereview.chromium.org/2637343002/diff/140001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTDescriptor.cpp
(right):

https://codereview.chromium.org/2637343002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTDescriptor.cpp:41:
// disconnected so we reject.
On 2017/01/24 10:21:02, dcheng wrote:
> Out of curiosity, aren't the checks on line 36/37 sufficient for checking
frame
> disconnection?

Yes.  Lines 42-46 are handling the case where <device>.gatt.disconnect() was
called or the BLE device disconnected. It's unrelated to frame disconnection.

[dcheng, 2017-01-24 17:35:52]

LGTM with a nit

https://codereview.chromium.org/2637343002/diff/140001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTDescriptor.cpp
(right):

https://codereview.chromium.org/2637343002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTDescriptor.cpp:41:
// disconnected so we reject.
On 2017/01/24 16:14:29, dougt wrote:
> On 2017/01/24 10:21:02, dcheng wrote:
> > Out of curiosity, aren't the checks on line 36/37 sufficient for checking
> frame
> > disconnection?
> 
> Yes.  Lines 42-46 are handling the case where <device>.gatt.disconnect() was
> called or the BLE device disconnected. It's unrelated to frame disconnection.

Please update this comment then =)

[scheib, 2017-01-24 17:43:57]

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

https://codereview.chromium.org/2637343002/diff/140001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:791:
base::MakeUnique<NiceMockBluetoothGattDescriptor>(
On 2017/01/24 16:14:29, dougt wrote:
> On 2017/01/24 10:21:02, dcheng wrote:
> > FWIW, I think this would be more clearly written with explicit test fakes,
> > rather than using the magic boilerplate gmock generates, especially since we
> > don't use any of the EXPECT_CALL magic.
> > 
> > P.S. I'm not too sure what the current thinking on gmock is. One previous
> thread
> > is
> >
>
https://groups.google.com/a/chromium.org/forum/#!msg/chromium-dev/-KH_IP0rIWQ....
> 
> 
> I agree. There is a plan to move these tests to Web Platform tests. Until that
> works starts, we're going to continue using this framework.
> 

I've been happy with GMOCK, it's much less code to generate fakes, even simple
ones, and the moment you need to add even some trivial behavior to the fake
there's a good way to it with gmock. Over the years it has been discussed some,
and like many tools it can be used poorly, but I wouldn't move away from it and
I think the system build up in device/bluetooth/test is done fairly well.

BTW, when we move layout tests to web platform tests we'll still need to build
dynamic fakes. It's pretty likely we'll continue to use & extend the
device/bluetooth/test fakes to do that.

[dougt, 2017-01-24 18:32:47]

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

[commit-bot: I haz the power, 2017-01-24 18:33:18]

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, 2017-01-24 20:42:01]

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

[commit-bot: I haz the power, 2017-01-24 20:42:02]

Dry run: This issue passed the CQ dry run.

[dougt, 2017-01-25 00:23:23]

The CQ bit was checked by dougt@chromium.org

[dougt, 2017-01-25 00:23:24]

The patchset sent to the CQ was uploaded after l-g-t-m from ortuno@chromium.org,
scheib@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2637343002/#ps160001
(title: "#5")

[commit-bot: I haz the power, 2017-01-25 00:24:05]

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-25 00:35:11]

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

[commit-bot: I haz the power, 2017-01-25 00:35:13]

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...)

[dougt, 2017-01-25 00:42:09]

dgozman, ptal: content/browser/bad_message.h

[ortuno, 2017-01-25 01:01:56]

Don't forget to run the bad_messages script!

[dougt, 2017-01-25 01:08:29]

On 2017/01/25 01:01:56, ortuno wrote:
> Don't forget to run the bad_messages script!

ran it. random changes to histograms.xml?

[ortuno, 2017-01-25 03:39:40]

On 2017/01/25 at 01:08:29, dougt wrote:
> On 2017/01/25 01:01:56, ortuno wrote:
> > Don't forget to run the bad_messages script!
> 
> ran it. random changes to histograms.xml?

huh, BDH_INVALID_DESCRIPTOR_ID was already there...

[dougt, 2017-01-25 06:22:13]

On 2017/01/25 03:39:40, ortuno wrote:
> On 2017/01/25 at 01:08:29, dougt wrote:
> > On 2017/01/25 01:01:56, ortuno wrote:
> > > Don't forget to run the bad_messages script!
> > 
> > ran it. random changes to histograms.xml?
> 
> huh, BDH_INVALID_DESCRIPTOR_ID was already there...

I suspect I didn't run bad_messages after I removed BDH_INVALID_DESCRIPTOR_ID
from https://codereview.chromium.org/2466223002/.

Maybe we should have a hook that ensures bad_messages gets run?

[dgozman, 2017-01-25 20:01:13]

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

[dgozman, 2017-01-25 20:01:14]

bad_message and histograms lgtm

[dougt, 2017-01-25 22:36:40]

The CQ bit was checked by dougt@chromium.org

[dougt, 2017-01-25 22:36:40]

The patchset sent to the CQ was uploaded after l-g-t-m from scheib@chromium.org,
ortuno@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2637343002/#ps180001
(title: "run bad_message_reasons")

[commit-bot: I haz the power, 2017-01-25 22:37:29]

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-25 22:47:18]

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

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

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

[dougt, 2017-01-26 00:58:50]

isherman, ptal: tools/metrics/histograms/histograms.xml

[Ilya Sherman, 2017-01-26 23:17:39]

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

[Ilya Sherman, 2017-01-26 23:17:42]

histograms.xml lgtm

[dougt, 2017-01-26 23:27:57]

The CQ bit was checked by dougt@chromium.org

[commit-bot: I haz the power, 2017-01-26 23:28:46]

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 05:35:35]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1485473277657390,
"parent_rev": "79db18ef48f6b3a9da4ca6be99beb7697d342a7c", "commit_rev":
"a2fe05521add5347b1024ca582045447cc469325"}

[commit-bot: I haz the power, 2017-01-27 05:36:06]

Description was changed from

==========
Implement WebBluetooth descriptor.readValue()

A GATT Descriptor provides further information about a characteristic's
value. For each characteristic there may be many descriptors as defined by
https://webbluetoothcg.github.io/web-bluetooth/#bluetoothremotegattdescriptor.

In Bug 660699, we added support for accessing descriptors from characteristics.
In this CL, we add support for reading the value of the descriptor.

BUG=668837
R=scheib
==========

to

==========
Implement WebBluetooth descriptor.readValue()

A GATT Descriptor provides further information about a characteristic's
value. For each characteristic there may be many descriptors as defined by
https://webbluetoothcg.github.io/web-bluetooth/#bluetoothremotegattdescriptor.

In Bug 660699, we added support for accessing descriptors from characteristics.
In this CL, we add support for reading the value of the descriptor.

BUG=668837
R=scheib

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

[commit-bot: I haz the power, 2017-01-27 05:36:09]

Committed patchset #6 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/a2fe05521add5347b1024ca58204...