Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2369423003:
bluetooth: Expose service data from BlueZ (Closed)
|
Created:
4 years, 2 months ago by puthik_chromium Modified:
4 years, 2 months ago CC:
chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionbluetooth: Expose service data from BlueZ
BlueZ exposed Bluetooth device's ServiceData as a{sv} property[1]
where the dict value variant is an array of byte.
This CL exposes that to upper layer by
- Add support to map<string, vector<uint8_t>> in dbus::Property
- Add new service_data property in BluetoothDeviceClient
- Implement GetServiceDataUUIDs() and GetServiceDataForUUID()
in BluetoothDeviceBlueZ
- Fix misc style issues in original code to make linter happy
[1] http://git.kernel.org/cgit/bluetooth/bluez.git/tree/src/device.c#n2551
BUG=618442, 653310, b:28670943
TEST=Manually tested.
Committed: https://crrev.com/0dd82b1c975662a10f34a53b3df56d98526b36d0
Cr-Commit-Position: refs/heads/master@{#423434}
Patch Set 1 #Patch Set 2 : Change scope to not cover just dbus property #
Total comments: 9
Patch Set 3 : Use property changed approach / Add unit test #Patch Set 4 : fix typo #
Total comments: 23
Patch Set 5 : Address ortuno comment (Better test / more comment) #Patch Set 6 : More comment #
Total comments: 10
Patch Set 7 : Fix comment #
Messages
Total messages: 39 (17 generated)
puthik@chromium.org changed reviewers: + hashimoto@chromium.org, steel@chromium.org
Description was changed from
==========
dbus: Add support to map<string, vector<uint8_t>> in dbus::Property
BlueZ exposed Bluetooth device's ServiceData as a{sv} property[1]
where the dict value varient is an array of byte.
The existing PopValueFromReader() and AppendSetValueToWriter() in
dbus::Property do not support this type yet. This CL adds support
for this type
This CL also adds #include <memory> to make linter happy.
BUG=618442,b:28670943
TEST=manually tested
==========
to
==========
dbus: Add support to map<string, vector<uint8_t>> in dbus::Property
BlueZ exposed Bluetooth device's ServiceData as a{sv} property[1]
where the dict value varient is an array of byte.
The existing PopValueFromReader() and AppendSetValueToWriter() in
dbus::Property do not support this type yet. This CL adds support
for this type
This CL also adds #include <memory> to make linter happy.
[1] http://git.kernel.org/cgit/bluetooth/bluez.git/tree/src/device.c#n2551
BUG=618442,b:28670943
TEST=Manually tested.
==========
Ideally we should be adding code in the CL that is using that code. Any reason why this needs to go in separately?
On 2016/09/27 19:05:20, Rahul Chaturvedi wrote: > Ideally we should be adding code in the CL that is using that code. Any reason > why this needs to go in separately? It just that in ChromeOS we normally separate CL that touch different component. I will add the the code that use this to this CL then.
Description was changed from
==========
dbus: Add support to map<string, vector<uint8_t>> in dbus::Property
BlueZ exposed Bluetooth device's ServiceData as a{sv} property[1]
where the dict value varient is an array of byte.
The existing PopValueFromReader() and AppendSetValueToWriter() in
dbus::Property do not support this type yet. This CL adds support
for this type
This CL also adds #include <memory> to make linter happy.
[1] http://git.kernel.org/cgit/bluetooth/bluez.git/tree/src/device.c#n2551
BUG=618442,b:28670943
TEST=Manually tested.
==========
to
==========
bluetooth: Expose service data from BlueZ
BlueZ exposed Bluetooth device's ServiceData as a{sv} property[1]
where the dict value variant is an array of byte.
This CL exposes that to upper layer by
- Add support to map<string, vector<uint8_t>> in dbus::Property
- Add new service_data property in BluetoothDeviceClient
- Implement GetServiceDataUUIDs() and GetServiceDataForUUID()
in BluetoothDeviceBlueZ
- Fix misc style issues in original code to make linter happy
[1] http://git.kernel.org/cgit/bluetooth/bluez.git/tree/src/device.c#n2551
BUG=618442,b:28670943
TEST=Manually tested.
==========
puthik@chromium.org changed reviewers: + ortuno@chromium.org
+ortuno for BT owner Address Rahul's comment by add scope of the CL. Quick question to Ortuno. I found BluetoothDevice::UpdateAdvertisementData() but look like no one is using that. Should I use that instead of this approach?
The CQ bit was checked by puthik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
quick comment. Will do a full pass later today. https://codereview.chromium.org/2369423003/diff/20001/device/bluetooth/bluez/... File device/bluetooth/bluez/bluetooth_device_bluez.cc (right): https://codereview.chromium.org/2369423003/diff/20001/device/bluetooth/bluez/... device/bluetooth/bluez/bluetooth_device_bluez.cc:802: DCHECK_GE(num_connecting_calls_, 0); Cleanup should be in its own CL. Usually conflating cleanup with functionality can lead to a confusing history.
https://codereview.chromium.org/2369423003/diff/20001/device/bluetooth/bluez/... File device/bluetooth/bluez/bluetooth_device_bluez.cc (right): https://codereview.chromium.org/2369423003/diff/20001/device/bluetooth/bluez/... device/bluetooth/bluez/bluetooth_device_bluez.cc:802: DCHECK_GE(num_connecting_calls_, 0); On 2016/09/27 20:52:59, Rahul Chaturvedi wrote: > Cleanup should be in its own CL. Usually conflating cleanup with functionality > can lead to a confusing history. Acknowledged. Will fix this once I got your full comment
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
dbus/property.cc lgtm with a nit https://codereview.chromium.org/2369423003/diff/20001/dbus/property.cc File dbus/property.cc (right): https://codereview.chromium.org/2369423003/diff/20001/dbus/property.cc#newcod... dbus/property.cc:672: MessageReader variant_reader(NULL); Please use nullptr instead of NULL. The same goes for others.
re UpdateAdvertisementData(). That's for platforms in which you get notified of each advertisement and you can update all fields at once which we can't do for BlueZ :( That said maybe you can reuse the advertisement_data_ member and just update it when you get the property changed signal. It would be nice if the behavior matched other platforms i.e. if once you stop scanning advertisement data is deleted. But we can wait for BlueZ to expose something similar to ScanRecord for it. Also please add tests. https://codereview.chromium.org/2369423003/diff/20001/device/bluetooth/blueto... File device/bluetooth/bluetooth_device.h (right): https://codereview.chromium.org/2369423003/diff/20001/device/bluetooth/blueto... device/bluetooth/bluetooth_device.h:333: virtual UUIDSet GetServiceDataUUIDs() const; Please also implement GetServiceData() https://codereview.chromium.org/2369423003/diff/20001/device/bluetooth/dbus/b... File device/bluetooth/dbus/bluetooth_device_client.h (right): https://codereview.chromium.org/2369423003/diff/20001/device/bluetooth/dbus/b... device/bluetooth/dbus/bluetooth_device_client.h:116: dbus::Property<std::map<std::string, std::vector<uint8_t>>> service_data; Could this be an unordered map?
I'll defer to ortuno@ here for the review since he has more context here.
The CQ bit was checked by puthik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Change this to use property changed signal to update advertisement_data_ instead. Also this depends on http://crrev.com/2391973003 to work correctly for ARC++ https://codereview.chromium.org/2369423003/diff/20001/dbus/property.cc File dbus/property.cc (right): https://codereview.chromium.org/2369423003/diff/20001/dbus/property.cc#newcod... dbus/property.cc:672: MessageReader variant_reader(NULL); On 2016/09/28 04:22:56, hashimoto wrote: > Please use nullptr instead of NULL. > The same goes for others. Done. I saw the existing code above use NULL and I thought it is the convention of this directory. https://codereview.chromium.org/2369423003/diff/20001/device/bluetooth/blueto... File device/bluetooth/bluetooth_device.h (right): https://codereview.chromium.org/2369423003/diff/20001/device/bluetooth/blueto... device/bluetooth/bluetooth_device.h:333: virtual UUIDSet GetServiceDataUUIDs() const; On 2016/09/28 08:39:24, ortuno wrote: > Please also implement GetServiceData() Fix this by use Property change event to update service_data_ and remove these code instead. https://codereview.chromium.org/2369423003/diff/20001/device/bluetooth/dbus/b... File device/bluetooth/dbus/bluetooth_device_client.h (right): https://codereview.chromium.org/2369423003/diff/20001/device/bluetooth/dbus/b... device/bluetooth/dbus/bluetooth_device_client.h:116: dbus::Property<std::map<std::string, std::vector<uint8_t>>> service_data; On 2016/09/28 08:39:24, ortuno wrote: > Could this be an unordered map? Done.
https://codereview.chromium.org/2369423003/diff/60001/device/bluetooth/bluez/... File device/bluetooth/bluez/bluetooth_bluez_unittest.cc (right): https://codereview.chromium.org/2369423003/diff/60001/device/bluetooth/bluez/... device/bluetooth/bluez/bluetooth_bluez_unittest.cc:4493: BluetoothAdapter::DeviceList devices = adapter_->GetDevices(); Just save a pointer to the device: BluetoothDevice* device = adapter_->GetDevice(bluez::FakeBluetoothDeviceClient::kPairedDeviceAddress); https://codereview.chromium.org/2369423003/diff/60001/device/bluetooth/bluez/... device/bluetooth/bluez/bluetooth_bluez_unittest.cc:4513: static BluetoothUUID kUuid("1f08"); nit: Can you use kGapUuid? https://codereview.chromium.org/2369423003/diff/60001/device/bluetooth/bluez/... device/bluetooth/bluez/bluetooth_bluez_unittest.cc:4515: std::unordered_map<std::string, std::vector<uint8_t>> service_data; You could do: std::unordered_map<std::string, std::vector<uint8_t>> service_data1({{kGapUuid, {1, 2, 3}}}); properties->service_data.ReplaceValue(service_data1); https://codereview.chromium.org/2369423003/diff/60001/device/bluetooth/bluez/... device/bluetooth/bluez/bluetooth_bluez_unittest.cc:4519: properties->service_data.ReplaceValue(service_data); Could you make this test a bit more thorough? i.e. more than one entry, update with new uuid, update with same uuid but different service data, etc. https://codereview.chromium.org/2369423003/diff/60001/device/bluetooth/bluez/... device/bluetooth/bluez/bluetooth_bluez_unittest.cc:4524: BluetoothDevice::UUIDSet uuids = devices[idx]->GetServiceDataUUIDs(); Also test GetServiceData. You can use initializer lists to make it easier to test. https://codereview.chromium.org/2369423003/diff/60001/device/bluetooth/bluez/... device/bluetooth/bluez/bluetooth_bluez_unittest.cc:4526: EXPECT_EQ(kUuid.canonical_value(), uuids.begin()->canonical_value()); Using initializer lists will make testing easier and more readable: EXPECT_EQ({BluetoothUUID(kGapUuid)}, device->GetServiceDataUUIDs()); https://codereview.chromium.org/2369423003/diff/60001/device/bluetooth/bluez/... device/bluetooth/bluez/bluetooth_bluez_unittest.cc:4530: EXPECT_EQ(kData[i], (*data)[i]); Same here: EXPECT_EQ({1, 2, 3}, *GetServiceDataForUUID(BluetoothUUID(kGapUuid))); https://codereview.chromium.org/2369423003/diff/60001/device/bluetooth/bluez/... File device/bluetooth/bluez/bluetooth_device_bluez.cc (right): https://codereview.chromium.org/2369423003/diff/60001/device/bluetooth/bluez/... device/bluetooth/bluez/bluetooth_device_bluez.cc:603: DCHECK(properties->service_data.is_valid()); Does bluez cache service data i.e. if a device stops advertising service data will bluez send a property changed event? If so please add a comment here; otherwise, update service_data_ accordingly and add a test for it. What about changing service data i.e. if a device is constantly changing its service data would bluez cache them all or would it change for each? Please document the BlueZ behavior in the GetServiceData docs, similar to GetUUIDs. https://codereview.chromium.org/2369423003/diff/60001/device/bluetooth/bluez/... File device/bluetooth/bluez/bluetooth_device_bluez.h (right): https://codereview.chromium.org/2369423003/diff/60001/device/bluetooth/bluez/... device/bluetooth/bluez/bluetooth_device_bluez.h:110: void UpdateServiceData(); nit: [...] when we receive a DevicePropertyChanged event for the service data property. https://codereview.chromium.org/2369423003/diff/60001/device/bluetooth/dbus/b... File device/bluetooth/dbus/bluetooth_device_client.cc (right): https://codereview.chromium.org/2369423003/diff/60001/device/bluetooth/dbus/b... device/bluetooth/dbus/bluetooth_device_client.cc:207: RegisterProperty(bluetooth_device::kServiceDataProperty, &service_data); nit: move this before the services resolved property to match the order of the bluez docs. https://codereview.chromium.org/2369423003/diff/60001/device/bluetooth/dbus/b... File device/bluetooth/dbus/bluetooth_device_client.h (right): https://codereview.chromium.org/2369423003/diff/60001/device/bluetooth/dbus/b... device/bluetooth/dbus/bluetooth_device_client.h:115: // List of Service data. Read-only. nit: just copy the description from the bluez docs: Service advertisement data. Keys are the UUIDs in string format followed by its byte array value.
Description was changed from
==========
bluetooth: Expose service data from BlueZ
BlueZ exposed Bluetooth device's ServiceData as a{sv} property[1]
where the dict value variant is an array of byte.
This CL exposes that to upper layer by
- Add support to map<string, vector<uint8_t>> in dbus::Property
- Add new service_data property in BluetoothDeviceClient
- Implement GetServiceDataUUIDs() and GetServiceDataForUUID()
in BluetoothDeviceBlueZ
- Fix misc style issues in original code to make linter happy
[1] http://git.kernel.org/cgit/bluetooth/bluez.git/tree/src/device.c#n2551
BUG=618442,b:28670943
TEST=Manually tested.
==========
to
==========
bluetooth: Expose service data from BlueZ
BlueZ exposed Bluetooth device's ServiceData as a{sv} property[1]
where the dict value variant is an array of byte.
This CL exposes that to upper layer by
- Add support to map<string, vector<uint8_t>> in dbus::Property
- Add new service_data property in BluetoothDeviceClient
- Implement GetServiceDataUUIDs() and GetServiceDataForUUID()
in BluetoothDeviceBlueZ
- Fix misc style issues in original code to make linter happy
[1] http://git.kernel.org/cgit/bluetooth/bluez.git/tree/src/device.c#n2551
BUG=618442,653310,b:28670943
TEST=Manually tested.
==========
https://codereview.chromium.org/2369423003/diff/20001/device/bluetooth/bluez/... File device/bluetooth/bluez/bluetooth_device_bluez.cc (right): https://codereview.chromium.org/2369423003/diff/20001/device/bluetooth/bluez/... device/bluetooth/bluez/bluetooth_device_bluez.cc:802: DCHECK_GE(num_connecting_calls_, 0); On 2016/09/27 21:04:07, puthik_chromium wrote: > On 2016/09/27 20:52:59, Rahul Chaturvedi wrote: > > Cleanup should be in its own CL. Usually conflating cleanup with functionality > > can lead to a confusing history. > > Acknowledged. > Will fix this once I got your full comment Done. https://codereview.chromium.org/2369423003/diff/60001/device/bluetooth/bluez/... File device/bluetooth/bluez/bluetooth_bluez_unittest.cc (right): https://codereview.chromium.org/2369423003/diff/60001/device/bluetooth/bluez/... device/bluetooth/bluez/bluetooth_bluez_unittest.cc:4493: BluetoothAdapter::DeviceList devices = adapter_->GetDevices(); On 2016/10/05 06:27:09, ortuno wrote: > Just save a pointer to the device: > > BluetoothDevice* device = > adapter_->GetDevice(bluez::FakeBluetoothDeviceClient::kPairedDeviceAddress); Done. https://codereview.chromium.org/2369423003/diff/60001/device/bluetooth/bluez/... device/bluetooth/bluez/bluetooth_bluez_unittest.cc:4513: static BluetoothUUID kUuid("1f08"); On 2016/10/05 06:27:09, ortuno wrote: > nit: Can you use kGapUuid? Sure https://codereview.chromium.org/2369423003/diff/60001/device/bluetooth/bluez/... device/bluetooth/bluez/bluetooth_bluez_unittest.cc:4515: std::unordered_map<std::string, std::vector<uint8_t>> service_data; On 2016/10/05 06:27:09, ortuno wrote: > You could do: > > std::unordered_map<std::string, std::vector<uint8_t>> service_data1({{kGapUuid, > {1, 2, 3}}}); > properties->service_data.ReplaceValue(service_data1); Done. https://codereview.chromium.org/2369423003/diff/60001/device/bluetooth/bluez/... device/bluetooth/bluez/bluetooth_bluez_unittest.cc:4519: properties->service_data.ReplaceValue(service_data); On 2016/10/05 06:27:10, ortuno wrote: > Could you make this test a bit more thorough? i.e. more than one entry, update > with new uuid, update with same uuid but different service data, etc. Done. https://codereview.chromium.org/2369423003/diff/60001/device/bluetooth/bluez/... device/bluetooth/bluez/bluetooth_bluez_unittest.cc:4524: BluetoothDevice::UUIDSet uuids = devices[idx]->GetServiceDataUUIDs(); On 2016/10/05 06:27:09, ortuno wrote: > Also test GetServiceData. You can use initializer lists to make it easier to > test. Done. https://codereview.chromium.org/2369423003/diff/60001/device/bluetooth/bluez/... device/bluetooth/bluez/bluetooth_bluez_unittest.cc:4524: BluetoothDevice::UUIDSet uuids = devices[idx]->GetServiceDataUUIDs(); On 2016/10/05 06:27:09, ortuno wrote: > Also test GetServiceData. You can use initializer lists to make it easier to > test. Done. https://codereview.chromium.org/2369423003/diff/60001/device/bluetooth/bluez/... device/bluetooth/bluez/bluetooth_bluez_unittest.cc:4526: EXPECT_EQ(kUuid.canonical_value(), uuids.begin()->canonical_value()); On 2016/10/05 06:27:09, ortuno wrote: > Using initializer lists will make testing easier and more readable: > > EXPECT_EQ({BluetoothUUID(kGapUuid)}, device->GetServiceDataUUIDs()); Done https://codereview.chromium.org/2369423003/diff/60001/device/bluetooth/bluez/... device/bluetooth/bluez/bluetooth_bluez_unittest.cc:4530: EXPECT_EQ(kData[i], (*data)[i]); On 2016/10/05 06:27:10, ortuno wrote: > Same here: > > EXPECT_EQ({1, 2, 3}, *GetServiceDataForUUID(BluetoothUUID(kGapUuid))); Done https://codereview.chromium.org/2369423003/diff/60001/device/bluetooth/bluez/... File device/bluetooth/bluez/bluetooth_device_bluez.cc (right): https://codereview.chromium.org/2369423003/diff/60001/device/bluetooth/bluez/... device/bluetooth/bluez/bluetooth_device_bluez.cc:603: DCHECK(properties->service_data.is_valid()); On 2016/10/05 06:27:10, ortuno wrote: > Does bluez cache service data i.e. if a device stops advertising service data > will bluez send a property changed event? > > If so please add a comment here; otherwise, update service_data_ accordingly and > add a test for it. > > What about changing service data i.e. if a device is constantly changing its > service data would bluez cache them all or would it change for each? > > Please document the BlueZ behavior in the GetServiceData docs, similar to > GetUUIDs. Add comment in header file instead. - BlueZ won't send property change event when device stops advertising service data - BlueZ will send property change event for each service data changed. https://codereview.chromium.org/2369423003/diff/60001/device/bluetooth/bluez/... File device/bluetooth/bluez/bluetooth_device_bluez.h (right): https://codereview.chromium.org/2369423003/diff/60001/device/bluetooth/bluez/... device/bluetooth/bluez/bluetooth_device_bluez.h:110: void UpdateServiceData(); On 2016/10/05 06:27:10, ortuno wrote: > nit: [...] when we receive a DevicePropertyChanged event for the service data > property. Done. https://codereview.chromium.org/2369423003/diff/60001/device/bluetooth/dbus/b... File device/bluetooth/dbus/bluetooth_device_client.cc (right): https://codereview.chromium.org/2369423003/diff/60001/device/bluetooth/dbus/b... device/bluetooth/dbus/bluetooth_device_client.cc:207: RegisterProperty(bluetooth_device::kServiceDataProperty, &service_data); On 2016/10/05 06:27:10, ortuno wrote: > nit: move this before the services resolved property to match the order of the > bluez docs. Done. https://codereview.chromium.org/2369423003/diff/60001/device/bluetooth/dbus/b... File device/bluetooth/dbus/bluetooth_device_client.h (right): https://codereview.chromium.org/2369423003/diff/60001/device/bluetooth/dbus/b... device/bluetooth/dbus/bluetooth_device_client.h:115: // List of Service data. Read-only. On 2016/10/05 06:27:10, ortuno wrote: > nit: just copy the description from the bluez docs: > > Service advertisement data. Keys are the UUIDs in string format followed by its > byte array value. Done.
lgtm bar some nits https://codereview.chromium.org/2369423003/diff/100001/device/bluetooth/bluet... File device/bluetooth/bluetooth_device.h (right): https://codereview.chromium.org/2369423003/diff/100001/device/bluetooth/bluet... device/bluetooth/bluetooth_device.h:329: // a device stops advertising service data this function will still return nit: // a device stops advertising service data for a UUID, this function will still return // the cached value for that UUID. https://codereview.chromium.org/2369423003/diff/100001/device/bluetooth/bluez... File device/bluetooth/bluez/bluetooth_device_bluez.cc (right): https://codereview.chromium.org/2369423003/diff/100001/device/bluetooth/bluez... device/bluetooth/bluez/bluetooth_device_bluez.cc:607: service_data_.insert(std::pair<BluetoothUUID, std::vector<uint8_t>>( optional nit: You could do service_data_[BluetootohUUID(pair.first)] = pair.second; https://codereview.chromium.org/2369423003/diff/100001/device/bluetooth/bluez... File device/bluetooth/bluez/bluetooth_device_bluez.h (right): https://codereview.chromium.org/2369423003/diff/100001/device/bluetooth/bluez... device/bluetooth/bluez/bluetooth_device_bluez.h:115: // multiple times when there are multiple UUIDs that service data changed. nit: s/meaning/Meaning Damn, this is unfortunate... Hopefully we can soon get the Advertising Data property to avoid this. https://codereview.chromium.org/2369423003/diff/100001/device/bluetooth/bluez... device/bluetooth/bluez/bluetooth_device_bluez.h:117: // UUID if it is already exist. If not BlueZ added that data for UUID. s/added/adds/ https://codereview.chromium.org/2369423003/diff/100001/device/bluetooth/bluez... device/bluetooth/bluez/bluetooth_device_bluez.h:118: // This means BlueZ won't remove service data even a device stops advertise s/even/even when/ and s/advertise/advertising/
https://codereview.chromium.org/2369423003/diff/100001/device/bluetooth/bluet... File device/bluetooth/bluetooth_device.h (right): https://codereview.chromium.org/2369423003/diff/100001/device/bluetooth/bluet... device/bluetooth/bluetooth_device.h:329: // a device stops advertising service data this function will still return On 2016/10/06 02:09:01, ortuno wrote: > nit: > // a device stops advertising service data for a UUID, this function will still > return > // the cached value for that UUID. Done. https://codereview.chromium.org/2369423003/diff/100001/device/bluetooth/bluez... File device/bluetooth/bluez/bluetooth_device_bluez.cc (right): https://codereview.chromium.org/2369423003/diff/100001/device/bluetooth/bluez... device/bluetooth/bluez/bluetooth_device_bluez.cc:607: service_data_.insert(std::pair<BluetoothUUID, std::vector<uint8_t>>( On 2016/10/06 02:09:01, ortuno wrote: > > optional nit: You could do > > service_data_[BluetootohUUID(pair.first)] = pair.second; Done. https://codereview.chromium.org/2369423003/diff/100001/device/bluetooth/bluez... File device/bluetooth/bluez/bluetooth_device_bluez.h (right): https://codereview.chromium.org/2369423003/diff/100001/device/bluetooth/bluez... device/bluetooth/bluez/bluetooth_device_bluez.h:115: // multiple times when there are multiple UUIDs that service data changed. On 2016/10/06 02:09:01, ortuno wrote: > nit: s/meaning/Meaning > > Damn, this is unfortunate... Hopefully we can soon get the Advertising Data > property to avoid this. Done. https://codereview.chromium.org/2369423003/diff/100001/device/bluetooth/bluez... device/bluetooth/bluez/bluetooth_device_bluez.h:117: // UUID if it is already exist. If not BlueZ added that data for UUID. On 2016/10/06 02:09:01, ortuno wrote: > s/added/adds/ Done. https://codereview.chromium.org/2369423003/diff/100001/device/bluetooth/bluez... device/bluetooth/bluez/bluetooth_device_bluez.h:118: // This means BlueZ won't remove service data even a device stops advertise On 2016/10/06 02:09:01, ortuno wrote: > s/even/even when/ and s/advertise/advertising/ Done.
The CQ bit was checked by puthik@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hashimoto@chromium.org, ortuno@chromium.org Link to the patchset: https://codereview.chromium.org/2369423003/#ps120001 (title: "Fix comment")
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 hashimoto@chromium.org
Code under dbus/ was heavily changed since the last time I reviewed. Let me review it again.
On 2016/10/06 02:31:18, hashimoto wrote: > Code under dbus/ was heavily changed since the last time I reviewed. > Let me review it again. I just add unit test for that.
lgtm++
The CQ bit was checked by hashimoto@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from
==========
bluetooth: Expose service data from BlueZ
BlueZ exposed Bluetooth device's ServiceData as a{sv} property[1]
where the dict value variant is an array of byte.
This CL exposes that to upper layer by
- Add support to map<string, vector<uint8_t>> in dbus::Property
- Add new service_data property in BluetoothDeviceClient
- Implement GetServiceDataUUIDs() and GetServiceDataForUUID()
in BluetoothDeviceBlueZ
- Fix misc style issues in original code to make linter happy
[1] http://git.kernel.org/cgit/bluetooth/bluez.git/tree/src/device.c#n2551
BUG=618442,653310,b:28670943
TEST=Manually tested.
==========
to
==========
bluetooth: Expose service data from BlueZ
BlueZ exposed Bluetooth device's ServiceData as a{sv} property[1]
where the dict value variant is an array of byte.
This CL exposes that to upper layer by
- Add support to map<string, vector<uint8_t>> in dbus::Property
- Add new service_data property in BluetoothDeviceClient
- Implement GetServiceDataUUIDs() and GetServiceDataForUUID()
in BluetoothDeviceBlueZ
- Fix misc style issues in original code to make linter happy
[1] http://git.kernel.org/cgit/bluetooth/bluez.git/tree/src/device.c#n2551
BUG=618442,653310,b:28670943
TEST=Manually tested.
==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from
==========
bluetooth: Expose service data from BlueZ
BlueZ exposed Bluetooth device's ServiceData as a{sv} property[1]
where the dict value variant is an array of byte.
This CL exposes that to upper layer by
- Add support to map<string, vector<uint8_t>> in dbus::Property
- Add new service_data property in BluetoothDeviceClient
- Implement GetServiceDataUUIDs() and GetServiceDataForUUID()
in BluetoothDeviceBlueZ
- Fix misc style issues in original code to make linter happy
[1] http://git.kernel.org/cgit/bluetooth/bluez.git/tree/src/device.c#n2551
BUG=618442,653310,b:28670943
TEST=Manually tested.
==========
to
==========
bluetooth: Expose service data from BlueZ
BlueZ exposed Bluetooth device's ServiceData as a{sv} property[1]
where the dict value variant is an array of byte.
This CL exposes that to upper layer by
- Add support to map<string, vector<uint8_t>> in dbus::Property
- Add new service_data property in BluetoothDeviceClient
- Implement GetServiceDataUUIDs() and GetServiceDataForUUID()
in BluetoothDeviceBlueZ
- Fix misc style issues in original code to make linter happy
[1] http://git.kernel.org/cgit/bluetooth/bluez.git/tree/src/device.c#n2551
BUG=618442,653310,b:28670943
TEST=Manually tested.
Committed: https://crrev.com/0dd82b1c975662a10f34a53b3df56d98526b36d0
Cr-Commit-Position: refs/heads/master@{#423434}
==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/0dd82b1c975662a10f34a53b3df56d98526b36d0 Cr-Commit-Position: refs/heads/master@{#423434} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
