Implement WebBluetooth getDescriptor[s]

Implement WebBluetooth getDescriptor[s]

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.

BUG=660699
R=ortuno@chromium.org

Review-Url: https://codereview.chromium.org/2466223002
Cr-Commit-Position: refs/heads/master@{#443784}
Committed: https://chromium.googlesource.com/chromium/src/+/4f2237ccffefb9642173b872d4a0aafbac57f65e

[dougt, 2016-11-01 03:28:28]

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

[commit-bot: I haz the power, 2016-11-01 03:28:46]

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

[commit-bot: I haz the power, 2016-11-01 05:54:46]

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

[commit-bot: I haz the power, 2016-11-01 05:54:47]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[dougt, 2016-11-10 20:26:41]

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

[commit-bot: I haz the power, 2016-11-10 20:27:05]

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

[commit-bot: I haz the power, 2016-11-10 23:21:52]

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

[commit-bot: I haz the power, 2016-11-10 23:21:52]

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, 2016-11-11 17:42:46]

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

[commit-bot: I haz the power, 2016-11-11 17:43:16]

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

[commit-bot: I haz the power, 2016-11-11 19:58:27]

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

[commit-bot: I haz the power, 2016-11-11 19:58:28]

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, 2016-11-12 01:45:00]

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

[commit-bot: I haz the power, 2016-11-12 01:45:33]

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

[commit-bot: I haz the power, 2016-11-12 03:30:41]

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

[commit-bot: I haz the power, 2016-11-12 03:30:43]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[dougt, 2016-11-16 23:01:59]

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

[commit-bot: I haz the power, 2016-11-16 23:03:21]

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

[commit-bot: I haz the power, 2016-11-17 01:29:52]

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

[commit-bot: I haz the power, 2016-11-17 01:29:53]

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

[dougt, 2016-11-17 22:15:55]

ortuno, can you take a look?

[dougt, 2016-11-18 01:55:15]

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

[commit-bot: I haz the power, 2016-11-18 01:56:41]

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

[commit-bot: I haz the power, 2016-11-18 04:09:49]

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

[commit-bot: I haz the power, 2016-11-18 04:09:50]

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

[ortuno, 2016-11-21 03:34:09]

I didn't have time to look at the tests in details but I think there are some
missing:

- Add tests for readValue and writeValue; See readValue/ and writeValue/.
Specially disconnection and reconnection tests. We might have to fix
generator.py to distinguish between characteristic.readValue and
descriptor.readValue.
- Modify script-tests/characteristic-disconnect-invalidates-objects.js to test
getDescriptor(s).
- Add tests for garbage collection during promise. See
script-tests/characteristic-garbage-collection-ran-during-error.js

https://codereview.chromium.org/2466223002/diff/60001/content/browser/bluetoo...
File content/browser/bluetooth/bluetooth_metrics.cc (right):

https://codereview.chromium.org/2466223002/diff/60001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_metrics.cc:266: case
UMAGATTOperation::DESCRIPTOR_READ:  // TODO dft
TODO(crbug.com/[new bug number]): Record descriptor interactions.

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

