Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1093633003:
bluetooh: Define new connectGATT and WebBluetoothGATTRemoteServer (Closed)
Descriptionbluetooh: Define new connectGATT and WebBluetoothGATTRemoteServer
Initial implementation of connectGATT. Next steps is to actually connect to the device.
First of a set of 3 patches to implement connectGATT.
[1] This patch.
[2] https://codereview.chromium.org/1082273006
[3] https://codereview.chromium.org/1060593004
BUG=421668, 476735
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=194058
Patch Set 1 : #Patch Set 2 : #
Total comments: 20
Patch Set 3 : Fixes based on comments #Patch Set 4 : const reference #
Messages
Total messages: 22 (13 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
ortuno@chromium.org changed reviewers: + jyasskin@chromium.org, scheib@chromium.org
Please review jyasskin@, scheib@
https://codereview.chromium.org/1093633003/diff/60001/public/platform/modules... File public/platform/modules/bluetooth/WebBluetooth.h (right): https://codereview.chromium.org/1093633003/diff/60001/public/platform/modules... public/platform/modules/bluetooth/WebBluetooth.h:29: // WebBluetoothRequestDeviceCallbacks ownership transferred to the client. Should we have a comment like this for the ownership transfer in connectGATT()? If so, use "callee" instead of "client" or s/to the client/into connectGATT()/. (I'd read "client" as referring to the caller, but there's no way to pass ownership to the caller with this interface.) https://codereview.chromium.org/1093633003/diff/60001/public/platform/modules... public/platform/modules/bluetooth/WebBluetooth.h:34: virtual void connectGATT(std::string, WebBluetoothConnectGATTCallbacks*) { } Great! You're following the advice from https://github.com/WebBluetoothCG/web-bluetooth/issues/83. https://codereview.chromium.org/1093633003/diff/60001/public/platform/modules... public/platform/modules/bluetooth/WebBluetooth.h:34: virtual void connectGATT(std::string, WebBluetoothConnectGATTCallbacks*) { } Comment what this string means, or give the argument a descriptive name. It's the device's instanceID, right? https://codereview.chromium.org/1093633003/diff/60001/public/platform/modules... public/platform/modules/bluetooth/WebBluetooth.h:39: // TODO(ortuno): Properly define this methods once WebBluetoothServiceUuid and s/this/these/ https://codereview.chromium.org/1093633003/diff/60001/public/platform/modules... File public/platform/modules/bluetooth/WebBluetoothGATTRemoteServer.h (right): https://codereview.chromium.org/1093633003/diff/60001/public/platform/modules... public/platform/modules/bluetooth/WebBluetoothGATTRemoteServer.h:14: struct WebBluetoothGATTRemoteServer { Should you also be adding an .idl file to define this? https://codereview.chromium.org/1093633003/diff/60001/public/platform/modules... public/platform/modules/bluetooth/WebBluetoothGATTRemoteServer.h:15: WebBluetoothGATTRemoteServer(const WebString& instanceID, bool connected) Maybe "deviceInstanceID"? to make it clear what kind? Or comment that this is the instanceID of the device in which this server lives. https://codereview.chromium.org/1093633003/diff/60001/public/platform/modules... public/platform/modules/bluetooth/WebBluetoothGATTRemoteServer.h:21: // Members corresponding to BluetoothGATTRemoteServer attribute as s/attribute/attributes/
LGTM https://codereview.chromium.org/1093633003/diff/60001/public/platform/modules... File public/platform/modules/bluetooth/WebBluetooth.h (right): https://codereview.chromium.org/1093633003/diff/60001/public/platform/modules... public/platform/modules/bluetooth/WebBluetooth.h:32: // BluetoothDevice methods: OK, we're going 'monolithic' which we agreed with prudent for the expected size of APIs and overhead in splitting them. We'll need to have methods not conflict. Most won't, but there are a few: BluetoothGATTCharacteristic.readValue BluetoothGATTDescriptor.readValue BluetoothGATTCharacteristic.writeValue BluetoothGATTDescriptor.writeValue BluetoothInteraction.getCharacteristic BluetoothGATTService.getCharacteristic BluetoothInteraction.getDescriptor BluetoothGATTCharacteristic.getDescriptor We could decorate the names of all methods we add here, giving them context. But, I think that's overkill since most won't conflict. We can decorate in a few ways for the few that will conflict, here's my guess of what we can do: <method name>Of<Interface> readValueOfCharacteristic readValueOfDescriptor ... getCharacteristic getCharacteristicOfService So, for now this is good, just wanted to think that through as changing the interface is painful. https://codereview.chromium.org/1093633003/diff/60001/public/platform/modules... public/platform/modules/bluetooth/WebBluetooth.h:39: // TODO(ortuno): Properly define this methods once WebBluetoothServiceUuid and s/this/these/. If line wrapping, do so before 80 columns. https://codereview.chromium.org/1093633003/diff/60001/public/platform/modules... File public/platform/modules/bluetooth/WebBluetoothGATTRemoteServer.h (right): https://codereview.chromium.org/1093633003/diff/60001/public/platform/modules... public/platform/modules/bluetooth/WebBluetoothGATTRemoteServer.h:12: struct WebBluetoothDevice; This seems to no longer be needed.
https://codereview.chromium.org/1093633003/diff/60001/public/platform/modules... File public/platform/modules/bluetooth/WebBluetooth.h (right): https://codereview.chromium.org/1093633003/diff/60001/public/platform/modules... public/platform/modules/bluetooth/WebBluetooth.h:34: virtual void connectGATT(std::string, WebBluetoothConnectGATTCallbacks*) { } On 2015/04/17 23:45:08, Jeffrey Yasskin wrote: > Comment what this string means, or give the argument a descriptive name. It's > the device's instanceID, right? It is instanceID, oops I overlooked this too: Please use a consistent type for instanceID across the blink interface. WebString https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
The CQ bit was checked by ortuno@chromium.org
https://codereview.chromium.org/1093633003/diff/60001/public/platform/modules... File public/platform/modules/bluetooth/WebBluetooth.h (right): https://codereview.chromium.org/1093633003/diff/60001/public/platform/modules... public/platform/modules/bluetooth/WebBluetooth.h:29: // WebBluetoothRequestDeviceCallbacks ownership transferred to the client. On 2015/04/17 at 23:45:08, Jeffrey Yasskin wrote: > Should we have a comment like this for the ownership transfer in connectGATT()? If so, use "callee" instead of "client" or s/to the client/into connectGATT()/. (I'd read "client" as referring to the caller, but there's no way to pass ownership to the caller with this interface.) Done https://codereview.chromium.org/1093633003/diff/60001/public/platform/modules... public/platform/modules/bluetooth/WebBluetooth.h:32: // BluetoothDevice methods: On 2015/04/18 at 00:06:52, scheib wrote: > OK, we're going 'monolithic' which we agreed with prudent for the expected size of APIs and overhead in splitting them. We'll need to have methods not conflict. Most won't, but there are a few: > > BluetoothGATTCharacteristic.readValue > BluetoothGATTDescriptor.readValue > > BluetoothGATTCharacteristic.writeValue > BluetoothGATTDescriptor.writeValue > > BluetoothInteraction.getCharacteristic > BluetoothGATTService.getCharacteristic > > BluetoothInteraction.getDescriptor > BluetoothGATTCharacteristic.getDescriptor > > We could decorate the names of all methods we add here, giving them context. But, I think that's overkill since most won't conflict. > > We can decorate in a few ways for the few that will conflict, here's my guess of what we can do: <method name>Of<Interface> > readValueOfCharacteristic > readValueOfDescriptor > ... > getCharacteristic > getCharacteristicOfService > > So, for now this is good, just wanted to think that through as changing the interface is painful. Ack. https://codereview.chromium.org/1093633003/diff/60001/public/platform/modules... public/platform/modules/bluetooth/WebBluetooth.h:34: virtual void connectGATT(std::string, WebBluetoothConnectGATTCallbacks*) { } On 2015/04/17 at 23:45:08, Jeffrey Yasskin wrote: > Comment what this string means, or give the argument a descriptive name. It's the device's instanceID, right? Done. https://codereview.chromium.org/1093633003/diff/60001/public/platform/modules... public/platform/modules/bluetooth/WebBluetooth.h:34: virtual void connectGATT(std::string, WebBluetoothConnectGATTCallbacks*) { } On 2015/04/19 at 05:40:37, scheib wrote: > On 2015/04/17 23:45:08, Jeffrey Yasskin wrote: > > Comment what this string means, or give the argument a descriptive name. It's > > the device's instanceID, right? > > It is instanceID, oops I overlooked this too: Please use a consistent type for instanceID across the blink interface. WebString > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... Done. https://codereview.chromium.org/1093633003/diff/60001/public/platform/modules... public/platform/modules/bluetooth/WebBluetooth.h:39: // TODO(ortuno): Properly define this methods once WebBluetoothServiceUuid and On 2015/04/18 at 00:06:52, scheib wrote: > s/this/these/. If line wrapping, do so before 80 columns. Done. https://codereview.chromium.org/1093633003/diff/60001/public/platform/modules... File public/platform/modules/bluetooth/WebBluetoothGATTRemoteServer.h (right): https://codereview.chromium.org/1093633003/diff/60001/public/platform/modules... public/platform/modules/bluetooth/WebBluetoothGATTRemoteServer.h:12: struct WebBluetoothDevice; On 2015/04/18 at 00:06:52, scheib wrote: > This seems to no longer be needed. Oops Done. https://codereview.chromium.org/1093633003/diff/60001/public/platform/modules... public/platform/modules/bluetooth/WebBluetoothGATTRemoteServer.h:14: struct WebBluetoothGATTRemoteServer { On 2015/04/17 at 23:45:08, Jeffrey Yasskin wrote: > Should you also be adding an .idl file to define this? I added the .idl files in this patch: https://codereview.chromium.org/1052023004 I'll try to include any new definitions with their idl files in future patches. https://codereview.chromium.org/1093633003/diff/60001/public/platform/modules... public/platform/modules/bluetooth/WebBluetoothGATTRemoteServer.h:15: WebBluetoothGATTRemoteServer(const WebString& instanceID, bool connected) On 2015/04/17 at 23:45:08, Jeffrey Yasskin wrote: > Maybe "deviceInstanceID"? to make it clear what kind? Or comment that this is the instanceID of the device in which this server lives. Done. https://codereview.chromium.org/1093633003/diff/60001/public/platform/modules... public/platform/modules/bluetooth/WebBluetoothGATTRemoteServer.h:21: // Members corresponding to BluetoothGATTRemoteServer attribute as On 2015/04/17 at 23:45:08, Jeffrey Yasskin wrote: > s/attribute/attributes/ Done.
The patchset sent to the CQ was uploaded after l-g-t-m from scheib@chromium.org Link to the patchset: https://codereview.chromium.org/1093633003/#ps80001 (title: "Fixes based on comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1093633003/80001
The CQ bit was unchecked by ortuno@chromium.org
Patchset #3 (id:80001) has been deleted
Patchset #3 (id:80001) has been deleted
The CQ bit was checked by ortuno@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from scheib@chromium.org Link to the patchset: https://codereview.chromium.org/1093633003/#ps100001 (title: "Fixes based on comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1093633003/100001
The CQ bit was unchecked by ortuno@chromium.org
The CQ bit was checked by ortuno@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from scheib@chromium.org Link to the patchset: https://codereview.chromium.org/1093633003/#ps120001 (title: "const reference")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1093633003/120001
Message was sent while issue was closed.
Committed patchset #4 (id:120001) as https://src.chromium.org/viewvc/blink?view=rev&revision=194058 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
