bluetooth: Blink-side implementation of getCharacteristic.

bluetooth: Blink-side implementation of getCharacteristic.

This is the third of a set of three patches to implement 
getCharacteristic:

  [1] http://crrev.com/1153343002
  [2] http://crrev.com/1156573005
  [3] This patch.

BUG=483344
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=196986

[ortuno, 2015-06-02 19:50:11]

Patchset #1 (id:1) has been deleted

[ortuno, 2015-06-02 19:50:38]

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

[ortuno, 2015-06-02 19:50:39]

@jyasskin: PTAL

[Jeffrey Yasskin, 2015-06-03 00:24:05]

https://codereview.chromium.org/1146163005/diff/20001/LayoutTests/bluetooth/g...
File LayoutTests/bluetooth/getCharacteristic.html (right):

https://codereview.chromium.org/1146163005/diff/20001/LayoutTests/bluetooth/g...
LayoutTests/bluetooth/getCharacteristic.html:9: var service1 =
"00001800-0000-1000-8000-00805f9b34fb";
Comment what these services and characteristics are, or give them descriptive
names.

https://codereview.chromium.org/1146163005/diff/20001/LayoutTests/bluetooth/g...
LayoutTests/bluetooth/getCharacteristic.html:16: return
device.connectGATT().then(function(gattServer) {
I think this could be:

sequential_promise_test(function() {
  testRunner.setBluetoothMockDataSet('ConnectableDeviceAdapter');
  return navigator.bluetooth.requestDevice().then(function(device) {
    return device.connectGATT();
  }).then(function(gattServer) {
    return gattServer.getPrimaryService(service1);
  }).then(function(service) {
    testRunner.setBluetoothMockDataSet('EmptyAdapter');
    return service.getCharacteristic(characteristic1).then(function() {
      assert_unreached("Device went out of range, should fail.");
    }, function(e) {
      assert_equals(e.name, "NetworkError");
    });
  });
});

https://codereview.chromium.org/1146163005/diff/20001/LayoutTests/bluetooth/g...
LayoutTests/bluetooth/getCharacteristic.html:36: return
service.getCharacteristic(characteristic2);
It'd be better to query for a characteristic that definitely doesn't appear on
the Generic Access service, rather than Appearance which we someday might change
the ConnectableDeviceAdapter to include.

https://codereview.chromium.org/1146163005/diff/20001/Source/modules/bluetoot...
File Source/modules/bluetooth/BluetoothGATTCharacteristic.cpp (right):

https://codereview.chromium.org/1146163005/diff/20001/Source/modules/bluetoot...
Source/modules/bluetooth/BluetoothGATTCharacteristic.cpp:31: return new
BluetoothGATTCharacteristic(*webCharacteristic);
This is a little weird (was probably weird in your other patch too, but I didn't
look as closely). Why is it right to copy the WebBluetoothGATTCharacteristic
into the BluetoothGATTCharacteristic instead of having
BluetoothGATTCharacteristic store an OwnPtr<WebBluetoothGATTCharacteristic> and
just take ownership of the already-allocated one?

https://codereview.chromium.org/1146163005/diff/20001/Source/modules/bluetoot...
File Source/modules/bluetooth/BluetoothGATTService.idl (right):

https://codereview.chromium.org/1146163005/diff/20001/Source/modules/bluetoot...
Source/modules/bluetooth/BluetoothGATTService.idl:9: typedef DOMString
BluetoothCharacteristicUUID;
Add the same TODO here as you have in GATTRemoteServer, I think.

[ortuno, 2015-06-03 21:49:51]

Patchset #2 (id:40001) has been deleted

[ortuno, 2015-06-03 21:55:17]

Patchset #2 (id:60001) has been deleted

[ortuno, 2015-06-03 21:57:04]

@jyasskin: Ready for review again!

https://codereview.chromium.org/1146163005/diff/20001/LayoutTests/bluetooth/g...
File LayoutTests/bluetooth/getCharacteristic.html (right):

https://codereview.chromium.org/1146163005/diff/20001/LayoutTests/bluetooth/g...
LayoutTests/bluetooth/getCharacteristic.html:9: var service1 =
"00001800-0000-1000-8000-00805f9b34fb";
On 2015/06/03 at 00:24:04, Jeffrey Yasskin wrote:
> Comment what these services and characteristics are, or give them descriptive
names.

Done.

https://codereview.chromium.org/1146163005/diff/20001/LayoutTests/bluetooth/g...
LayoutTests/bluetooth/getCharacteristic.html:16: return
device.connectGATT().then(function(gattServer) {
On 2015/06/03 at 00:24:04, Jeffrey Yasskin wrote:
> I think this could be:
> 
> sequential_promise_test(function() {
>   testRunner.setBluetoothMockDataSet('ConnectableDeviceAdapter');
>   return navigator.bluetooth.requestDevice().then(function(device) {
>     return device.connectGATT();
>   }).then(function(gattServer) {
>     return gattServer.getPrimaryService(service1);
>   }).then(function(service) {
>     testRunner.setBluetoothMockDataSet('EmptyAdapter');
>     return service.getCharacteristic(characteristic1).then(function() {
>       assert_unreached("Device went out of range, should fail.");
>     }, function(e) {
>       assert_equals(e.name, "NetworkError");
>     });
>   });
> });

Done.

https://codereview.chromium.org/1146163005/diff/20001/LayoutTests/bluetooth/g...
LayoutTests/bluetooth/getCharacteristic.html:36: return
service.getCharacteristic(characteristic2);
On 2015/06/03 at 00:24:04, Jeffrey Yasskin wrote:
> It'd be better to query for a characteristic that definitely doesn't appear on
the Generic Access service, rather than Appearance which we someday might change
the ConnectableDeviceAdapter to include.

Done.

https://codereview.chromium.org/1146163005/diff/20001/Source/modules/bluetoot...
File Source/modules/bluetooth/BluetoothGATTCharacteristic.cpp (right):

https://codereview.chromium.org/1146163005/diff/20001/Source/modules/bluetoot...
Source/modules/bluetooth/BluetoothGATTCharacteristic.cpp:31: return new
BluetoothGATTCharacteristic(*webCharacteristic);
On 2015/06/03 at 00:24:04, Jeffrey Yasskin wrote:
> This is a little weird (was probably weird in your other patch too, but I
didn't look as closely). Why is it right to copy the
WebBluetoothGATTCharacteristic into the BluetoothGATTCharacteristic instead of
having BluetoothGATTCharacteristic store an
OwnPtr<WebBluetoothGATTCharacteristic> and just take ownership of the
already-allocated one?

Changed to PassOwnPtr. OwnPtr alone won't compile.

https://codereview.chromium.org/1146163005/diff/20001/Source/modules/bluetoot...
File Source/modules/bluetooth/BluetoothGATTService.idl (right):

https://codereview.chromium.org/1146163005/diff/20001/Source/modules/bluetoot...
Source/modules/bluetooth/BluetoothGATTService.idl:9: typedef DOMString
BluetoothCharacteristicUUID;
On 2015/06/03 at 00:24:04, Jeffrey Yasskin wrote:
> Add the same TODO here as you have in GATTRemoteServer, I think.

I have another patch where I implemented some of BluetoothUUID. It seems that we
can't share typedefs across idl files. So we have to define them in all IDL
files :(

[Jeffrey Yasskin, 2015-06-03 23:22:53]

lgtm

https://codereview.chromium.org/1146163005/diff/20001/Source/modules/bluetoot...
File Source/modules/bluetooth/BluetoothGATTService.idl (right):

https://codereview.chromium.org/1146163005/diff/20001/Source/modules/bluetoot...
Source/modules/bluetooth/BluetoothGATTService.idl:9: typedef DOMString
BluetoothCharacteristicUUID;
On 2015/06/03 21:57:04, ortuno wrote:
> On 2015/06/03 at 00:24:04, Jeffrey Yasskin wrote:
> > Add the same TODO here as you have in GATTRemoteServer, I think.
> 
> I have another patch where I implemented some of BluetoothUUID. It seems that
we
> can't share typedefs across idl files. So we have to define them in all IDL
> files :(

Oh, fun.

[ortuno, 2015-06-03 23:23:44]

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

[ortuno, 2015-06-03 23:23:45]

@haraken: PTAL

[haraken, 2015-06-04 00:41:40]

LGTM

[scheib, 2015-06-04 18:30:51]

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

[scheib, 2015-06-04 18:30:52]

https://codereview.chromium.org/1146163005/diff/90001/LayoutTests/bluetooth/g...
File LayoutTests/bluetooth/getCharacteristic.html (right):

https://codereview.chromium.org/1146163005/diff/90001/LayoutTests/bluetooth/g...
LayoutTests/bluetooth/getCharacteristic.html:12: var characteristic1 =
"00002a00-0000-1000-8000-00805f9b34fb";
Name these more meaninfully than '1' '2'. How about:
var serviceUUID
var includedCharacteristicUUID
var nonIncludedCharacteristicUUID

https://codereview.chromium.org/1146163005/diff/90001/Source/modules/bluetoot...
File Source/modules/bluetooth/BluetoothGATTCharacteristic.idl (right):

https://codereview.chromium.org/1146163005/diff/90001/Source/modules/bluetoot...
Source/modules/bluetooth/BluetoothGATTCharacteristic.idl:14: // readonly   
attribute BluetoothGATTService     service;
TODO that the test should verify that this matches the service that was used to
call getCharacteristic.

[ortuno, 2015-06-05 20:58:00]

@scheib: Ready for review again!

https://codereview.chromium.org/1146163005/diff/90001/LayoutTests/bluetooth/g...
File LayoutTests/bluetooth/getCharacteristic.html (right):

https://codereview.chromium.org/1146163005/diff/90001/LayoutTests/bluetooth/g...
LayoutTests/bluetooth/getCharacteristic.html:12: var characteristic1 =
"00002a00-0000-1000-8000-00805f9b34fb";
On 2015/06/04 18:30:52, scheib wrote:
> Name these more meaninfully than '1' '2'. How about:
> var serviceUUID
> var includedCharacteristicUUID
> var nonIncludedCharacteristicUUID

Done.

https://codereview.chromium.org/1146163005/diff/90001/Source/modules/bluetoot...
File Source/modules/bluetooth/BluetoothGATTCharacteristic.idl (right):

https://codereview.chromium.org/1146163005/diff/90001/Source/modules/bluetoot...
Source/modules/bluetooth/BluetoothGATTCharacteristic.idl:14: // readonly   
attribute BluetoothGATTService     service;
On 2015/06/04 18:30:52, scheib wrote:
> TODO that the test should verify that this matches the service that was used
to
> call getCharacteristic. 

Done.

[scheib, 2015-06-08 16:46:50]

LGTM

[ortuno, 2015-06-12 00:57:01]

The CQ bit was checked by ortuno@chromium.org

[ortuno, 2015-06-12 00:57:01]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, jyasskin@chromium.org
Link to the patchset: https://codereview.chromium.org/1146163005/#ps130001
(title: "Merged with TOT")

[commit-bot: I haz the power, 2015-06-12 00:57:09]

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

[commit-bot: I haz the power, 2015-06-12 01:01:12]

Committed patchset #5 (id:130001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=196986