https://codereview.chromium.org/2466223002/diff/60001/content/browser/bluetoo...
content/browser/bluetooth/web_bluetooth_service_impl.cc:583: /* todo dft
TODO(crbug.com/[new bug number]): Record descriptor interactions.

Also remove the commented out code.

Also your user name is dougt :p (You can get your chromium user name changed if
you want)

https://codereview.chromium.org/2466223002/diff/60001/content/browser/bluetoo...
content/browser/bluetooth/web_bluetooth_service_impl.cc:593: todo DFT
TODO(crbug.com/[new bug number]): Record descriptor interactions.

https://codereview.chromium.org/2466223002/diff/60001/content/browser/bluetoo...
content/browser/bluetooth/web_bluetooth_service_impl.cc:610: // TODO dft. 
RecordGetCharacteristicsOutcome(quantity,
Same format as above.

https://codereview.chromium.org/2466223002/diff/60001/content/browser/bluetoo...
content/browser/bluetooth/web_bluetooth_service_impl.cc:629: auto insert_result
= descriptor_id_to_characteristic_id_.insert(
.insert({descriptor_instance_id, characteristic_instance_id})

https://codereview.chromium.org/2466223002/diff/60001/content/browser/bluetoo...
content/browser/bluetooth/web_bluetooth_service_impl.cc:638:
descriptor_ptr->instance_id = mojo::String(descriptor_instance_id);
No need for mojo::String

https://codereview.chromium.org/2466223002/diff/60001/content/browser/bluetoo...
content/browser/bluetooth/web_bluetooth_service_impl.cc:648: /* TODO dft
Same format as above. No need for the commented out code.

https://codereview.chromium.org/2466223002/diff/60001/content/browser/bluetoo...
content/browser/bluetooth/web_bluetooth_service_impl.cc:657: /* TODO dft
Same format as above. No need for the commented out code.

https://codereview.chromium.org/2466223002/diff/60001/content/browser/bluetoo...
content/browser/bluetooth/web_bluetooth_service_impl.cc:674: // TODO dft
Same format as above. Same for all instances in the rest of the file.

https://codereview.chromium.org/2466223002/diff/60001/content/browser/bluetoo...
content/browser/bluetooth/web_bluetooth_service_impl.cc:1273:
descriptor_id_to_characteristic_id_.clear();
nit: Move this above characteristic_id_to_service_id.clear();

https://codereview.chromium.org/2466223002/diff/60001/content/browser/bluetoo...
File content/browser/bluetooth/web_bluetooth_service_impl.h (right):

https://codereview.chromium.org/2466223002/diff/60001/content/browser/bluetoo...
content/browser/bluetooth/web_bluetooth_service_impl.h:99: void
GattDescriptorValueChanged(
per comment in web_bluetooth.mojom, we don't need these two.

https://codereview.chromium.org/2466223002/diff/60001/content/browser/bluetoo...
content/browser/bluetooth/web_bluetooth_service_impl.h:155: // Callbacks for
BluetoothRemoteGattDescriptor::ReadRemoteDescriptor.
Move these down with the rest of the callbacks.

https://codereview.chromium.org/2466223002/diff/60001/content/browser/bluetoo...
content/browser/bluetooth/web_bluetooth_service_impl.h:255: // |service| and
|descriptor| fields if successful.
|service|, |characteristic|, and |descriptor|

https://codereview.chromium.org/2466223002/diff/60001/content/renderer/blueto...
File content/renderer/bluetooth/web_bluetooth_impl.h (right):

https://codereview.chromium.org/2466223002/diff/60001/content/renderer/blueto...
content/renderer/bluetooth/web_bluetooth_impl.h:101: void
registerDescriptorObject(
Per comment in web_bluetooth.mojom, no need for these two functions

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

https://codereview.chromium.org/2466223002/diff/60001/content/shell/browser/l...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:95:
// client config is in our blacklist.  it must not be writable
s/client/Client/ and s/it/It/

https://codereview.chromium.org/2466223002/diff/60001/content/shell/browser/l...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:96:
const char kClientConfigUUID[] = "00002902-0000-1000-8000-00805f9b34fb";
Since it's a standard service you don't need the suffix; 2902 should be enough.

https://codereview.chromium.org/2466223002/diff/60001/content/shell/browser/l...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:1126:
LayoutTestBluetoothAdapterProvider::GetFailingGATTOperationsAdapter() {
Also add descriptors to the device in this adapter.

https://codereview.chromium.org/2466223002/diff/60001/content/shell/browser/l...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:1518:
// Add two descriptors on the heart rate.  One for user description, and one
I would add these to DisconnectingHealthThermometer instead. 

1. We started with heart rate adapter which had one characteristic for read, one
for write and one for notifications. But we then found out about health
thermometer which has a single characteristic that does read, write and
notifications. I want to eventually move all tests to use health thermometer
device.
2. You will need the disconnection characteristic for some tests.

Also instead of user description I would use valid_range which is an actual
descriptor of measurement interval.

Also don't forget to update the docs in the .h file!

https://codereview.chromium.org/2466223002/diff/60001/device/bluetooth/test/m...
File device/bluetooth/test/mock_bluetooth_gatt_characteristic.h (right):

https://codereview.chromium.org/2466223002/diff/60001/device/bluetooth/test/m...
device/bluetooth/test/mock_bluetooth_gatt_characteristic.h:64: // Also see
mock_bluetooth_gatt_service
Copy the comment in mock_bluetooth_gatt_service :p

https://codereview.chromium.org/2466223002/diff/60001/device/bluetooth/test/m...
device/bluetooth/test/mock_bluetooth_gatt_characteristic.h:82:
ScopedVector<MockBluetoothGattDescriptor> mock_descriptors_;
ScopedVector is deprecated, use
std::vector<std::unique_ptr<MockBluetoothGattDescriptor>>

https://codereview.chromium.org/2466223002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/bluetooth/getDescriptor/descriptor-blacklisted.html
(right):

https://codereview.chromium.org/2466223002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/getDescriptor/descriptor-blacklisted.html:1:
<!DOCTYPE html>
Can you change the name to descriptor-write-blacklisted.html

https://codereview.chromium.org/2466223002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/getDescriptor/descriptor-blacklisted.html:17:
.then(characteristic =>
characteristic.getDescriptor("00002902-0000-1000-8000-00805f9b34fb"))
No need for the whole UUID since it's a standard UUID:
'gatt.characteristic_user_description' should be enough.

https://codereview.chromium.org/2466223002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/getDescriptor/descriptor-blacklisted.html:19:
assert_promise_rejects_with_message(
Either return or don't use '{' so that it's implicitly returned. Otherwise the
test is not going to fail because the promise is not being chained.

https://codereview.chromium.org/2466223002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/getDescriptor/descriptor-blacklisted.html:20:
descriptor.writeValue(new Uint8Array(1)),
Also add a test for excluded-read. There is an excluded-read descriptor already:

https://cs.chromium.org/chromium/src/content/browser/bluetooth/bluetooth_blac...

https://codereview.chromium.org/2466223002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/bluetooth/getDescriptor/descriptor-found.html
(right):

https://codereview.chromium.org/2466223002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/getDescriptor/descriptor-found.html:7: 
Use the test generator e.g.

// script-tests/descriptor-found.js

'use strict';

promise_test(() => {
  return setBluetoothFakeAdapter('DisconnectingHealthThermometerAdapter')
    .then(() => requestDeviceWithKeyDown({filters: ['health_thermometer']})
    .then(device => device.gatt.connect())
    .then(gattServer => gattServer.getPrimaryService('health_thermometer'))
    .then(service => service.getCharacteristic('measurement_interval'))
    .then(characteristic => characteristic.CALLS([
       getDescriptor('valid_range')|
       getDescriptors('valid_range')[UUID]]))
    .then(descriptors => {
      // Convert to array if necessary.
      descriptors = [].concat(descriptors);
      descriptors.forEach(descriptor => {
        assert_equals(descriptor.uuid,
BluetoothUUID.getDescriptor('valid_range'));
      });
    });
}, 'Request for descriptor. Should return right descriptor.');

Because of CALLS([getDescriptor(...)| getDescriptors(...)[UUID]), generator.py
will generate two tests:

 getDescriptor/gen-descriptor-found.html
 getDescriptors/gen-descriptor-found-with-uuid.html

https://codereview.chromium.org/2466223002/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/resources/bluetooth/bluetooth-helpers.js
(right):

https://codereview.chromium.org/2466223002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/resources/bluetooth/bluetooth-helpers.js:57:
name: 'user_description',
This should be: 'gatt.characteristic_user_description'.

https://codereview.chromium.org/2466223002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/resources/bluetooth/bluetooth-helpers.js:62:
name: 'client_characteristic_configuration',
This one should be: 'gatt.client_characteristic_configuration'

https://codereview.chromium.org/2466223002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/events/EventTypeNames.in (right):

https://codereview.chromium.org/2466223002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/events/EventTypeNames.in:68:
descriptorvaluechanged
We don't have a descriptorvaluechanged event. Maybe we should, to match
characteristics but that's a separate issue. Removing it would actually simplify
things quite a bit.

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

https://codereview.chromium.org/2466223002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp:122:
// TODO dft:  maybe we can share with GetCharacteristicsCallback
TODO(crbug.com/[new bug number]): Make class that can be shared with
GetCharacteristicsCallback.

fwiw: This pattern will go away once we Onion Soup this. So the new issue might
not be worth it.

https://codereview.chromium.org/2466223002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp:127:
GetDescriptorsCallback(BluetoothRemoteGATTService* service,
I think you forgot to change this to BluetoothRemoteGATTCharacteristic. The
characteristic object has a gatt() method so you don't have to do
m_characteristic->service()->device()->gatt().

https://codereview.chromium.org/2466223002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp:148:
DOMException::create(NetworkError, kGATTServerDisconnected));
Add a new error for GetDescriptor(s) similar to Service's "GATT Server
disconnected while retrieving characteristics.";

https://codereview.chromium.org/2466223002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp:154:
m_resolver->resolve(BluetoothRemoteGATTDescriptor::take(
Add TODO for adding the descriptor to the map e.g.:

TODO(crbug.com/495270): Add the descriptor to the Attribute Instance Map when
the map supports it.

https://codereview.chromium.org/2466223002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp:162:
descriptors.append(BluetoothRemoteGATTDescriptor::take(
Add same TODO here.

https://codereview.chromium.org/2466223002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp:206:

Also check that the characteristic is still valid
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/blueto...

https://codereview.chromium.org/2466223002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTDescriptor.h
(right):

https://codereview.chromium.org/2466223002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTDescriptor.h:1:
// Copyright 2015 The Chromium Authors. All rights reserved.
nit: 2016

https://codereview.chromium.org/2466223002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTDescriptor.h:33:
// descriptor.
s/descriptor/characteristic/

https://codereview.chromium.org/2466223002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTDescriptor.h:103:
BluetoothRemoteGATTServer* gatt() { return m_service->device()->gatt(); }
s/m_service/m_characteristic/ Also could you rename this to getGatt()? Since we
changed the style guide gatt is no longer allowed.

https://codereview.chromium.org/2466223002/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTDescriptor.idl
(right):

https://codereview.chromium.org/2466223002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTDescriptor.idl:5:
//
https://webbluetoothchrome.github.io/web-bluetooth/#idl-def-bluetoothgattdesc...
That spec is no longer being updated use:

https://webbluetoothcg.github.io/web-bluetooth/#bluetoothremotegattdescriptor

https://codereview.chromium.org/2466223002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTDescriptor.idl:12:
] interface BluetoothRemoteGATTDescriptor : EventTarget {//:
DescriptorEventHandlers {
There are no descriptor events so no need for EventTarget.

https://codereview.chromium.org/2466223002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTDescriptor.idl:18:
[CallWith=ScriptState] Promise<DataView>                          readValue();
nit: Line the names up.

https://codereview.chromium.org/2466223002/diff/60001/third_party/WebKit/publ...
File
third_party/WebKit/public/platform/modules/bluetooth/WebBluetoothRemoteGATTDescriptorInit.h
(right):

https://codereview.chromium.org/2466223002/diff/60001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/bluetooth/WebBluetoothRemoteGATTDescriptorInit.h:24:
const WebString characteristicInstanceID;
Remove this attribute. I think we intended for it to be used when we implemented
the object tree but we ended up implemented without using it, so it's safe to
remove.

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

https://codereview.chromium.org/2466223002/diff/60001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom:225:
RemoteDescriptorValueChanged(string descriptor_instance_id,
I don't think we need this. We do it for characteristics because we need to get
characteristics notifications but there is no such a thing for descriptors.

[ortuno, 2016-11-21 03:37:01]

Also add all descriptor related tests to TestExpectations since we don't support
descriptors on macOS yet.

[ortuno, 2016-11-21 03:37:28]

Also sorry for the delay! I was trying to finish other work before branch point.

[dougt, 2016-11-22 01:44:34]

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

[commit-bot: I haz the power, 2016-11-22 01:45:10]

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

[dougt, 2016-11-22 01:47:17]

I addressed most of your feedback. Thank you.

I have not started on any of the requested test changes yet.

https://codereview.chromium.org/2466223002/diff/60001/content/browser/bluetoo...
File content/browser/bluetooth/bluetooth_metrics.cc (right):

https://codereview.chromium.org/2466223002/diff/60001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_metrics.cc:266: case
UMAGATTOperation::DESCRIPTOR_READ:  // TODO dft
On 2016/11/21 03:34:08, ortuno wrote:
> TODO(crbug.com/[new bug number]): Record descriptor interactions.

Acknowledged.

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

https://codereview.chromium.org/2466223002/diff/60001/content/browser/bluetoo...
content/browser/bluetooth/web_bluetooth_service_impl.cc:583: /* todo dft
On 2016/11/21 03:34:08, ortuno wrote:
> TODO(crbug.com/[new bug number]): Record descriptor interactions.
> 
> Also remove the commented out code.
> 
> Also your user name is dougt :p (You can get your chromium user name changed
if
> you want)

Acknowledged.

https://codereview.chromium.org/2466223002/diff/60001/content/browser/bluetoo...
content/browser/bluetooth/web_bluetooth_service_impl.cc:593: todo DFT
On 2016/11/21 03:34:08, ortuno wrote:
> TODO(crbug.com/[new bug number]): Record descriptor interactions.

Acknowledged.

https://codereview.chromium.org/2466223002/diff/60001/content/browser/bluetoo...
content/browser/bluetooth/web_bluetooth_service_impl.cc:610: // TODO dft. 
RecordGetCharacteristicsOutcome(quantity,
On 2016/11/21 03:34:08, ortuno wrote:
> Same format as above.

Acknowledged.

https://codereview.chromium.org/2466223002/diff/60001/content/browser/bluetoo...
content/browser/bluetooth/web_bluetooth_service_impl.cc:629: auto insert_result
= descriptor_id_to_characteristic_id_.insert(
On 2016/11/21 03:34:08, ortuno wrote:
> .insert({descriptor_instance_id, characteristic_instance_id})

Acknowledged.

https://codereview.chromium.org/2466223002/diff/60001/content/browser/bluetoo...
content/browser/bluetooth/web_bluetooth_service_impl.cc:638:
descriptor_ptr->instance_id = mojo::String(descriptor_instance_id);
On 2016/11/21 03:34:08, ortuno wrote:
> No need for mojo::String

Acknowledged.

https://codereview.chromium.org/2466223002/diff/60001/content/browser/bluetoo...
content/browser/bluetooth/web_bluetooth_service_impl.cc:648: /* TODO dft
On 2016/11/21 03:34:08, ortuno wrote:
> Same format as above. No need for the commented out code.

Acknowledged.

https://codereview.chromium.org/2466223002/diff/60001/content/browser/bluetoo...
content/browser/bluetooth/web_bluetooth_service_impl.cc:657: /* TODO dft
On 2016/11/21 03:34:08, ortuno wrote:
> Same format as above. No need for the commented out code.

Acknowledged.

https://codereview.chromium.org/2466223002/diff/60001/content/browser/bluetoo...
content/browser/bluetooth/web_bluetooth_service_impl.cc:674: // TODO dft
On 2016/11/21 03:34:08, ortuno wrote:
> Same format as above. Same for all instances in the rest of the file.

Acknowledged.

https://codereview.chromium.org/2466223002/diff/60001/content/browser/bluetoo...
content/browser/bluetooth/web_bluetooth_service_impl.cc:1273:
descriptor_id_to_characteristic_id_.clear();
On 2016/11/21 03:34:08, ortuno wrote:
> nit: Move this above characteristic_id_to_service_id.clear();

Acknowledged.

https://codereview.chromium.org/2466223002/diff/60001/content/browser/bluetoo...
File content/browser/bluetooth/web_bluetooth_service_impl.h (right):

https://codereview.chromium.org/2466223002/diff/60001/content/browser/bluetoo...
content/browser/bluetooth/web_bluetooth_service_impl.h:99: void
GattDescriptorValueChanged(
On 2016/11/21 03:34:08, ortuno wrote:
> per comment in web_bluetooth.mojom, we don't need these two.

Acknowledged.

https://codereview.chromium.org/2466223002/diff/60001/content/browser/bluetoo...
content/browser/bluetooth/web_bluetooth_service_impl.h:155: // Callbacks for
BluetoothRemoteGattDescriptor::ReadRemoteDescriptor.
On 2016/11/21 03:34:08, ortuno wrote:
> Move these down with the rest of the callbacks.

Acknowledged.

https://codereview.chromium.org/2466223002/diff/60001/content/browser/bluetoo...
content/browser/bluetooth/web_bluetooth_service_impl.h:255: // |service| and
|descriptor| fields if successful.
On 2016/11/21 03:34:08, ortuno wrote:
> |service|, |characteristic|, and |descriptor|

Acknowledged.

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

https://codereview.chromium.org/2466223002/diff/60001/content/shell/browser/l...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:95:
// client config is in our blacklist.  it must not be writable
On 2016/11/21 03:34:08, ortuno wrote:
> s/client/Client/ and s/it/It/

Acknowledged.

https://codereview.chromium.org/2466223002/diff/60001/content/shell/browser/l...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:96:
const char kClientConfigUUID[] = "00002902-0000-1000-8000-00805f9b34fb";
On 2016/11/21 03:34:08, ortuno wrote:
> Since it's a standard service you don't need the suffix; 2902 should be
enough.

Acknowledged.

https://codereview.chromium.org/2466223002/diff/60001/device/bluetooth/test/m...
File device/bluetooth/test/mock_bluetooth_gatt_characteristic.h (right):

https://codereview.chromium.org/2466223002/diff/60001/device/bluetooth/test/m...
device/bluetooth/test/mock_bluetooth_gatt_characteristic.h:64: // Also see
mock_bluetooth_gatt_service
On 2016/11/21 03:34:08, ortuno wrote:
> Copy the comment in mock_bluetooth_gatt_service :p

Acknowledged.

https://codereview.chromium.org/2466223002/diff/60001/device/bluetooth/test/m...
device/bluetooth/test/mock_bluetooth_gatt_characteristic.h:82:
ScopedVector<MockBluetoothGattDescriptor> mock_descriptors_;
On 2016/11/21 03:34:08, ortuno wrote:
> ScopedVector is deprecated, use
> std::vector<std::unique_ptr<MockBluetoothGattDescriptor>>

Acknowledged.

https://codereview.chromium.org/2466223002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/bluetooth/getDescriptor/descriptor-blacklisted.html
(right):

https://codereview.chromium.org/2466223002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/getDescriptor/descriptor-blacklisted.html:1:
<!DOCTYPE html>
On 2016/11/21 03:34:09, ortuno wrote:
> Can you change the name to descriptor-write-blacklisted.html

Acknowledged.

https://codereview.chromium.org/2466223002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/getDescriptor/descriptor-blacklisted.html:17:
.then(characteristic =>
characteristic.getDescriptor("00002902-0000-1000-8000-00805f9b34fb"))
On 2016/11/21 03:34:08, ortuno wrote:
> No need for the whole UUID since it's a standard UUID:
> 'gatt.characteristic_user_description' should be enough.

Acknowledged.

https://codereview.chromium.org/2466223002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/getDescriptor/descriptor-blacklisted.html:19:
assert_promise_rejects_with_message(
On 2016/11/21 03:34:08, ortuno wrote:
> Either return or don't use '{' so that it's implicitly returned. Otherwise the
> test is not going to fail because the promise is not being chained.

Acknowledged.

https://codereview.chromium.org/2466223002/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/resources/bluetooth/bluetooth-helpers.js
(right):

https://codereview.chromium.org/2466223002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/resources/bluetooth/bluetooth-helpers.js:57:
name: 'user_description',
On 2016/11/21 03:34:09, ortuno wrote:
> This should be: 'gatt.characteristic_user_description'.

Acknowledged.

https://codereview.chromium.org/2466223002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/resources/bluetooth/bluetooth-helpers.js:62:
name: 'client_characteristic_configuration',
On 2016/11/21 03:34:09, ortuno wrote:
> This one should be: 'gatt.client_characteristic_configuration'

Acknowledged.

https://codereview.chromium.org/2466223002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/events/EventTypeNames.in (right):

https://codereview.chromium.org/2466223002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/events/EventTypeNames.in:68:
descriptorvaluechanged
On 2016/11/21 03:34:09, ortuno wrote:
> We don't have a descriptorvaluechanged event. Maybe we should, to match
> characteristics but that's a separate issue. Removing it would actually
simplify
> things quite a bit.

Acknowledged.  There is nothing in the spec yet about this.  Removing.

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

https://codereview.chromium.org/2466223002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp:122:
// TODO dft:  maybe we can share with GetCharacteristicsCallback
On 2016/11/21 03:34:09, ortuno wrote:
> TODO(crbug.com/[new bug number]): Make class that can be shared with
> GetCharacteristicsCallback.
> 
> fwiw: This pattern will go away once we Onion Soup this. So the new issue
might
> not be worth it.

Onion Soup for the win.  I'll remove my comment for now.

https://codereview.chromium.org/2466223002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp:127:
GetDescriptorsCallback(BluetoothRemoteGATTService* service,
On 2016/11/21 03:34:09, ortuno wrote:
> I think you forgot to change this to BluetoothRemoteGATTCharacteristic. The
> characteristic object has a gatt() method so you don't have to do
> m_characteristic->service()->device()->gatt().

Acknowledged.

https://codereview.chromium.org/2466223002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp:148:
DOMException::create(NetworkError, kGATTServerDisconnected));
On 2016/11/21 03:34:09, ortuno wrote:
> Add a new error for GetDescriptor(s) similar to Service's "GATT Server
> disconnected while retrieving characteristics.";


For most of the calls on characteristics, you'll get an error message like
"Cannot perform GATT operations."

Do you want us to clean up all of the error messages in a follow up?

https://codereview.chromium.org/2466223002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp:154:
m_resolver->resolve(BluetoothRemoteGATTDescriptor::take(
On 2016/11/21 03:34:09, ortuno wrote:
> Add TODO for adding the descriptor to the map e.g.:
> 
> TODO(crbug.com/495270): Add the descriptor to the Attribute Instance Map when
> the map supports it.

Acknowledged.

https://codereview.chromium.org/2466223002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp:162:
descriptors.append(BluetoothRemoteGATTDescriptor::take(
On 2016/11/21 03:34:09, ortuno wrote:
> Add same TODO here.

Acknowledged.

https://codereview.chromium.org/2466223002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp:206:

On 2016/11/21 03:34:09, ortuno wrote:
> Also check that the characteristic is still valid
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/blueto...

Acknowledged.

https://codereview.chromium.org/2466223002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTDescriptor.h
(right):

https://codereview.chromium.org/2466223002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTDescriptor.h:1:
// Copyright 2015 The Chromium Authors. All rights reserved.
On 2016/11/21 03:34:09, ortuno wrote:
> nit: 2016

Acknowledged.

https://codereview.chromium.org/2466223002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTDescriptor.h:33:
// descriptor.
On 2016/11/21 03:34:09, ortuno wrote:
> s/descriptor/characteristic/

Acknowledged.

https://codereview.chromium.org/2466223002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTDescriptor.h:103:
BluetoothRemoteGATTServer* gatt() { return m_service->device()->gatt(); }
On 2016/11/21 03:34:09, ortuno wrote:
> s/m_service/m_characteristic/ Also could you rename this to getGatt()? Since
we
> changed the style guide gatt is no longer allowed.

Acknowledged.

https://codereview.chromium.org/2466223002/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTDescriptor.idl
(right):

https://codereview.chromium.org/2466223002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTDescriptor.idl:5:
//
https://webbluetoothchrome.github.io/web-bluetooth/#idl-def-bluetoothgattdesc...
On 2016/11/21 03:34:09, ortuno wrote:
> That spec is no longer being updated use:
> 
> https://webbluetoothcg.github.io/web-bluetooth/#bluetoothremotegattdescriptor

Acknowledged.

https://codereview.chromium.org/2466223002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTDescriptor.idl:12:
] interface BluetoothRemoteGATTDescriptor : EventTarget {//:
DescriptorEventHandlers {
On 2016/11/21 03:34:09, ortuno wrote:
> There are no descriptor events so no need for EventTarget.

Acknowledged.

https://codereview.chromium.org/2466223002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTDescriptor.idl:18:
[CallWith=ScriptState] Promise<DataView>                          readValue();
On 2016/11/21 03:34:09, ortuno wrote:
> nit: Line the names up.

Acknowledged.

https://codereview.chromium.org/2466223002/diff/60001/third_party/WebKit/publ...
File
third_party/WebKit/public/platform/modules/bluetooth/WebBluetoothRemoteGATTDescriptorInit.h
(right):

https://codereview.chromium.org/2466223002/diff/60001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/bluetooth/WebBluetoothRemoteGATTDescriptorInit.h:24:
const WebString characteristicInstanceID;
On 2016/11/21 03:34:09, ortuno wrote:
> Remove this attribute. I think we intended for it to be used when we
implemented
> the object tree but we ended up implemented without using it, so it's safe to
> remove.

Acknowledged.

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

https://codereview.chromium.org/2466223002/diff/60001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom:225:
RemoteDescriptorValueChanged(string descriptor_instance_id,
On 2016/11/21 03:34:09, ortuno wrote:
> I don't think we need this. We do it for characteristics because we need to
get
> characteristics notifications but there is no such a thing for descriptors.

Acknowledged.

[commit-bot: I haz the power, 2016-11-22 02:42:11]

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

[commit-bot: I haz the power, 2016-11-22 02:42:12]

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

[ortuno, 2016-11-22 03:02:02]

https://codereview.chromium.org/2466223002/diff/80001/content/browser/bluetoo...
File content/browser/bluetooth/web_bluetooth_service_impl.h (right):

https://codereview.chromium.org/2466223002/diff/80001/content/browser/bluetoo...
content/browser/bluetooth/web_bluetooth_service_impl.h:141: mojo::Array<uint8_t>
value,
juncai's patch changes this to vector so you might have to rebase soon
http://crrev.com/2506813003

https://codereview.chromium.org/2466223002/diff/80001/device/bluetooth/test/m...
File device/bluetooth/test/mock_bluetooth_gatt_characteristic.h (right):

https://codereview.chromium.org/2466223002/diff/80001/device/bluetooth/test/m...
device/bluetooth/test/mock_bluetooth_gatt_characteristic.h:81: // dougt TODO
I think you are forgetting something here?

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

https://codereview.chromium.org/2466223002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp:197:
Persistent<BluetoothRemoteGATTService> m_service;
No need to pass in the service. You can use m_characteristic->gatt();

https://codereview.chromium.org/2466223002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp:209:

Check the characteristic is still valid and also add an #ifdef for macOS. See
stopNotifications()

https://codereview.chromium.org/2466223002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTDescriptor.h
(right):

https://codereview.chromium.org/2466223002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTDescriptor.h:35:
: public GarbageCollectedFinalized<BluetoothRemoteGATTDescriptor>,
Does this need to be Finalized? Could it be just GarbageCollected? There is a
clang plugging that will complain if it shouldn't.

https://codereview.chromium.org/2466223002/diff/80001/third_party/WebKit/publ...
File
third_party/WebKit/public/platform/modules/bluetooth/WebBluetoothRemoteGATTDescriptor.h
(right):

https://codereview.chromium.org/2466223002/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/bluetooth/WebBluetoothRemoteGATTDescriptor.h:14:
class WebBluetoothRemoteGATTDescriptor {
I don't think we need this file anymore.

https://codereview.chromium.org/2466223002/diff/80001/third_party/WebKit/publ...
File
third_party/WebKit/public/platform/modules/bluetooth/WebBluetoothRemoteGATTDescriptorInit.h
(right):

https://codereview.chromium.org/2466223002/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/bluetooth/WebBluetoothRemoteGATTDescriptorInit.h:20:
: characteristicInstanceID(characteristicInstanceID),  // dougt??
You forgot to remove :p

[dougt, 2016-11-24 06:58:30]

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

[commit-bot: I haz the power, 2016-11-24 06:58:49]

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

[commit-bot: I haz the power, 2016-11-24 07:03:07]

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

[commit-bot: I haz the power, 2016-11-24 07:03:10]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  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, 2016-11-24 22:14:39]

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

[commit-bot: I haz the power, 2016-11-24 22:14:55]

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

[commit-bot: I haz the power, 2016-11-24 23:35:05]

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

[commit-bot: I haz the power, 2016-11-24 23:35:06]

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

[ortuno, 2016-11-26 06:21:57]

As discussed offline we'll should:

1. Only implement getDescriptor(s) in this patch and add generator tests.
2. Add histograms
3. Fix generator to distinguish between characteristic.readValue/writeValue and
descriptor.readValue/writeValue.
4. Implement descriptor.readValue
5. Implement descriptor.writeValue.

Thanks!

[dougt, 2016-11-26 23:24:35]

On 2016/11/26 06:21:57, ortuno wrote:
> As discussed offline we'll should:
> 
> 1. Only implement getDescriptor(s) in this patch and add generator tests.
> 2. Add histograms
> 3. Fix generator to distinguish between characteristic.readValue/writeValue
and
> descriptor.readValue/writeValue.
> 4. Implement descriptor.readValue
> 5. Implement descriptor.writeValue.
> 
> Thanks!

SGTM

1. This cl.
2. Implement BluetoothRemoteGATTDescriptor reporting - crbug.com/667319
3. Fix generator to distinguish between characteristic.readValue/writeValue and
> descriptor.readValue/writeValue. - crbug.com/668836
4. Implement WebBluetooth descriptor.readValue() - crbug.com/668837
5. Implement WebBluetooth descriptor.writeValue() - crbug.com/668838

[dougt, 2016-11-29 22:39:55]

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

[commit-bot: I haz the power, 2016-11-29 22:41:07]

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

[commit-bot: I haz the power, 2016-11-29 22:45:23]

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

[commit-bot: I haz the power, 2016-11-29 22:45:24]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[dougt, 2016-11-29 22:50:48]

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

https://codereview.chromium.org/2466223002/diff/60001/content/shell/browser/l...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:1518:
// Add two descriptors on the heart rate.  One for user description, and one
On 2016/11/21 03:34:08, ortuno wrote:
> I would add these to DisconnectingHealthThermometer instead. 
> 
> 1. We started with heart rate adapter which had one characteristic for read,
one
> for write and one for notifications. But we then found out about health
> thermometer which has a single characteristic that does read, write and
> notifications. I want to eventually move all tests to use health thermometer
> device.
> 2. You will need the disconnection characteristic for some tests.
> 
> Also instead of user description I would use valid_range which is an actual
> descriptor of measurement interval.
> 
> Also don't forget to update the docs in the .h file!

Done.

https://codereview.chromium.org/2466223002/diff/80001/content/browser/bluetoo...
File content/browser/bluetooth/web_bluetooth_service_impl.h (right):

https://codereview.chromium.org/2466223002/diff/80001/content/browser/bluetoo...
content/browser/bluetooth/web_bluetooth_service_impl.h:141: mojo::Array<uint8_t>
value,
On 2016/11/22 03:02:02, ortuno wrote:
> juncai's patch changes this to vector so you might have to rebase soon
> http://crrev.com/2506813003

Done.

https://codereview.chromium.org/2466223002/diff/80001/device/bluetooth/test/m...
File device/bluetooth/test/mock_bluetooth_gatt_characteristic.h (right):

https://codereview.chromium.org/2466223002/diff/80001/device/bluetooth/test/m...
device/bluetooth/test/mock_bluetooth_gatt_characteristic.h:81: // dougt TODO
On 2016/11/22 03:02:02, ortuno wrote:
> I think you are forgetting something here?

Done.

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

https://codereview.chromium.org/2466223002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp:197:
Persistent<BluetoothRemoteGATTService> m_service;
On 2016/11/22 03:02:02, ortuno wrote:
> No need to pass in the service. You can use m_characteristic->gatt();

Done.

https://codereview.chromium.org/2466223002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp:209:

On 2016/11/22 03:02:02, ortuno wrote:
> Check the characteristic is still valid and also add an #ifdef for macOS. See
> stopNotifications()


A few lines below (216) I check to see if the characteristic is valid.

I added a check for MACOS and returning a not supported error.

https://codereview.chromium.org/2466223002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTDescriptor.h
(right):

https://codereview.chromium.org/2466223002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTDescriptor.h:35:
: public GarbageCollectedFinalized<BluetoothRemoteGATTDescriptor>,
On 2016/11/22 03:02:02, ortuno wrote:
> Does this need to be Finalized? Could it be just GarbageCollected? There is a
> clang plugging that will complain if it shouldn't.


It needs to be finalized.

../../third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTDescriptor.h:34:1:
error: [blink-gc] Class 'BluetoothRemoteGATTDescriptor' requires finalization.
class BluetoothRemoteGATTDescriptor final
^
../../third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTDescriptor.h:69:3:
note: [blink-gc] Field 'm_webDescriptor' requiring finalization declared here:
  std::unique_ptr<WebBluetoothRemoteGATTDescriptorInit> m_webDescriptor;

https://codereview.chromium.org/2466223002/diff/80001/third_party/WebKit/publ...
File
third_party/WebKit/public/platform/modules/bluetooth/WebBluetoothRemoteGATTDescriptor.h
(right):

https://codereview.chromium.org/2466223002/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/bluetooth/WebBluetoothRemoteGATTDescriptor.h:14:
class WebBluetoothRemoteGATTDescriptor {
On 2016/11/22 03:02:02, ortuno wrote:
> I don't think we need this file anymore.

Done.

https://codereview.chromium.org/2466223002/diff/80001/third_party/WebKit/publ...
File
third_party/WebKit/public/platform/modules/bluetooth/WebBluetoothRemoteGATTDescriptorInit.h
(right):

https://codereview.chromium.org/2466223002/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/bluetooth/WebBluetoothRemoteGATTDescriptorInit.h:20:
: characteristicInstanceID(characteristicInstanceID),  // dougt??
On 2016/11/22 03:02:02, ortuno wrote:
> You forgot to remove :p

Done.

[dougt, 2016-11-30 06:33:40]

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

[commit-bot: I haz the power, 2016-11-30 06:34:12]

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

[commit-bot: I haz the power, 2016-11-30 09:08:20]

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

[commit-bot: I haz the power, 2016-11-30 09:08:21]

Dry run: This issue passed the CQ dry run.

[ortuno, 2016-12-02 06:14:51]

Almost there! I think we are missing the following tests:

 - request a blocklisted descriptor
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/bluetooth...
 - descriptor(s) is(are) found
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/bluetooth...
 - device goes out of range
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/bluetooth...
 - service is removed
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/bluetooth...
 - characteristic is removed
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/bluetooth...
 - make sure blocklisted characteristics are not returned if getDescriptors() is
called on a characteristic with blacklisted descriptors
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/bluetooth...
 - multiple descriptors with same UUID. We might have to wait for read for this
one
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/bluetooth...

(I can't believe I've been writing all these tests and their variations manually
😅)

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

https://codereview.chromium.org/2466223002/diff/160001/content/browser/blueto...
content/browser/bluetooth/bluetooth_metrics.cc:234: case
CacheQueryOutcome::NO_DESCRIPTOR:
nit: Similarly here. I don't think we need these changes in this patch.

https://codereview.chromium.org/2466223002/diff/160001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_metrics.h (right):

https://codereview.chromium.org/2466223002/diff/160001/content/browser/blueto...
content/browser/bluetooth/bluetooth_metrics.h:54: NO_DESCRIPTOR = 5,
nit: Changes in this file are no longer relevant to this CL since we don't
implement read/write anymore.

https://codereview.chromium.org/2466223002/diff/160001/content/browser/blueto...
File content/browser/bluetooth/cache_query_result.h (right):

https://codereview.chromium.org/2466223002/diff/160001/content/browser/blueto...
content/browser/bluetooth/cache_query_result.h:15: class
BluetoothRemoteGattDescriptor;
nit: Similarly here. I don't think we need these changes in this patch.

https://codereview.chromium.org/2466223002/diff/160001/content/browser/blueto...
File content/browser/bluetooth/web_bluetooth_service_impl.h (right):

https://codereview.chromium.org/2466223002/diff/160001/content/browser/blueto...
content/browser/bluetooth/web_bluetooth_service_impl.h:136: void
RemoteDescriptorReadValue(
No need for RemoteDescriptorReadValue/WriteValue since we never get here.

https://codereview.chromium.org/2466223002/diff/160001/content/browser/blueto...
content/browser/bluetooth/web_bluetooth_service_impl.h:205: // Callbacks for
BluetoothRemoteGattDescriptor::ReadRemoteDescriptor.
Same here. No need for these callbacks.

https://codereview.chromium.org/2466223002/diff/160001/content/browser/blueto...
content/browser/bluetooth/web_bluetooth_service_impl.h:242: // Queries the
platform cache for a descriptor with
No need for this one either.

https://codereview.chromium.org/2466223002/diff/160001/content/renderer/bluet...
File content/renderer/bluetooth/web_bluetooth_impl.h (right):

https://codereview.chromium.org/2466223002/diff/160001/content/renderer/bluet...
content/renderer/bluetooth/web_bluetooth_impl.h:92: void descriptorReadValue(
No need for these since we don't implement it yet.

https://codereview.chromium.org/2466223002/diff/160001/content/renderer/bluet...
content/renderer/bluetooth/web_bluetooth_impl.h:125: const blink::WebString&
characteristic_instance_id,
I think it was correct before i.e. service_instance_id.

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

https://codereview.chromium.org/2466223002/diff/160001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:95:
// Client Config is in our blacklist.  It must not be writable
s/blacklist/blocklist/

https://codereview.chromium.org/2466223002/diff/160001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:667:
void LayoutTestBluetoothAdapterProvider::AddDescriptorsToCharacteristic(
Given that this will only be used in one place (see note below), I think we
should just inline it.

https://codereview.chromium.org/2466223002/diff/160001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:681:
ON_CALL(*client_config, ReadRemoteDescriptor(_, _))
No need for Write/Read yet.

https://codereview.chromium.org/2466223002/diff/160001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:691:
ON_CALL(*client_config, WriteRemoteDescriptor(_, _, _))
Rather than succeeding this should DCHECK. See other blocklisted operations.

https://codereview.chromium.org/2466223002/diff/160001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:694:
ON_CALL(*characteristic, GetDescriptors())
Add these to GetBaseGATTCharacteristic so that all characteristics have these
methods.

https://codereview.chromium.org/2466223002/diff/160001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:1052:
AddDescriptorsToCharacteristic(measurement_interval.get());
I don't think this applies here. When you read/write to a descriptor in this
adapter the callback gets queued. Once you disconnect and then reconnect we
succeed/fail the request.

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

https://codereview.chromium.org/2466223002/diff/160001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:328:
//                 Note: This description must be blacklisted.
We should make it fail like the other blacklisted operations. Also
s/blacklist/blocklist/

https://codereview.chromium.org/2466223002/diff/160001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:843:
//    Note: This description must be blacklisted.
We should make it fail like the other blacklisted operations. Also
s/blacklist/blocklist/

https://codereview.chromium.org/2466223002/diff/160001/device/bluetooth/test/...
File device/bluetooth/test/mock_bluetooth_gatt_characteristic.h (right):

https://codereview.chromium.org/2466223002/diff/160001/device/bluetooth/test/...
device/bluetooth/test/mock_bluetooth_gatt_characteristic.h:64: void
AddMockDescriptor(
nit: Comment, similar to AddMockCharacteristic in service.

https://codereview.chromium.org/2466223002/diff/160001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/bluetooth/script-tests/descriptor-garbage-collection-ran-during-error.js
(right):

https://codereview.chromium.org/2466223002/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/script-tests/descriptor-garbage-collection-ran-during-error.js:10:

nit: Remove blank line.

https://codereview.chromium.org/2466223002/diff/160001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/bluetooth/script-tests/descriptor-garbage-collection-ran-during-success.js
(right):

https://codereview.chromium.org/2466223002/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/script-tests/descriptor-garbage-collection-ran-during-success.js:10:

nit: Remove blank line.

https://codereview.chromium.org/2466223002/diff/160001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/bluetooth/script-tests/descriptor-get-same-object.js
(right):

https://codereview.chromium.org/2466223002/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/script-tests/descriptor-get-same-object.js:9:

nit: Remove blank line.

https://codereview.chromium.org/2466223002/diff/160001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/bluetooth/script-tests/descriptor-invalid-name.js
(right):

https://codereview.chromium.org/2466223002/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/script-tests/descriptor-invalid-name.js:10:

nit: remote blank line.

https://codereview.chromium.org/2466223002/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/script-tests/descriptor-invalid-name.js:12:
promise = assert_promise_rejects_with_message(
You can just return the promise. No need to save it to garbage collect.

https://codereview.chromium.org/2466223002/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/script-tests/descriptor-invalid-name.js:30:
characteristic.service.device.gatt.disconnect();
No need to disconnect.

https://codereview.chromium.org/2466223002/diff/160001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/bluetooth/script-tests/gatt-op-device-disconnects-before.js
(right):

https://codereview.chromium.org/2466223002/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/script-tests/gatt-op-device-disconnects-before.js:19:
getDescriptor(user_description.name)|
We are going to have to put these in a separate test if we change the error
message.

https://codereview.chromium.org/2466223002/diff/160001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/bluetooth/script-tests/gatt-op-reconnect-during-error.js
(right):

https://codereview.chromium.org/2466223002/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/script-tests/gatt-op-reconnect-during-error.js:12:
readValue()|
nit: unrelated to the CL.

https://codereview.chromium.org/2466223002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/EventTargetModulesFactory.in (right):

https://codereview.chromium.org/2466223002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/EventTargetModulesFactory.in:7:
modules/bluetooth/BluetoothRemoteGATTDescriptor
I don't think we need this anymore?

https://codereview.chromium.org/2466223002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/bluetooth/BluetoothAttributeInstanceMap.h
(right):

https://codereview.chromium.org/2466223002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothAttributeInstanceMap.h:62:
BluetoothRemoteGATTDescriptor* getOrCreateBluetoothRemoteGATTDescriptor(
Nice!

https://codereview.chromium.org/2466223002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/bluetooth/BluetoothError.cpp (right):

https://codereview.chromium.org/2466223002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothError.cpp:34:
MAP_ERROR(DESCRIPTOR_NO_LONGER_EXISTS, InvalidStateError,
No longer relevant to this CL.

https://codereview.chromium.org/2466223002/diff/160001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp
(right):

https://codereview.chromium.org/2466223002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp:150:
DOMException::create(NetworkError, kGATTServerDisconnected));
I like that we can test all of the characteristic's methods in one script-test
but not returning a specific error message for descriptors like we do for
services and characteristics is really bothering me. I think we should introduce
a new error message. WDYT?

https://codereview.chromium.org/2466223002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp:201:
ScriptPromise BluetoothRemoteGATTCharacteristic::getDescriptorsImpl(
nit: Order of declaration should match order of implementation.

https://codereview.chromium.org/2466223002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp:206:
// TODO(ortuno): Remove when getDescriptor[s] is implemented.
nit: TODO(crbug.com/624017): Remove when descriptors are implemented on MacOS.

https://codereview.chromium.org/2466223002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTDescriptor.h
(right):

https://codereview.chromium.org/2466223002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTDescriptor.h:30: 
nit: Remove blank line.

https://codereview.chromium.org/2466223002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTDescriptor.h:37:
explicit BluetoothRemoteGATTDescriptor(
No need for explicit since the function doesn't take a single parameter.

https://codereview.chromium.org/2466223002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTDescriptor.h:41:
static BluetoothRemoteGATTDescriptor* create(
We don't do anything special in create so just use new I think.

https://codereview.chromium.org/2466223002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTDescriptor.h:45:
// Save value.
No need for this yet.

https://codereview.chromium.org/2466223002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTDescriptor.h:67:
Member<DOMDataView> m_value;
No need for value yet.

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

https://codereview.chromium.org/2466223002/diff/160001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/bluetooth/WebBluetooth.h:126: virtual
void descriptorReadValue(const WebString& descriptorInstanceID,
No need for these yet.

https://codereview.chromium.org/2466223002/diff/160001/third_party/WebKit/pub...
File
third_party/WebKit/public/platform/modules/bluetooth/WebBluetoothRemoteGATTDescriptorInit.h
(right):

https://codereview.chromium.org/2466223002/diff/160001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/bluetooth/WebBluetoothRemoteGATTDescriptorInit.h:22:
const WebVector<uint8_t> value;
I don't think we need value.

https://codereview.chromium.org/2466223002/diff/160001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom
(right):

https://codereview.chromium.org/2466223002/diff/160001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom:23:
DESCRIPTOR_NO_LONGER_EXISTS,
Not needed in this CL.

[dougt, 2016-12-02 18:03:38]

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

[commit-bot: I haz the power, 2016-12-02 18:04:37]

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

[dougt, 2016-12-02 18:31:29]

I added the following tests:

blocklist test
service-is-removed test
out-of-range test

Also, I converted characteristic-is-removed to be generated and added
getDescriptor[s].

Descriptors found is tested by:
   descriptor-get-same-object.js

correct-characteristics will need to wait for readValue.

A test to ensure that getDescriptors() doesn't return a blocklisted descriptor.

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

https://codereview.chromium.org/2466223002/diff/160001/content/browser/blueto...
content/browser/bluetooth/bluetooth_metrics.cc:234: case
CacheQueryOutcome::NO_DESCRIPTOR:
On 2016/12/02 06:14:49, ortuno wrote:
> nit: Similarly here. I don't think we need these changes in this patch.

Done.

https://codereview.chromium.org/2466223002/diff/160001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_metrics.h (right):

https://codereview.chromium.org/2466223002/diff/160001/content/browser/blueto...
content/browser/bluetooth/bluetooth_metrics.h:54: NO_DESCRIPTOR = 5,
On 2016/12/02 06:14:49, ortuno wrote:
> nit: Changes in this file are no longer relevant to this CL since we don't
> implement read/write anymore.

Done.

https://codereview.chromium.org/2466223002/diff/160001/content/browser/blueto...
File content/browser/bluetooth/cache_query_result.h (right):

https://codereview.chromium.org/2466223002/diff/160001/content/browser/blueto...
content/browser/bluetooth/cache_query_result.h:15: class
BluetoothRemoteGattDescriptor;
On 2016/12/02 06:14:50, ortuno wrote:
> nit: Similarly here. I don't think we need these changes in this patch.

Done.

https://codereview.chromium.org/2466223002/diff/160001/content/browser/blueto...
File content/browser/bluetooth/web_bluetooth_service_impl.h (right):

https://codereview.chromium.org/2466223002/diff/160001/content/browser/blueto...
content/browser/bluetooth/web_bluetooth_service_impl.h:136: void
RemoteDescriptorReadValue(
On 2016/12/02 06:14:50, ortuno wrote:
> No need for RemoteDescriptorReadValue/WriteValue since we never get here.

Done.

https://codereview.chromium.org/2466223002/diff/160001/content/browser/blueto...
content/browser/bluetooth/web_bluetooth_service_impl.h:205: // Callbacks for
BluetoothRemoteGattDescriptor::ReadRemoteDescriptor.
On 2016/12/02 06:14:50, ortuno wrote:
> Same here. No need for these callbacks.

Done.

https://codereview.chromium.org/2466223002/diff/160001/content/browser/blueto...
content/browser/bluetooth/web_bluetooth_service_impl.h:242: // Queries the
platform cache for a descriptor with
On 2016/12/02 06:14:50, ortuno wrote:
> No need for this one either.

Done.

https://codereview.chromium.org/2466223002/diff/160001/content/renderer/bluet...
File content/renderer/bluetooth/web_bluetooth_impl.h (right):

https://codereview.chromium.org/2466223002/diff/160001/content/renderer/bluet...
content/renderer/bluetooth/web_bluetooth_impl.h:92: void descriptorReadValue(
On 2016/12/02 06:14:50, ortuno wrote:
> No need for these since we don't implement it yet.

Done.

https://codereview.chromium.org/2466223002/diff/160001/content/renderer/bluet...
content/renderer/bluetooth/web_bluetooth_impl.h:125: const blink::WebString&
characteristic_instance_id,
On 2016/12/02 06:14:50, ortuno wrote:
> I think it was correct before i.e. service_instance_id.

Done.

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

https://codereview.chromium.org/2466223002/diff/160001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:95:
// Client Config is in our blacklist.  It must not be writable
On 2016/12/02 06:14:50, ortuno wrote:
> s/blacklist/blocklist/

Done.

https://codereview.chromium.org/2466223002/diff/160001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:667:
void LayoutTestBluetoothAdapterProvider::AddDescriptorsToCharacteristic(
On 2016/12/02 06:14:50, ortuno wrote:
> Given that this will only be used in one place (see note below), I think we
> should just inline it.

Acknowledged.

I was thinking that it might be needed in more places as we implemented
read|write.  I can add a inline if you think it matters.

https://codereview.chromium.org/2466223002/diff/160001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:681:
ON_CALL(*client_config, ReadRemoteDescriptor(_, _))
On 2016/12/02 06:14:50, ortuno wrote:
> No need for Write/Read yet.

Done.

https://codereview.chromium.org/2466223002/diff/160001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:691:
ON_CALL(*client_config, WriteRemoteDescriptor(_, _, _))
On 2016/12/02 06:14:50, ortuno wrote:
> Rather than succeeding this should DCHECK. See other blocklisted operations.

Acknowledged.  Removing as we're not doing write in this cl.

https://codereview.chromium.org/2466223002/diff/160001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:694:
ON_CALL(*characteristic, GetDescriptors())
On 2016/12/02 06:14:50, ortuno wrote:
> Add these to GetBaseGATTCharacteristic so that all characteristics have these
> methods.

Done.

https://codereview.chromium.org/2466223002/diff/160001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:1052:
AddDescriptorsToCharacteristic(measurement_interval.get());
On 2016/12/02 06:14:50, ortuno wrote:
> I don't think this applies here. When you read/write to a descriptor in this
> adapter the callback gets queued. Once you disconnect and then reconnect we
> succeed/fail the request.

Done.

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

https://codereview.chromium.org/2466223002/diff/160001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:328:
//                 Note: This description must be blacklisted.
On 2016/12/02 06:14:50, ortuno wrote:
> We should make it fail like the other blacklisted operations. Also
> s/blacklist/blocklist/

Acknowledged blocklist name change (following what was there).

https://codereview.chromium.org/2466223002/diff/160001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:843:
//    Note: This description must be blacklisted.
On 2016/12/02 06:14:50, ortuno wrote:
> We should make it fail like the other blacklisted operations. Also
> s/blacklist/blocklist/

Acknowledged.

https://codereview.chromium.org/2466223002/diff/160001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/bluetooth/script-tests/descriptor-garbage-collection-ran-during-error.js
(right):

https://codereview.chromium.org/2466223002/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/script-tests/descriptor-garbage-collection-ran-during-error.js:10:

On 2016/12/02 06:14:50, ortuno wrote:
> nit: Remove blank line.

Done.

https://codereview.chromium.org/2466223002/diff/160001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/bluetooth/script-tests/descriptor-garbage-collection-ran-during-success.js
(right):

https://codereview.chromium.org/2466223002/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/script-tests/descriptor-garbage-collection-ran-during-success.js:10:

On 2016/12/02 06:14:50, ortuno wrote:
> nit: Remove blank line.

Done.

https://codereview.chromium.org/2466223002/diff/160001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/bluetooth/script-tests/descriptor-get-same-object.js
(right):

https://codereview.chromium.org/2466223002/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/script-tests/descriptor-get-same-object.js:9:

On 2016/12/02 06:14:50, ortuno wrote:
> nit: Remove blank line.

Done.

https://codereview.chromium.org/2466223002/diff/160001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/bluetooth/script-tests/descriptor-invalid-name.js
(right):

https://codereview.chromium.org/2466223002/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/script-tests/descriptor-invalid-name.js:10:

On 2016/12/02 06:14:50, ortuno wrote:
> nit: remote blank line.

Done.

https://codereview.chromium.org/2466223002/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/script-tests/descriptor-invalid-name.js:12:
promise = assert_promise_rejects_with_message(
On 2016/12/02 06:14:50, ortuno wrote:
> You can just return the promise. No need to save it to garbage collect.

Done.

https://codereview.chromium.org/2466223002/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/script-tests/descriptor-invalid-name.js:30:
characteristic.service.device.gatt.disconnect();
On 2016/12/02 06:14:50, ortuno wrote:
> No need to disconnect.

Done.

https://codereview.chromium.org/2466223002/diff/160001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/bluetooth/script-tests/gatt-op-device-disconnects-before.js
(right):

https://codereview.chromium.org/2466223002/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/script-tests/gatt-op-device-disconnects-before.js:19:
getDescriptor(user_description.name)|
On 2016/12/02 06:14:50, ortuno wrote:
> We are going to have to put these in a separate test if we change the error
> message.

Acknowledged.

https://codereview.chromium.org/2466223002/diff/160001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/bluetooth/script-tests/gatt-op-reconnect-during-error.js
(right):

https://codereview.chromium.org/2466223002/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/script-tests/gatt-op-reconnect-during-error.js:12:
readValue()|
On 2016/12/02 06:14:50, ortuno wrote:
> nit: unrelated to the CL.

Acknowledged.

https://codereview.chromium.org/2466223002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/EventTargetModulesFactory.in (right):

https://codereview.chromium.org/2466223002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/EventTargetModulesFactory.in:7:
modules/bluetooth/BluetoothRemoteGATTDescriptor
On 2016/12/02 06:14:50, ortuno wrote:
> I don't think we need this anymore?

Done.

https://codereview.chromium.org/2466223002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/bluetooth/BluetoothError.cpp (right):

https://codereview.chromium.org/2466223002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothError.cpp:34:
MAP_ERROR(DESCRIPTOR_NO_LONGER_EXISTS, InvalidStateError,
On 2016/12/02 06:14:50, ortuno wrote:
> No longer relevant to this CL.

Done.

https://codereview.chromium.org/2466223002/diff/160001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp
(right):

https://codereview.chromium.org/2466223002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp:150:
DOMException::create(NetworkError, kGATTServerDisconnected));
On 2016/12/02 06:14:51, ortuno wrote:
> I like that we can test all of the characteristic's methods in one script-test
> but not returning a specific error message for descriptors like we do for
> services and characteristics is really bothering me. I think we should
introduce
> a new error message. WDYT?

If anything, a possible spec bug?  I am not really sure at this point what sorta
detail info you'd want as a webdev when something bad happens like a disconnect.

https://codereview.chromium.org/2466223002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp:201:
ScriptPromise BluetoothRemoteGATTCharacteristic::getDescriptorsImpl(
On 2016/12/02 06:14:51, ortuno wrote:
> nit: Order of declaration should match order of implementation.

Done.

https://codereview.chromium.org/2466223002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp:206:
// TODO(ortuno): Remove when getDescriptor[s] is implemented.
On 2016/12/02 06:14:51, ortuno wrote:
> nit: TODO(crbug.com/624017): Remove when descriptors are implemented on MacOS.

Done.

https://codereview.chromium.org/2466223002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTDescriptor.h
(right):

https://codereview.chromium.org/2466223002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTDescriptor.h:30: 
On 2016/12/02 06:14:51, ortuno wrote:
> nit: Remove blank line.

Done.

https://codereview.chromium.org/2466223002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTDescriptor.h:37:
explicit BluetoothRemoteGATTDescriptor(
On 2016/12/02 06:14:51, ortuno wrote:
> No need for explicit since the function doesn't take a single parameter.

Done.

https://codereview.chromium.org/2466223002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTDescriptor.h:41:
static BluetoothRemoteGATTDescriptor* create(
On 2016/12/02 06:14:51, ortuno wrote:
> We don't do anything special in create so just use new I think.


Acknowledged.

We test for a null input and this sort of factory static pattern is used by the
characteristic.  Keep it for symmetry.?

https://codereview.chromium.org/2466223002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTDescriptor.h:45:
// Save value.
On 2016/12/02 06:14:51, ortuno wrote:
> No need for this yet.

Done.

https://codereview.chromium.org/2466223002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTDescriptor.h:67:
Member<DOMDataView> m_value;
On 2016/12/02 06:14:51, ortuno wrote:
> No need for value yet.

Done.

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

https://codereview.chromium.org/2466223002/diff/160001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/bluetooth/WebBluetooth.h:126: virtual
void descriptorReadValue(const WebString& descriptorInstanceID,
On 2016/12/02 06:14:51, ortuno wrote:
> No need for these yet.

Done.

https://codereview.chromium.org/2466223002/diff/160001/third_party/WebKit/pub...
File
third_party/WebKit/public/platform/modules/bluetooth/WebBluetoothRemoteGATTDescriptorInit.h
(right):

https://codereview.chromium.org/2466223002/diff/160001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/bluetooth/WebBluetoothRemoteGATTDescriptorInit.h:22:
const WebVector<uint8_t> value;
On 2016/12/02 06:14:51, ortuno wrote:
> I don't think we need value.

Done.

https://codereview.chromium.org/2466223002/diff/160001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom
(right):

https://codereview.chromium.org/2466223002/diff/160001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom:23:
DESCRIPTOR_NO_LONGER_EXISTS,
On 2016/12/02 06:14:51, ortuno wrote:
> Not needed in this CL.

Done.

[commit-bot: I haz the power, 2016-12-02 18:45:50]

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

[commit-bot: I haz the power, 2016-12-02 18:45:51]

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

[dougt, 2016-12-03 19:35:32]

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

[commit-bot: I haz the power, 2016-12-03 19:35:46]

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

[commit-bot: I haz the power, 2016-12-03 20:12:49]

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

[commit-bot: I haz the power, 2016-12-03 20:12:51]

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

[dougt, 2016-12-03 21:06:07]

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

[commit-bot: I haz the power, 2016-12-03 21:06:20]

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

[commit-bot: I haz the power, 2016-12-03 21:10:30]

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

[commit-bot: I haz the power, 2016-12-03 21:10:31]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[dougt, 2016-12-04 01:33:32]

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

[commit-bot: I haz the power, 2016-12-04 01:33:38]

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

[commit-bot: I haz the power, 2016-12-04 03:13:46]

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

[commit-bot: I haz the power, 2016-12-04 03:13:48]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[dougt, 2016-12-04 05:49:19]

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

[commit-bot: I haz the power, 2016-12-04 05:49:27]

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

[commit-bot: I haz the power, 2016-12-04 07:31:19]

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

[commit-bot: I haz the power, 2016-12-04 07:31:20]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[dougt, 2016-12-05 01:05:04]

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

[commit-bot: I haz the power, 2016-12-05 01:05:22]

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

[commit-bot: I haz the power, 2016-12-05 02:59:20]

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

[commit-bot: I haz the power, 2016-12-05 02:59:22]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[dougt, 2016-12-05 19:29:47]

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

[commit-bot: I haz the power, 2016-12-05 19:30:35]

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

[commit-bot: I haz the power, 2016-12-05 21:54:27]

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

[commit-bot: I haz the power, 2016-12-05 21:54:29]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[dougt, 2016-12-06 06:29:55]

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

[commit-bot: I haz the power, 2016-12-06 06:30:15]

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

[dougt, 2016-12-06 06:35:27]

Patchset #10 (id:180001) has been deleted

[dougt, 2016-12-06 06:36:05]

Patchset #10 (id:200001) has been deleted

[dougt, 2016-12-06 06:36:44]

Patchset #12 (id:260001) has been deleted

[dougt, 2016-12-06 06:37:19]

Patchset #10 (id:220001) has been deleted

[dougt, 2016-12-06 06:37:48]

Patchset #10 (id:240001) has been deleted

[dougt, 2016-12-06 06:38:16]

Patchset #6 (id:100001) has been deleted

[dougt, 2016-12-06 06:38:44]

Patchset #2 (id:20001) has been deleted

[dougt, 2016-12-06 06:39:12]

Patchset #2 (id:40001) has been deleted

[dougt, 2016-12-06 06:39:38]

Patchset #4 (id:120001) has been deleted

[dougt, 2016-12-06 06:40:12]

Patchset #4 (id:140001) has been deleted

[dougt, 2016-12-06 06:41:41]

Patchset #5 (id:280001) has been deleted

[dougt, 2016-12-06 07:08:49]

Patchset #5 (id:300001) has been deleted

[dougt, 2016-12-06 07:09:31]

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

[commit-bot: I haz the power, 2016-12-06 07:09:44]

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

[commit-bot: I haz the power, 2016-12-06 08:47:05]

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

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

Dry run: This issue passed the CQ dry run.

[dougt, 2016-12-06 15:24:44]

ortuno, ptal.

[ortuno, 2016-12-07 08:05:13]

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

https://codereview.chromium.org/2466223002/diff/160001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:667:
void LayoutTestBluetoothAdapterProvider::AddDescriptorsToCharacteristic(
On 2016/12/02 at 18:31:28, dougt wrote:
> On 2016/12/02 06:14:50, ortuno wrote:
> > Given that this will only be used in one place (see note below), I think we
> > should just inline it.
> 
> Acknowledged.
> 
> I was thinking that it might be needed in more places as we implemented
read|write.  I can add a inline if you think it matters.

hmm I can't see us using this but maybe I'm missing something. So there are
three adapters that will need descriptors:

1. GetDisconnectingHealthThermometer
2. GetGATTOperationFinishesAfterReconnectionAdapter
3. GetFailingGATTOperationsAdapter

The descriptors in the those adapters are pretty different. The first one needs
write/read to succeed and the second needs to queue the requests and run them
after reconnection and the third one needs read/writes to fail.

https://codereview.chromium.org/2466223002/diff/160001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp
(right):

https://codereview.chromium.org/2466223002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp:150:
DOMException::create(NetworkError, kGATTServerDisconnected));
On 2016/12/02 at 18:31:29, dougt wrote:
> On 2016/12/02 06:14:51, ortuno wrote:
> > I like that we can test all of the characteristic's methods in one
script-test
> > but not returning a specific error message for descriptors like we do for
> > services and characteristics is really bothering me. I think we should
introduce
> > a new error message. WDYT?
> 
> If anything, a possible spec bug?  I am not really sure at this point what
sorta detail info you'd want as a webdev when something bad happens like a
disconnect.

hmm then we should change the message since this is not really a GATT operation.

https://codereview.chromium.org/2466223002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTDescriptor.h
(right):

https://codereview.chromium.org/2466223002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTDescriptor.h:41:
static BluetoothRemoteGATTDescriptor* create(
On 2016/12/02 at 18:31:29, dougt wrote:
> On 2016/12/02 06:14:51, ortuno wrote:
> > We don't do anything special in create so just use new I think.
> 
> 
> Acknowledged.
> 
> We test for a null input and this sort of factory static pattern is used by
the characteristic.  Keep it for symmetry.?

Characteristic needs it because it's an ActiveDOMObject that needs to some stuff
after it's constructed. Service does not have a create method.

https://codereview.chromium.org/2466223002/diff/320001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_blocklist.cc (right):

https://codereview.chromium.org/2466223002/diff/320001/content/browser/blueto...
content/browser/bluetooth/bluetooth_blocklist.cc:180: Add(BluetoothUUID("bad0"),
Value::EXCLUDE);
hmm why do we need this? We already have a Value::EXCLUDE below.

https://codereview.chromium.org/2466223002/diff/320001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_metrics.cc (right):

https://codereview.chromium.org/2466223002/diff/320001/content/browser/blueto...
content/browser/bluetooth/bluetooth_metrics.cc:278: case
UMAGATTOperation::DESCRIPTOR_READ:
No need for these changes.

https://codereview.chromium.org/2466223002/diff/320001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_metrics.h (right):

https://codereview.chromium.org/2466223002/diff/320001/content/browser/blueto...
content/browser/bluetooth/bluetooth_metrics.h:239: DESCRIPTOR_READ,
No need for these changes either.

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

https://codereview.chromium.org/2466223002/diff/320001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:687:
// add it here with full permission as the blocklist should prevent us from
nit: s/add/Add/ Also this should DCHECK to match other operations on blacklisted
properties and also to fail the test.

https://codereview.chromium.org/2466223002/diff/320001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:1630:
// getDescriptors response
nit: I don't think the comment is necessary.

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

https://codereview.chromium.org/2466223002/diff/320001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:336:
//             - user_description
nit: User Description (uuid here)

https://codereview.chromium.org/2466223002/diff/320001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:337:
//                 Read calls success callback
nit: Read: Calls success callback.

https://codereview.chromium.org/2466223002/diff/320001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:338:
//             - client_characteristic_configuration
nit: Client Characteristic Configuration (uuid here)

https://codereview.chromium.org/2466223002/diff/320001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:339:
//                 Write calls success callback
nit: It should DCHECK since it's EXCLUDED WRITES.

https://codereview.chromium.org/2466223002/diff/320001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:341:
//             - bad0
nit: Use 128bit UUIDs.

https://codereview.chromium.org/2466223002/diff/320001/device/bluetooth/test/...
File device/bluetooth/test/mock_bluetooth_gatt_characteristic.h (right):

https://codereview.chromium.org/2466223002/diff/320001/device/bluetooth/test/...
device/bluetooth/test/mock_bluetooth_gatt_characteristic.h:64: void
AddMockDescriptor(
Same comment as characteristic please :p

https://codereview.chromium.org/2466223002/diff/320001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/bluetooth/script-tests/characteristic/characteristic-is-removed.js
(right):

https://codereview.chromium.org/2466223002/diff/320001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/script-tests/characteristic/characteristic-is-removed.js:15:
getDescriptor(user_description.name)|
Also getDescriptors(user_description.name)[UUID]

https://codereview.chromium.org/2466223002/diff/320001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/bluetooth/script-tests/characteristic/descriptor-blocklisted.js
(right):

https://codereview.chromium.org/2466223002/diff/320001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/script-tests/characteristic/descriptor-blocklisted.js:14:
getDescriptor(0xbad0)|
Also add test to make sure that blacklisted descriptors are not returned if you
call getDescriptors with no UUID.

https://codereview.chromium.org/2466223002/diff/320001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/bluetooth/script-tests/characteristic/descriptor-garbage-collection-ran-during-error.js
(right):

https://codereview.chromium.org/2466223002/diff/320001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/script-tests/characteristic/descriptor-garbage-collection-ran-during-error.js:12:
/* 0x0101 doesn't exist in this characteristic */
hmm you moved this to another file but you kept the same error message. Either
change the error message to be descriptor specific or move this to
gatt-op-garbage-collection-ran-during-error.js

