Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2149713002:
arc: bluetooth: Add SDP host side support (Closed)
|
Created:
4 years, 5 months ago by Miao Modified:
4 years, 3 months ago Reviewers:
rkc, Luis Héctor Chávez, dcheng, rickyz (no longer on Chrome), yzshen1, puthik_chromium, Sameer Nanda CC:
Aaron Boodman, abarth-chromium, ben+mojo_chromium.org, chromium-reviews, darin (slow to review), elijahtaylor+arcwatch_chromium.org, hidehiko+watch_chromium.org, lhchavez+watch_chromium.org, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yusukes+watch_chromium.org, yzshen+watch_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 SDP host side support
This implements the Chrome side support for Service Discovery Protocol.
- Retrieve SDP records from a remote device
- Create/remove a SDP record for the local device
- Convert service record/attribute between Chrome and Mojo
BUG=b:29314637
TEST=None (The manual test is blocked on chromium:627907.)
Committed: https://crrev.com/702552c9b1fb144ae942387b22a772ec545c1008
Cr-Commit-Position: refs/heads/master@{#417210}
Patch Set 1 #
Total comments: 23
Patch Set 2 : Addresses comments on patch set 1 #
Total comments: 10
Patch Set 3 : Update bluetooth.mojom and change type converter accordingly #
Total comments: 4
Patch Set 4 : Change GetSdpRecords() to be async and add OnGetSdpRecords() to BluetoothInstance interface #
Total comments: 24
Patch Set 5 : Address comments on patch set 4 #
Total comments: 4
Patch Set 6 : Adopt typemap for type conversion and use base::ListValue for passing variants #Patch Set 7 : Rebase against issue 2254973006 #Patch Set 8 : Fix tryjob failures #
Total comments: 2
Patch Set 9 : Fix nit in bluetooth.mojom #Patch Set 10 : Rebase after issue 2254973006 landed #Patch Set 11 : Remove unused includes #
Total comments: 11
Patch Set 12 : Address security concern by defining attribute layer structures and add type mappings for new struc… #Patch Set 13 : arc: bluetooth: Add SDP host side support #
Total comments: 1
Patch Set 14 : arc: bluetooth: Add SDP host side support #Patch Set 15 : arc: bluetooth: Add SDP host side support #
Total comments: 8
Patch Set 16 : Rewind to the previous approach using generic attribute and type converter #
Total comments: 20
Patch Set 17 : Add unittests for type convertors #Patch Set 18 : Rebase against issue 2302963002. #
Total comments: 3
Patch Set 19 : Fix trybot failures. #
Total comments: 11
Patch Set 20 : Fix nits. #Patch Set 21 : Rebase on ToT. #
Total comments: 20
Patch Set 22 : Address comments on patch set 21 and rebase on ToT #
Total comments: 3
Patch Set 23 : arc: bluetooth: Add SDP host side support #Patch Set 24 : arc: bluetooth: Add SDP host side support #Patch Set 25 : arc: bluetooth: Add SDP host side support #Messages
Total messages: 149 (74 generated)
mcchou@chromium.org changed reviewers: + puthik@chromium.org, rkc@chromium.org
Hi Opal and Rahul, PTAL at this patch. Thanks. :)
lhchavez@chromium.org changed reviewers: + lhchavez@chromium.org
drive-by https://codereview.chromium.org/2149713002/diff/1/components/arc/common/bluet... File components/arc/common/bluetooth.mojom (right): https://codereview.chromium.org/2149713002/diff/1/components/arc/common/bluet... components/arc/common/bluetooth.mojom:229: enum BluetoothSdpAttrType { Does this need an [Exetnsible] attribute?
First pass https://codereview.chromium.org/2149713002/diff/1/components/arc/bluetooth/ar... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2149713002/diff/1/components/arc/bluetooth/ar... components/arc/bluetooth/arc_bluetooth_bridge.cc:978: } Remove line 975-978? This codes does not change anything. https://codereview.chromium.org/2149713002/diff/1/components/arc/bluetooth/ar... components/arc/bluetooth/arc_bluetooth_bridge.cc:1123: result->status = arc::mojom::BluetoothStatus::SUCCESS; Why success if there is no device? https://codereview.chromium.org/2149713002/diff/1/components/arc/bluetooth/ar... components/arc/bluetooth/arc_bluetooth_bridge.cc:1145: std::set<uint16_t> ids (v.begin(), v.end()); Why do we need both vector and set? https://codereview.chromium.org/2149713002/diff/1/components/arc/bluetooth/ar... components/arc/bluetooth/arc_bluetooth_bridge.cc:1149: if (ids.find(0x0001) == ids.end()) { Add constexpr in anonymouse namespace above for 0x0001
https://codereview.chromium.org/2149713002/diff/1/components/arc/bluetooth/ar... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2149713002/diff/1/components/arc/bluetooth/ar... components/arc/bluetooth/arc_bluetooth_bridge.cc:967: void ArcBluetoothBridge::OnGetServiceRecordsDone( This function doesn't seem to need any members from the class. Can it be in the anonymous namespace? Same for the other ones that are in the same case. https://codereview.chromium.org/2149713002/diff/1/components/arc/bluetooth/ar... components/arc/bluetooth/arc_bluetooth_bridge.cc:980: for (auto& rcd : records) braces are required if the only statement is line-wrapped. https://codereview.chromium.org/2149713002/diff/1/components/arc/bluetooth/ar... components/arc/bluetooth/arc_bluetooth_bridge.cc:1145: std::set<uint16_t> ids (v.begin(), v.end()); Prefer std::unordered_set if you are searching for elements multiple times. In this case you are only interested in finding a single element, so the conversion from vector to set is already O(n), so you don't gain anything from the fast O(1) lookup and you only pay O(n) extra memory. https://codereview.chromium.org/2149713002/diff/1/components/arc/bluetooth/bl... File components/arc/bluetooth/bluetooth_type_converters.cc (right): https://codereview.chromium.org/2149713002/diff/1/components/arc/bluetooth/bl... components/arc/bluetooth/bluetooth_type_converters.cc:40: uint8_t* value = (uint8_t*)malloc(sizeof(uint8_t) * size); avoid using malloc/raw pointers at all costs. Let's try to use the base::Value marshaling, but if it's not possible, this function should have returned a std::unique_ptr<uint8_t[]> and should have been initialized like this: std::unique_ptr<uint8_t[]> value = base::MakeUnique<uint8_t[]>(size); Also, in this case, |bytes| outlives the desired scope of the raw pointer, so the only thing you should have done is: bytes.data(); which already returns a uint8_t*. https://codereview.chromium.org/2149713002/diff/1/components/arc/bluetooth/bl... components/arc/bluetooth/bluetooth_type_converters.cc:168: int32_t n; Avoid this by adding braces to each case and declaring the variables where you need them. https://codereview.chromium.org/2149713002/diff/1/components/arc/bluetooth/bl... components/arc/bluetooth/bluetooth_type_converters.cc:335: uint16_t target_ids[] = {0x0001, 0x0004, 0x0005, 0x0009, 0x0100}; this is not being used as an array, so it's better to have these all be named constants: constexpr uint16_t kServiceClassIDList = 0x0001; ...
https://codereview.chromium.org/2149713002/diff/1/components/arc/bluetooth/ar... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2149713002/diff/1/components/arc/bluetooth/ar... components/arc/bluetooth/arc_bluetooth_bridge.cc:967: void ArcBluetoothBridge::OnGetServiceRecordsDone( On 2016/07/14 23:13:14, Luis Héctor Chávez wrote: > This function doesn't seem to need any members from the class. Can it be in the > anonymous namespace? > > Same for the other ones that are in the same case. Done. https://codereview.chromium.org/2149713002/diff/1/components/arc/bluetooth/ar... components/arc/bluetooth/arc_bluetooth_bridge.cc:978: } On 2016/07/14 01:32:34, puthik_chromium wrote: > Remove line 975-978? > This codes does not change anything. Done. https://codereview.chromium.org/2149713002/diff/1/components/arc/bluetooth/ar... components/arc/bluetooth/arc_bluetooth_bridge.cc:980: for (auto& rcd : records) On 2016/07/14 23:13:14, Luis Héctor Chávez wrote: > braces are required if the only statement is line-wrapped. Done. https://codereview.chromium.org/2149713002/diff/1/components/arc/bluetooth/ar... components/arc/bluetooth/arc_bluetooth_bridge.cc:1123: result->status = arc::mojom::BluetoothStatus::SUCCESS; On 2016/07/14 01:32:34, puthik_chromium wrote: > Why success if there is no device? This should be FAIL instead. https://codereview.chromium.org/2149713002/diff/1/components/arc/bluetooth/ar... components/arc/bluetooth/arc_bluetooth_bridge.cc:1145: std::set<uint16_t> ids (v.begin(), v.end()); On 2016/07/14 23:13:14, Luis Héctor Chávez wrote: > Prefer std::unordered_set if you are searching for elements multiple times. > > In this case you are only interested in finding a single element, so the > conversion from vector to set is already O(n), so you don't gain anything from > the fast O(1) lookup and you only pay O(n) extra memory. Removed set and find the element in the vector instead. https://codereview.chromium.org/2149713002/diff/1/components/arc/bluetooth/ar... components/arc/bluetooth/arc_bluetooth_bridge.cc:1149: if (ids.find(0x0001) == ids.end()) { On 2016/07/14 01:32:34, puthik_chromium wrote: > Add constexpr in anonymouse namespace above for 0x0001 Done. https://codereview.chromium.org/2149713002/diff/1/components/arc/bluetooth/bl... File components/arc/bluetooth/bluetooth_type_converters.cc (right): https://codereview.chromium.org/2149713002/diff/1/components/arc/bluetooth/bl... components/arc/bluetooth/bluetooth_type_converters.cc:40: uint8_t* value = (uint8_t*)malloc(sizeof(uint8_t) * size); On 2016/07/14 23:13:14, Luis Héctor Chávez wrote: > avoid using malloc/raw pointers at all costs. Let's try to use the base::Value > marshaling, but if it's not possible, this function should have returned a > std::unique_ptr<uint8_t[]> and should have been initialized like this: > > std::unique_ptr<uint8_t[]> value = base::MakeUnique<uint8_t[]>(size); > > Also, in this case, |bytes| outlives the desired scope of the raw pointer, so > the only thing you should have done is: > > bytes.data(); > > which already returns a uint8_t*. I'd be more than happy to use the base::Value marshaling than the current approach once the support is there. :) mojo::Array does not provide data(), so I converted it to std::vector and use data() from there. https://codereview.chromium.org/2149713002/diff/1/components/arc/bluetooth/bl... components/arc/bluetooth/bluetooth_type_converters.cc:168: int32_t n; On 2016/07/14 23:13:14, Luis Héctor Chávez wrote: > Avoid this by adding braces to each case and declaring the variables where you > need them. Done. https://codereview.chromium.org/2149713002/diff/1/components/arc/bluetooth/bl... components/arc/bluetooth/bluetooth_type_converters.cc:335: uint16_t target_ids[] = {0x0001, 0x0004, 0x0005, 0x0009, 0x0100}; On 2016/07/14 23:13:14, Luis Héctor Chávez wrote: > this is not being used as an array, so it's better to have these all be named > constants: > > constexpr uint16_t kServiceClassIDList = 0x0001; > ... Done. https://codereview.chromium.org/2149713002/diff/1/components/arc/common/bluet... File components/arc/common/bluetooth.mojom (right): https://codereview.chromium.org/2149713002/diff/1/components/arc/common/bluet... components/arc/common/bluetooth.mojom:229: enum BluetoothSdpAttrType { On 2016/07/13 18:52:46, Luis Héctor Chávez wrote: > Does this need an [Exetnsible] attribute? Done.
https://codereview.chromium.org/2149713002/diff/1/components/arc/bluetooth/bl... File components/arc/bluetooth/bluetooth_type_converters.cc (right): https://codereview.chromium.org/2149713002/diff/1/components/arc/bluetooth/bl... components/arc/bluetooth/bluetooth_type_converters.cc:40: uint8_t* value = (uint8_t*)malloc(sizeof(uint8_t) * size); On 2016/07/15 08:39:21, Miao wrote: > On 2016/07/14 23:13:14, Luis Héctor Chávez wrote: > > avoid using malloc/raw pointers at all costs. Let's try to use the base::Value > > marshaling, but if it's not possible, this function should have returned a > > std::unique_ptr<uint8_t[]> and should have been initialized like this: > > > > std::unique_ptr<uint8_t[]> value = base::MakeUnique<uint8_t[]>(size); > > > > Also, in this case, |bytes| outlives the desired scope of the raw pointer, so > > the only thing you should have done is: > > > > bytes.data(); > > > > which already returns a uint8_t*. > > I'd be more than happy to use the base::Value marshaling than the current > approach once the support is there. :) > mojo::Array does not provide data(), so I converted it to std::vector and use > data() from there. > On it :) Also, my apologies, the snippet should have been bytes.storage().data() to avoid a conversion. https://codereview.chromium.org/2149713002/diff/20001/components/arc/bluetoot... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2149713002/diff/20001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:1159: if (!found) { nit: if (std::find(v.begin(), v.end(), kServiceClassIdListAttributeId) == v.end()) https://codereview.chromium.org/2149713002/diff/20001/components/arc/bluetoot... File components/arc/bluetooth/bluetooth_type_converters.cc (right): https://codereview.chromium.org/2149713002/diff/20001/components/arc/bluetoot... components/arc/bluetooth/bluetooth_type_converters.cc:292: seq; seq = base::MakeUnique<...>(); https://codereview.chromium.org/2149713002/diff/20001/components/arc/bluetoot... components/arc/bluetooth/bluetooth_type_converters.cc:336: if (ids.find(kServiceClassIDList) != ids.end()) What happens if you do for (uint16_t id : ids) { switch (id) { case kServiceClassIDList: result->service_class_id_list = ... break; } } can the other side handle nulls correctly?
https://codereview.chromium.org/2149713002/diff/20001/components/arc/common/b... File components/arc/common/bluetooth.mojom (right): https://codereview.chromium.org/2149713002/diff/20001/components/arc/common/b... components/arc/common/bluetooth.mojom:328: // Bluetooth SDP functions FYI: I'm going to land http://crrev.com/2104023002 soon. Please update method id. https://codereview.chromium.org/2149713002/diff/20001/components/arc/common/b... components/arc/common/bluetooth.mojom:329: GetSdpRecords@29(BluetoothAddress remote_addr) => (BluetoothGetSdpRecordsResult result); Add [MinVersion=4] to all the new Method
https://codereview.chromium.org/2149713002/diff/1/components/arc/bluetooth/bl... File components/arc/bluetooth/bluetooth_type_converters.cc (right): https://codereview.chromium.org/2149713002/diff/1/components/arc/bluetooth/bl... components/arc/bluetooth/bluetooth_type_converters.cc:40: uint8_t* value = (uint8_t*)malloc(sizeof(uint8_t) * size); On 2016/07/18 15:42:02, Luis Héctor Chávez wrote: > On 2016/07/15 08:39:21, Miao wrote: > > On 2016/07/14 23:13:14, Luis Héctor Chávez wrote: > > > avoid using malloc/raw pointers at all costs. Let's try to use the > base::Value > > > marshaling, but if it's not possible, this function should have returned a > > > std::unique_ptr<uint8_t[]> and should have been initialized like this: > > > > > > std::unique_ptr<uint8_t[]> value = base::MakeUnique<uint8_t[]>(size); > > > > > > Also, in this case, |bytes| outlives the desired scope of the raw pointer, > so > > > the only thing you should have done is: > > > > > > bytes.data(); > > > > > > which already returns a uint8_t*. > > > > I'd be more than happy to use the base::Value marshaling than the current > > approach once the support is there. :) > > mojo::Array does not provide data(), so I converted it to std::vector and use > > data() from there. > > > > On it :) Also, my apologies, the snippet should have been bytes.storage().data() > to avoid a conversion. Done. https://codereview.chromium.org/2149713002/diff/20001/components/arc/bluetoot... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2149713002/diff/20001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:1159: if (!found) { On 2016/07/18 15:42:02, Luis Héctor Chávez wrote: > nit: > > if (std::find(v.begin(), v.end(), > kServiceClassIdListAttributeId) == v.end()) Done. https://codereview.chromium.org/2149713002/diff/20001/components/arc/bluetoot... File components/arc/bluetooth/bluetooth_type_converters.cc (right): https://codereview.chromium.org/2149713002/diff/20001/components/arc/bluetoot... components/arc/bluetooth/bluetooth_type_converters.cc:292: seq; On 2016/07/18 15:42:02, Luis Héctor Chávez wrote: > seq = base::MakeUnique<...>(); Done. https://codereview.chromium.org/2149713002/diff/20001/components/arc/bluetoot... components/arc/bluetooth/bluetooth_type_converters.cc:336: if (ids.find(kServiceClassIDList) != ids.end()) On 2016/07/18 15:42:02, Luis Héctor Chávez wrote: > What happens if you do > > for (uint16_t id : ids) { > switch (id) { > case kServiceClassIDList: > result->service_class_id_list = ... > break; > } > } > > can the other side handle nulls correctly? Yes, I have changed Android side so that nulls will work just fine. https://codereview.chromium.org/2149713002/diff/20001/components/arc/common/b... File components/arc/common/bluetooth.mojom (right): https://codereview.chromium.org/2149713002/diff/20001/components/arc/common/b... components/arc/common/bluetooth.mojom:328: // Bluetooth SDP functions On 2016/07/25 18:44:16, puthik_chromium wrote: > FYI: I'm going to land http://crrev.com/2104023002 soon. > Please update method id. Done. https://codereview.chromium.org/2149713002/diff/20001/components/arc/common/b... components/arc/common/bluetooth.mojom:329: GetSdpRecords@29(BluetoothAddress remote_addr) => (BluetoothGetSdpRecordsResult result); On 2016/07/25 18:44:16, puthik_chromium wrote: > Add [MinVersion=4] to all the new Method Done.
The CQ bit was checked by mcchou@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.
bluetooth usage lgtm (%nits) https://codereview.chromium.org/2149713002/diff/40001/components/arc/bluetoot... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2149713002/diff/40001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:31: #include "device/bluetooth/bluetooth_remote_gatt_characteristic.h" Not needed? This and the remote_gatt includes below. https://codereview.chromium.org/2149713002/diff/40001/components/arc/bluetoot... File components/arc/bluetooth/arc_bluetooth_bridge.h (right): https://codereview.chromium.org/2149713002/diff/40001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.h:29: #include "device/bluetooth/bluez/bluetooth_service_record_bluez.h" Not needed?
https://codereview.chromium.org/2149713002/diff/40001/components/arc/bluetoot... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2149713002/diff/40001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:31: #include "device/bluetooth/bluetooth_remote_gatt_characteristic.h" On 2016/07/27 01:34:56, rkc wrote: > Not needed? > This and the remote_gatt includes below. Done. https://codereview.chromium.org/2149713002/diff/40001/components/arc/bluetoot... File components/arc/bluetooth/arc_bluetooth_bridge.h (right): https://codereview.chromium.org/2149713002/diff/40001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.h:29: #include "device/bluetooth/bluez/bluetooth_service_record_bluez.h" On 2016/07/27 01:34:56, rkc wrote: > Not needed? Done.
Hello Luis, there are changes on bluetooth.mojom, can you take a look? Thanks.
mcchou@chromium.org changed reviewers: + rickyz@chromium.org
Hello Ricky, Can you help with the security review? Thanks.
I'm still trying to get the typemaps working on Android to avoid all that scary manual conversion. https://codereview.chromium.org/2149713002/diff/60001/components/arc/bluetoot... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2149713002/diff/60001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:1749: mojo::Array<mojom::BluetoothSdpRecordPtr> rcds; nit: Try not to abbreviate. Same in all places you use "rcd". To have some consistency, we need a consistent way to refer to the records. you can choose either: * bluez::BluetoothServiceRecordBlueZ record_bluez and mojom::BluetoothSdpRecordPtr record * bluez::BluetoothServiceRecordBlueZ ecord and mojom::BluetoothSdpRecordPtr instance_record https://codereview.chromium.org/2149713002/diff/60001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:1753: if (!HasBluetoothInstance()) nit: move to beginning to avoid the allocation if there is no instance? https://codereview.chromium.org/2149713002/diff/60001/components/arc/bluetoot... File components/arc/bluetooth/bluetooth_type_converters.cc (right): https://codereview.chromium.org/2149713002/diff/60001/components/arc/bluetoot... components/arc/bluetooth/bluetooth_type_converters.cc:390: for (it = rcd->attrs.begin(); it != rcd->attrs.end(); it++) { for (const auto& it : rcd->attrs) ?
Adding dcheng@, as he might have more opinions about TypeConverters vs. type mapping (I'm not super familiar with either). https://codereview.chromium.org/2149713002/diff/60001/components/arc/bluetoot... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2149713002/diff/60001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:1374: bluez::BluetoothDeviceBlueZ* device_bluez = Is it documented anywhere that GetDevice always returns a BluetoothDeviceBlueZ? Even if it's true now, this is very scary, and could become an issue if this ever changes. https://codereview.chromium.org/2149713002/diff/60001/components/arc/bluetoot... File components/arc/bluetooth/bluetooth_type_converters.cc (right): https://codereview.chromium.org/2149713002/diff/60001/components/arc/bluetoot... components/arc/bluetooth/bluetooth_type_converters.cc:307: mojo::ConvertTo<bluez::BluetoothServiceAttributeValueBlueZ>( Can this lead to stack overflow via too much recursion? https://codereview.chromium.org/2149713002/diff/60001/components/arc/bluetoot... components/arc/bluetooth/bluetooth_type_converters.cc:345: std::set<uint16_t> ids(v.begin(), v.end()); Not used. https://codereview.chromium.org/2149713002/diff/60001/components/arc/bluetoot... components/arc/bluetooth/bluetooth_type_converters.cc:393: *attr_bluez = Does this work? attr_bluez is nullptr. Also, this line could have been hoisted outside of the switch. https://codereview.chromium.org/2149713002/diff/60001/components/arc/common/b... File components/arc/common/bluetooth.mojom (right): https://codereview.chromium.org/2149713002/diff/60001/components/arc/common/b... components/arc/common/bluetooth.mojom:244: array<uint8> value; Why do different integer types need to be stuffed into a byte array? Could this use optional fields for the various attr types instead?
https://codereview.chromium.org/2149713002/diff/60001/components/arc/common/b... File components/arc/common/bluetooth.mojom (right): https://codereview.chromium.org/2149713002/diff/60001/components/arc/common/b... components/arc/common/bluetooth.mojom:244: array<uint8> value; On 2016/08/16 at 23:55:07, rickyz wrote: > Why do different integer types need to be stuffed into a byte array? Could this use optional fields for the various attr types instead? Correction: It turns out mojo support unions - for example: https://cs.chromium.org/chromium/src/cc/ipc/quads.mojom?q=file:mojom+union&sq...
dcheng@chromium.org changed reviewers: + dcheng@chromium.org
This should definitely by a type map rather than a type converter. So let's do that, rather than adding more type converters.
On 2016/08/17 00:26:52, dcheng wrote: > This should definitely by a type map rather than a type converter. So let's do > that, rather than adding more type converters. lhchavez@, you mentioned Android is an issue, but why? Typemapping is for C++, and it should work for Android C++ already, no?
https://codereview.chromium.org/2149713002/diff/60001/components/arc/bluetoot... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2149713002/diff/60001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:1374: bluez::BluetoothDeviceBlueZ* device_bluez = On 2016/08/16 23:55:07, rickyz wrote: > Is it documented anywhere that GetDevice always returns a BluetoothDeviceBlueZ? > Even if it's true now, this is very scary, and could become an issue if this > ever changes. https://cs.chromium.org/chromium/src/device/bluetooth/bluez/bluetooth_device_... GetServiceRecords() is supported only on Chrome OS platform where the corresponding supports in BlueZ are landed recently. Although it is scary, we need to cast BluetoothDevice to BluetoothDeviceBlueZ in order to access that function. https://codereview.chromium.org/2149713002/diff/60001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:1749: mojo::Array<mojom::BluetoothSdpRecordPtr> rcds; On 2016/08/16 15:56:03, Luis Héctor Chávez wrote: > nit: Try not to abbreviate. Same in all places you use "rcd". To have some > consistency, we need a consistent way to refer to the records. you can choose > either: > > * bluez::BluetoothServiceRecordBlueZ record_bluez and > mojom::BluetoothSdpRecordPtr record > * bluez::BluetoothServiceRecordBlueZ ecord and mojom::BluetoothSdpRecordPtr > instance_record Done. https://codereview.chromium.org/2149713002/diff/60001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:1753: if (!HasBluetoothInstance()) On 2016/08/16 15:56:03, Luis Héctor Chávez wrote: > nit: move to beginning to avoid the allocation if there is no instance? Done. https://codereview.chromium.org/2149713002/diff/60001/components/arc/bluetoot... File components/arc/bluetooth/bluetooth_type_converters.cc (right): https://codereview.chromium.org/2149713002/diff/60001/components/arc/bluetoot... components/arc/bluetooth/bluetooth_type_converters.cc:307: mojo::ConvertTo<bluez::BluetoothServiceAttributeValueBlueZ>( On 2016/08/16 23:55:07, rickyz wrote: > Can this lead to stack overflow via too much recursion? There will be at most two layer of sequences, so it's unlikely to cause stack overflow. https://codereview.chromium.org/2149713002/diff/60001/components/arc/bluetoot... components/arc/bluetooth/bluetooth_type_converters.cc:345: std::set<uint16_t> ids(v.begin(), v.end()); On 2016/08/16 23:55:07, rickyz wrote: > Not used. Removed. https://codereview.chromium.org/2149713002/diff/60001/components/arc/bluetoot... components/arc/bluetooth/bluetooth_type_converters.cc:390: for (it = rcd->attrs.begin(); it != rcd->attrs.end(); it++) { On 2016/08/16 15:56:03, Luis Héctor Chávez wrote: > for (const auto& it : rcd->attrs) ? Done. https://codereview.chromium.org/2149713002/diff/60001/components/arc/bluetoot... components/arc/bluetooth/bluetooth_type_converters.cc:393: *attr_bluez = On 2016/08/16 23:55:07, rickyz wrote: > Does this work? attr_bluez is nullptr. > > Also, this line could have been hoisted outside of the switch. Create a new attribute inside each case. https://codereview.chromium.org/2149713002/diff/60001/components/arc/common/b... File components/arc/common/bluetooth.mojom (right): https://codereview.chromium.org/2149713002/diff/60001/components/arc/common/b... components/arc/common/bluetooth.mojom:244: array<uint8> value; On 2016/08/17 00:20:30, rickyz wrote: > On 2016/08/16 at 23:55:07, rickyz wrote: > > Why do different integer types need to be stuffed into a byte array? Could > this use optional fields for the various attr types instead? > > Correction: It turns out mojo support unions - for example: > https://cs.chromium.org/chromium/src/cc/ipc/quads.mojom?q=file:mojom+union&sq... The original plan is to use base::ListValue to pass the variant, but since Android side support is not there yet. I used a byte array here, so we can make less changes to adopt base::ListValue when it is ready. But since the uprev may take a while, I can use union if you have strong opinion about this.
On 2016/08/17 00:26:52, dcheng wrote: > This should definitely by a type map rather than a type converter. So let's do > that, rather than adding more type converters. dcheng@, can we make the type map changes in the upcoming CL? Since we are aiming at landing the initial patch for SDP support by the end of this week(08/19).
mcchou@chromium.org changed reviewers: + snanda@chromium.org
On 2016/08/17 00:27:33, dcheng wrote: > On 2016/08/17 00:26:52, dcheng wrote: > > This should definitely by a type map rather than a type converter. So let's do > > that, rather than adding more type converters. > > lhchavez@, you mentioned Android is an issue, but why? Typemapping is for C++, > and it should work for Android C++ already, no? Here is the context of Luis's comment. In bluetooth.mojom, struct BluetoothSdpServiceAttr can hold either a variant(primitive type of data) or a sequence, and BluetoothSdpServiceAttr is the mojom version of bluez::BluetoothServiceAttributeValueBlueZ which stores the variant as a base::Value. The original approach is to make use of Mojo supports of base::ListValue type mapping. However, the current Android Mojo version doesn't support base::ListValue type mapping, so the type mapping only works between Chrome and Mojo interface but not between mojo interface and Android. An alternitive is using union and list all possible types. Please refer to Ricky's comment on https://codereview.chromium.org/2149713002/diff/60001/components/arc/common/b....
On 2016/08/17 14:33:56, Miao wrote: > On 2016/08/17 00:26:52, dcheng wrote: > > This should definitely by a type map rather than a type converter. So let's do > > that, rather than adding more type converters. > > dcheng@, can we make the type map changes in the upcoming CL? Since we are > aiming at landing the initial patch for SDP support by the end of this > week(08/19). This isn't really a sustainable approach, there's always going to be more features that we want to land. At the very least, we shouldn't be adding new type converters. We don't have to typemap everything at once, but we can just typemap the new things, no?
On 2016/08/17 14:54:12, Miao wrote: > On 2016/08/17 00:27:33, dcheng wrote: > > On 2016/08/17 00:26:52, dcheng wrote: > > > This should definitely by a type map rather than a type converter. So let's > do > > > that, rather than adding more type converters. > > > > lhchavez@, you mentioned Android is an issue, but why? Typemapping is for C++, > > and it should work for Android C++ already, no? > > Here is the context of Luis's comment. In bluetooth.mojom, struct > BluetoothSdpServiceAttr can hold either a variant(primitive type of data) or a > sequence, and BluetoothSdpServiceAttr is the mojom version of > bluez::BluetoothServiceAttributeValueBlueZ which stores the variant as a > base::Value. The original approach is to make use of Mojo supports of > base::ListValue type mapping. However, the current Android Mojo version doesn't > support base::ListValue type mapping, so the type mapping only works between > Chrome and Mojo interface but not between mojo interface and Android. > > An alternitive is using union and list all possible types. Please refer to > Ricky's comment on > https://codereview.chromium.org/2149713002/diff/60001/components/arc/common/b.... I don't think I understand the background context here, as I've never reviewed the Android side of the ARC++ mojo CLs. Are you saying that the Android-side is written in C++ and uses these type converters?
On 2016/08/17 17:22:16, dcheng wrote: > On 2016/08/17 14:54:12, Miao wrote: > > On 2016/08/17 00:27:33, dcheng wrote: > > > On 2016/08/17 00:26:52, dcheng wrote: > > > > This should definitely by a type map rather than a type converter. So > let's > > do > > > > that, rather than adding more type converters. > > > > > > lhchavez@, you mentioned Android is an issue, but why? Typemapping is for > C++, > > > and it should work for Android C++ already, no? > > > > Here is the context of Luis's comment. In bluetooth.mojom, struct > > BluetoothSdpServiceAttr can hold either a variant(primitive type of data) or a > > sequence, and BluetoothSdpServiceAttr is the mojom version of > > bluez::BluetoothServiceAttributeValueBlueZ which stores the variant as a > > base::Value. The original approach is to make use of Mojo supports of > > base::ListValue type mapping. However, the current Android Mojo version > doesn't > > support base::ListValue type mapping, so the type mapping only works between > > Chrome and Mojo interface but not between mojo interface and Android. > > > > An alternitive is using union and list all possible types. Please refer to > > Ricky's comment on > > > https://codereview.chromium.org/2149713002/diff/60001/components/arc/common/b.... > > I don't think I understand the background context here, as I've never reviewed > the Android side of the ARC++ mojo CLs. > > Are you saying that the Android-side is written in C++ and uses these type > converters? Yes, but they currently don't compile there (I'm working on making that possible).
https://codereview.chromium.org/2149713002/diff/60001/components/arc/bluetoot... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2149713002/diff/60001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:1374: bluez::BluetoothDeviceBlueZ* device_bluez = On 2016/08/17 at 14:30:19, Miao wrote: > On 2016/08/16 23:55:07, rickyz wrote: > > Is it documented anywhere that GetDevice always returns a BluetoothDeviceBlueZ? > > Even if it's true now, this is very scary, and could become an issue if this > > ever changes. > > https://cs.chromium.org/chromium/src/device/bluetooth/bluez/bluetooth_device_... > GetServiceRecords() is supported only on Chrome OS platform where the corresponding supports in BlueZ are landed recently. Although it is scary, we need to cast BluetoothDevice to BluetoothDeviceBlueZ in order to access that function. I understand that this needs to be casted to access that function, but I'm trying to find some assurance that the cast is always legal - that is, that bluetooth_adapter_->GetDevice(...) always returns a BluetoothDeviceBlueZ*, and not some other type that cannot be safely casted. https://codereview.chromium.org/2149713002/diff/60001/components/arc/bluetoot... File components/arc/bluetooth/bluetooth_type_converters.cc (right): https://codereview.chromium.org/2149713002/diff/60001/components/arc/bluetoot... components/arc/bluetooth/bluetooth_type_converters.cc:307: mojo::ConvertTo<bluez::BluetoothServiceAttributeValueBlueZ>( On 2016/08/17 at 14:30:20, Miao wrote: > On 2016/08/16 23:55:07, rickyz wrote: > > Can this lead to stack overflow via too much recursion? > > There will be at most two layer of sequences, so it's unlikely to cause stack overflow. Sorry, I didn't give the full background here - from a security perspective, what I'm worried about is if the Android side is compromised, and intentionally sends a message with more than two layers in an attempt to exploit to exploit Chrome. https://codereview.chromium.org/2149713002/diff/60001/components/arc/bluetoot... components/arc/bluetooth/bluetooth_type_converters.cc:393: *attr_bluez = On 2016/08/17 at 14:30:19, Miao wrote: > On 2016/08/16 23:55:07, rickyz wrote: > > Does this work? attr_bluez is nullptr. > > > > Also, this line could have been hoisted outside of the switch. > > Create a new attribute inside each case. This would have been caught by a test :-) Mind adding some tests that exercise the code in change? https://codereview.chromium.org/2149713002/diff/60001/components/arc/common/b... File components/arc/common/bluetooth.mojom (right): https://codereview.chromium.org/2149713002/diff/60001/components/arc/common/b... components/arc/common/bluetooth.mojom:244: array<uint8> value; On 2016/08/17 at 14:30:20, Miao wrote: > On 2016/08/17 00:20:30, rickyz wrote: > > On 2016/08/16 at 23:55:07, rickyz wrote: > > > Why do different integer types need to be stuffed into a byte array? Could > > this use optional fields for the various attr types instead? > > > > Correction: It turns out mojo support unions - for example: > > https://cs.chromium.org/chromium/src/cc/ipc/quads.mojom?q=file:mojom+union&sq... > > The original plan is to use base::ListValue to pass the variant, but since Android side support is not there yet. I used a byte array here, so we can make less changes to adopt base::ListValue when it is ready. But since the uprev may take a while, I can use union if you have strong opinion about this. Yeah, I think a union would be preferable here, along with a TODO to remind us to switch over once the uprev happens. Glad there's a nicer solution coming. https://codereview.chromium.org/2149713002/diff/80001/components/arc/bluetoot... File components/arc/bluetooth/bluetooth_type_converters.cc (right): https://codereview.chromium.org/2149713002/diff/80001/components/arc/bluetoot... components/arc/bluetooth/bluetooth_type_converters.cc:391: bluez::BluetoothServiceAttributeValueBlueZ attr_bluez = This and the if statement are the same in every case - it can still be hoisted out of the switch.
https://codereview.chromium.org/2149713002/diff/60001/components/arc/bluetoot... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2149713002/diff/60001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:1374: bluez::BluetoothDeviceBlueZ* device_bluez = On 2016/08/17 18:54:54, rickyz wrote: > On 2016/08/17 at 14:30:19, Miao wrote: > > On 2016/08/16 23:55:07, rickyz wrote: > > > Is it documented anywhere that GetDevice always returns a > BluetoothDeviceBlueZ? > > > Even if it's true now, this is very scary, and could become an issue if this > > > ever changes. > > > > > https://cs.chromium.org/chromium/src/device/bluetooth/bluez/bluetooth_device_... > > GetServiceRecords() is supported only on Chrome OS platform where the > corresponding supports in BlueZ are landed recently. Although it is scary, we > need to cast BluetoothDevice to BluetoothDeviceBlueZ in order to access that > function. > > I understand that this needs to be casted to access that function, but I'm > trying to find some assurance that the cast is always legal - that is, that > bluetooth_adapter_->GetDevice(...) always returns a BluetoothDeviceBlueZ*, and > not some other type that cannot be safely casted. I am unsure of what you're looking for exactly but I'll try. On Chrome OS, the only type that is ever going be assigned to a BluetoothDevice pointer is going to be a BluetoothDeviceBlueZ. BluetoothDevice itself is an abstract class and no other type that derives from it compiles on Chrome OS. Would that be enough assurance?
https://codereview.chromium.org/2149713002/diff/60001/components/arc/bluetoot... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2149713002/diff/60001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:1374: bluez::BluetoothDeviceBlueZ* device_bluez = On 2016/08/17 at 19:16:13, rkc wrote: > On 2016/08/17 18:54:54, rickyz wrote: > > On 2016/08/17 at 14:30:19, Miao wrote: > > > On 2016/08/16 23:55:07, rickyz wrote: > > > > Is it documented anywhere that GetDevice always returns a > > BluetoothDeviceBlueZ? > > > > Even if it's true now, this is very scary, and could become an issue if this > > > > ever changes. > > > > > > > > https://cs.chromium.org/chromium/src/device/bluetooth/bluez/bluetooth_device_... > > > GetServiceRecords() is supported only on Chrome OS platform where the > > corresponding supports in BlueZ are landed recently. Although it is scary, we > > need to cast BluetoothDevice to BluetoothDeviceBlueZ in order to access that > > function. > > > > I understand that this needs to be casted to access that function, but I'm > > trying to find some assurance that the cast is always legal - that is, that > > bluetooth_adapter_->GetDevice(...) always returns a BluetoothDeviceBlueZ*, and > > not some other type that cannot be safely casted. > > I am unsure of what you're looking for exactly but I'll try. On Chrome OS, the only type that is ever going be assigned to a BluetoothDevice pointer is going to be a BluetoothDeviceBlueZ. BluetoothDevice itself is an abstract class and no other type that derives from it compiles on Chrome OS. > > Would that be enough assurance? Sure, even better would be if this assumption could be documented/checked somewhere so that nobody accidentally violates it in the future. In this CL, this line probably also deserves a comment explaining why it's safe.
https://codereview.chromium.org/2149713002/diff/80001/components/arc/common/b... File components/arc/common/bluetooth.mojom (right): https://codereview.chromium.org/2149713002/diff/80001/components/arc/common/b... components/arc/common/bluetooth.mojom:409: [MinVersion=4] OnGetSdpRecords@16(BluetoothStatus status, You need to add function stub at components/arc/test/fake_bluetooth_instance.cc for arc_bluetooth_bridge unit test.
On 2016/08/17 17:25:04, Luis Héctor Chávez wrote: > On 2016/08/17 17:22:16, dcheng wrote: > > On 2016/08/17 14:54:12, Miao wrote: > > > On 2016/08/17 00:27:33, dcheng wrote: > > > > On 2016/08/17 00:26:52, dcheng wrote: > > > > > This should definitely by a type map rather than a type converter. So > > let's > > > do > > > > > that, rather than adding more type converters. > > > > > > > > lhchavez@, you mentioned Android is an issue, but why? Typemapping is for > > C++, > > > > and it should work for Android C++ already, no? > > > > > > Here is the context of Luis's comment. In bluetooth.mojom, struct > > > BluetoothSdpServiceAttr can hold either a variant(primitive type of data) or > a > > > sequence, and BluetoothSdpServiceAttr is the mojom version of > > > bluez::BluetoothServiceAttributeValueBlueZ which stores the variant as a > > > base::Value. The original approach is to make use of Mojo supports of > > > base::ListValue type mapping. However, the current Android Mojo version > > doesn't > > > support base::ListValue type mapping, so the type mapping only works between > > > Chrome and Mojo interface but not between mojo interface and Android. > > > > > > An alternitive is using union and list all possible types. Please refer to > > > Ricky's comment on > > > > > > https://codereview.chromium.org/2149713002/diff/60001/components/arc/common/b.... > > > > I don't think I understand the background context here, as I've never reviewed > > the Android side of the ARC++ mojo CLs. > > > > Are you saying that the Android-side is written in C++ and uses these type > > converters? > > Yes, but they currently don't compile there (I'm working on making that > possible). Update on this: base::DictionaryValue (or any type in https://cs.chromium.org/chromium/src/mojo/common/common_custom_types.mojom) can now be used in Android as well.
The CQ bit was checked by mcchou@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 checked by mcchou@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/2149713002/diff/80001/components/arc/bluetoot... File components/arc/bluetooth/bluetooth_type_converters.cc (right): https://codereview.chromium.org/2149713002/diff/80001/components/arc/bluetoot... components/arc/bluetooth/bluetooth_type_converters.cc:391: bluez::BluetoothServiceAttributeValueBlueZ attr_bluez = On 2016/08/17 18:54:55, rickyz wrote: > This and the if statement are the same in every case - it can still be hoisted > out of the switch. The changes to bluetooth_type_converter will be replaced by typemap. https://codereview.chromium.org/2149713002/diff/80001/components/arc/common/b... File components/arc/common/bluetooth.mojom (right): https://codereview.chromium.org/2149713002/diff/80001/components/arc/common/b... components/arc/common/bluetooth.mojom:409: [MinVersion=4] OnGetSdpRecords@16(BluetoothStatus status, On 2016/08/17 22:24:19, puthik_chromium wrote: > You need to add function stub at components/arc/test/fake_bluetooth_instance.cc > for arc_bluetooth_bridge unit test. Done.
On 2016/08/17 17:21:08, dcheng wrote: > On 2016/08/17 14:33:56, Miao wrote: > > On 2016/08/17 00:26:52, dcheng wrote: > > > This should definitely by a type map rather than a type converter. So let's > do > > > that, rather than adding more type converters. > > > > dcheng@, can we make the type map changes in the upcoming CL? Since we are > > aiming at landing the initial patch for SDP support by the end of this > > week(08/19). > > This isn't really a sustainable approach, there's always going to be more > features that we want to land. At the very least, we shouldn't be adding new > type converters. We don't have to typemap everything at once, but we can just > typemap the new things, no? Agree.:) Please see patch set 7 for the typemap changes.
On 2016/08/18 04:34:12, Luis Héctor Chávez wrote: > On 2016/08/17 17:25:04, Luis Héctor Chávez wrote: > > On 2016/08/17 17:22:16, dcheng wrote: > > > On 2016/08/17 14:54:12, Miao wrote: > > > > On 2016/08/17 00:27:33, dcheng wrote: > > > > > On 2016/08/17 00:26:52, dcheng wrote: > > > > > > This should definitely by a type map rather than a type converter. So > > > let's > > > > do > > > > > > that, rather than adding more type converters. > > > > > > > > > > lhchavez@, you mentioned Android is an issue, but why? Typemapping is > for > > > C++, > > > > > and it should work for Android C++ already, no? > > > > > > > > Here is the context of Luis's comment. In bluetooth.mojom, struct > > > > BluetoothSdpServiceAttr can hold either a variant(primitive type of data) > or > > a > > > > sequence, and BluetoothSdpServiceAttr is the mojom version of > > > > bluez::BluetoothServiceAttributeValueBlueZ which stores the variant as a > > > > base::Value. The original approach is to make use of Mojo supports of > > > > base::ListValue type mapping. However, the current Android Mojo version > > > doesn't > > > > support base::ListValue type mapping, so the type mapping only works > between > > > > Chrome and Mojo interface but not between mojo interface and Android. > > > > > > > > An alternitive is using union and list all possible types. Please refer to > > > > Ricky's comment on > > > > > > > > > > https://codereview.chromium.org/2149713002/diff/60001/components/arc/common/b.... > > > > > > I don't think I understand the background context here, as I've never > reviewed > > > the Android side of the ARC++ mojo CLs. > > > > > > Are you saying that the Android-side is written in C++ and uses these type > > > converters? > > > > Yes, but they currently don't compile there (I'm working on making that > > possible). > > Update on this: base::DictionaryValue (or any type in > https://cs.chromium.org/chromium/src/mojo/common/common_custom_types.mojom) can > now be used in Android as well. Thanks for doing the uprev! I am now working on Android changes.
The CQ bit was unchecked by commit-bot@chromium.org
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_...) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
The CQ bit was checked by mcchou@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/2149713002/diff/140001/components/arc/common/... File components/arc/common/bluetooth.mojom (right): https://codereview.chromium.org/2149713002/diff/140001/components/arc/common/... components/arc/common/bluetooth.mojom:264: // Next Method ID: 40
https://codereview.chromium.org/2149713002/diff/140001/components/arc/common/... File components/arc/common/bluetooth.mojom (right): https://codereview.chromium.org/2149713002/diff/140001/components/arc/common/... components/arc/common/bluetooth.mojom:264: On 2016/08/19 17:38:53, puthik_chromium wrote: > // Next Method ID: 40 Done.
The CQ bit was checked by mcchou@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...
rickyz@ PTAL about mojo change to avoid type recursion. https://codereview.chromium.org/2149713002/diff/60001/components/arc/bluetoot... File components/arc/bluetooth/bluetooth_type_converters.cc (right): https://codereview.chromium.org/2149713002/diff/60001/components/arc/bluetoot... components/arc/bluetooth/bluetooth_type_converters.cc:307: mojo::ConvertTo<bluez::BluetoothServiceAttributeValueBlueZ>( On 2016/08/17 18:54:54, rickyz wrote: > On 2016/08/17 at 14:30:20, Miao wrote: > > On 2016/08/16 23:55:07, rickyz wrote: > > > Can this lead to stack overflow via too much recursion? > > > > There will be at most two layer of sequences, so it's unlikely to cause stack > overflow. > > Sorry, I didn't give the full background here - from a security perspective, > what I'm worried about is if the Android side is compromised, and intentionally > sends a message with more than two layers in an attempt to exploit to exploit > Chrome. I propose the we should change the mojo to something like this to avoid the recurstion. rickyz@ What do you think. struct BluetoothSdpServiceAttr { BluetoothSdpAttrType type; uint32 type_size; mojo.common.mojom.ListValue value; BluetoothSdpServiceAttrSequence sequence; }; // Descriptor can be 2 level deeps. Other type is one level deep. union BluetoothSdpServiceAttrSequence { array<BluetoothSdpServiceAttrDescriptorValue> descriptor; array<BluetoothSdpServiceAttrValue> value; }; struct BluetoothSdpServiceAttrDescriptorValue { BluetoothSdpAttrType type; uint32 type_size; mojo.common.mojom.ListValue value; array<BluetoothSdpServiceAttrValue> sequence; }; struct BluetoothSdpServiceAttrValue { BluetoothSdpAttrType type; uint32 type_size; mojo.common.mojom.ListValue value; };
On 2016/08/19 21:21:56, puthik_chromium wrote: > rickyz@ PTAL about mojo change to avoid type recursion. > > https://codereview.chromium.org/2149713002/diff/60001/components/arc/bluetoot... > File components/arc/bluetooth/bluetooth_type_converters.cc (right): > > https://codereview.chromium.org/2149713002/diff/60001/components/arc/bluetoot... > components/arc/bluetooth/bluetooth_type_converters.cc:307: > mojo::ConvertTo<bluez::BluetoothServiceAttributeValueBlueZ>( > On 2016/08/17 18:54:54, rickyz wrote: > > On 2016/08/17 at 14:30:20, Miao wrote: > > > On 2016/08/16 23:55:07, rickyz wrote: > > > > Can this lead to stack overflow via too much recursion? > > > > > > There will be at most two layer of sequences, so it's unlikely to cause > stack > > overflow. > > > > Sorry, I didn't give the full background here - from a security perspective, > > what I'm worried about is if the Android side is compromised, and > intentionally > > sends a message with more than two layers in an attempt to exploit to exploit > > Chrome. > > I propose the we should change the mojo to something like this to avoid the > recurstion. rickyz@ What do you think. > > struct BluetoothSdpServiceAttr { > BluetoothSdpAttrType type; > uint32 type_size; > mojo.common.mojom.ListValue value; > BluetoothSdpServiceAttrSequence sequence; > }; > > // Descriptor can be 2 level deeps. Other type is one level deep. > union BluetoothSdpServiceAttrSequence { > array<BluetoothSdpServiceAttrDescriptorValue> descriptor; > array<BluetoothSdpServiceAttrValue> value; > }; > > struct BluetoothSdpServiceAttrDescriptorValue { > BluetoothSdpAttrType type; > uint32 type_size; > mojo.common.mojom.ListValue value; > array<BluetoothSdpServiceAttrValue> sequence; > }; > > struct BluetoothSdpServiceAttrValue { > BluetoothSdpAttrType type; > uint32 type_size; > mojo.common.mojom.ListValue value; > }; Note that original type is define here. https://cs.chromium.org/chromium/src/device/bluetooth/bluez/bluetooth_service... Example of this is in BLUETOOTH SPECIFICATION Version 4.2 Vol 3, Part B section 2.6.1 Example Service Browsing Hierarchy Page 1921-1922 of https://www.bluetooth.org/DocMan/handlers/DownloadDoc.ashx?doc_id=286439
On 2016/08/19 at 22:03:11, puthik wrote: > On 2016/08/19 21:21:56, puthik_chromium wrote: > > rickyz@ PTAL about mojo change to avoid type recursion. > > > > https://codereview.chromium.org/2149713002/diff/60001/components/arc/bluetoot... > > File components/arc/bluetooth/bluetooth_type_converters.cc (right): > > > > https://codereview.chromium.org/2149713002/diff/60001/components/arc/bluetoot... > > components/arc/bluetooth/bluetooth_type_converters.cc:307: > > mojo::ConvertTo<bluez::BluetoothServiceAttributeValueBlueZ>( > > On 2016/08/17 18:54:54, rickyz wrote: > > > On 2016/08/17 at 14:30:20, Miao wrote: > > > > On 2016/08/16 23:55:07, rickyz wrote: > > > > > Can this lead to stack overflow via too much recursion? > > > > > > > > There will be at most two layer of sequences, so it's unlikely to cause > > stack > > > overflow. > > > > > > Sorry, I didn't give the full background here - from a security perspective, > > > what I'm worried about is if the Android side is compromised, and > > intentionally > > > sends a message with more than two layers in an attempt to exploit to exploit > > > Chrome. > > > > I propose the we should change the mojo to something like this to avoid the > > recurstion. rickyz@ What do you think. > > > > struct BluetoothSdpServiceAttr { > > BluetoothSdpAttrType type; > > uint32 type_size; > > mojo.common.mojom.ListValue value; > > BluetoothSdpServiceAttrSequence sequence; > > }; > > > > // Descriptor can be 2 level deeps. Other type is one level deep. > > union BluetoothSdpServiceAttrSequence { > > array<BluetoothSdpServiceAttrDescriptorValue> descriptor; > > array<BluetoothSdpServiceAttrValue> value; > > }; > > > > struct BluetoothSdpServiceAttrDescriptorValue { > > BluetoothSdpAttrType type; > > uint32 type_size; > > mojo.common.mojom.ListValue value; > > array<BluetoothSdpServiceAttrValue> sequence; > > }; > > > > struct BluetoothSdpServiceAttrValue { > > BluetoothSdpAttrType type; > > uint32 type_size; > > mojo.common.mojom.ListValue value; > > }; > > Note that original type is define here. > https://cs.chromium.org/chromium/src/device/bluetooth/bluez/bluetooth_service... > > Example of this is in BLUETOOTH SPECIFICATION Version 4.2 Vol 3, Part B section 2.6.1 Example Service Browsing Hierarchy > Page 1921-1922 of https://www.bluetooth.org/DocMan/handlers/DownloadDoc.ashx?doc_id=286439 Ah, I didn't realize you needed two levels :-( From a security point of view, this addresses my concerns, but it's unfortunately pretty ugly coed-quality wise (also, descriptor is probably not the best name here, lest it be confused with terminology from the bluetooth standard). This feels like something that might need to be fixed in Mojo. I'm going to defer to any opinions/ideas that dcheng@ might have, but fine with doing this as a temp solution if there isn't any other great way.
dcheng@chromium.org changed reviewers: + yzshen@chromium.org
+yzshen I guess this will work for now, but we probably need something for base::ListValue / base::DictionaryValue in the future anyway. Any thoughts on how we can integrate this more gracefully? https://codereview.chromium.org/2149713002/diff/200001/components/arc/bluetoo... File components/arc/bluetooth/bluetooth_struct_traits.h (right): https://codereview.chromium.org/2149713002/diff/200001/components/arc/bluetoo... components/arc/bluetooth/bluetooth_struct_traits.h:86: return std::move(list_value); Nit: std::move() has no effect here. More problematically, this is actually called twice during serialization, so it's a bit inefficient: what's the reason for wrapping it in a ListValue here rather than just using a base::Value directly? https://codereview.chromium.org/2149713002/diff/200001/components/arc/bluetoo... components/arc/bluetooth/bluetooth_struct_traits.h:103: std::map<uint16_t, bluez::BluetoothServiceAttributeValueBlueZ> attributes; Similar comment here: if we really need to create an intermediate data structure, we can use the SetUpContext / TearDownContext feature from https://www.chromium.org/developers/design-documents/mojo/type-mapping#TOC-St... to avoid building this map twice for each serialization. https://codereview.chromium.org/2149713002/diff/200001/components/arc/bluetoo... components/arc/bluetooth/bluetooth_struct_traits.h:107: attributes[it] = std::move(bluez::BluetoothServiceAttributeValueBlueZ( Note that std::move() here doesn't have an effect, since the argument is already an rvalue (http://en.cppreference.com/w/cpp/language/value_category#rvalue).
On 2016/08/20 02:39:19, dcheng wrote: > +yzshen > > I guess this will work for now, but we probably need something for > base::ListValue / base::DictionaryValue in the future anyway. Any thoughts on > how we can integrate this more gracefully? I am not sure I have understood the question. We do have base::ListValue/base::DictionaryValue custom type support currently (defined in //src/mojo/common/common_custom_types.mojom). They are using IPC::ParamTraits to do the serialization/deserialization. It is possible to rewrite them to use mojo::StructTraits. Has that answered the question? > > https://codereview.chromium.org/2149713002/diff/200001/components/arc/bluetoo... > File components/arc/bluetooth/bluetooth_struct_traits.h (right): > > https://codereview.chromium.org/2149713002/diff/200001/components/arc/bluetoo... > components/arc/bluetooth/bluetooth_struct_traits.h:86: return > std::move(list_value); > Nit: std::move() has no effect here. > > More problematically, this is actually called twice during serialization, so > it's a bit inefficient: what's the reason for wrapping it in a ListValue here > rather than just using a base::Value directly? > > https://codereview.chromium.org/2149713002/diff/200001/components/arc/bluetoo... > components/arc/bluetooth/bluetooth_struct_traits.h:103: std::map<uint16_t, > bluez::BluetoothServiceAttributeValueBlueZ> attributes; > Similar comment here: if we really need to create an intermediate data > structure, we can use the SetUpContext / TearDownContext feature from > https://www.chromium.org/developers/design-documents/mojo/type-mapping#TOC-St... > to avoid building this map twice for each serialization. > > https://codereview.chromium.org/2149713002/diff/200001/components/arc/bluetoo... > components/arc/bluetooth/bluetooth_struct_traits.h:107: attributes[it] = > std::move(bluez::BluetoothServiceAttributeValueBlueZ( > Note that std::move() here doesn't have an effect, since the argument is already > an rvalue (http://en.cppreference.com/w/cpp/language/value_category#rvalue).
https://codereview.chromium.org/2149713002/diff/200001/components/arc/bluetoo... File components/arc/bluetooth/bluetooth_struct_traits.cc (right): https://codereview.chromium.org/2149713002/diff/200001/components/arc/bluetoo... components/arc/bluetooth/bluetooth_struct_traits.cc:104: std::map<uint16_t, bluez::BluetoothServiceAttributeValueBlueZ> attributes; Performance tip: because |attributes| is not directly used, you could instead peek into the undeserialized data and add the elements into |output|. mojo::MapDataView<uint16_t, arc::mojom::BluetoothSdpServiceAttrDataView> map_data_view; data.GetAttrsDataView(&map_data_view); for (size_t i = 0; i < map_data_view.size(); ++i) { bluez::BluetoothServiceAttributeValueBlueZ value; if (!map_data_view.values().Read(i, &value)) return false; output->AddRecordEntry(map_data_view.keys()[i], value); // Or std::move(value) if |value| supports move. } A simpler alternative is to change bluez::BluetoothServiceRecordBlueZ and add a method to accept a std::map as input. https://codereview.chromium.org/2149713002/diff/200001/components/arc/bluetoo... File components/arc/bluetooth/bluetooth_struct_traits.h (right): https://codereview.chromium.org/2149713002/diff/200001/components/arc/bluetoo... components/arc/bluetooth/bluetooth_struct_traits.h:86: return std::move(list_value); On 2016/08/20 02:39:19, dcheng wrote: > Nit: std::move() has no effect here. > > More problematically, this is actually called twice during serialization, so > it's a bit inefficient: what's the reason for wrapping it in a ListValue here > rather than just using a base::Value directly? Ah, I think that is because we don't have any IPC::ParamTraits support from converting to/from std::unique_ptr<base::Value>. We could write a new IPC::ParamTraits or mojo::StructTraits for that. +1 for dcheng's comment, this getter is called twice. So it would be nice if we can avoid deep copy like this.
Puthikorn and I have had a chat about whether (1) using a recursive mojom BluetoothSdpServiceAttr definition or (2) using multiple definitions to enforce that there are at most two layers of nodes. The latter seems a better solution to me in that: - It enforces the max layer restriction at the mojom definition level, which seems more clear and less likely to mess up. - We could type map multiple mojom defintions to the same custom type, so the user code won't have to deal with multiple definitions. It makes sense to me to introduce some max-depth restriction in the mojo validation logic to avoid stack overflow while validating recursive structures. I will file a feature request for that. Thanks!
On 2016/08/23 19:19:05, yzshen1 wrote: > Puthikorn and I have had a chat about whether (1) using a recursive mojom > BluetoothSdpServiceAttr definition or (2) using multiple definitions to enforce > that there are at most two layers of nodes. > > The latter seems a better solution to me in that: > - It enforces the max layer restriction at the mojom definition level, which > seems more clear and less likely to mess up. > - We could type map multiple mojom defintions to the same custom type, so the > user code won't have to deal with multiple definitions. > > It makes sense to me to introduce some max-depth restriction in the mojo > validation logic to avoid stack overflow while validating recursive structures. > I will file a feature request for that. FYI, feature request is https://bugs.chromium.org/p/chromium/issues/detail?id=640298
https://codereview.chromium.org/2149713002/diff/200001/components/arc/bluetoo... File components/arc/bluetooth/bluetooth_struct_traits.cc (right): https://codereview.chromium.org/2149713002/diff/200001/components/arc/bluetoo... components/arc/bluetooth/bluetooth_struct_traits.cc:90: *output = bluez::BluetoothServiceAttributeValueBlueZ(std::move(sequence)); When I test this. Chrome crash at this line.
https://codereview.chromium.org/2149713002/diff/200001/components/arc/bluetoo... File components/arc/bluetooth/bluetooth_struct_traits.cc (right): https://codereview.chromium.org/2149713002/diff/200001/components/arc/bluetoo... components/arc/bluetooth/bluetooth_struct_traits.cc:90: *output = bluez::BluetoothServiceAttributeValueBlueZ(std::move(sequence)); On 2016/08/24 00:32:20, puthik_chromium wrote: > When I test this. Chrome crash at this line. I think that is by null field. It should be revolved by marking value and sequence nullable. https://codereview.chromium.org/2149713002/diff/200001/components/arc/bluetoo... components/arc/bluetooth/bluetooth_struct_traits.cc:104: std::map<uint16_t, bluez::BluetoothServiceAttributeValueBlueZ> attributes; On 2016/08/22 23:42:14, yzshen1 wrote: > Performance tip: because |attributes| is not directly used, you could instead > peek into the undeserialized data and add the elements into |output|. > > mojo::MapDataView<uint16_t, arc::mojom::BluetoothSdpServiceAttrDataView> > map_data_view; > data.GetAttrsDataView(&map_data_view); > > for (size_t i = 0; i < map_data_view.size(); ++i) { > bluez::BluetoothServiceAttributeValueBlueZ value; > if (!map_data_view.values().Read(i, &value)) > return false; > output->AddRecordEntry(map_data_view.keys()[i], value); // Or > std::move(value) if |value| supports move. > } > > A simpler alternative is to change bluez::BluetoothServiceRecordBlueZ and add a > method to accept a std::map as input. Adding a setting function is not a preferable way, since bluez::BluetoothServiceRecordBlueZ is reflecting the service record stored in underlying Bluetooth database where the attributes in a record do not change in a batch manner. So I will use DataView instead.:) https://codereview.chromium.org/2149713002/diff/200001/components/arc/bluetoo... File components/arc/bluetooth/bluetooth_struct_traits.h (right): https://codereview.chromium.org/2149713002/diff/200001/components/arc/bluetoo... components/arc/bluetooth/bluetooth_struct_traits.h:86: return std::move(list_value); On 2016/08/22 23:42:14, yzshen1 wrote: > On 2016/08/20 02:39:19, dcheng wrote: > > Nit: std::move() has no effect here. > > > > More problematically, this is actually called twice during serialization, so > > it's a bit inefficient: what's the reason for wrapping it in a ListValue here > > rather than just using a base::Value directly? > > Ah, I think that is because we don't have any IPC::ParamTraits support from > converting to/from std::unique_ptr<base::Value>. > > We could write a new IPC::ParamTraits or mojo::StructTraits for that. > > +1 for dcheng's comment, this getter is called twice. So it would be nice if we > can avoid deep copy like this. > Added the SetUpContext / TearDownContext feature for deep copying. value.value()'s return type is const reference, and we need to give the ownership of a base::Value to the base::ListValue, so we still need CreateDeepCopy(). https://codereview.chromium.org/2149713002/diff/200001/components/arc/bluetoo... components/arc/bluetooth/bluetooth_struct_traits.h:103: std::map<uint16_t, bluez::BluetoothServiceAttributeValueBlueZ> attributes; On 2016/08/20 02:39:19, dcheng wrote: > Similar comment here: if we really need to create an intermediate data > structure, we can use the SetUpContext / TearDownContext feature from > https://www.chromium.org/developers/design-documents/mojo/type-mapping#TOC-St... > to avoid building this map twice for each serialization. Done. https://codereview.chromium.org/2149713002/diff/200001/components/arc/bluetoo... components/arc/bluetooth/bluetooth_struct_traits.h:107: attributes[it] = std::move(bluez::BluetoothServiceAttributeValueBlueZ( On 2016/08/20 02:39:19, dcheng wrote: > Note that std::move() here doesn't have an effect, since the argument is already > an rvalue (http://en.cppreference.com/w/cpp/language/value_category#rvalue). Done.
The CQ bit was checked by mcchou@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: 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_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2149713002/diff/240001/components/arc/bluetoo... File components/arc/bluetooth/bluetooth_struct_traits.h (right): https://codereview.chromium.org/2149713002/diff/240001/components/arc/bluetoo... components/arc/bluetooth/bluetooth_struct_traits.h:70: struct StructTraits<arc::mojom::BluetoothSdpAttributeLayer2, Add data view to template type here and else where arc::mojom::BluetoothSdpAttributeLayer2DataView
The CQ bit was checked by mcchou@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 checked by mcchou@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/2149713002/diff/280001/components/arc/bluetoo... File components/arc/bluetooth/bluetooth_struct_traits.cc (right): https://codereview.chromium.org/2149713002/diff/280001/components/arc/bluetoo... components/arc/bluetooth/bluetooth_struct_traits.cc:382: *output = bluez::BluetoothServiceAttributeValueBlueZ( It crashes at this line when tested with CTS Verifier "Insecure Gatt Server" test. Look like some thing wrong in bluez::BluetoothServiceAttributeValueBlueZ constructor. Stack dump at http://gpaste/5069381809733632
https://codereview.chromium.org/2149713002/diff/280001/components/arc/bluetoo... File components/arc/bluetooth/bluetooth_struct_traits.cc (right): https://codereview.chromium.org/2149713002/diff/280001/components/arc/bluetoo... components/arc/bluetooth/bluetooth_struct_traits.cc:382: *output = bluez::BluetoothServiceAttributeValueBlueZ( On 2016/08/26 01:05:35, puthik_chromium wrote: > It crashes at this line when tested with CTS Verifier "Insecure Gatt Server" > test. > > Look like some thing wrong in bluez::BluetoothServiceAttributeValueBlueZ > constructor. > Stack dump at http://gpaste/5069381809733632 > > > This should be resolved by https://codereview.chromium.org/2284713002.
The first set of comments... https://codereview.chromium.org/2149713002/diff/280001/components/arc/bluetoo... File components/arc/bluetooth/bluetooth_struct_traits.cc (right): https://codereview.chromium.org/2149713002/diff/280001/components/arc/bluetoo... components/arc/bluetooth/bluetooth_struct_traits.cc:32: return new bluez::BluetoothServiceAttributeValueBlueZ::Sequence( why do you need to make a copy of this? https://codereview.chromium.org/2149713002/diff/280001/components/arc/bluetoo... components/arc/bluetooth/bluetooth_struct_traits.cc:132: return value.type(); Please move this kind of short methods into the .h file. The compiler will be able to inline them so that improve performance, and sometimes even save binary size because we save the code of setting up a method call. https://codereview.chromium.org/2149713002/diff/280001/components/arc/bluetoo... components/arc/bluetooth/bluetooth_struct_traits.cc:235: sequence(const bluez::BluetoothServiceAttributeValueBlueZ& value, why not directly returning value.sequence()? https://codereview.chromium.org/2149713002/diff/280001/components/arc/bluetoo... components/arc/bluetooth/bluetooth_struct_traits.cc:283: sequence.value())); Use std::move(sequence.value()) to save a copy here. https://codereview.chromium.org/2149713002/diff/280001/components/arc/bluetoo... components/arc/bluetooth/bluetooth_struct_traits.cc:385: sequence.value())); Use std::move(sequence.value()) to save a copy here. https://codereview.chromium.org/2149713002/diff/280001/components/arc/bluetoo... components/arc/bluetooth/bluetooth_struct_traits.cc:398: std::map<uint16_t, bluez::BluetoothServiceAttributeValueBlueZ>* attributes = Can you make BluetoothServiceRecordBluz expose the internal std::map as const& and return it directly from attrs? That way you don't have to make this copy at all.
The copy constructor of BluetoothServiceAttributeValueBlueZ is wrong. That might explain the issue you have seen. On Aug 26, 2016 11:41 AM, <yzshen@chromium.org> wrote: The first set of comments... https://codereview.chromium.org/2149713002/diff/280001/ components/arc/bluetooth/bluetooth_struct_traits.cc File components/arc/bluetooth/bluetooth_struct_traits.cc (right): https://codereview.chromium.org/2149713002/diff/280001/ components/arc/bluetooth/bluetooth_struct_traits.cc#newcode32 components/arc/bluetooth/bluetooth_struct_traits.cc:32: return new bluez::BluetoothServiceAttributeValueBlueZ::Sequence( why do you need to make a copy of this? https://codereview.chromium.org/2149713002/diff/280001/ components/arc/bluetooth/bluetooth_struct_traits.cc#newcode132 components/arc/bluetooth/bluetooth_struct_traits.cc:132: return value.type(); Please move this kind of short methods into the .h file. The compiler will be able to inline them so that improve performance, and sometimes even save binary size because we save the code of setting up a method call. https://codereview.chromium.org/2149713002/diff/280001/ components/arc/bluetooth/bluetooth_struct_traits.cc#newcode235 components/arc/bluetooth/bluetooth_struct_traits.cc:235: sequence(const bluez::BluetoothServiceAttributeValueBlueZ& value, why not directly returning value.sequence()? https://codereview.chromium.org/2149713002/diff/280001/ components/arc/bluetooth/bluetooth_struct_traits.cc#newcode283 components/arc/bluetooth/bluetooth_struct_traits.cc:283: sequence.value())); Use std::move(sequence.value()) to save a copy here. https://codereview.chromium.org/2149713002/diff/280001/ components/arc/bluetooth/bluetooth_struct_traits.cc#newcode385 components/arc/bluetooth/bluetooth_struct_traits.cc:385: sequence.value())); Use std::move(sequence.value()) to save a copy here. https://codereview.chromium.org/2149713002/diff/280001/ components/arc/bluetooth/bluetooth_struct_traits.cc#newcode398 components/arc/bluetooth/bluetooth_struct_traits.cc:398: std::map<uint16_t, bluez::BluetoothServiceAttributeValueBlueZ>* attributes = Can you make BluetoothServiceRecordBluz expose the internal std::map as const& and return it directly from attrs? That way you don't have to make this copy at all. https://codereview.chromium.org/2149713002/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Sorry I meant the assignment operator On Aug 26, 2016 11:46 AM, Yuzhu Shen <yzshen@chromium.org> wrote: > The copy constructor of BluetoothServiceAttributeValueBlueZ is wrong. > That might explain the issue you have seen. > > On Aug 26, 2016 11:41 AM, <yzshen@chromium.org> wrote: > > The first set of comments... > > > > https://codereview.chromium.org/2149713002/diff/280001/compo > nents/arc/bluetooth/bluetooth_struct_traits.cc > File components/arc/bluetooth/bluetooth_struct_traits.cc (right): > > https://codereview.chromium.org/2149713002/diff/280001/compo > nents/arc/bluetooth/bluetooth_struct_traits.cc#newcode32 > components/arc/bluetooth/bluetooth_struct_traits.cc:32: return new > bluez::BluetoothServiceAttributeValueBlueZ::Sequence( > why do you need to make a copy of this? > > https://codereview.chromium.org/2149713002/diff/280001/compo > nents/arc/bluetooth/bluetooth_struct_traits.cc#newcode132 > components/arc/bluetooth/bluetooth_struct_traits.cc:132: return > value.type(); > Please move this kind of short methods into the .h file. The compiler > will be able to inline them so that improve performance, and sometimes > even save binary size because we save the code of setting up a method > call. > > https://codereview.chromium.org/2149713002/diff/280001/compo > nents/arc/bluetooth/bluetooth_struct_traits.cc#newcode235 > components/arc/bluetooth/bluetooth_struct_traits.cc:235: sequence(const > bluez::BluetoothServiceAttributeValueBlueZ& value, > why not directly returning value.sequence()? > > https://codereview.chromium.org/2149713002/diff/280001/compo > nents/arc/bluetooth/bluetooth_struct_traits.cc#newcode283 > components/arc/bluetooth/bluetooth_struct_traits.cc:283: > sequence.value())); > Use std::move(sequence.value()) to save a copy here. > > https://codereview.chromium.org/2149713002/diff/280001/compo > nents/arc/bluetooth/bluetooth_struct_traits.cc#newcode385 > components/arc/bluetooth/bluetooth_struct_traits.cc:385: > sequence.value())); > Use std::move(sequence.value()) to save a copy here. > > https://codereview.chromium.org/2149713002/diff/280001/compo > nents/arc/bluetooth/bluetooth_struct_traits.cc#newcode398 > components/arc/bluetooth/bluetooth_struct_traits.cc:398: > std::map<uint16_t, bluez::BluetoothServiceAttributeValueBlueZ>* > attributes = > Can you make BluetoothServiceRecordBluz expose the internal std::map as > const& and return it directly from attrs? That way you don't have to > make this copy at all. > > https://codereview.chromium.org/2149713002/ > > > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On Aug 26, 2016 11:46 AM, Yuzhu Shen <yzshen@chromium.org> wrote: > > The copy constructor of Sorry I meant the assignment operator. BluetoothServiceAttributeValueBlueZ is wrong. That might explain the issue you have seen. > > > On Aug 26, 2016 11:41 AM, <yzshen@chromium.org> wrote: >> >> The first set of comments... >> >> >> >> https://codereview.chromium.org/2149713002/diff/280001/components/arc/bluetoo... >> File components/arc/bluetooth/bluetooth_struct_traits.cc (right): >> >> https://codereview.chromium.org/2149713002/diff/280001/components/arc/bluetoo... >> components/arc/bluetooth/bluetooth_struct_traits.cc:32: return new >> bluez::BluetoothServiceAttributeValueBlueZ::Sequence( >> why do you need to make a copy of this? >> >> https://codereview.chromium.org/2149713002/diff/280001/components/arc/bluetoo... >> components/arc/bluetooth/bluetooth_struct_traits.cc:132: return >> value.type(); >> Please move this kind of short methods into the .h file. The compiler >> will be able to inline them so that improve performance, and sometimes >> even save binary size because we save the code of setting up a method >> call. >> >> https://codereview.chromium.org/2149713002/diff/280001/components/arc/bluetoo... >> components/arc/bluetooth/bluetooth_struct_traits.cc:235: sequence(const >> bluez::BluetoothServiceAttributeValueBlueZ& value, >> why not directly returning value.sequence()? >> >> https://codereview.chromium.org/2149713002/diff/280001/components/arc/bluetoo... >> components/arc/bluetooth/bluetooth_struct_traits.cc:283: >> sequence.value())); >> Use std::move(sequence.value()) to save a copy here. >> >> https://codereview.chromium.org/2149713002/diff/280001/components/arc/bluetoo... >> components/arc/bluetooth/bluetooth_struct_traits.cc:385: >> sequence.value())); >> Use std::move(sequence.value()) to save a copy here. >> >> https://codereview.chromium.org/2149713002/diff/280001/components/arc/bluetoo... >> components/arc/bluetooth/bluetooth_struct_traits.cc:398: >> std::map<uint16_t, bluez::BluetoothServiceAttributeValueBlueZ>* >> attributes = >> Can you make BluetoothServiceRecordBluz expose the internal std::map as >> const& and return it directly from attrs? That way you don't have to >> make this copy at all. >> >> https://codereview.chromium.org/2149713002/ > > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by mcchou@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...
In our last meeting, we agree on rewinding to the original approach where we define generic attribute structure and the type converter, and the type converter should has recursion depth check enforced. https://codereview.chromium.org/2284713002/ has landed. The following two patches are out to fix the crash caused by incorrect values included in sequences and incorrect construction of DBus message. https://codereview.chromium.org/2287023002/ https://codereview.chromium.org/2284183002/
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Just some nit. https://codereview.chromium.org/2149713002/diff/300001/components/arc/bluetoo... File components/arc/bluetooth/bluetooth_struct_traits.h (right): https://codereview.chromium.org/2149713002/diff/300001/components/arc/bluetoo... components/arc/bluetooth/bluetooth_struct_traits.h:8: #include <map> We didn't need all these #include except one in line 14 now. https://codereview.chromium.org/2149713002/diff/300001/components/arc/bluetoo... File components/arc/bluetooth/bluetooth_type_converters.cc (right): https://codereview.chromium.org/2149713002/diff/300001/components/arc/bluetoo... components/arc/bluetooth/bluetooth_type_converters.cc:181: case bluez::BluetoothServiceAttributeValueBlueZ::Type::BOOL: { nit: Remove brace. https://codereview.chromium.org/2149713002/diff/300001/components/arc/bluetoo... components/arc/bluetooth/bluetooth_type_converters.cc:192: for (auto& child : attr_bluez.sequence()) { const auto & https://codereview.chromium.org/2149713002/diff/300001/components/arc/bluetoo... components/arc/bluetooth/bluetooth_type_converters.cc:237: for (auto& child : attr->sequence) { const auto & https://codereview.chromium.org/2149713002/diff/300001/components/arc/common/... File components/arc/common/bluetooth.typemap (right): https://codereview.chromium.org/2149713002/diff/300001/components/arc/common/... components/arc/common/bluetooth.typemap:5: "//device/bluetooth/bluez/bluetooth_service_record_bluez.h", Remove this line.
https://codereview.chromium.org/2149713002/diff/300001/components/arc/bluetoo... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2149713002/diff/300001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:1476: std::vector<uint16_t> v = record.GetAttributeIds(); Wouldn't it be better to extend bluez::BluetoothServiceRecordBlueZ to support checking for the presence of an attribute? https://codereview.chromium.org/2149713002/diff/300001/components/arc/bluetoo... File components/arc/bluetooth/bluetooth_type_converters.cc (right): https://codereview.chromium.org/2149713002/diff/300001/components/arc/bluetoo... components/arc/bluetooth/bluetooth_type_converters.cc:191: result->type_size = result->sequence.size(); Is this correct? This will always be zero. This code should have unit tests which verify it produces the correct results for various inputs. https://codereview.chromium.org/2149713002/diff/300001/components/arc/bluetoo... components/arc/bluetooth/bluetooth_type_converters.cc:209: std::unique_ptr<base::Value> value; not used https://codereview.chromium.org/2149713002/diff/300001/components/arc/bluetoo... components/arc/bluetooth/bluetooth_type_converters.cc:214: break; break is not needed after return here and below. https://codereview.chromium.org/2149713002/diff/300001/components/arc/bluetoo... components/arc/bluetooth/bluetooth_type_converters.cc:222: attr->value->Remove(0, &value); If the ListValue has 0 elements, this will construct a BluetoothServiceAttributeValueBlueZ with a null value, which may crash later. This code should have unit tests which try to convert various invalid mojo structs.
The CQ bit was checked by mcchou@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/2149713002/diff/300001/components/arc/bluetoo... File components/arc/bluetooth/bluetooth_struct_traits.h (right): https://codereview.chromium.org/2149713002/diff/300001/components/arc/bluetoo... components/arc/bluetooth/bluetooth_struct_traits.h:8: #include <map> On 2016/08/28 00:05:39, puthik_chromium wrote: > We didn't need all these #include except one in line 14 now. Done. https://codereview.chromium.org/2149713002/diff/300001/components/arc/bluetoo... File components/arc/bluetooth/bluetooth_type_converters.cc (right): https://codereview.chromium.org/2149713002/diff/300001/components/arc/bluetoo... components/arc/bluetooth/bluetooth_type_converters.cc:181: case bluez::BluetoothServiceAttributeValueBlueZ::Type::BOOL: { On 2016/08/28 00:05:39, puthik_chromium wrote: > nit: Remove brace. Done. https://codereview.chromium.org/2149713002/diff/300001/components/arc/bluetoo... components/arc/bluetooth/bluetooth_type_converters.cc:191: result->type_size = result->sequence.size(); On 2016/08/29 00:48:06, rickyz wrote: > Is this correct? This will always be zero. This code should have unit tests > which verify it produces the correct results for various inputs. Done. https://codereview.chromium.org/2149713002/diff/300001/components/arc/bluetoo... components/arc/bluetooth/bluetooth_type_converters.cc:192: for (auto& child : attr_bluez.sequence()) { On 2016/08/28 00:05:39, puthik_chromium wrote: > const auto & Done. https://codereview.chromium.org/2149713002/diff/300001/components/arc/bluetoo... components/arc/bluetooth/bluetooth_type_converters.cc:209: std::unique_ptr<base::Value> value; On 2016/08/29 00:48:06, rickyz wrote: > not used Done. https://codereview.chromium.org/2149713002/diff/300001/components/arc/bluetoo... components/arc/bluetooth/bluetooth_type_converters.cc:214: break; On 2016/08/29 00:48:06, rickyz wrote: > break is not needed after return here and below. Done. https://codereview.chromium.org/2149713002/diff/300001/components/arc/bluetoo... components/arc/bluetooth/bluetooth_type_converters.cc:222: attr->value->Remove(0, &value); On 2016/08/29 00:48:06, rickyz wrote: > If the ListValue has 0 elements, this will construct a > BluetoothServiceAttributeValueBlueZ with a null value, which may crash later. > This code should have unit tests which try to convert various invalid mojo > structs. Done. https://codereview.chromium.org/2149713002/diff/300001/components/arc/bluetoo... components/arc/bluetooth/bluetooth_type_converters.cc:237: for (auto& child : attr->sequence) { On 2016/08/28 00:05:39, puthik_chromium wrote: > const auto & Done. https://codereview.chromium.org/2149713002/diff/300001/components/arc/common/... File components/arc/common/bluetooth.typemap (right): https://codereview.chromium.org/2149713002/diff/300001/components/arc/common/... components/arc/common/bluetooth.typemap:5: "//device/bluetooth/bluez/bluetooth_service_record_bluez.h", On 2016/08/28 00:05:39, puthik_chromium wrote: > Remove this line. Done.
The CQ bit was checked by mcchou@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/2149713002/diff/300001/components/arc/bluetoo... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2149713002/diff/300001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:1476: std::vector<uint16_t> v = record.GetAttributeIds(); On 2016/08/29 00:48:06, rickyz wrote: > Wouldn't it be better to extend bluez::BluetoothServiceRecordBlueZ to support > checking for the presence of an attribute? Please see https://codereview.chromium.org/2302963002/.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: 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-...) linux_chromium_chromeos_compile_dbg_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 mcchou@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.
I am a little sad about adding back type converters. But LGTM if dcheng is okay with this. https://codereview.chromium.org/2149713002/diff/360001/components/arc/BUILD.gn File components/arc/BUILD.gn (right): https://codereview.chromium.org/2149713002/diff/360001/components/arc/BUILD.g... components/arc/BUILD.gn:166: use_new_wrapper_types = false Ideally, you should add your new mojom definition in a new mojom target without this config. That way you will use std::vector/std::string instead of mojo::Array/mojo::String. Eventually we would like to remove mojo::Array/mojo::String. At least, could you add a TODO and file bug to remove this? Thanks!
lgtm with a few minor nits - thanks for adding the tests! https://codereview.chromium.org/2149713002/diff/340001/components/arc/bluetoo... File components/arc/bluetooth/bluetooth_type_converters.cc (right): https://codereview.chromium.org/2149713002/diff/340001/components/arc/bluetoo... components/arc/bluetooth/bluetooth_type_converters.cc:182: result->value.Append(attr_bluez.value().DeepCopy()); According to https://cs.chromium.org/chromium/src/base/values.h?sq=package:chromium&dr=CSs..., CreateDeepCopy is preferred over DeepCopy. https://codereview.chromium.org/2149713002/diff/340001/components/arc/bluetoo... components/arc/bluetooth/bluetooth_type_converters.cc:199: result->value.Clear(); Is this needed? https://codereview.chromium.org/2149713002/diff/340001/components/arc/bluetoo... components/arc/bluetooth/bluetooth_type_converters.cc:235: break; No need to break after return here and below. https://codereview.chromium.org/2149713002/diff/360001/components/arc/bluetoo... File components/arc/bluetooth/bluetooth_type_converters.cc (right): https://codereview.chromium.org/2149713002/diff/360001/components/arc/bluetoo... components/arc/bluetooth/bluetooth_type_converters.cc:182: result->value.Append(attr_bluez.value().DeepCopy()); https://cs.chromium.org/chromium/src/base/values.h?q=DeepCopy&sq=package:chro..., CreateDeepCopy is preferred now. https://codereview.chromium.org/2149713002/diff/360001/components/arc/bluetoo... components/arc/bluetooth/bluetooth_type_converters.cc:235: break; No need to break after return here and below. https://codereview.chromium.org/2149713002/diff/360001/components/arc/bluetoo... File components/arc/bluetooth/bluetooth_type_converters_unittest.cc (right): https://codereview.chromium.org/2149713002/diff/360001/components/arc/bluetoo... components/arc/bluetooth/bluetooth_type_converters_unittest.cc:51: int CountDepthOfBlueZAttribute( I think the normal way to write this is without the extra parameter: int get_depth(obj) { int depth = 1; for (child : obj.children) { depth = max(depth, 1 + get_depth(child)); } return depth; } https://codereview.chromium.org/2149713002/diff/360001/components/arc/bluetoo... components/arc/bluetooth/bluetooth_type_converters_unittest.cc:163: EXPECT_EQ((size_t)0, nulltypeAttributeBlueZ.size()); nit: Would prefer 0u or size_t(0) here and elsewhere.
https://codereview.chromium.org/2149713002/diff/360001/components/arc/BUILD.gn File components/arc/BUILD.gn (right): https://codereview.chromium.org/2149713002/diff/360001/components/arc/BUILD.g... components/arc/BUILD.gn:166: use_new_wrapper_types = false On 2016/09/01 23:34:11, yzshen1 wrote: > Ideally, you should add your new mojom definition in a new mojom target without > this config. That way you will use std::vector/std::string instead of > mojo::Array/mojo::String. Eventually we would like to remove > mojo::Array/mojo::String. > > At least, could you add a TODO and file bug to remove this? Thanks! I didn't change the wrapper flag in this code, and it would be a bad idea for me to change this in this patch, since it's going to affect other .mojom files. When I look it up, 4e4d53c69 is the original commit for this line. https://codereview.chromium.org/2149713002/diff/360001/components/arc/bluetoo... File components/arc/bluetooth/bluetooth_type_converters.cc (right): https://codereview.chromium.org/2149713002/diff/360001/components/arc/bluetoo... components/arc/bluetooth/bluetooth_type_converters.cc:182: result->value.Append(attr_bluez.value().DeepCopy()); On 2016/09/02 02:25:20, rickyz wrote: > https://cs.chromium.org/chromium/src/base/values.h?q=DeepCopy&sq=package:chro..., > CreateDeepCopy is preferred now. Done. https://codereview.chromium.org/2149713002/diff/360001/components/arc/bluetoo... components/arc/bluetooth/bluetooth_type_converters.cc:235: break; On 2016/09/02 02:25:20, rickyz wrote: > No need to break after return here and below. Done. https://codereview.chromium.org/2149713002/diff/360001/components/arc/bluetoo... File components/arc/bluetooth/bluetooth_type_converters_unittest.cc (right): https://codereview.chromium.org/2149713002/diff/360001/components/arc/bluetoo... components/arc/bluetooth/bluetooth_type_converters_unittest.cc:51: int CountDepthOfBlueZAttribute( On 2016/09/02 02:25:20, rickyz wrote: > I think the normal way to write this is without the extra parameter: > > int get_depth(obj) { > int depth = 1; > for (child : obj.children) { > depth = max(depth, 1 + get_depth(child)); > } > > return depth; > } Done. https://codereview.chromium.org/2149713002/diff/360001/components/arc/bluetoo... components/arc/bluetooth/bluetooth_type_converters_unittest.cc:163: EXPECT_EQ((size_t)0, nulltypeAttributeBlueZ.size()); On 2016/09/02 02:25:20, rickyz wrote: > nit: Would prefer 0u or size_t(0) here and elsewhere. Done.
The CQ bit was checked by mcchou@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: 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_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by mcchou@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: 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...)
https://codereview.chromium.org/2149713002/diff/360001/components/arc/BUILD.gn File components/arc/BUILD.gn (right): https://codereview.chromium.org/2149713002/diff/360001/components/arc/BUILD.g... components/arc/BUILD.gn:166: use_new_wrapper_types = false On 2016/09/02 18:10:07, Miao wrote: > On 2016/09/01 23:34:11, yzshen1 wrote: > > Ideally, you should add your new mojom definition in a new mojom target > without > > this config. That way you will use std::vector/std::string instead of > > mojo::Array/mojo::String. Eventually we would like to remove > > mojo::Array/mojo::String. > > > > At least, could you add a TODO and file bug to remove this? Thanks! > > I didn't change the wrapper flag in this code, and it would be a bad idea for me > to change this in this patch, since it's going to affect other .mojom files. I was saying you could add your new mojom definitions to a new mojom file, and make it a separate mojom target. (And mojom targets using different wrapper types can depend on each other.) > When I look it up, 4e4d53c69 is the original commit for this line. I know. I added this to preserve the old behavior during the transition. But I hope we don't add new mojom definitions under this old flag if possible. I asked for code owners' help to remove this flag when they get a chance. ;)
https://codereview.chromium.org/2149713002/diff/400001/components/arc/bluetoo... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2149713002/diff/400001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:221: bluez::BluetoothServiceRecordBlueZ::ErrorCode::ERROR_ADAPTER_NOT_READY) nit: you cannot elide the braces when the if statement spans more than one line. Same in L237. Can you run git clang-format? https://codereview.chromium.org/2149713002/diff/400001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:1958: arc_bridge_service()->bluetooth()->instance()->OnGetSdpRecords( this needs the HasBluetoothInstance() check, as well as the version check for consistency. https://codereview.chromium.org/2149713002/diff/400001/components/arc/bluetoo... File components/arc/bluetooth/bluetooth_type_converters.cc (right): https://codereview.chromium.org/2149713002/diff/400001/components/arc/bluetoo... components/arc/bluetooth/bluetooth_type_converters.cc:166: int layer) { I couldn't find any mention of the term 'layer' in the bluetooth spec (at least in the part you linked). Can you change 'layer' to something more descriptive like 'depth'? Same throughout the rest of the change. https://codereview.chromium.org/2149713002/diff/400001/components/arc/bluetoo... File components/arc/bluetooth/bluetooth_type_converters.h (right): https://codereview.chromium.org/2149713002/diff/400001/components/arc/bluetoo... components/arc/bluetooth/bluetooth_type_converters.h:25: constexpr int BLUETOOTH_SDP_MAX_LAYER = 32; kBluetoothSDPMaxDepth, in the arc (or arc::mojom?) namespace. Also please explain what this is and why it was introduced. Does this deviate from the Bluetooth standard? (I couldn't find any limits there). https://codereview.chromium.org/2149713002/diff/400001/components/arc/bluetoo... File components/arc/bluetooth/bluetooth_type_converters_unittest.cc (right): https://codereview.chromium.org/2149713002/diff/400001/components/arc/bluetoo... components/arc/bluetooth/bluetooth_type_converters_unittest.cc:33: int layer) { Can you make this such that this parameter indicates the number of levels deep that you want to create? That way you don't need to encode the constant in L36, you just check against zero. https://codereview.chromium.org/2149713002/diff/400001/components/arc/bluetoo... components/arc/bluetooth/bluetooth_type_converters_unittest.cc:51: int CountDepthOfBlueZAttribute( Can you swap the order of CountDepthOfMojoAttribute and this function so that the Mojo functions are together? Also, can you rename them to "GetDepthOfXxxAttribute"? https://codereview.chromium.org/2149713002/diff/400001/components/arc/bluetoo... components/arc/bluetooth/bluetooth_type_converters_unittest.cc:63: int layer) { Same as L33. https://codereview.chromium.org/2149713002/diff/400001/components/arc/bluetoo... components/arc/bluetooth/bluetooth_type_converters_unittest.cc:354: // Create a Mojo attriubte with the depth = BLUETOOTH_SDP_MAX_LAYER + 3. nit: s/attriubte/attribute/ https://codereview.chromium.org/2149713002/diff/400001/components/arc/common/... File components/arc/common/bluetooth.mojom (right): https://codereview.chromium.org/2149713002/diff/400001/components/arc/common/... components/arc/common/bluetooth.mojom:9: import "mojo/common/common_custom_types.mojom"; Is this still needed? :( https://codereview.chromium.org/2149713002/diff/400001/components/arc/common/... components/arc/common/bluetooth.mojom:252: * design is intended. The user of this structure should pay extra attention to "should pay extra attention" how? Just checking that the depth never exceeds 32?
btw, if you want to merge this into M53, you want to have a crbug bug in the description in addition to the b/ one. That'll make merging easier.
On 2016/09/06 17:41:32, yzshen1 wrote: > https://codereview.chromium.org/2149713002/diff/360001/components/arc/BUILD.gn > File components/arc/BUILD.gn (right): > > https://codereview.chromium.org/2149713002/diff/360001/components/arc/BUILD.g... > components/arc/BUILD.gn:166: use_new_wrapper_types = false > On 2016/09/02 18:10:07, Miao wrote: > > On 2016/09/01 23:34:11, yzshen1 wrote: > > > Ideally, you should add your new mojom definition in a new mojom target > > without > > > this config. That way you will use std::vector/std::string instead of > > > mojo::Array/mojo::String. Eventually we would like to remove > > > mojo::Array/mojo::String. > > > > > > At least, could you add a TODO and file bug to remove this? Thanks! > > > > I didn't change the wrapper flag in this code, and it would be a bad idea for > me > > to change this in this patch, since it's going to affect other .mojom files. > > I was saying you could add your new mojom definitions to a new mojom file, and > make it a separate mojom target. (And mojom targets using different wrapper > types can depend on each other.) > > > When I look it up, 4e4d53c69 is the original commit for this line. > > I know. I added this to preserve the old behavior during the transition. But I > hope we don't add new mojom definitions under this old flag if possible. I asked > for code owners' help to remove this flag when they get a chance. ;) Since we plan to merge this back to M53, changing the flag may cause trouble, so let's try to do this in the following CL but not this one.:)
On 2016/09/06 17:41:32, yzshen1 wrote: > https://codereview.chromium.org/2149713002/diff/360001/components/arc/BUILD.gn > File components/arc/BUILD.gn (right): > > https://codereview.chromium.org/2149713002/diff/360001/components/arc/BUILD.g... > components/arc/BUILD.gn:166: use_new_wrapper_types = false > On 2016/09/02 18:10:07, Miao wrote: > > On 2016/09/01 23:34:11, yzshen1 wrote: > > > Ideally, you should add your new mojom definition in a new mojom target > > without > > > this config. That way you will use std::vector/std::string instead of > > > mojo::Array/mojo::String. Eventually we would like to remove > > > mojo::Array/mojo::String. > > > > > > At least, could you add a TODO and file bug to remove this? Thanks! > > > > I didn't change the wrapper flag in this code, and it would be a bad idea for > me > > to change this in this patch, since it's going to affect other .mojom files. > > I was saying you could add your new mojom definitions to a new mojom file, and > make it a separate mojom target. (And mojom targets using different wrapper > types can depend on each other.) > > > When I look it up, 4e4d53c69 is the original commit for this line. > > I know. I added this to preserve the old behavior during the transition. But I > hope we don't add new mojom definitions under this old flag if possible. I asked > for code owners' help to remove this flag when they get a chance. ;) Since we plan to merge this back to M53, changing the flag may cause trouble, so let's try to do this in the following CL but not this one.:)
https://codereview.chromium.org/2149713002/diff/400001/components/arc/bluetoo... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2149713002/diff/400001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:221: bluez::BluetoothServiceRecordBlueZ::ErrorCode::ERROR_ADAPTER_NOT_READY) On 2016/09/06 17:42:29, Luis Héctor Chávez wrote: > nit: you cannot elide the braces when the if statement spans more than one line. > Same in L237. Can you run git clang-format? Done. https://codereview.chromium.org/2149713002/diff/400001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:1958: arc_bridge_service()->bluetooth()->instance()->OnGetSdpRecords( On 2016/09/06 17:42:29, Luis Héctor Chávez wrote: > this needs the HasBluetoothInstance() check, as well as the version check for > consistency. Done. https://codereview.chromium.org/2149713002/diff/400001/components/arc/bluetoo... File components/arc/bluetooth/bluetooth_type_converters.cc (right): https://codereview.chromium.org/2149713002/diff/400001/components/arc/bluetoo... components/arc/bluetooth/bluetooth_type_converters.cc:166: int layer) { On 2016/09/06 17:42:29, Luis Héctor Chávez wrote: > I couldn't find any mention of the term 'layer' in the bluetooth spec (at least > in the part you linked). Can you change 'layer' to something more descriptive > like 'depth'? Same throughout the rest of the change. Done. https://codereview.chromium.org/2149713002/diff/400001/components/arc/bluetoo... File components/arc/bluetooth/bluetooth_type_converters.h (right): https://codereview.chromium.org/2149713002/diff/400001/components/arc/bluetoo... components/arc/bluetooth/bluetooth_type_converters.h:25: constexpr int BLUETOOTH_SDP_MAX_LAYER = 32; On 2016/09/06 17:42:29, Luis Héctor Chávez wrote: > kBluetoothSDPMaxDepth, in the arc (or arc::mojom?) namespace. > > Also please explain what this is and why it was introduced. Does this deviate > from the Bluetooth standard? (I couldn't find any limits there). Moved kBluetoothSDPMaxDepth to arc namespace. There is no such limit defined for SDP attribute in the Bluetooth core spec. However, we need to define a maximum depth allowed for the SDP sequence conversion, and this is something that we made up. https://codereview.chromium.org/2149713002/diff/400001/components/arc/bluetoo... File components/arc/bluetooth/bluetooth_type_converters_unittest.cc (right): https://codereview.chromium.org/2149713002/diff/400001/components/arc/bluetoo... components/arc/bluetooth/bluetooth_type_converters_unittest.cc:33: int layer) { On 2016/09/06 17:42:29, Luis Héctor Chávez wrote: > Can you make this such that this parameter indicates the number of levels deep > that you want to create? That way you don't need to encode the constant in L36, > you just check against zero. Done. https://codereview.chromium.org/2149713002/diff/400001/components/arc/bluetoo... components/arc/bluetooth/bluetooth_type_converters_unittest.cc:51: int CountDepthOfBlueZAttribute( On 2016/09/06 17:42:29, Luis Héctor Chávez wrote: > Can you swap the order of CountDepthOfMojoAttribute and this function so that > the Mojo functions are together? Also, can you rename them to > "GetDepthOfXxxAttribute"? Done. https://codereview.chromium.org/2149713002/diff/400001/components/arc/bluetoo... components/arc/bluetooth/bluetooth_type_converters_unittest.cc:63: int layer) { On 2016/09/06 17:42:29, Luis Héctor Chávez wrote: > Same as L33. Done. https://codereview.chromium.org/2149713002/diff/400001/components/arc/bluetoo... components/arc/bluetooth/bluetooth_type_converters_unittest.cc:354: // Create a Mojo attriubte with the depth = BLUETOOTH_SDP_MAX_LAYER + 3. On 2016/09/06 17:42:30, Luis Héctor Chávez wrote: > nit: s/attriubte/attribute/ Done. https://codereview.chromium.org/2149713002/diff/400001/components/arc/common/... File components/arc/common/bluetooth.mojom (right): https://codereview.chromium.org/2149713002/diff/400001/components/arc/common/... components/arc/common/bluetooth.mojom:9: import "mojo/common/common_custom_types.mojom"; On 2016/09/06 17:42:30, Luis Héctor Chávez wrote: > Is this still needed? :( Yes, although we are not using type map, we still need this for base::ListValue. https://codereview.chromium.org/2149713002/diff/400001/components/arc/common/... components/arc/common/bluetooth.mojom:252: * design is intended. The user of this structure should pay extra attention to On 2016/09/06 17:42:30, Luis Héctor Chávez wrote: > "should pay extra attention" how? Just checking that the depth never exceeds 32? Replaced that sentence with "Note BluetoothSdpAttribute supports up to depth 32 for the attribute with multi-layer sequences. The attributes within a sequence which has the depth beyond the maximum supported depth will be invalidated and ignored." The callers should be aware of the fact that we only support up to the depth 32 when they construct a BluetoothSdpAttribute.
The CQ bit was checked by mcchou@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...
lgtm with one more nit, but I still want to hear back from dcheng. https://codereview.chromium.org/2149713002/diff/420001/components/arc/bluetoo... File components/arc/bluetooth/bluetooth_type_converters.h (right): https://codereview.chromium.org/2149713002/diff/420001/components/arc/bluetoo... components/arc/bluetooth/bluetooth_type_converters.h:24: constexpr int kBluetoothSDPMaxDepth = 32; Sorry if I wasn't clear. By "explain" I meant "please add a comment as to why you are adding this, for future readers". Also, after your refactor in the test code, I realize this can be a size_t now, instead of an int.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2149713002/diff/420001/components/arc/bluetoo... File components/arc/bluetooth/bluetooth_type_converters.h (right): https://codereview.chromium.org/2149713002/diff/420001/components/arc/bluetoo... components/arc/bluetooth/bluetooth_type_converters.h:24: constexpr int kBluetoothSDPMaxDepth = 32; On 2016/09/07 18:04:16, Luis Héctor Chávez wrote: > Sorry if I wasn't clear. By "explain" I meant "please add a comment as to why > you are adding this, for future readers". > > Also, after your refactor in the test code, I realize this can be a size_t now, > instead of an int. Done.
rickyz already l-g-t-med, and he is a mojo reviewer, so his review is sufficient =) I'm happy to look again though, if there's a specific question you have.
The CQ bit was checked by mcchou@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 checked by mcchou@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rkc@chromium.org, rickyz@chromium.org, yzshen@chromium.org, lhchavez@chromium.org Link to the patchset: https://codereview.chromium.org/2149713002/#ps440001 (title: "arc: bluetooth: Add SDP host side support")
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
Try jobs failed on following builders: 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-...)
https://codereview.chromium.org/2149713002/diff/420001/components/arc/bluetoo... File components/arc/bluetooth/bluetooth_type_converters.h (right): https://codereview.chromium.org/2149713002/diff/420001/components/arc/bluetoo... components/arc/bluetooth/bluetooth_type_converters.h:24: constexpr int kBluetoothSDPMaxDepth = 32; On 2016/09/07 22:25:47, Miao wrote: > On 2016/09/07 18:04:16, Luis Héctor Chávez wrote: > > Sorry if I wasn't clear. By "explain" I meant "please add a comment as to why > > you are adding this, for future readers". > > > > Also, after your refactor in the test code, I realize this can be a size_t > now, > > instead of an int. > > Done. re: build failure, you also need to make all the "depth" parameters size_t, otherwise you'll run into signed/unsigned comparisons.
The CQ bit was checked by mcchou@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: 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_...) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) linux_chromium_chromeos_ozone_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 mcchou@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: 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...)
The CQ bit was checked by mcchou@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rkc@chromium.org, lhchavez@chromium.org, rickyz@chromium.org, yzshen@chromium.org Link to the patchset: https://codereview.chromium.org/2149713002/#ps480001 (title: "arc: bluetooth: Add SDP host side support")
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.
Committed patchset #25 (id:480001)
Message was sent while issue was closed.
Description was changed from ========== arc: bluetooth: Add SDP host side support This implements the Chrome side support for Service Discovery Protocol. - Retrieve SDP records from a remote device - Create/remove a SDP record for the local device - Convert service record/attribute between Chrome and Mojo BUG=b:29314637 TEST=None (The manual test is blocked on chromium:627907.) ========== to ========== arc: bluetooth: Add SDP host side support This implements the Chrome side support for Service Discovery Protocol. - Retrieve SDP records from a remote device - Create/remove a SDP record for the local device - Convert service record/attribute between Chrome and Mojo BUG=b:29314637 TEST=None (The manual test is blocked on chromium:627907.) Committed: https://crrev.com/702552c9b1fb144ae942387b22a772ec545c1008 Cr-Commit-Position: refs/heads/master@{#417210} ==========
Message was sent while issue was closed.
Patchset 25 (id:??) landed as https://crrev.com/702552c9b1fb144ae942387b22a772ec545c1008 Cr-Commit-Position: refs/heads/master@{#417210} |
