Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2104023002:
arc: bluetooth: Add Gatt server mojo API (Closed)
|
Created:
4 years, 5 months ago by puthik_chromium Modified:
4 years, 5 months ago CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, yusukes+watch_chromium.org, viettrungluu+watch_chromium.org, hidehiko+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, lhchavez+watch_chromium.org, darin (slow to review), ben+mojo_chromium.org, qsr+mojo_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionarc: bluetooth: Add Gatt server mojo API
This patch adds the mojo definition for Gatt Server API
BUG=629210
TEST=Build
Committed: https://crrev.com/9c5baa3ebd453f8b0778d922bd418c47f042cb68
Cr-Commit-Position: refs/heads/master@{#407546}
Patch Set 1 #
Total comments: 8
Patch Set 2 : transport -> devicetype #Patch Set 3 : fix patch#2 #
Total comments: 6
Patch Set 4 : rebase #
Total comments: 9
Patch Set 5 : Improve comment / rebase #Patch Set 6 : Remove sendResponse #Patch Set 7 : rebase #
Total comments: 2
Patch Set 8 : Bump mojo id #
Dependent Patchsets: Messages
Total messages: 51 (28 generated)
puthik@chromium.org changed reviewers: + rkc@chromium.org
WIP cl. Head up early to make sure that I use Gatt Server API correctly.
It is very hard to parse these CLs without any documentation of what these functions are supposed to be doing. Please add header comments with some details at least about what each method is supposed to be doing and what all the parameters mean. https://codereview.chromium.org/2104023002/diff/1/components/arc/common/bluet... File components/arc/common/bluetooth.mojom (right): https://codereview.chromium.org/2104023002/diff/1/components/arc/common/bluet... components/arc/common/bluetooth.mojom:293: int32 num_handles) What is num_handles? Is this the handles of the included service? or? Please add documentation to each of these functions clarifying what they do at some location. Either here, or the header file. https://codereview.chromium.org/2104023002/diff/1/components/arc/common/bluet... components/arc/common/bluetooth.mojom:304: [MinVersion=3] StartService@32(int32 service_handle, int32 transport) What is transport here? https://codereview.chromium.org/2104023002/diff/1/components/arc/common/bluet... components/arc/common/bluetooth.mojom:310: [MinVersion=3] SendIndication@35(int32 attribute_handle, Should this be called SendNotification? The Android Java API uses the word notify and the argument confirm. Also, why is confirm an int32?
ejcaruso@chromium.org changed reviewers: + ejcaruso@chromium.org
https://codereview.chromium.org/2104023002/diff/1/components/arc/common/bluet... File components/arc/common/bluetooth.mojom (right): https://codereview.chromium.org/2104023002/diff/1/components/arc/common/bluet... components/arc/common/bluetooth.mojom:304: [MinVersion=3] StartService@32(int32 service_handle, int32 transport) On 2016/06/29 21:54:17, rkc wrote: > What is transport here? It looks like this is BR/EDR, LE, etc. I refactored the transport type so we have an enum for this in device/bluetooth/bluetooth_common.h. Why don't we use the mojo DeviceType, which gets typemapped to one of these? https://codereview.chromium.org/2104023002/diff/1/components/arc/common/bluet... components/arc/common/bluetooth.mojom:310: [MinVersion=3] SendIndication@35(int32 attribute_handle, On 2016/06/29 21:54:17, rkc wrote: > Should this be called SendNotification? The Android Java API uses the word > notify and the argument confirm. > > Also, why is confirm an int32? http://androidxref.com/6.0.1_r10/xref/hardware/libhardware/include/hardware/b... Looks like this is libhardware's term for this function.
https://codereview.chromium.org/2104023002/diff/1/components/arc/common/bluet... File components/arc/common/bluetooth.mojom (right): https://codereview.chromium.org/2104023002/diff/1/components/arc/common/bluet... components/arc/common/bluetooth.mojom:293: int32 num_handles) On 2016/06/29 21:54:17, rkc wrote: > What is num_handles? Is this the handles of the included service? or? > > Please add documentation to each of these functions clarifying what they do at > some location. Either here, or the header file. Added at the header file. https://codereview.chromium.org/2104023002/diff/1/components/arc/common/bluet... components/arc/common/bluetooth.mojom:304: [MinVersion=3] StartService@32(int32 service_handle, int32 transport) On 2016/06/30 23:19:09, Eric Caruso wrote: > On 2016/06/29 21:54:17, rkc wrote: > > What is transport here? > > It looks like this is BR/EDR, LE, etc. > > I refactored the transport type so we have an enum for this in > device/bluetooth/bluetooth_common.h. Why don't we use the mojo DeviceType, which > gets typemapped to one of these? Done. https://codereview.chromium.org/2104023002/diff/1/components/arc/common/bluet... components/arc/common/bluetooth.mojom:310: [MinVersion=3] SendIndication@35(int32 attribute_handle, On 2016/06/29 21:54:17, rkc wrote: > Should this be called SendNotification? The Android Java API uses the word > notify and the argument confirm. > > Also, why is confirm an int32? It's int32 in Android HAL, but the upper Java layer in Android is a boolean. Boolean make more sense here so I change it to that.
https://codereview.chromium.org/2104023002/diff/40001/components/arc/bluetoot... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2104023002/diff/40001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:29: #include "device/bluetooth/bluetooth_local_gatt_service.h" Nit: Already included in header. https://codereview.chromium.org/2104023002/diff/40001/components/arc/bluetoot... File components/arc/bluetooth/arc_bluetooth_bridge.h (right): https://codereview.chromium.org/2104023002/diff/40001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.h:23: #include "device/bluetooth/bluetooth_local_gatt_characteristic.h" Forward declare the class and keep the header include only in the .cc Same with the local descriptor class. You should only need to include the local gatt service header. https://codereview.chromium.org/2104023002/diff/40001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.h:257: device::BluetoothTransport device_type, The transport doesn't make sense for starting a service. We can only start a service on LE. On Android's HAL implementation, it looks like this parameter is just ignored.
https://codereview.chromium.org/2104023002/diff/40001/components/arc/bluetoot... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2104023002/diff/40001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:29: #include "device/bluetooth/bluetooth_local_gatt_service.h" On 2016/07/01 21:28:27, rkc wrote: > Nit: Already included in header. Done. https://codereview.chromium.org/2104023002/diff/40001/components/arc/bluetoot... File components/arc/bluetooth/arc_bluetooth_bridge.h (right): https://codereview.chromium.org/2104023002/diff/40001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.h:23: #include "device/bluetooth/bluetooth_local_gatt_characteristic.h" On 2016/07/01 21:28:27, rkc wrote: > Forward declare the class and keep the header include only in the .cc > Same with the local descriptor class. You should only need to include the local > gatt service header. Done. https://codereview.chromium.org/2104023002/diff/40001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.h:257: device::BluetoothTransport device_type, On 2016/07/01 21:28:27, rkc wrote: > The transport doesn't make sense for starting a service. We can only start a > service on LE. On Android's HAL implementation, it looks like this parameter is > just ignored. I agree that it does not make sense. Removed.
bluetooth lgtm
puthik@chromium.org changed reviewers: + lhchavez@chromium.org, palmer@chromium.org
On 2016/07/15 18:56:06, puthik_chromium wrote: The implementation is in http://crrev.com/2104043002 and http://crrev.com/2101283003
This CL does not LGTM until I know what's going on. Also, open source code should refer to a bug in the Chromium bug tracker, not in the Google-internal bug tracker. https://codereview.chromium.org/2104023002/diff/60001/components/arc/bluetoot... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2104023002/diff/60001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:298: const ErrorCallback& error_callback) {} Are these functions going to be filled in with implementations in a later CL? The actual implementations may be security-critical, and hence would need security review. But if the only thing triggering security review is the .mojom OWNERS file rules, we won't automatically see that code. And I'm concerned by that |offset| argument. It's (probably) the wrong type, and it's important to range-check and overflow-check integer math (which is something people often forget). https://codereview.chromium.org/2104023002/diff/60001/components/arc/bluetoot... File components/arc/bluetooth/arc_bluetooth_bridge.h (right): https://codereview.chromium.org/2104023002/diff/60001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.h:270: // Send a response to a read/write operation It's much better for comments to be complete, punctuated sentences that describe the purpose of the arguments.
https://codereview.chromium.org/2104023002/diff/60001/components/arc/bluetoot... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2104023002/diff/60001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:298: const ErrorCallback& error_callback) {} On 2016/07/16 00:21:04, palmer wrote: > Are these functions going to be filled in with implementations in a later CL? > The actual implementations may be security-critical, and hence would need > security review. But if the only thing triggering security review is the .mojom > OWNERS file rules, we won't automatically see that code. > > And I'm concerned by that |offset| argument. It's (probably) the wrong type, and > it's important to range-check and overflow-check integer math (which is > something people often forget). The implementation is in http://crrev.com/2104043002 and http://crrev.com/2101283003 I separate the CLs to make it not too big.
https://codereview.chromium.org/2104023002/diff/60001/components/arc/bluetoot... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2104023002/diff/60001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:298: const ErrorCallback& error_callback) {} On 2016/07/16 00:27:18, puthik_chromium wrote: > On 2016/07/16 00:21:04, palmer wrote: > > Are these functions going to be filled in with implementations in a later CL? > > The actual implementations may be security-critical, and hence would need > > security review. But if the only thing triggering security review is the > .mojom > > OWNERS file rules, we won't automatically see that code. > > > > And I'm concerned by that |offset| argument. It's (probably) the wrong type, > and > > it's important to range-check and overflow-check integer math (which is > > something people often forget). > > The implementation is in http://crrev.com/2104043002 and > http://crrev.com/2101283003 > > I separate the CLs to make it not too big. The |offset| is |int| type in Android API. https://source.android.com/devices/halref/bt__gatt__server_8h_source.html line 89 https://codereview.chromium.org/2104023002/diff/60001/components/arc/bluetoot... File components/arc/bluetooth/arc_bluetooth_bridge.h (right): https://codereview.chromium.org/2104023002/diff/60001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.h:270: // Send a response to a read/write operation On 2016/07/16 00:21:04, palmer wrote: > It's much better for comments to be complete, punctuated sentences that describe > the purpose of the arguments. The comment is copy from Android code. https://source.android.com/devices/halref/bt__gatt__server_8h_source.html
Description was changed from ========== arc: bluetooth: Add Gatt server mojo API BUG=b:29612626 TEST=Build ========== to ========== arc: bluetooth: Add Gatt server mojo API This patch adds the mojo definition for Gatt Server API BUG=629210 TEST=Build ==========
I also open new Chrome bug for this.
https://codereview.chromium.org/2104023002/diff/60001/components/arc/bluetoot... File components/arc/bluetooth/arc_bluetooth_bridge.h (right): https://codereview.chromium.org/2104023002/diff/60001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.h:270: // Send a response to a read/write operation On 2016/07/18 21:27:39, puthik_chromium wrote: > On 2016/07/16 00:21:04, palmer wrote: > > It's much better for comments to be complete, punctuated sentences that > describe > > the purpose of the arguments. > > The comment is copy from Android code. > https://source.android.com/devices/halref/bt__gatt__server_8h_source.html You can still improve the comments. Also, it would be beneficial to also include a link to that document as part of the comments. https://codereview.chromium.org/2104023002/diff/60001/components/arc/common/b... File components/arc/common/bluetooth.mojom (right): https://codereview.chromium.org/2104023002/diff/60001/components/arc/common/b... components/arc/common/bluetooth.mojom:291: // Copy from Android API nit: Copied
The CQ bit was checked by puthik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by puthik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2104023002/diff/60001/components/arc/bluetoot... File components/arc/bluetooth/arc_bluetooth_bridge.h (right): https://codereview.chromium.org/2104023002/diff/60001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.h:270: // Send a response to a read/write operation On 2016/07/19 01:26:02, Luis Héctor Chávez wrote: > On 2016/07/18 21:27:39, puthik_chromium wrote: > > On 2016/07/16 00:21:04, palmer wrote: > > > It's much better for comments to be complete, punctuated sentences that > > describe > > > the purpose of the arguments. > > > > The comment is copy from Android code. > > https://source.android.com/devices/halref/bt__gatt__server_8h_source.html > > You can still improve the comments. Also, it would be beneficial to also include > a link to that document as part of the comments. Link added https://codereview.chromium.org/2104023002/diff/60001/components/arc/common/b... File components/arc/common/bluetooth.mojom (right): https://codereview.chromium.org/2104023002/diff/60001/components/arc/common/b... components/arc/common/bluetooth.mojom:291: // Copy from Android API On 2016/07/19 01:26:02, Luis Héctor Chávez wrote: > nit: Copied Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by puthik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by puthik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2104023002/diff/120001/components/arc/common/... File components/arc/common/bluetooth.mojom (right): https://codereview.chromium.org/2104023002/diff/120001/components/arc/common/... components/arc/common/bluetooth.mojom:293: [MinVersion=3] AddService@29(BluetoothGattServiceID service_id, FYI: https://codereview.chromium.org/2166143002/patch/60001/70003 you'll need to bump the ids.
The CQ bit was checked by puthik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2104023002/diff/120001/components/arc/common/... File components/arc/common/bluetooth.mojom (right): https://codereview.chromium.org/2104023002/diff/120001/components/arc/common/... components/arc/common/bluetooth.mojom:293: [MinVersion=3] AddService@29(BluetoothGattServiceID service_id, On 2016/07/22 01:56:41, Luis Héctor Chávez wrote: > FYI: https://codereview.chromium.org/2166143002/patch/60001/70003 > > you'll need to bump the ids. Done.
LGTM. Thanks for working with me on the other CLs.
The CQ bit was checked by puthik@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rkc@chromium.org, lhchavez@chromium.org Link to the patchset: https://codereview.chromium.org/2104023002/#ps140001 (title: "Bump mojo id")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== arc: bluetooth: Add Gatt server mojo API This patch adds the mojo definition for Gatt Server API BUG=629210 TEST=Build ========== to ========== arc: bluetooth: Add Gatt server mojo API This patch adds the mojo definition for Gatt Server API BUG=629210 TEST=Build ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== arc: bluetooth: Add Gatt server mojo API This patch adds the mojo definition for Gatt Server API BUG=629210 TEST=Build ========== to ========== arc: bluetooth: Add Gatt server mojo API This patch adds the mojo definition for Gatt Server API BUG=629210 TEST=Build Committed: https://crrev.com/9c5baa3ebd453f8b0778d922bd418c47f042cb68 Cr-Commit-Position: refs/heads/master@{#407546} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/9c5baa3ebd453f8b0778d922bd418c47f042cb68 Cr-Commit-Position: refs/heads/master@{#407546} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