https://codereview.chromium.org/2466223002/diff/320001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/bluetooth/script-tests/characteristic/descriptor-garbage-collection-ran-during-success.js
(right):

https://codereview.chromium.org/2466223002/diff/320001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/script-tests/characteristic/descriptor-garbage-collection-ran-during-success.js:12:
characteristic.CALLS([
Same here, either change the error message or move back to similar test.

https://codereview.chromium.org/2466223002/diff/320001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/bluetooth/script-tests/characteristic/descriptor-get-same-object.js
(right):

https://codereview.chromium.org/2466223002/diff/320001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/script-tests/characteristic/descriptor-get-same-object.js:11:
getDescriptor(user_description.name)|
We want to test that all argument variations i.e. number, name, full uuid
string, are able to find the descriptor. Similar to
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/bluetooth...

https://codereview.chromium.org/2466223002/diff/320001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/script-tests/characteristic/descriptor-get-same-object.js:28:
descriptors.forEach(
Also we are not really checking what happens when a descriptor is not found.

https://codereview.chromium.org/2466223002/diff/320001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/bluetooth/script-tests/characteristic/descriptor-invalid-name.js
(right):

https://codereview.chromium.org/2466223002/diff/320001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/script-tests/characteristic/descriptor-invalid-name.js:29:
}, 'Garbage Collection ran during a FUNCTION_NAME call that succeeds. ' +
Fix test description.

https://codereview.chromium.org/2466223002/diff/320001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/bluetooth/script-tests/characteristic/descriptor-out-of-range.js
(right):

https://codereview.chromium.org/2466223002/diff/320001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/script-tests/characteristic/descriptor-out-of-range.js:2:
promise_test(() => {
I think this should be called 'devices-goes-out-of-range.js' with a TODO to use
this for the rest characteristic's functions.

https://codereview.chromium.org/2466223002/diff/320001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/script-tests/characteristic/descriptor-out-of-range.js:6:
optionalServices: ['generic_access']}))
no need for generic access.

https://codereview.chromium.org/2466223002/diff/320001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/script-tests/characteristic/descriptor-out-of-range.js:11:
{
nit: move to line above

https://codereview.chromium.org/2466223002/diff/320001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/bluetooth/script-tests/characteristic/descriptor-service-is-removed.js
(right):

https://codereview.chromium.org/2466223002/diff/320001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/script-tests/characteristic/descriptor-service-is-removed.js:2:
promise_test(() => {
Same here. 'service-is-removed.js' with a todo to add the rest of the functions.

https://codereview.chromium.org/2466223002/diff/320001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/script-tests/characteristic/descriptor-service-is-removed.js:6:
optionalServices: ['generic_access']}))
no need for generic access.

https://codereview.chromium.org/2466223002/diff/320001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/script-tests/characteristic/descriptor-service-is-removed.js:11:
return setBluetoothFakeAdapter('MissingServiceHeartRateAdapter')
I'm surprised this work. This should really be
MissingServiceHealthThermometerAdapter.

https://codereview.chromium.org/2466223002/diff/320001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/bluetooth/script-tests/characteristic/gatt-op-device-disconnects-before.js
(right):

https://codereview.chromium.org/2466223002/diff/320001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/script-tests/characteristic/gatt-op-device-disconnects-before.js:19:
getDescriptor(user_description.name)|
[UUID] variant

https://codereview.chromium.org/2466223002/diff/320001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/bluetooth/script-tests/characteristic/gatt-op-device-disconnects-during-error.js
(right):

https://codereview.chromium.org/2466223002/diff/320001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/script-tests/characteristic/gatt-op-device-disconnects-during-error.js:21:
getDescriptor(user_description.name)|
[UUID] variant

https://codereview.chromium.org/2466223002/diff/320001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/bluetooth/script-tests/characteristic/gatt-op-device-disconnects-during-success.js
(right):

https://codereview.chromium.org/2466223002/diff/320001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/script-tests/characteristic/gatt-op-device-disconnects-during-success.js:21:
getDescriptor(user_description.name)|
[uuid] variant

https://codereview.chromium.org/2466223002/diff/320001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/bluetooth/script-tests/characteristic/gatt-op-disconnect-called-before.js
(right):

https://codereview.chromium.org/2466223002/diff/320001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/script-tests/characteristic/gatt-op-disconnect-called-before.js:15:
getDescriptor(user_description.name)|
[uuid] variant

https://codereview.chromium.org/2466223002/diff/320001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/bluetooth/script-tests/characteristic/gatt-op-disconnect-called-during-error.js
(right):

https://codereview.chromium.org/2466223002/diff/320001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/script-tests/characteristic/gatt-op-disconnect-called-during-error.js:14:
getDescriptor(user_description.name)|
[UUID] variant

https://codereview.chromium.org/2466223002/diff/320001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/bluetooth/script-tests/characteristic/gatt-op-disconnect-called-during-success.js
(right):

https://codereview.chromium.org/2466223002/diff/320001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/script-tests/characteristic/gatt-op-disconnect-called-during-success.js:15:
getDescriptor(user_description.name)|
[UUID] variant

https://codereview.chromium.org/2466223002/diff/320001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/bluetooth/script-tests/characteristic/gatt-op-garbage-collection-ran-during-error.js
(right):

https://codereview.chromium.org/2466223002/diff/320001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/script-tests/characteristic/gatt-op-garbage-collection-ran-during-error.js:14:
getDescriptor(user_description.name)|
You already have this test: descriptor-garbage-collection-ran-during-error.js.

gatt-op really means read/write/notify so getDescriptor is a bit out of place.
We should either change the name of the tests to encompass getDescriptor(s) or
move getDescriptor(s) to another test.

https://codereview.chromium.org/2466223002/diff/320001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTDescriptor.idl
(right):

https://codereview.chromium.org/2466223002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTDescriptor.idl:10:
DependentLifetime,
No need for this:
https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/So...

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

https://codereview.chromium.org/2466223002/diff/320001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/bluetooth/WebBluetooth.h:117: const
WebString& serviceInstanceID,
characteristicInstanceID

https://codereview.chromium.org/2466223002/diff/320001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom
(right):

https://codereview.chromium.org/2466223002/diff/320001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom:191: //
Returns the Descriptors of a GATT Characteristics with |service_instance_id|.
s/Characteristics/Characteristic/

[dougt, 2016-12-07 18:56:57]

> The descriptors in the those adapters are pretty different. The first one
needs
> write/read to succeed and the second needs to queue the requests and run them
> after reconnection and the third one needs read/writes to fail.

Agreed.  I removed this method.

> 
> hmm then we should change the message since this is not really a GATT
operation.

I agree we should use a different error message. I can do this by changing the
tests to not validate the text of the error message fairly easily.  If we want
to also make sure that the error text matches, that's a bigger change and we
probably should do it in a different CL.  WDYT?

> content/browser/bluetooth/bluetooth_blocklist.cc:180:
Add(BluetoothUUID("bad0"),
> Value::EXCLUDE);
> hmm why do we need this? We already have a Value::EXCLUDE below.

We don't need it any more. I am reusing an existing blocklisted value.

[dougt, 2016-12-07 19:48:58]

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

https://codereview.chromium.org/2466223002/diff/320001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:687:
// add it here with full permission as the blocklist should prevent us from
On 2016/12/07 08:05:12, ortuno wrote:
> nit: s/add/Add/ Also this should DCHECK to match other operations on
blacklisted
> properties and also to fail the test.

You mean reading and writing to it, correct?  We have tests to ensure that the
blocklisted descriptors are prevented from being returned from getDescriptor[s]

https://codereview.chromium.org/2466223002/diff/320001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:1630:
// getDescriptors response
On 2016/12/07 08:05:12, ortuno wrote:
> nit: I don't think the comment is necessary.

I agree.  I will also remove the three other comments above the ON_CALL lines.

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

https://codereview.chromium.org/2466223002/diff/320001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:336:
//             - user_description
On 2016/12/07 08:05:12, ortuno wrote:
> nit: User Description (uuid here)

Done.

https://codereview.chromium.org/2466223002/diff/320001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:337:
//                 Read calls success callback
On 2016/12/07 08:05:12, ortuno wrote:
> nit: Read: Calls success callback.

Removed.  We aren't doing read or write in this cl.

https://codereview.chromium.org/2466223002/diff/320001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:338:
//             - client_characteristic_configuration
On 2016/12/07 08:05:12, ortuno wrote:
> nit: Client Characteristic Configuration (uuid here)

Done.

https://codereview.chromium.org/2466223002/diff/320001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:339:
//                 Write calls success callback
On 2016/12/07 08:05:12, ortuno wrote:
> nit: It should DCHECK since it's EXCLUDED WRITES.


Turns out we don't do this anymore in this patch as we punted on read/write from
this Cl.  I'll update the comment.

https://codereview.chromium.org/2466223002/diff/320001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:341:
//             - bad0
On 2016/12/07 08:05:12, ortuno wrote:
> nit: Use 128bit UUIDs.

Done.

https://codereview.chromium.org/2466223002/diff/320001/device/bluetooth/test/...
File device/bluetooth/test/mock_bluetooth_gatt_characteristic.h (right):

https://codereview.chromium.org/2466223002/diff/320001/device/bluetooth/test/...
device/bluetooth/test/mock_bluetooth_gatt_characteristic.h:64: void
AddMockDescriptor(
On 2016/12/07 08:05:12, ortuno wrote:
> Same comment as characteristic please :p

Acknowledged.

This method only pushes a descriptor into a list.  I am not sure what kind of
comment you want, really.

https://codereview.chromium.org/2466223002/diff/320001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/bluetooth/script-tests/characteristic/characteristic-is-removed.js
(right):

https://codereview.chromium.org/2466223002/diff/320001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/script-tests/characteristic/characteristic-is-removed.js:15:
getDescriptor(user_description.name)|
On 2016/12/07 08:05:12, ortuno wrote:
> Also getDescriptors(user_description.name)[UUID]

Done.

https://codereview.chromium.org/2466223002/diff/320001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/bluetooth/script-tests/characteristic/descriptor-blocklisted.js
(right):

https://codereview.chromium.org/2466223002/diff/320001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/script-tests/characteristic/descriptor-blocklisted.js:14:
getDescriptor(0xbad0)|
On 2016/12/07 08:05:12, ortuno wrote:
> Also add test to make sure that blacklisted descriptors are not returned if
you
> call getDescriptors with no UUID.

See blocklisted-descriptors-not-present.html

https://codereview.chromium.org/2466223002/diff/320001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/bluetooth/script-tests/characteristic/descriptor-garbage-collection-ran-during-error.js
(right):

https://codereview.chromium.org/2466223002/diff/320001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/script-tests/characteristic/descriptor-garbage-collection-ran-during-error.js:12:
/* 0x0101 doesn't exist in this characteristic */
On 2016/12/07 08:05:12, ortuno wrote:
> hmm you moved this to another file but you kept the same error message. Either
> change the error message to be descriptor specific or move this to
> gatt-op-garbage-collection-ran-during-error.js

Done.

https://codereview.chromium.org/2466223002/diff/320001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/bluetooth/script-tests/characteristic/descriptor-garbage-collection-ran-during-success.js
(right):

https://codereview.chromium.org/2466223002/diff/320001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/script-tests/characteristic/descriptor-garbage-collection-ran-during-success.js:12:
characteristic.CALLS([
On 2016/12/07 08:05:12, ortuno wrote:
> Same here, either change the error message or move back to similar test.

Done.

https://codereview.chromium.org/2466223002/diff/320001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/bluetooth/script-tests/characteristic/descriptor-get-same-object.js
(right):

https://codereview.chromium.org/2466223002/diff/320001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/script-tests/characteristic/descriptor-get-same-object.js:11:
getDescriptor(user_description.name)|
On 2016/12/07 08:05:12, ortuno wrote:
> We want to test that all argument variations i.e. number, name, full uuid
> string, are able to find the descriptor. Similar to
>
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/bluetooth...

Done.

https://codereview.chromium.org/2466223002/diff/320001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/script-tests/characteristic/descriptor-get-same-object.js:28:
descriptors.forEach(
On 2016/12/07 08:05:12, ortuno wrote:
> Also we are not really checking what happens when a descriptor is not found.

We are checking that all calls return identical results.  If they all return
empty sets this test will pass.  I will add a test to ensure that
descriptors_arrays is not empty.

https://codereview.chromium.org/2466223002/diff/320001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/bluetooth/script-tests/characteristic/descriptor-invalid-name.js
(right):

https://codereview.chromium.org/2466223002/diff/320001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/script-tests/characteristic/descriptor-invalid-name.js:29:
}, 'Garbage Collection ran during a FUNCTION_NAME call that succeeds. ' +
On 2016/12/07 08:05:12, ortuno wrote:
> Fix test description.

Done.

https://codereview.chromium.org/2466223002/diff/320001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/bluetooth/script-tests/characteristic/descriptor-out-of-range.js
(right):

https://codereview.chromium.org/2466223002/diff/320001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/script-tests/characteristic/descriptor-out-of-range.js:6:
optionalServices: ['generic_access']}))
On 2016/12/07 08:05:13, ortuno wrote:
> no need for generic access.

Done.

https://codereview.chromium.org/2466223002/diff/320001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/script-tests/characteristic/descriptor-out-of-range.js:11:
{
On 2016/12/07 08:05:13, ortuno wrote:
> nit: move to line above

Done.

https://codereview.chromium.org/2466223002/diff/320001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/bluetooth/script-tests/characteristic/descriptor-service-is-removed.js
(right):

https://codereview.chromium.org/2466223002/diff/320001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/script-tests/characteristic/descriptor-service-is-removed.js:2:
promise_test(() => {
On 2016/12/07 08:05:13, ortuno wrote:
> Same here. 'service-is-removed.js' with a todo to add the rest of the
functions.

Done.

https://codereview.chromium.org/2466223002/diff/320001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/script-tests/characteristic/descriptor-service-is-removed.js:6:
optionalServices: ['generic_access']}))
On 2016/12/07 08:05:13, ortuno wrote:
> no need for generic access.

Done.

https://codereview.chromium.org/2466223002/diff/320001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/script-tests/characteristic/descriptor-service-is-removed.js:11:
return setBluetoothFakeAdapter('MissingServiceHeartRateAdapter')
On 2016/12/07 08:05:13, ortuno wrote:
> I'm surprised this work. This should really be
> MissingServiceHealthThermometerAdapter.

Done.

https://codereview.chromium.org/2466223002/diff/320001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/bluetooth/script-tests/characteristic/gatt-op-device-disconnects-before.js
(right):

https://codereview.chromium.org/2466223002/diff/320001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/script-tests/characteristic/gatt-op-device-disconnects-before.js:19:
getDescriptor(user_description.name)|
On 2016/12/07 08:05:13, ortuno wrote:
> [UUID] variant

Done.

https://codereview.chromium.org/2466223002/diff/320001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/bluetooth/script-tests/characteristic/gatt-op-device-disconnects-during-error.js
(right):

https://codereview.chromium.org/2466223002/diff/320001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/script-tests/characteristic/gatt-op-device-disconnects-during-error.js:21:
getDescriptor(user_description.name)|
On 2016/12/07 08:05:13, ortuno wrote:
> [UUID] variant

Done.

https://codereview.chromium.org/2466223002/diff/320001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/bluetooth/script-tests/characteristic/gatt-op-device-disconnects-during-success.js
(right):

https://codereview.chromium.org/2466223002/diff/320001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/script-tests/characteristic/gatt-op-device-disconnects-during-success.js:21:
getDescriptor(user_description.name)|
On 2016/12/07 08:05:13, ortuno wrote:
> [uuid] variant

Done.

https://codereview.chromium.org/2466223002/diff/320001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/bluetooth/script-tests/characteristic/gatt-op-disconnect-called-before.js
(right):

https://codereview.chromium.org/2466223002/diff/320001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/script-tests/characteristic/gatt-op-disconnect-called-before.js:15:
getDescriptor(user_description.name)|
On 2016/12/07 08:05:13, ortuno wrote:
> [uuid] variant

Done.

https://codereview.chromium.org/2466223002/diff/320001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/bluetooth/script-tests/characteristic/gatt-op-disconnect-called-during-error.js
(right):

https://codereview.chromium.org/2466223002/diff/320001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/script-tests/characteristic/gatt-op-disconnect-called-during-error.js:14:
getDescriptor(user_description.name)|
On 2016/12/07 08:05:13, ortuno wrote:
> [UUID] variant

Done.

https://codereview.chromium.org/2466223002/diff/320001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/bluetooth/script-tests/characteristic/gatt-op-disconnect-called-during-success.js
(right):

https://codereview.chromium.org/2466223002/diff/320001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/script-tests/characteristic/gatt-op-disconnect-called-during-success.js:15:
getDescriptor(user_description.name)|
On 2016/12/07 08:05:13, ortuno wrote:
> [UUID] variant

Done.

https://codereview.chromium.org/2466223002/diff/320001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/bluetooth/script-tests/characteristic/gatt-op-garbage-collection-ran-during-error.js
(right):

https://codereview.chromium.org/2466223002/diff/320001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/script-tests/characteristic/gatt-op-garbage-collection-ran-during-error.js:14:
getDescriptor(user_description.name)|
On 2016/12/07 08:05:13, ortuno wrote:
> You already have this test: descriptor-garbage-collection-ran-during-error.js.
> 
> gatt-op really means read/write/notify so getDescriptor is a bit out of place.
> We should either change the name of the tests to encompass getDescriptor(s) or
> move getDescriptor(s) to another test.

removed.  Same for gatt-op-garbage-collection-ran-during-success.js.

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

https://codereview.chromium.org/2466223002/diff/320001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/bluetooth/WebBluetooth.h:117: const
WebString& serviceInstanceID,
On 2016/12/07 08:05:13, ortuno wrote:
> characteristicInstanceID

Done.

https://codereview.chromium.org/2466223002/diff/320001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom
(right):

https://codereview.chromium.org/2466223002/diff/320001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom:191: //
Returns the Descriptors of a GATT Characteristics with |service_instance_id|.
On 2016/12/07 08:05:13, ortuno wrote:
> s/Characteristics/Characteristic/

Done.

[dougt, 2016-12-07 19:50:34]

https://codereview.chromium.org/2466223002/diff/320001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_blocklist.cc (right):

https://codereview.chromium.org/2466223002/diff/320001/content/browser/blueto...
content/browser/bluetooth/bluetooth_blocklist.cc:180: Add(BluetoothUUID("bad0"),
Value::EXCLUDE);
On 2016/12/07 08:05:12, ortuno wrote:
> hmm why do we need this? We already have a Value::EXCLUDE below.

Done. Removed.

https://codereview.chromium.org/2466223002/diff/320001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_metrics.cc (right):

https://codereview.chromium.org/2466223002/diff/320001/content/browser/blueto...
content/browser/bluetooth/bluetooth_metrics.cc:278: case
UMAGATTOperation::DESCRIPTOR_READ:
On 2016/12/07 08:05:12, ortuno wrote:
> No need for these changes.

Done.

https://codereview.chromium.org/2466223002/diff/320001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_metrics.h (right):

https://codereview.chromium.org/2466223002/diff/320001/content/browser/blueto...
content/browser/bluetooth/bluetooth_metrics.h:239: DESCRIPTOR_READ,
On 2016/12/07 08:05:12, ortuno wrote:
> No need for these changes either.

Done.

https://codereview.chromium.org/2466223002/diff/320001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTDescriptor.idl
(right):

https://codereview.chromium.org/2466223002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTDescriptor.idl:10:
DependentLifetime,
On 2016/12/07 08:05:13, ortuno wrote:
> No need for this:
>
https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/So...

Done.

[dougt, 2016-12-07 21:35:09]

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

[commit-bot: I haz the power, 2016-12-07 21:35:41]

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

[commit-bot: I haz the power, 2016-12-07 21:39:41]

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

[commit-bot: I haz the power, 2016-12-07 21:39:42]

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

[dougt, 2016-12-07 22:21:44]

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

https://codereview.chromium.org/2466223002/diff/320001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:687:
// add it here with full permission as the blocklist should prevent us from
On 2016/12/07 19:48:57, dougt wrote:
> On 2016/12/07 08:05:12, ortuno wrote:
> > nit: s/add/Add/ Also this should DCHECK to match other operations on
> blacklisted
> > properties and also to fail the test.
> 
> You mean reading and writing to it, correct?  We have tests to ensure that the
> blocklisted descriptors are prevented from being returned from
getDescriptor[s]

In any case, in this CL, we don't read or write yet.

[dougt, 2016-12-07 22:25:50]

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

[commit-bot: I haz the power, 2016-12-07 22:26:26]

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

[commit-bot: I haz the power, 2016-12-07 22:44:31]

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

[commit-bot: I haz the power, 2016-12-07 22:44:32]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[dougt, 2016-12-07 22:48:42]

Patchset #7 (id:360001) has been deleted

[dougt, 2016-12-07 23:05:18]

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

[commit-bot: I haz the power, 2016-12-07 23:05:36]

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

[commit-bot: I haz the power, 2016-12-08 00:39:04]

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

[commit-bot: I haz the power, 2016-12-08 00:39:07]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[loftislydia, 2016-12-08 04:45:44]

Here is the details of methods to implement WebBluetooth getdescriptor. Anyway
it was a useful thread. http://essayssos.com/

[ortuno, 2016-12-08 05:42:52]

Almost there!

https://codereview.chromium.org/2466223002/diff/160001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp
(right):

https://codereview.chromium.org/2466223002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp:150:
DOMException::create(NetworkError, kGATTServerDisconnected));
On 2016/12/07 at 08:05:12, ortuno wrote:
> On 2016/12/02 at 18:31:29, dougt wrote:
> > On 2016/12/02 06:14:51, ortuno wrote:
> > > I like that we can test all of the characteristic's methods in one
script-test
> > > but not returning a specific error message for descriptors like we do for
> > > services and characteristics is really bothering me. I think we should
introduce
> > > a new error message. WDYT?
> > 
> > If anything, a possible spec bug?  I am not really sure at this point what
sorta detail info you'd want as a webdev when something bad happens like a
disconnect.
> 
> hmm then we should change the message since this is not really a GATT
operation.

As discussed offline, I think it's fine to use the same message for now. We can
fix in a follow up CL. This patch is big enough as is :p

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

https://codereview.chromium.org/2466223002/diff/320001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:687:
// add it here with full permission as the blocklist should prevent us from
On 2016/12/07 at 22:21:43, dougt wrote:
> On 2016/12/07 19:48:57, dougt wrote:
> > On 2016/12/07 08:05:12, ortuno wrote:
> > > nit: s/add/Add/ Also this should DCHECK to match other operations on
> > blacklisted
> > > properties and also to fail the test.
> > 
> > You mean reading and writing to it, correct?  We have tests to ensure that
the
> > blocklisted descriptors are prevented from being returned from
getDescriptor[s]
> 
> In any case, in this CL, we don't read or write yet.

For future reference. The intent is to make sure we never ever run read/write on
fully excluded descriptors. Having DCHECK when someone calls them ensure that if
there is a bug in our tests we still catch this. But as you said, since we are
not going read/write in this patch OK to leave out for now.

https://codereview.chromium.org/2466223002/diff/320001/device/bluetooth/test/...
File device/bluetooth/test/mock_bluetooth_gatt_characteristic.h (right):

https://codereview.chromium.org/2466223002/diff/320001/device/bluetooth/test/...
device/bluetooth/test/mock_bluetooth_gatt_characteristic.h:64: void
AddMockDescriptor(
I think we want a comment about how this is supposed to be used something like:

  // BluetoothRemoteGattCharacteristic manages the lifetime of its
  // BluetoothGATTDescriptors.
  // This method takes ownership of the MockBluetoothGATTDescriptors. This
  // is only for convenience as far as testing is concerned, and it's possible
  // to write test cases without using these functions.
  // Example:
  // ON_CALL(*mock_characteristic, GetDescriptors))
  //   .WillByDefault(Invoke(
  //     *mock_characteristic,
  //      &MockBluetoothGattCharacteristic::GetMockDescriptors));

https://codereview.chromium.org/2466223002/diff/320001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/bluetooth/script-tests/characteristic/descriptor-get-same-object.js
(right):

https://codereview.chromium.org/2466223002/diff/320001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/script-tests/characteristic/descriptor-get-same-object.js:28:
descriptors.forEach(
On 2016/12/07 19:48:57, dougt wrote:
> On 2016/12/07 08:05:12, ortuno wrote:
> > Also we are not really checking what happens when a descriptor is not found.
> 
> We are checking that all calls return identical results.  If they all return
> empty sets this test will pass.  I will add a test to ensure that
> descriptors_arrays is not empty.

Good thing to add to the test but I was thinking something more like the
following test:
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/bluetooth...

We need to be sure that the error message: 'No Descriptors with specified UUID
found in Characteristic' is actually being returned if the descriptor is not in
the device.

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

https://codereview.chromium.org/2466223002/diff/380001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:734:
device::BluetoothGattCharacteristic::Permission::PERMISSION_WRITE));
I think this is Read and write:

https://www.bluetooth.com/specifications/gatt/viewer?attributeXmlFile=org.blu...

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

https://codereview.chromium.org/2466223002/diff/380001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:338:
//                 Note: This descriptor is blocklisted.
Only for writes, right?

https://codereview.chromium.org/2466223002/diff/380001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:340:
//                 A test descriptor that does is blocklisted.
Remove 'does'.

https://codereview.chromium.org/2466223002/diff/380001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/TestExpectations (right):

https://codereview.chromium.org/2466223002/diff/380001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/TestExpectations:1431: # TODO(jlebel): Remove
when methods are implemented.
nit: no need for duplicate line.

https://codereview.chromium.org/2466223002/diff/380001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/TestExpectations:1451: crbug.com/624019 [ Mac ]
bluetooth/service/getCharacteristics/gen-disconnect-invalidates-objects-with-uuid.html
[ Skip ]
fyi: We skip these because they have stopNotifications() in them.

https://codereview.chromium.org/2466223002/diff/380001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/bluetooth/characteristic/getDescriptors/blocklisted-descriptors-not-present.html
(right):

https://codereview.chromium.org/2466223002/diff/380001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/characteristic/getDescriptors/blocklisted-descriptors-not-present.html:20:
descriptor => {
nit: Move to previous line.

https://codereview.chromium.org/2466223002/diff/380001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/characteristic/getDescriptors/blocklisted-descriptors-not-present.html:21:
let bad_uuid = 'bad2ddcf-60db-45cd-bef9-fd72b153cf7c';
nit: Move this to resources.
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/resources...

https://codereview.chromium.org/2466223002/diff/380001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/bluetooth/characteristic/getDescriptors/gen-descriptor-invalid-name.html
(right):

https://codereview.chromium.org/2466223002/diff/380001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/characteristic/getDescriptors/gen-descriptor-invalid-name.html:34:
'Should not crash.');
I mean it's technically correct :P but also not specific to this test. Just
remove.

https://codereview.chromium.org/2466223002/diff/380001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/bluetooth/script-tests/characteristic/descriptor-blocklisted.js
(right):

https://codereview.chromium.org/2466223002/diff/380001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/script-tests/characteristic/descriptor-blocklisted.js:16:
getDescriptors('bad2ddcf-60db-45cd-bef9-fd72b153cf7c')]),
Remove non [UUID] variant since it generates same test.

https://codereview.chromium.org/2466223002/diff/380001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/script-tests/characteristic/descriptor-blocklisted.js:21:
}, 'Making sure FUNCTION_NAME can not access blocklisted descriptors. ');
nit: Remove space at the end of the string.

https://codereview.chromium.org/2466223002/diff/380001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/bluetooth/script-tests/characteristic/descriptor-get-same-object.js
(right):

https://codereview.chromium.org/2466223002/diff/380001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/script-tests/characteristic/descriptor-get-same-object.js:11:
getDescriptor(user_description.alias),
hmm this will generate a single file with the first call. I think we will need a
manual test for this.

https://codereview.chromium.org/2466223002/diff/380001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/bluetooth/script-tests/characteristic/descriptor-invalid-name.js
(right):

https://codereview.chromium.org/2466223002/diff/380001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/script-tests/characteristic/descriptor-invalid-name.js:15:
getDescriptors('invalid-name')]),
Remove the non [UUID] variant since it generates the same test.

https://codereview.chromium.org/2466223002/diff/380001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/script-tests/characteristic/descriptor-invalid-name.js:31:
'Should not crash.');
Technically correct but not relevant for this test haha

https://codereview.chromium.org/2466223002/diff/380001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/bluetooth/script-tests/characteristic/device-out-of-range.js
(right):

https://codereview.chromium.org/2466223002/diff/380001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/script-tests/characteristic/device-out-of-range.js:16:
getDescriptors(user_description.name)]),
Remove user_description.name from second getDescriptors call.

https://codereview.chromium.org/2466223002/diff/380001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/bluetooth/script-tests/characteristic/service-is-removed.js
(right):

https://codereview.chromium.org/2466223002/diff/380001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/script-tests/characteristic/service-is-removed.js:20:

Remove extra lines.

https://codereview.chromium.org/2466223002/diff/380001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/bluetooth/BluetoothAttributeInstanceMap.cpp
(right):

https://codereview.chromium.org/2466223002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothAttributeInstanceMap.cpp:80:
if (!webDescriptor)
Just DCHECK. I don't think this will ever be called with a null webDescriptor.

https://codereview.chromium.org/2466223002/diff/380001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom
(right):

https://codereview.chromium.org/2466223002/diff/380001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom:191: //
Returns the Descriptors of a GATT Characteristic with |service_instance_id|.
Ah sorry I missed it on the first go:

s/service_instance_id/characteristic_instance_id/

[dougt, 2016-12-08 08:35:59]

ortuno, ptal

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

https://codereview.chromium.org/2466223002/diff/380001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:734:
device::BluetoothGattCharacteristic::Permission::PERMISSION_WRITE));
On 2016/12/08 05:42:51, ortuno wrote:
> I think this is Read and write:
> 
>
https://www.bluetooth.com/specifications/gatt/viewer?attributeXmlFile=org.blu...

Done.

Note, I also converted calls sites here to use
device::BluetoothRemoteGattCharacteristic::PROPERTY_* which is more correct than
Permission::*.  These values are the same, but hold a slightly different
meaning.

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

https://codereview.chromium.org/2466223002/diff/380001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:338:
//                 Note: This descriptor is blocklisted.
On 2016/12/08 05:42:51, ortuno wrote:
> Only for writes, right?

Done.

https://codereview.chromium.org/2466223002/diff/380001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:340:
//                 A test descriptor that does is blocklisted.
On 2016/12/08 05:42:51, ortuno wrote:
> Remove 'does'.

Done.

https://codereview.chromium.org/2466223002/diff/380001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/TestExpectations (right):

https://codereview.chromium.org/2466223002/diff/380001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/TestExpectations:1431: # TODO(jlebel): Remove
when methods are implemented.
On 2016/12/08 05:42:51, ortuno wrote:
> nit: no need for duplicate line.

Done.

https://codereview.chromium.org/2466223002/diff/380001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/bluetooth/characteristic/getDescriptors/blocklisted-descriptors-not-present.html
(right):

https://codereview.chromium.org/2466223002/diff/380001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/characteristic/getDescriptors/blocklisted-descriptors-not-present.html:20:
descriptor => {
On 2016/12/08 05:42:51, ortuno wrote:
> nit: Move to previous line.

Done.

https://codereview.chromium.org/2466223002/diff/380001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/characteristic/getDescriptors/blocklisted-descriptors-not-present.html:21:
let bad_uuid = 'bad2ddcf-60db-45cd-bef9-fd72b153cf7c';
On 2016/12/08 05:42:51, ortuno wrote:
> nit: Move this to resources.
>
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/resources...

Done.

https://codereview.chromium.org/2466223002/diff/380001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/bluetooth/characteristic/getDescriptors/gen-descriptor-invalid-name.html
(right):

https://codereview.chromium.org/2466223002/diff/380001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/characteristic/getDescriptors/gen-descriptor-invalid-name.html:34:
'Should not crash.');
On 2016/12/08 05:42:51, ortuno wrote:
> I mean it's technically correct :P but also not specific to this test. Just
> remove.

Done.

https://codereview.chromium.org/2466223002/diff/380001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/bluetooth/script-tests/characteristic/descriptor-blocklisted.js
(right):

https://codereview.chromium.org/2466223002/diff/380001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/script-tests/characteristic/descriptor-blocklisted.js:16:
getDescriptors('bad2ddcf-60db-45cd-bef9-fd72b153cf7c')]),
On 2016/12/08 05:42:51, ortuno wrote:
> Remove non [UUID] variant since it generates same test.

Done.

https://codereview.chromium.org/2466223002/diff/380001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/script-tests/characteristic/descriptor-blocklisted.js:21:
}, 'Making sure FUNCTION_NAME can not access blocklisted descriptors. ');
On 2016/12/08 05:42:51, ortuno wrote:
> nit: Remove space at the end of the string.

Done.

https://codereview.chromium.org/2466223002/diff/380001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/bluetooth/script-tests/characteristic/descriptor-get-same-object.js
(right):

https://codereview.chromium.org/2466223002/diff/380001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/script-tests/characteristic/descriptor-get-same-object.js:11:
getDescriptor(user_description.alias),
On 2016/12/08 05:42:52, ortuno wrote:
> hmm this will generate a single file with the first call. I think we will need
a
> manual test for this.

Same is true for the other same-object tests for characteristics, etc. :/  I
think i broke this in 668836. I am going to leave this as is for now, and fix
them all in bug 672379.

https://codereview.chromium.org/2466223002/diff/380001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/bluetooth/script-tests/characteristic/descriptor-invalid-name.js
(right):

https://codereview.chromium.org/2466223002/diff/380001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/script-tests/characteristic/descriptor-invalid-name.js:15:
getDescriptors('invalid-name')]),
On 2016/12/08 05:42:52, ortuno wrote:
> Remove the non [UUID] variant since it generates the same test.

Done.

https://codereview.chromium.org/2466223002/diff/380001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/script-tests/characteristic/descriptor-invalid-name.js:31:
'Should not crash.');
On 2016/12/08 05:42:52, ortuno wrote:
> Technically correct but not relevant for this test haha

Done.

https://codereview.chromium.org/2466223002/diff/380001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/bluetooth/script-tests/characteristic/device-out-of-range.js
(right):

https://codereview.chromium.org/2466223002/diff/380001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/script-tests/characteristic/device-out-of-range.js:16:
getDescriptors(user_description.name)]),
On 2016/12/08 05:42:52, ortuno wrote:
> Remove user_description.name from second getDescriptors call.

Done.

https://codereview.chromium.org/2466223002/diff/380001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/bluetooth/script-tests/characteristic/service-is-removed.js
(right):

https://codereview.chromium.org/2466223002/diff/380001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/script-tests/characteristic/service-is-removed.js:20:

On 2016/12/08 05:42:52, ortuno wrote:
> Remove extra lines.

Done.

https://codereview.chromium.org/2466223002/diff/380001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/bluetooth/BluetoothAttributeInstanceMap.cpp
(right):

https://codereview.chromium.org/2466223002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothAttributeInstanceMap.cpp:80:
if (!webDescriptor)
On 2016/12/08 05:42:52, ortuno wrote:
> Just DCHECK. I don't think this will ever be called with a null webDescriptor.

Done.

https://codereview.chromium.org/2466223002/diff/380001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom
(right):

https://codereview.chromium.org/2466223002/diff/380001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom:191: //
Returns the Descriptors of a GATT Characteristic with |service_instance_id|.
On 2016/12/08 05:42:52, ortuno wrote:
> Ah sorry I missed it on the first go:
> 
> s/service_instance_id/characteristic_instance_id/

Done.

[ortuno, 2016-12-08 20:49:14]

lgtm!

[ortuno, 2016-12-08 20:49:26]

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

[commit-bot: I haz the power, 2016-12-08 20:50:34]

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

[ortuno, 2016-12-08 20:57:21]

Ah just noticed that you updated bad_messages but there are no new histograms
for it. Please run:

python tools/metrics/histograms/update_bad_message_reasons.py

and check in those changes.

[dougt, 2016-12-08 21:15:49]

dougt@chromium.org changed reviewers:
+ dcheng@chromium.org, haraken@chromium.org, nasko@chromium.org

[dougt, 2016-12-08 21:15:51]

nasko: PTAL at content/browser/bad_messages.h

dcheng: Security review for web_bluetooth.mojom

haraken: PTAL at t/W/public/BUILD.gn &
third_party/WebKit/Source/modules/modules_idl_files.gni

[dougt, 2016-12-08 21:23:46]

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

[dougt, 2016-12-08 21:23:47]

isherman, ptal at the histrogram change.

[Ilya Sherman, 2016-12-08 21:35:14]

histograms.xml lgtm

[nasko, 2016-12-08 22:32:51]

I didn't see where in the patch is BDH_INVALID_DESCRIPTOR_ID actually used. Did
I miss a file?

https://codereview.chromium.org/2466223002/diff/420001/content/browser/bad_me...
File content/browser/bad_message.h (right):

https://codereview.chromium.org/2466223002/diff/420001/content/browser/bad_me...
content/browser/bad_message.h:184: BDH_INVALID_DESCRIPTOR_ID = 160,
FYI, there is another CL in flight that might commit that uses the same last
number. Just double check before landing that we don't end up with the same
numbers.

[dougt, 2016-12-08 22:39:56]

https://codereview.chromium.org/2466223002/diff/420001/content/browser/bad_me...
File content/browser/bad_message.h (right):

https://codereview.chromium.org/2466223002/diff/420001/content/browser/bad_me...
content/browser/bad_message.h:184: BDH_INVALID_DESCRIPTOR_ID = 160,
On 2016/12/08 22:32:51, nasko (very slow til Dec 13th) wrote:
> FYI, there is another CL in flight that might commit that uses the same last
> number. Just double check before landing that we don't end up with the same
> numbers.

Oh, this was used for read/write values.  we decided to move that to another cl
and i forgot to remove this value.  sorry for the noise.

[haraken, 2016-12-09 00:26:05]

LGTM

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

https://codereview.chromium.org/2466223002/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTDescriptor.cpp:14:
#include "core/inspector/ConsoleMessage.h"

Remove unused header includes from this file.

https://codereview.chromium.org/2466223002/diff/440001/third_party/WebKit/pub...
File
third_party/WebKit/public/platform/modules/bluetooth/WebBluetoothRemoteGATTDescriptorInit.h
(right):

https://codereview.chromium.org/2466223002/diff/440001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/bluetooth/WebBluetoothRemoteGATTDescriptorInit.h:1:
// Copyright 2015 The Chromium Authors. All rights reserved.

2016

[dcheng, 2016-12-09 08:17:57]

https://codereview.chromium.org/2466223002/diff/440001/content/renderer/bluet...
File content/renderer/bluetooth/web_bluetooth_impl.cc (right):

https://codereview.chromium.org/2466223002/diff/440001/content/renderer/bluet...
content/renderer/bluetooth/web_bluetooth_impl.cc:299: // TODO(dcheng): This
WebVector should use smart pointers.
Hmm =)

https://codereview.chromium.org/2466223002/diff/440001/device/bluetooth/test/...
File device/bluetooth/test/mock_bluetooth_gatt_characteristic.h (right):

https://codereview.chromium.org/2466223002/diff/440001/device/bluetooth/test/...
device/bluetooth/test/mock_bluetooth_gatt_characteristic.h:81:
ScopedVector<MockBluetoothGattDescriptor> mock_descriptors_;
Please use std::vector<std::unique_ptr<...>> in new code.

https://codereview.chromium.org/2466223002/diff/440001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/bluetooth/BluetoothDevice.h (right):

https://codereview.chromium.org/2466223002/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothDevice.h:66: bool
isValidDescriptor(const String& descriptorInstanceId);
I might be missing something, but this doesn't appear to be called?

https://codereview.chromium.org/2466223002/diff/440001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp
(right):

https://codereview.chromium.org/2466223002/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp:135:
CHECK(m_characteristic->gatt()->connected());
Why is this a CHECK? Either we should handle the error case properly, or this is
attempting to enforce some invariant (so it should be a DCHECK)

https://codereview.chromium.org/2466223002/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp:170:
wrapUnique(webDescriptor), m_characteristic));
It seems slightly surprising to me that we're transferring (?) ownership out of
a const WebVector<T*>&. If there's ownership transfer involved here, can we just
make the WebVector own unique ptrs and move here?

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

https://codereview.chromium.org/2466223002/diff/440001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/bluetooth/WebBluetooth.h:119: int32_t
quantity,
Out of curiosity... why can't we use an enum type directly here? Is this because
part of it is implemented in content/renderer, and another part is in Blink, so
the mojo variants would conflict?

[dougt, 2016-12-09 19:20:07]

https://codereview.chromium.org/2466223002/diff/440001/content/renderer/bluet...
File content/renderer/bluetooth/web_bluetooth_impl.cc (right):

https://codereview.chromium.org/2466223002/diff/440001/content/renderer/bluet...
content/renderer/bluetooth/web_bluetooth_impl.cc:299: // TODO(dcheng): This
WebVector should use smart pointers.
On 2016/12/09 08:17:57, dcheng wrote:
> Hmm =)

Acknowledged.

Also see: OnGetPrimaryServicesComplete, OnGetCharacteristicsComplete.

https://codereview.chromium.org/2466223002/diff/440001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/bluetooth/BluetoothDevice.h (right):

https://codereview.chromium.org/2466223002/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothDevice.h:66: bool
isValidDescriptor(const String& descriptorInstanceId);
On 2016/12/09 08:17:57, dcheng wrote:
> I might be missing something, but this doesn't appear to be called?

removed.  this is work for descriptor readValue/writeValue methods which were
teased out of this CL.  I'll remove here.

https://codereview.chromium.org/2466223002/diff/440001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp
(right):

https://codereview.chromium.org/2466223002/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp:135:
CHECK(m_characteristic->gatt()->connected());
On 2016/12/09 08:17:57, dcheng wrote:
> Why is this a CHECK? Either we should handle the error case properly, or this
is
> attempting to enforce some invariant (so it should be a DCHECK)

Hrm.  I think this should be handled properly and we should fail and pass up
that error when the callback object is created.

Also see:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/blueto...

ortuno, what do you think?

https://codereview.chromium.org/2466223002/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp:170:
wrapUnique(webDescriptor), m_characteristic));
On 2016/12/09 08:17:57, dcheng wrote:
> It seems slightly surprising to me that we're transferring (?) ownership out
of
> a const WebVector<T*>&. If there's ownership transfer involved here, can we
just
> make the WebVector own unique ptrs and move here?

Another question for ortuno or vince -- not sure why we copy them, but most of
the web bluetooth callbacks do just that.

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

https://codereview.chromium.org/2466223002/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTDescriptor.cpp:14:
#include "core/inspector/ConsoleMessage.h"
On 2016/12/09 00:26:04, haraken wrote:
> 
> Remove unused header includes from this file.

Done.

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

https://codereview.chromium.org/2466223002/diff/440001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/bluetooth/WebBluetooth.h:119: int32_t
quantity,
On 2016/12/09 08:17:57, dcheng wrote:
> Out of curiosity... why can't we use an enum type directly here? Is this
because
> part of it is implemented in content/renderer, and another part is in Blink,
so
> the mojo variants would conflict?

I think that's right. You can't include
third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom-blink.h
from
third_party/WebKit/public/platform/modules/bluetooth/WebBluetoothRemoteGATTDescriptorInit.h.

Vince might know the history.

https://codereview.chromium.org/2466223002/diff/440001/third_party/WebKit/pub...
File
third_party/WebKit/public/platform/modules/bluetooth/WebBluetoothRemoteGATTDescriptorInit.h
(right):

https://codereview.chromium.org/2466223002/diff/440001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/bluetooth/WebBluetoothRemoteGATTDescriptorInit.h:1:
// Copyright 2015 The Chromium Authors. All rights reserved.
On 2016/12/09 00:26:04, haraken wrote:
> 
> 2016

Done.

[scheib, 2016-12-10 01:12:57]

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

[scheib, 2016-12-10 01:12:59]

https://codereview.chromium.org/2466223002/diff/440001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp
(right):

https://codereview.chromium.org/2466223002/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp:135:
CHECK(m_characteristic->gatt()->connected());
On 2016/12/09 19:20:07, dougt wrote:
> On 2016/12/09 08:17:57, dcheng wrote:
> > Why is this a CHECK? Either we should handle the error case properly, or
this
> is
> > attempting to enforce some invariant (so it should be a DCHECK)
> 
> Hrm.  I think this should be handled properly and we should fail and pass up
> that error when the callback object is created.
> 
> Also see:
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/blueto...
> 
> ortuno, what do you think?

DCHECK is right here. (dougt, see
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...
if you haven't yet). The original patch used CHECK and this wasn't debated, but
the context remains the same. https://codereview.chromium.org/2387293008

https://codereview.chromium.org/2466223002/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp:170:
wrapUnique(webDescriptor), m_characteristic));
On 2016/12/09 19:20:07, dougt wrote:
> On 2016/12/09 08:17:57, dcheng wrote:
> > It seems slightly surprising to me that we're transferring (?) ownership out
> of
> > a const WebVector<T*>&. If there's ownership transfer involved here, can we
> just
> > make the WebVector own unique ptrs and move here?
> 
> Another question for ortuno or vince -- not sure why we copy them, but most of
> the web bluetooth callbacks do just that.

Related to the comment in content/renderer/bluetooth/web_bluetooth_impl.cc:
  // TODO(dcheng): This WebVector should use smart pointers.

This will be cleaned up as we go through onion soup and no longer rely on
WebBluetoothGetCharacteristicsCallbacks. This pattern was introduced (from
another confusing one of a unique ptr to a web vector of pointers) in
https://codereview.chromium.org/1861013005. dcheng you're not on that as a
reviewer -- so I'm presuming ortuno added your TODO after discussing with you in
another context.

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

https://codereview.chromium.org/2466223002/diff/440001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/bluetooth/WebBluetooth.h:119: int32_t
quantity,
On 2016/12/09 19:20:07, dougt wrote:
> On 2016/12/09 08:17:57, dcheng wrote:
> > Out of curiosity... why can't we use an enum type directly here? Is this
> because
> > part of it is implemented in content/renderer, and another part is in Blink,
> so
> > the mojo variants would conflict?
> 
> I think that's right. You can't include
>
third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom-blink.h
> from
>
third_party/WebKit/public/platform/modules/bluetooth/WebBluetoothRemoteGATTDescriptorInit.h.
> 
> Vince might know the history.

This file in 
  third_party/WebKit/public/platform
is included in both
  content/renderer/
  third_party/WebKit/Source/
So, yep, nope, can't use mojo generated files.

wait for ... Onion Soup!

[ortuno, 2016-12-11 22:12:21]

https://codereview.chromium.org/2466223002/diff/440001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp
(right):

https://codereview.chromium.org/2466223002/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp:170:
wrapUnique(webDescriptor), m_characteristic));
On 2016/12/10 at 01:12:59, scheib wrote:
> On 2016/12/09 19:20:07, dougt wrote:
> > On 2016/12/09 08:17:57, dcheng wrote:
> > > It seems slightly surprising to me that we're transferring (?) ownership
out
> > of
> > > a const WebVector<T*>&. If there's ownership transfer involved here, can
we
> > just
> > > make the WebVector own unique ptrs and move here?
> > 
> > Another question for ortuno or vince -- not sure why we copy them, but most
of
> > the web bluetooth callbacks do just that.
> 
> Related to the comment in content/renderer/bluetooth/web_bluetooth_impl.cc:
>   // TODO(dcheng): This WebVector should use smart pointers.
> 
> This will be cleaned up as we go through onion soup and no longer rely on
WebBluetoothGetCharacteristicsCallbacks. This pattern was introduced (from
another confusing one of a unique ptr to a web vector of pointers) in
https://codereview.chromium.org/1861013005. dcheng you're not on that as a
reviewer -- so I'm presuming ortuno added your TODO after discussing with you in
another context.

I first introduced the raw pointers pattern in http://crrev.com/1723723002
because WebPassOwnPtr didn't have a move operator. Then the pattern and TODO
were introduced by dcheng when we got rid of WebPassOwnPtr
http://crrev.com/1865913005. I just copied the code when moving from
bluetooth_dispatcher to web_bluetooth_impl.

If WebVector supports unique_ptrs then I think we could just use unique_ptr and
stop adding this TODO.

[dougt, 2016-12-12 20:12:06]

dcheng, ptal.

[dcheng, 2016-12-12 20:56:06]

https://codereview.chromium.org/2466223002/diff/440001/device/bluetooth/test/...
File device/bluetooth/test/mock_bluetooth_gatt_characteristic.h (right):

https://codereview.chromium.org/2466223002/diff/440001/device/bluetooth/test/...
device/bluetooth/test/mock_bluetooth_gatt_characteristic.h:81:
ScopedVector<MockBluetoothGattDescriptor> mock_descriptors_;
On 2016/12/09 08:17:57, dcheng wrote:
> Please use std::vector<std::unique_ptr<...>> in new code.

Looks like the uploaded patchset didn't include changes for this. It's separate
from the WebVector<> changes requested (we're actively trying to remove
ScopedVector from //base, so I'd rather not introduce new usage)

[patriciatabor17, 2016-12-17 05:56:49]

Coding is the most difficult part and sometimes it takes more time to fix the
error. http://essayssos.com/

[dougt, 2016-12-22 05:16:44]

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

[commit-bot: I haz the power, 2016-12-22 05:16:57]

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

[dougt, 2016-12-22 05:17:46]

Hey Dcheng,

Please take another look.

fwiw, I also removed all of the ScopedVector and ScopedPtrHashMap from
device/bluetooth in https://codereview.chromium.org/2567903004

[commit-bot: I haz the power, 2016-12-22 05:20:15]

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

[commit-bot: I haz the power, 2016-12-22 05:20:16]

Dry run: Try jobs failed on following builders:
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[dougt, 2016-12-22 05:25:44]

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

[commit-bot: I haz the power, 2016-12-22 05:26:02]

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

[commit-bot: I haz the power, 2016-12-22 05:28:30]

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

[commit-bot: I haz the power, 2016-12-22 05:28:32]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[dougt, 2016-12-22 05:52:41]

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

[commit-bot: I haz the power, 2016-12-22 05:52:59]

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

[dougt, 2016-12-22 05:53:00]

Patchset #12 (id:480001) has been deleted

[commit-bot: I haz the power, 2016-12-22 07:52:35]

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

[commit-bot: I haz the power, 2016-12-22 07:52:37]

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, 2016-12-22 19:58:57]

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

[commit-bot: I haz the power, 2016-12-22 19:59:34]

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

[commit-bot: I haz the power, 2016-12-22 21:38:15]

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

[commit-bot: I haz the power, 2016-12-22 21:38:17]

Dry run: This issue passed the CQ dry run.

[dcheng, 2016-12-29 09:07:18]

Any chance this can wait for https://codereview.chromium.org/2565913002/ to land
and rebase on top of that? That CL is pretty close to landing.

https://codereview.chromium.org/2466223002/diff/520001/content/browser/blueto...
File content/browser/bluetooth/web_bluetooth_service_impl.cc (right):

https://codereview.chromium.org/2466223002/diff/520001/content/browser/blueto...
content/browser/bluetooth/web_bluetooth_service_impl.cc:136: VLOG(1) << "Looking
for descriptor: " << descriptor_uuid.canonical_value();
DVLOG

https://codereview.chromium.org/2466223002/diff/520001/content/browser/blueto...
content/browser/bluetooth/web_bluetooth_service_impl.cc:139: VLOG(1) <<
"Descriptor in cache: "
Ditto: DVLOG

https://codereview.chromium.org/2466223002/diff/520001/content/browser/blueto...
content/browser/bluetooth/web_bluetooth_service_impl.cc:607:
descriptor_ptr->uuid = descriptor->GetUUID().canonical_value();
Please use a typemap for this, rather than building the mojo struct by hand.

https://codereview.chromium.org/2466223002/diff/520001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom
(right):

https://codereview.chromium.org/2466223002/diff/520001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom:118:
string uuid;
What's instance_id? Can it be any arbitrary string? Is it ever parsed in some
way?

In general, these structs all need more documentation on what the freeform
string fields can hold. The pre-existing structs can be done in a followup, but
this new one should be documented.

Also, use bluetooth.mojom.UUID for the uuid?

https://codereview.chromium.org/2466223002/diff/520001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom:195:
string characteristics_instance_id,
Similar question here: what is a characteristics_instance_id: is it any freeform
string or something more specific?

[dougt, 2017-01-12 02:29:00]

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

[commit-bot: I haz the power, 2017-01-12 02:29:34]

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-12 02:38:59]

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

[commit-bot: I haz the power, 2017-01-12 02:39:01]

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

[dougt, 2017-01-12 02:39:29]

Patchset #14 (id:540001) has been deleted

[dougt, 2017-01-12 02:40:03]

Patchset #12 (id:500001) has been deleted

[dougt, 2017-01-12 02:40:30]

Patchset #11 (id:460001) has been deleted

[dougt, 2017-01-12 02:53:21]

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

[commit-bot: I haz the power, 2017-01-12 02:53:45]

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-12 03:09:32]

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

[commit-bot: I haz the power, 2017-01-12 03:09:34]

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

[dougt, 2017-01-12 15:12:29]

Patchset #12 (id:560001) has been deleted

[dougt, 2017-01-12 15:15:04]

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

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

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-12 19:07:43]

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

[commit-bot: I haz the power, 2017-01-12 19:07:45]

Dry run: This issue passed the CQ dry run.

[dougt, 2017-01-12 19:38:16]

This changed a bit because of the onion soup work.

dcheng, scheib, PTAL

https://codereview.chromium.org/2466223002/diff/520001/content/browser/blueto...
File content/browser/bluetooth/web_bluetooth_service_impl.cc (right):

https://codereview.chromium.org/2466223002/diff/520001/content/browser/blueto...
content/browser/bluetooth/web_bluetooth_service_impl.cc:136: VLOG(1) << "Looking
for descriptor: " << descriptor_uuid.canonical_value();
On 2016/12/29 09:07:18, dcheng wrote:
> DVLOG

Other logging messages in this file use VLOG().

https://codereview.chromium.org/2466223002/diff/520001/content/browser/blueto...
content/browser/bluetooth/web_bluetooth_service_impl.cc:139: VLOG(1) <<
"Descriptor in cache: "
On 2016/12/29 09:07:18, dcheng wrote:
> Ditto: DVLOG

Acknowledged.

https://codereview.chromium.org/2466223002/diff/520001/content/browser/blueto...
content/browser/bluetooth/web_bluetooth_service_impl.cc:607:
descriptor_ptr->uuid = descriptor->GetUUID().canonical_value();
On 2016/12/29 09:07:18, dcheng wrote:
> Please use a typemap for this, rather than building the mojo struct by hand.

We'll do this in a follow up.

https://codereview.chromium.org/2466223002/diff/520001/content/browser/blueto...
content/browser/bluetooth/web_bluetooth_service_impl.cc:607:
descriptor_ptr->uuid = descriptor->GetUUID().canonical_value();
On 2016/12/29 09:07:18, dcheng wrote:
> Please use a typemap for this, rather than building the mojo struct by hand.

We will do this in a follow.  dcheng will send mail to chromium-mojo to discuss
approach.

https://codereview.chromium.org/2466223002/diff/520001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom
(right):

https://codereview.chromium.org/2466223002/diff/520001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom:118:
string uuid;
On 2016/12/29 09:07:18, dcheng wrote:
> What's instance_id? Can it be any arbitrary string? Is it ever parsed in some
> way?
> 
> In general, these structs all need more documentation on what the freeform
> string fields can hold. The pre-existing structs can be done in a followup,
but
> this new one should be documented.
> 
> Also, use bluetooth.mojom.UUID for the uuid?

Acknowledged.  Filed 676720.

https://codereview.chromium.org/2466223002/diff/520001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom:195:
string characteristics_instance_id,
On 2016/12/29 09:07:18, dcheng wrote:
> Similar question here: what is a characteristics_instance_id: is it any
freeform
> string or something more specific?

Acknowledged.

This is used id throughout. We can follow up and add better comments.

[scheib, 2017-01-12 23:41:36]

https://codereview.chromium.org/2466223002/diff/520001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom
(right):

https://codereview.chromium.org/2466223002/diff/520001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom:118:
string uuid;
On 2017/01/12 19:38:16, dougt wrote:
> On 2016/12/29 09:07:18, dcheng wrote:
> > What's instance_id? Can it be any arbitrary string? Is it ever parsed in
some
> > way?
> > 
> > In general, these structs all need more documentation on what the freeform
> > string fields can hold. The pre-existing structs can be done in a followup,
> but
> > this new one should be documented.
> > 
> > Also, use bluetooth.mojom.UUID for the uuid?
> 
> Acknowledged.  Filed 676720.

https://bugs.chromium.org/p/chromium/issues/detail?id=680587 I think?

FYI: the IDs are platform specific opaque identifiers. Each platform generates
them differently, and e.g. on linux they come from the drivers, while on Android
we generate our own. They are not intended to be exposed to javascript code, but
are necessary for the renderer implementation to be able to refer to specific
instances.

https://codereview.chromium.org/2466223002/diff/580001/content/browser/blueto...
File content/browser/bluetooth/web_bluetooth_service_impl.cc (right):

https://codereview.chromium.org/2466223002/diff/580001/content/browser/blueto...
content/browser/bluetooth/web_bluetooth_service_impl.cc:132:
std::vector<device::BluetoothRemoteGattDescriptor*> GetDescriptorsByUUID(
Empty line above this line.

Also, ;) jun recently moved the characteristic versions to device/bluetooth.
This should probably go there instead.
https://codereview.chromium.org/2615323002

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

https://codereview.chromium.org/2466223002/diff/580001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:747:
measurement_interval->AddMockDescriptor(std::move(client_config));
add blocklisted_descriptor?

https://codereview.chromium.org/2466223002/diff/580001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp
(right):

https://codereview.chromium.org/2466223002/diff/580001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp:390:
// Callback that allows us to resolve the promise with a single Characteristic
comment: characteristic -> descriptor

https://codereview.chromium.org/2466223002/diff/580001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTDescriptor.idl
(right):

https://codereview.chromium.org/2466223002/diff/580001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTDescriptor.idl:16:
[CallWith=ScriptState] Promise<DataView>             readValue();
Hmm, I would have left read/write commented out of IDL so that they don't even
appear available yet (feature detectable). But... this is OK too I suppose.

https://codereview.chromium.org/2466223002/diff/580001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom
(right):

https://codereview.chromium.org/2466223002/diff/580001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom:191: //
Returns the Descriptors of a GATT Characteristic with
|characteristics_instance_id|.
format to 80chars

[dougt, 2017-01-13 00:13:15]

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

[commit-bot: I haz the power, 2017-01-13 00:15:06]

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-13 00:16:29]

addresses scheib comments. PTAL

https://codereview.chromium.org/2466223002/diff/580001/content/browser/blueto...
File content/browser/bluetooth/web_bluetooth_service_impl.cc (right):

https://codereview.chromium.org/2466223002/diff/580001/content/browser/blueto...
content/browser/bluetooth/web_bluetooth_service_impl.cc:132:
std::vector<device::BluetoothRemoteGattDescriptor*> GetDescriptorsByUUID(
On 2017/01/12 23:41:35, scheib wrote:
> Empty line above this line.
> 
> Also, ;) jun recently moved the characteristic versions to device/bluetooth.
> This should probably go there instead.
> https://codereview.chromium.org/2615323002

Done.

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

https://codereview.chromium.org/2466223002/diff/580001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:747:
measurement_interval->AddMockDescriptor(std::move(client_config));
On 2017/01/12 23:41:35, scheib wrote:
> add blocklisted_descriptor?

Yup!  added.

https://codereview.chromium.org/2466223002/diff/580001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp
(right):

https://codereview.chromium.org/2466223002/diff/580001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp:390:
// Callback that allows us to resolve the promise with a single Characteristic
On 2017/01/12 23:41:35, scheib wrote:
> comment: characteristic -> descriptor

Done.

https://codereview.chromium.org/2466223002/diff/580001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTDescriptor.idl
(right):

https://codereview.chromium.org/2466223002/diff/580001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTDescriptor.idl:16:
[CallWith=ScriptState] Promise<DataView>             readValue();
On 2017/01/12 23:41:35, scheib wrote:
> Hmm, I would have left read/write commented out of IDL so that they don't even
> appear available yet (feature detectable). But... this is OK too I suppose.

Acknowledged.

[commit-bot: I haz the power, 2017-01-13 02:15:41]

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

[commit-bot: I haz the power, 2017-01-13 02:15:43]

Dry run: This issue passed the CQ dry run.

[scheib, 2017-01-13 05:26:30]

LGTM

[dcheng, 2017-01-13 09:28:42]

https://codereview.chromium.org/2466223002/diff/520001/content/browser/blueto...
File content/browser/bluetooth/web_bluetooth_service_impl.cc (right):

https://codereview.chromium.org/2466223002/diff/520001/content/browser/blueto...
content/browser/bluetooth/web_bluetooth_service_impl.cc:136: VLOG(1) << "Looking
for descriptor: " << descriptor_uuid.canonical_value();
On 2017/01/12 19:38:16, dougt wrote:
> On 2016/12/29 09:07:18, dcheng wrote:
> > DVLOG
> 
> Other logging messages in this file use VLOG().

They should be changed to DVLOG unless there's a good reason to do otherwise.
These strings take up space in an official build, even though they're basically
never seen.

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

https://codereview.chromium.org/2466223002/diff/600001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:723:
std::unique_ptr<NiceMockBluetoothGattDescriptor> user_description(
Prefer auto user_description =
base::MakeUnique<NiceMockBluetoothGattDescriptor>(...)

https://codereview.chromium.org/2466223002/diff/600001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp
(right):

https://codereview.chromium.org/2466223002/diff/600001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp:376:
auto service = m_device->bluetooth()->service();
As the type of this expression is unclear, let's write it out explicitly rather
than using auto.

https://codereview.chromium.org/2466223002/diff/600001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp:377:
WTF::Optional<String> uuid = WTF::nullopt;
Two nits:
1. Optional's default ctor is to construct an empty optional, so there's no need
to initialize with nullopt.
2. String is nullable by default already, so Optional is unnecessary.

[dougt, 2017-01-13 20:08:04]

dcheng, ptal

https://codereview.chromium.org/2466223002/diff/520001/content/browser/blueto...
File content/browser/bluetooth/web_bluetooth_service_impl.cc (right):

https://codereview.chromium.org/2466223002/diff/520001/content/browser/blueto...
content/browser/bluetooth/web_bluetooth_service_impl.cc:139: VLOG(1) <<
"Descriptor in cache: "
On 2017/01/12 19:38:16, dougt wrote:
> On 2016/12/29 09:07:18, dcheng wrote:
> > Ditto: DVLOG
> 
> Acknowledged.

I am removing the VLOG in this directory in:
    https://codereview.chromium.org/2631773002/

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

https://codereview.chromium.org/2466223002/diff/600001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:723:
std::unique_ptr<NiceMockBluetoothGattDescriptor> user_description(
On 2017/01/13 09:28:42, dcheng wrote:
> Prefer auto user_description =
> base::MakeUnique<NiceMockBluetoothGattDescriptor>(...)

I do too, but... there are ~30 other places in the this file that follow this
same pattern.

https://codereview.chromium.org/2466223002/diff/600001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp
(right):

https://codereview.chromium.org/2466223002/diff/600001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp:376:
auto service = m_device->bluetooth()->service();
On 2017/01/13 09:28:42, dcheng wrote:
> As the type of this expression is unclear, let's write it out explicitly
rather
> than using auto.

Done.

https://codereview.chromium.org/2466223002/diff/600001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTCharacteristic.cpp:377:
WTF::Optional<String> uuid = WTF::nullopt;
On 2017/01/13 09:28:42, dcheng wrote:
> Two nits:
> 1. Optional's default ctor is to construct an empty optional, so there's no
need
> to initialize with nullopt.
> 2. String is nullable by default already, so Optional is unnecessary.

I am going to fix this in a follow up.  This exists in
BluetoothRemoteGATTServer.cpp, and in BluetoothRemoteGATTService.cpp

[dcheng, 2017-01-13 22:48:33]

mojo lgtm with nits

https://codereview.chromium.org/2466223002/diff/620001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/bluetooth/BluetoothAttributeInstanceMap.cpp
(right):

https://codereview.chromium.org/2466223002/diff/620001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothAttributeInstanceMap.cpp:69:
String instanceId = descriptor->instance_id;
nit: const String& so it's just a cheap alias?

(String is already cheap to copy, but it's even cheaper if we don't copy at all)

https://codereview.chromium.org/2466223002/diff/620001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom
(right):

https://codereview.chromium.org/2466223002/diff/620001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom:192: //
|characteristics_instance_id|.
Elaborate when |descriptor_uuid| can be null and what it's used for.

[dougt, 2017-01-14 02:34:20]

https://codereview.chromium.org/2466223002/diff/620001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/bluetooth/BluetoothAttributeInstanceMap.cpp
(right):

https://codereview.chromium.org/2466223002/diff/620001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothAttributeInstanceMap.cpp:69:
String instanceId = descriptor->instance_id;
On 2017/01/13 22:48:33, dcheng wrote:
> nit: const String& so it's just a cheap alias?
> 
> (String is already cheap to copy, but it's even cheaper if we don't copy at
all)

That goes boom.

|descriptor| is std::moved into an object |result| that is stored in the
m_descriptorIdToObject map. After the move, |descriptor| is in a valid
but unspecified state.  So, we must not use it.

This is a modified version of getOrCreateRemoteGATTCharacteristic:

const String& instanceId =
  remoteGATTCharacteristic->instance_id;  <--- ref to member of moved-from
BluetoothRemoteGATTCharacteristic* characteristic =
    m_characteristicIdToObject.get(instanceId);
if (!characteristic) {
  characteristic = BluetoothRemoteGATTCharacteristic::create(
      context,
      std::move(remoteGATTCharacteristic), <-- move
      service, m_device);
  m_characteristicIdToObject.add(instanceId,   <-- usage
                                 characteristic);
}

https://codereview.chromium.org/2466223002/diff/620001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom
(right):

https://codereview.chromium.org/2466223002/diff/620001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom:192: //
|characteristics_instance_id|.
On 2017/01/13 22:48:33, dcheng wrote:
> Elaborate when |descriptor_uuid| can be null and what it's used for.

yes.  Tracked in bug 680587.

[dougt, 2017-01-14 02:34:52]

The CQ bit was checked by dougt@chromium.org

[dougt, 2017-01-14 02:34:53]

The patchset sent to the CQ was uploaded after l-g-t-m from ortuno@chromium.org,
isherman@chromium.org, haraken@chromium.org, scheib@chromium.org
Link to the patchset: https://codereview.chromium.org/2466223002/#ps620001
(title: "Implement WebBluetooth getDescriptor[s]")

[commit-bot: I haz the power, 2017-01-14 02:35:17]

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

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

CQ is committing da patch.

Bot data: {"patchset_id": 620001, "attempt_start_ts": 1484361292177120,
"parent_rev": "7f843b7b95d90150c661339f1a42d8bea32152f9", "commit_rev":
"4f2237ccffefb9642173b872d4a0aafbac57f65e"}

[commit-bot: I haz the power, 2017-01-14 04:15:41]

Description was changed from

==========
Implement WebBluetooth getDescriptor[s]

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.

BUG=660699

R=ortuno@chromium.org
==========

to

==========
Implement WebBluetooth getDescriptor[s]

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.

BUG=660699

R=ortuno@chromium.org

Review-Url: https://codereview.chromium.org/2466223002
Cr-Commit-Position: refs/heads/master@{#443784}
Committed:
https://chromium.googlesource.com/chromium/src/+/4f2237ccffefb9642173b872d4a0...
==========

[commit-bot: I haz the power, 2017-01-14 04:15:44]

Committed patchset #14 (id:620001) as
https://chromium.googlesource.com/chromium/src/+/4f2237ccffefb9642173b872d4a0...

[s.balikci23, 2017-01-25 20:04:56]

On 2017/01/14 04:15:44, commit-bot: I haz the power wrote:
> Committed patchset #14 (id:620001) as
>
https://chromium.googlesource.com/chromium/src/+/4f2237ccffefb9642173b872d4a0...

https://gifamerica.com/

[sehen1653, 2017-02-10 01:29:30]

On 2017/01/25 20:04:56, s.balikci23 wrote:
> On 2017/01/14 04:15:44, commit-bot: I haz the power wrote:
> > Committed patchset #14 (id:620001) as
> >
>
https://chromium.googlesource.com/chromium/src/+/4f2237ccffefb9642173b872d4a0...
> 
> https://gifamerica.com/

https://memesuper.com/

[melihusa2016, 2017-04-16 05:51:53]

On 2017/02/10 01:29:30, sehen1653 wrote:
> On 2017/01/25 20:04:56, s.balikci23 wrote:
> > On 2017/01/14 04:15:44, commit-bot: I haz the power wrote:
> > > Committed patchset #14 (id:620001) as
> > >
> >
>
https://chromium.googlesource.com/chromium/src/+/4f2237ccffefb9642173b872d4a0...
> > 
> > https://gifamerica.com/
> 
> https://memesuper.com/

https://vehiclepad.com/

[melihusa2016, 2017-04-16 05:52:00]

On 2017/02/10 01:29:30, sehen1653 wrote:
> On 2017/01/25 20:04:56, s.balikci23 wrote:
> > On 2017/01/14 04:15:44, commit-bot: I haz the power wrote:
> > > Committed patchset #14 (id:620001) as
> > >
> >
>
https://chromium.googlesource.com/chromium/src/+/4f2237ccffefb9642173b872d4a0...
> > 
> > https://gifamerica.com/
> 
> https://memesuper.com/

https://vehiclepad.com/

[melihusa2015, 2017-06-02 07:42:20]

On 2016/12/22 05:17:46, dougt wrote:
> Hey Dcheng,
> 
> Please take another look.
> 
> fwiw, I also removed all of the ScopedVector and ScopedPtrHashMap from
> device/bluetooth in https://codereview.chromium.org/2567903004

https://insta724.com/

[charlie7yedi, 2017-08-13 20:09:05]

On 2017/06/02 07:42:20, melihusa2015 wrote:
> On 2016/12/22 05:17:46, dougt wrote:
> > Hey Dcheng,
> > 
> > Please take another look.
> > 
> > fwiw, I also removed all of the ScopedVector and ScopedPtrHashMap from
> > device/bluetooth in https://codereview.chromium.org/2567903004
> 
> https://insta724.com/

https://instasafari.com/

[charlie7yedi, 2017-08-13 20:13:51]

On 2017/08/13 20:09:05, charlie7yedi wrote:
> On 2017/06/02 07:42:20, melihusa2015 wrote:
> > On 2016/12/22 05:17:46, dougt wrote:
> > > Hey Dcheng,
> > > 
> > > Please take another look.
> > > 
> > > fwiw, I also removed all of the ScopedVector and ScopedPtrHashMap from
> > > device/bluetooth in https://codereview.chromium.org/2567903004
> > 
> > https://insta724.com/
> 
> https://instasafari.com/

https://instasafari.com/post/BXvzfWbALNo