Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2783733002:
Bluetooth: Add service data unit tests for chromeos (Closed)
|
Created:
3 years, 8 months ago by xiaoyinh(OOO Sep 11-29) Modified:
3 years, 8 months ago CC:
chromium-reviews, ortuno+watch_chromium.org, scheib+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionBluetooth: Add service data unit tests for chromeos
This CL verifies that:
1. service data is populated correctly during device creation.
2. service data is updated correctly on property changed.
BUG=688566
Review-Url: https://codereview.chromium.org/2783733002
Cr-Commit-Position: refs/heads/master@{#461565}
Committed: https://chromium.googlesource.com/chromium/src/+/ab0e2203cdb233620ef341cc297fc3d87ca0ddc5
Patch Set 1 #
Total comments: 9
Patch Set 2 : Incoporate comments #
Total comments: 15
Patch Set 3 : mimic bluez caching behavior #Patch Set 4 : trybot #
Total comments: 10
Patch Set 5 : comments and rebase #
Total comments: 1
Messages
Total messages: 51 (27 generated)
The CQ bit was checked by xiaoyinh@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.
xiaoyinh@chromium.org changed reviewers: + mcchou@chromium.org, ortuno@chromium.org, rkc@chromium.org
Hi, Could you take a look and let me know? Thanks!
Description was changed from ========== Verify service data has been population during device creation. BUG=688566 TEST=device_unittests ========== to ========== Verify service data has been populated during device creation. BUG=688566 TEST=device_unittests ==========
On 2017/03/28 21:05:08, xiaoyinh wrote: > Hi, Could you take a look and let me know? Thanks! Don't forget to kick off trybots after uploading the patch. :)
https://codereview.chromium.org/2783733002/diff/1/device/bluetooth/dbus/fake_... File device/bluetooth/dbus/fake_bluetooth_device_client.cc (right): https://codereview.chromium.org/2783733002/diff/1/device/bluetooth/dbus/fake_... device/bluetooth/dbus/fake_bluetooth_device_client.cc:1822: properties->service_data.set_valid(true); The validity of |service_data| should depend on whether there is data in it. https://codereview.chromium.org/2783733002/diff/1/device/bluetooth/test/bluet... File device/bluetooth/test/bluetooth_test_bluez.cc (right): https://codereview.chromium.org/2783733002/diff/1/device/bluetooth/test/bluet... device/bluetooth/test/bluetooth_test_bluez.cc:131: service_data[kTestUUIDHeartRate] = {1}; nit: It is better to use the original format (0x01) of Service Data here and elsewhere. Also, the test cases do not include the case where the length of Service Data is more than 1 byte for a given UUID key. Don't forget to modify device/bluetooth/test/bluetooth_test.h to make sure the comments match with the implementation. :) https://codereview.chromium.org/2783733002/diff/1/device/bluetooth/test/bluet... device/bluetooth/test/bluetooth_test_bluez.cc:155: if (device) { nit: The brackets can be removed here.
In the future it would be better if you added reviewers as they lgtm your change that way we don't conflict with each other. Also the TEST line is usually used if manual tests are required. device_unittests run for all tryjobs so we shouldn't need any manual test https://www.chromium.org/developers/contributing-code#TOC-Writing-change-list... https://codereview.chromium.org/2783733002/diff/1/device/bluetooth/test/bluet... File device/bluetooth/test/bluetooth_test_bluez.cc (right): https://codereview.chromium.org/2783733002/diff/1/device/bluetooth/test/bluet... device/bluetooth/test/bluetooth_test_bluez.cc:155: if (device) { SimulateLowEnergyDevice is actually a misnomer. It should be called SimulateAdvertisement; sorry for the confusion. This means that calling: SimulateLowEnergyDevice(2); SimulateLowEnergyDevice(3); should result in one new device (for the first call) and changes to that device for the second call and not a new device each time.
https://codereview.chromium.org/2783733002/diff/1/device/bluetooth/test/bluet... File device/bluetooth/test/bluetooth_test_bluez.cc (right): https://codereview.chromium.org/2783733002/diff/1/device/bluetooth/test/bluet... device/bluetooth/test/bluetooth_test_bluez.cc:155: if (device) { On 2017/03/28 23:56:36, ortuno wrote: > SimulateLowEnergyDevice is actually a misnomer. It should be called > SimulateAdvertisement; sorry for the confusion. This means that calling: > > SimulateLowEnergyDevice(2); > SimulateLowEnergyDevice(3); > > should result in one new device (for the first call) and changes to that device > for the second call and not a new device each time. > It makes more sense with your explanation now.
The CQ bit was checked by xiaoyinh@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...
Description was changed from ========== Verify service data has been populated during device creation. BUG=688566 TEST=device_unittests ========== to ========== Bluetooth: Add service data unit tests for chromeos This CL verifies that: 1. service data is populated correctly during device creation. 2. service data is updated correctly on property changed. BUG=688566 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Miao, ortuno, thanks for your comments. Please take another look, thanks! https://codereview.chromium.org/2783733002/diff/1/device/bluetooth/dbus/fake_... File device/bluetooth/dbus/fake_bluetooth_device_client.cc (right): https://codereview.chromium.org/2783733002/diff/1/device/bluetooth/dbus/fake_... device/bluetooth/dbus/fake_bluetooth_device_client.cc:1822: properties->service_data.set_valid(true); On 2017/03/28 22:56:40, Miao wrote: > The validity of |service_data| should depend on whether there is data in it. Done. https://codereview.chromium.org/2783733002/diff/1/device/bluetooth/test/bluet... File device/bluetooth/test/bluetooth_test_bluez.cc (right): https://codereview.chromium.org/2783733002/diff/1/device/bluetooth/test/bluet... device/bluetooth/test/bluetooth_test_bluez.cc:131: service_data[kTestUUIDHeartRate] = {1}; On 2017/03/28 22:56:40, Miao wrote: > nit: It is better to use the original format (0x01) of Service Data here and > elsewhere. > > Also, the test cases do not include the case where the length of Service Data is > more than 1 byte for a given UUID key. > > Don't forget to modify device/bluetooth/test/bluetooth_test.h to make sure the > comments match with the implementation. :) Thanks for the comments. I modified device_ordinal 2 to include additional cases: 1. 2 pieces of service data for kTestUUIDImmediateAlert 2. Empty service data for kTestUUIDHeartRate https://codereview.chromium.org/2783733002/diff/1/device/bluetooth/test/bluet... device/bluetooth/test/bluetooth_test_bluez.cc:155: if (device) { On 2017/03/28 23:56:36, ortuno wrote: > SimulateLowEnergyDevice is actually a misnomer. It should be called > SimulateAdvertisement; sorry for the confusion. This means that calling: > > SimulateLowEnergyDevice(2); > SimulateLowEnergyDevice(3); > > should result in one new device (for the first call) and changes to that device > for the second call and not a new device each time. > Thanks for the clarification! I've changed the code and also updated the description in bluetooth_test.h. Please let me know if these make sense. https://codereview.chromium.org/2783733002/diff/1/device/bluetooth/test/bluet... device/bluetooth/test/bluetooth_test_bluez.cc:155: if (device) { On 2017/03/28 22:56:40, Miao wrote: > nit: The brackets can be removed here. Thanks! Changed to another approach.
Also please remove the TODO in https://cs.chromium.org/chromium/src/device/bluetooth/bluez/bluetooth_device_... https://codereview.chromium.org/2783733002/diff/20001/device/bluetooth/blueto... File device/bluetooth/bluetooth_device_unittest.cc (right): https://codereview.chromium.org/2783733002/diff/20001/device/bluetooth/blueto... device/bluetooth/bluetooth_device_unittest.cc:170: #if defined(OS_MACOSX) || defined(OS_CHROMEOS) Would you mind enabling this on Linux as well? Also we can remove: https://cs.chromium.org/chromium/src/device/bluetooth/bluez/bluetooth_bluez_u... https://codereview.chromium.org/2783733002/diff/20001/device/bluetooth/blueto... device/bluetooth/bluetooth_device_unittest.cc:181: // StartLowEnergyDiscoverySession is not yet implemented on ChromeOS|bluez. nit: TODO(crbug.com/706043): Remove #if once StartLowEnergyDiscoverySession is implemented for bluez. https://codereview.chromium.org/2783733002/diff/20001/device/bluetooth/blueto... device/bluetooth/bluetooth_device_unittest.cc:219: #if !defined(OS_CHROMEOS) TODO(crbug.com/706043): Remove #if once StartLowEnergyDiscoverySession is implemented for bluez. https://codereview.chromium.org/2783733002/diff/20001/device/bluetooth/dbus/f... File device/bluetooth/dbus/fake_bluetooth_device_client.cc (right): https://codereview.chromium.org/2783733002/diff/20001/device/bluetooth/dbus/f... device/bluetooth/dbus/fake_bluetooth_device_client.cc:1565: properties->service_data.ReplaceValue(service_data); +scheib fyi *sigh* I wish BlueZ worked like this. Sadly BlueZ caches and combines all values. Attempts to convince BlueZ folks to change this have failed, so we are stuck with this until the new stack. For example: 1. Receive advertisement with service data only for UUID 0x180a 2. Receive advertisement with service data only for UUID 0x180b After the two advertisements arrive properties->service_data will contain both values :( Because bluetooth_test is supposed to simulate the behavior of each platform this function should match what BlueZ does and combine the new values with the old values. This will cause the test to fail D: D: which is expected because the behavior in BlueZ is different. So we need to add #if defined(OS_CHROMEOS) to the test explaining that BlueZ behaves differently. See [1] for docs explaining how the behavior is different and [2] for other instances of this: [1] https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_device.h?sq=p... [2] https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_adapter_unitt...
ortuno@chromium.org changed reviewers: + scheib@chromium.org
+scheib for realz
https://codereview.chromium.org/2783733002/diff/20001/device/bluetooth/dbus/f... File device/bluetooth/dbus/fake_bluetooth_device_client.cc (right): https://codereview.chromium.org/2783733002/diff/20001/device/bluetooth/dbus/f... device/bluetooth/dbus/fake_bluetooth_device_client.cc:1565: properties->service_data.ReplaceValue(service_data); On 2017/03/30 02:58:14, ortuno wrote: > +scheib fyi > > *sigh* I wish BlueZ worked like this. Sadly BlueZ caches and combines all > values. Attempts to convince BlueZ folks to change this have failed, so we are > stuck with this until the new stack. For example: > > 1. Receive advertisement with service data only for UUID 0x180a > 2. Receive advertisement with service data only for UUID 0x180b > > After the two advertisements arrive properties->service_data will contain both > values :( > > Because bluetooth_test is supposed to simulate the behavior of each platform > this function should match what BlueZ does and combine the new values with the > old values. > > This will cause the test to fail D: D: which is expected because the behavior in > BlueZ is different. So we need to add #if defined(OS_CHROMEOS) to the test > explaining that BlueZ behaves differently. See [1] for docs explaining how the > behavior is different and [2] for other instances of this: > > [1] > https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_device.h?sq=p... > [2] > https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_adapter_unitt... > Grr... Bluez. I didn't go dig into Bluez code to see where it merges. I see that our code using BlueZ just reads out all the values BlueZ provides when a BluetoothAdapterBlueZ::DevicePropertyChanged is called. The fake here should mimic what BlueZ does -- and please include a comment pointing to the BlueZ code to blame for it. If our platforms behave differently, our unit tests should represent that and have the regrettable #if defined sections (as in [2] above). If we can fix it, we should file an issue and // TODO(issue-number) to fix it. Most ideally we'd fix BlueZ by having a way to clear the cached values. Can our BluetoothDeviceBlueZ::UpdateServiceData clear the properties->service_data?
https://codereview.chromium.org/2783733002/diff/20001/device/bluetooth/blueto... File device/bluetooth/bluetooth_device_unittest.cc (right): https://codereview.chromium.org/2783733002/diff/20001/device/bluetooth/blueto... device/bluetooth/bluetooth_device_unittest.cc:170: #if defined(OS_MACOSX) || defined(OS_CHROMEOS) On 2017/03/30 02:58:14, ortuno wrote: > Would you mind enabling this on Linux as well? > > Also we can remove: > https://cs.chromium.org/chromium/src/device/bluetooth/bluez/bluetooth_bluez_u... Agree on this, we don't have any Chromium specific changes on parsing and storing the service data. The test here is good enough to cover the old one. https://codereview.chromium.org/2783733002/diff/20001/device/bluetooth/blueto... device/bluetooth/bluetooth_device_unittest.cc:197: SimulateLowEnergyDevice(3); The expected outcomes of service data and UUIDs are different between CrOS/Linux and Mac if the new advertisement event does not contain any service data. In this case, BlueZ does not clean up the previously-found service data. Therefore, separate checks are needed for CrOS/Linux. https://codereview.chromium.org/2783733002/diff/20001/device/bluetooth/dbus/f... File device/bluetooth/dbus/fake_bluetooth_device_client.cc (right): https://codereview.chromium.org/2783733002/diff/20001/device/bluetooth/dbus/f... device/bluetooth/dbus/fake_bluetooth_device_client.cc:1565: properties->service_data.ReplaceValue(service_data); On 2017/03/30 03:44:10, scheib wrote: > On 2017/03/30 02:58:14, ortuno wrote: > > +scheib fyi > > > > *sigh* I wish BlueZ worked like this. Sadly BlueZ caches and combines all > > values. Attempts to convince BlueZ folks to change this have failed, so we are > > stuck with this until the new stack. For example: > > > > 1. Receive advertisement with service data only for UUID 0x180a > > 2. Receive advertisement with service data only for UUID 0x180b > > > > After the two advertisements arrive properties->service_data will contain both > > values :( > > > > Because bluetooth_test is supposed to simulate the behavior of each platform > > this function should match what BlueZ does and combine the new values with the > > old values. > > > > This will cause the test to fail D: D: which is expected because the behavior > in > > BlueZ is different. So we need to add #if defined(OS_CHROMEOS) to the test > > explaining that BlueZ behaves differently. See [1] for docs explaining how the > > behavior is different and [2] for other instances of this: > > > > [1] > > > https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_device.h?sq=p... > > [2] > > > https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_adapter_unitt... > > > > Grr... Bluez. > > I didn't go dig into Bluez code to see where it merges. I see that our code > using BlueZ just reads out all the values BlueZ provides when a > BluetoothAdapterBlueZ::DevicePropertyChanged is called. > > The fake here should mimic what BlueZ does -- and please include a comment > pointing to the BlueZ code to blame for it. > > If our platforms behave differently, our unit tests should represent that and > have the regrettable #if defined sections (as in [2] above). If we can fix it, > we should file an issue and // TODO(issue-number) to fix it. > > Most ideally we'd fix BlueZ by having a way to clear the cached values. Can our > BluetoothDeviceBlueZ::UpdateServiceData clear the properties->service_data? As Ortuno mentioned, the current BlueZ caches all previously-found service data and UUIDs, and the merging[1] happens when device found event is received by bluetoothd. And I also agree that we should get it fixed as a CHROMIUM specific fix, so please fix a bug and marked as p2 and marked the related issues within the new bug. Out of curious, is there any existing user case where you observe issues caused by the caching? [1]https://cs.corp.google.com/chromeos_public/src/third_party/bluez/src/shared/ad.c?q=src/shared/ad.c+file:%5Esrc/third_party/bluez/+package:%5Echromeos_public$&dr&l=557
https://codereview.chromium.org/2783733002/diff/20001/device/bluetooth/dbus/f... File device/bluetooth/dbus/fake_bluetooth_device_client.cc (right): https://codereview.chromium.org/2783733002/diff/20001/device/bluetooth/dbus/f... device/bluetooth/dbus/fake_bluetooth_device_client.cc:1565: properties->service_data.ReplaceValue(service_data); On 2017/03/30 at 21:20:07, Miao wrote: > On 2017/03/30 03:44:10, scheib wrote: > > On 2017/03/30 02:58:14, ortuno wrote: > > > +scheib fyi > > > > > > *sigh* I wish BlueZ worked like this. Sadly BlueZ caches and combines all > > > values. Attempts to convince BlueZ folks to change this have failed, so we are > > > stuck with this until the new stack. For example: > > > > > > 1. Receive advertisement with service data only for UUID 0x180a > > > 2. Receive advertisement with service data only for UUID 0x180b > > > > > > After the two advertisements arrive properties->service_data will contain both > > > values :( > > > > > > Because bluetooth_test is supposed to simulate the behavior of each platform > > > this function should match what BlueZ does and combine the new values with the > > > old values. > > > > > > This will cause the test to fail D: D: which is expected because the behavior > > in > > > BlueZ is different. So we need to add #if defined(OS_CHROMEOS) to the test > > > explaining that BlueZ behaves differently. See [1] for docs explaining how the > > > behavior is different and [2] for other instances of this: > > > > > > [1] > > > > > https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_device.h?sq=p... > > > [2] > > > > > https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_adapter_unitt... > > > > > > > Grr... Bluez. > > > > I didn't go dig into Bluez code to see where it merges. I see that our code > > using BlueZ just reads out all the values BlueZ provides when a > > BluetoothAdapterBlueZ::DevicePropertyChanged is called. > > > > The fake here should mimic what BlueZ does -- and please include a comment > > pointing to the BlueZ code to blame for it. > > > > If our platforms behave differently, our unit tests should represent that and > > have the regrettable #if defined sections (as in [2] above). If we can fix it, > > we should file an issue and // TODO(issue-number) to fix it. > > > > Most ideally we'd fix BlueZ by having a way to clear the cached values. Can our > > BluetoothDeviceBlueZ::UpdateServiceData clear the properties->service_data? > > As Ortuno mentioned, the current BlueZ caches all previously-found service data and UUIDs, and the merging[1] happens when device found event is received by bluetoothd. And I also agree that we should get it fixed as a CHROMIUM specific fix, so please fix a bug and marked as p2 and marked the related issues within the new bug. > We discussed a solution with puthik a while ago. Left comments on the issue. > Out of curious, is there any existing user case where you observe issues caused by the caching? > > [1]https://cs.corp.google.com/chromeos_public/src/third_party/bluez/src/shared/ad.c?q=src/shared/ad.c+file:%5Esrc/third_party/bluez/+package:%5Echromeos_public$&dr&l=557 hmm we haven't seen any use cases/problems on the Web Bluetooth side because we don't use this yet, but Arc++ uses it. The Android API[1] requires Arc++ to send an event for each new packet with the contents of that packet only. Because we are caching Service Data, Arc++ could be sending UUIDs, Service Data, etc. values that are not part of the latest packet. As mentioned before we haven't seen problems on the Web Bluetooth side because we don't use it yet, but we do have problems with UUIDs, which have the same caching policy. Eddystone beacons change their advertised services after being configured to indicate that config is done. Because BlueZ caches UUIDs these beacons appear as if they were still configurable even after config is done. [1] https://developer.android.com/reference/android/bluetooth/le/ScanCallback.htm..., android.bluetooth.le.ScanResult)
The CQ bit was checked by xiaoyinh@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: linux_chromium_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 xiaoyinh@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/2783733002/diff/20001/device/bluetooth/blueto... File device/bluetooth/bluetooth_device_unittest.cc (right): https://codereview.chromium.org/2783733002/diff/20001/device/bluetooth/blueto... device/bluetooth/bluetooth_device_unittest.cc:170: #if defined(OS_MACOSX) || defined(OS_CHROMEOS) On 2017/03/30 02:58:14, ortuno wrote: > Would you mind enabling this on Linux as well? > > Also we can remove: > https://cs.chromium.org/chromium/src/device/bluetooth/bluez/bluetooth_bluez_u... Done. https://codereview.chromium.org/2783733002/diff/20001/device/bluetooth/blueto... device/bluetooth/bluetooth_device_unittest.cc:181: // StartLowEnergyDiscoverySession is not yet implemented on ChromeOS|bluez. On 2017/03/30 02:58:14, ortuno wrote: > nit: TODO(crbug.com/706043): Remove #if once StartLowEnergyDiscoverySession is > implemented for bluez. Done. https://codereview.chromium.org/2783733002/diff/20001/device/bluetooth/blueto... device/bluetooth/bluetooth_device_unittest.cc:197: SimulateLowEnergyDevice(3); On 2017/03/30 21:20:07, Miao wrote: > The expected outcomes of service data and UUIDs are different between CrOS/Linux > and Mac if the new advertisement event does not contain any service data. In > this case, BlueZ does not clean up the previously-found service data. Therefore, > separate checks are needed for CrOS/Linux. Done. https://codereview.chromium.org/2783733002/diff/20001/device/bluetooth/blueto... device/bluetooth/bluetooth_device_unittest.cc:219: #if !defined(OS_CHROMEOS) On 2017/03/30 02:58:14, ortuno wrote: > TODO(crbug.com/706043): Remove #if once StartLowEnergyDiscoverySession is > implemented for bluez. Done. https://codereview.chromium.org/2783733002/diff/20001/device/bluetooth/dbus/f... File device/bluetooth/dbus/fake_bluetooth_device_client.cc (right): https://codereview.chromium.org/2783733002/diff/20001/device/bluetooth/dbus/f... device/bluetooth/dbus/fake_bluetooth_device_client.cc:1565: properties->service_data.ReplaceValue(service_data); On 2017/03/30 21:20:07, Miao wrote: > On 2017/03/30 03:44:10, scheib wrote: > > On 2017/03/30 02:58:14, ortuno wrote: > > > +scheib fyi > > > > > > *sigh* I wish BlueZ worked like this. Sadly BlueZ caches and combines all > > > values. Attempts to convince BlueZ folks to change this have failed, so we > are > > > stuck with this until the new stack. For example: > > > > > > 1. Receive advertisement with service data only for UUID 0x180a > > > 2. Receive advertisement with service data only for UUID 0x180b > > > > > > After the two advertisements arrive properties->service_data will contain > both > > > values :( > > > > > > Because bluetooth_test is supposed to simulate the behavior of each platform > > > this function should match what BlueZ does and combine the new values with > the > > > old values. > > > > > > This will cause the test to fail D: D: which is expected because the > behavior > > in > > > BlueZ is different. So we need to add #if defined(OS_CHROMEOS) to the test > > > explaining that BlueZ behaves differently. See [1] for docs explaining how > the > > > behavior is different and [2] for other instances of this: > > > > > > [1] > > > > > > https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_device.h?sq=p... > > > [2] > > > > > > https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_adapter_unitt... > > > > > > > Grr... Bluez. > > > > I didn't go dig into Bluez code to see where it merges. I see that our code > > using BlueZ just reads out all the values BlueZ provides when a > > BluetoothAdapterBlueZ::DevicePropertyChanged is called. > > > > The fake here should mimic what BlueZ does -- and please include a comment > > pointing to the BlueZ code to blame for it. > > > > If our platforms behave differently, our unit tests should represent that and > > have the regrettable #if defined sections (as in [2] above). If we can fix it, > > we should file an issue and // TODO(issue-number) to fix it. > > > > Most ideally we'd fix BlueZ by having a way to clear the cached values. Can > our > > BluetoothDeviceBlueZ::UpdateServiceData clear the properties->service_data? > > As Ortuno mentioned, the current BlueZ caches all previously-found service data > and UUIDs, and the merging[1] happens when device found event is received by > bluetoothd. And I also agree that we should get it fixed as a CHROMIUM specific > fix, so please fix a bug and marked as p2 and marked the related issues within > the new bug. > > Out of curious, is there any existing user case where you observe issues caused > by the caching? > > [1]https://cs.corp.google.com/chromeos_public/src/third_party/bluez/src/shared/ad.c?q=src/shared/ad.c+file:%5Esrc/third_party/bluez/+package:%5Echromeos_public$&dr&l=557 Done. https://codereview.chromium.org/2783733002/diff/20001/device/bluetooth/dbus/f... device/bluetooth/dbus/fake_bluetooth_device_client.cc:1565: properties->service_data.ReplaceValue(service_data); On 2017/03/30 02:58:14, ortuno wrote: > +scheib fyi > > *sigh* I wish BlueZ worked like this. Sadly BlueZ caches and combines all > values. Attempts to convince BlueZ folks to change this have failed, so we are > stuck with this until the new stack. For example: > > 1. Receive advertisement with service data only for UUID 0x180a > 2. Receive advertisement with service data only for UUID 0x180b > > After the two advertisements arrive properties->service_data will contain both > values :( > > Because bluetooth_test is supposed to simulate the behavior of each platform > this function should match what BlueZ does and combine the new values with the > old values. > > This will cause the test to fail D: D: which is expected because the behavior in > BlueZ is different. So we need to add #if defined(OS_CHROMEOS) to the test > explaining that BlueZ behaves differently. See [1] for docs explaining how the > behavior is different and [2] for other instances of this: > > [1] > https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_device.h?sq=p... > [2] > https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_adapter_unitt... > Changed it to merge data like what BlueZ does.
lgtm with some minor comments https://codereview.chromium.org/2783733002/diff/60001/device/bluetooth/blueto... File device/bluetooth/bluetooth_device_unittest.cc (right): https://codereview.chromium.org/2783733002/diff/60001/device/bluetooth/blueto... device/bluetooth/bluetooth_device_unittest.cc:180: #if defined(OS_MACOSX) #if !defined(OS_LINUX) && !defined(OS_CHROMEOS) Since we will eventually run this on Windows and Android. Don't forget to change the comment below :) https://codereview.chromium.org/2783733002/diff/60001/device/bluetooth/blueto... device/bluetooth/bluetooth_device_unittest.cc:186: // Receive Advertisement with service data. optional: Could we add a test for when the service_data property is not valid? This can be done by adding a call to SimulateLowEnergyDevice(3) before SimulateLowEnergyDevice(1) and check that all maps and pointers are correct. https://codereview.chromium.org/2783733002/diff/60001/device/bluetooth/blueto... device/bluetooth/bluetooth_device_unittest.cc:201: #if defined(OS_MACOSX) To avoid having this #if for future platforms: #if defined(OS_CHROMEOS) || defined(OS_LINUX) // ... #else // ... #endif https://codereview.chromium.org/2783733002/diff/60001/device/bluetooth/blueto... device/bluetooth/bluetooth_device_unittest.cc:234: #if defined(OS_MACOSX) #if !defined(OS_LINUX) && !defined(OS_CHROMEOS) https://codereview.chromium.org/2783733002/diff/60001/device/bluetooth/test/b... File device/bluetooth/test/bluetooth_test.h (right): https://codereview.chromium.org/2783733002/diff/60001/device/bluetooth/test/b... device/bluetooth/test/bluetooth_test.h:133: // |device_ordinal| with the same device address stands for the same fake Thanks for this!
The CQ bit was checked by xiaoyinh@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/2783733002/diff/60001/device/bluetooth/blueto... File device/bluetooth/bluetooth_device_unittest.cc (right): https://codereview.chromium.org/2783733002/diff/60001/device/bluetooth/blueto... device/bluetooth/bluetooth_device_unittest.cc:180: #if defined(OS_MACOSX) On 2017/04/03 02:47:24, ortuno wrote: > #if !defined(OS_LINUX) && !defined(OS_CHROMEOS) > > Since we will eventually run this on Windows and Android. Don't forget to change > the comment below :) Done. https://codereview.chromium.org/2783733002/diff/60001/device/bluetooth/blueto... device/bluetooth/bluetooth_device_unittest.cc:186: // Receive Advertisement with service data. On 2017/04/03 02:47:24, ortuno wrote: > optional: Could we add a test for when the service_data property is not valid? > This can be done by adding a call to SimulateLowEnergyDevice(3) before > SimulateLowEnergyDevice(1) and check that all maps and pointers are correct. Thanks for the suggestion. I added another device SimulateLowEnergyDevice(4) for the invalid property case, that way we could verify both valid and invalid property case upon device creation. Let me know if this makes sense. https://codereview.chromium.org/2783733002/diff/60001/device/bluetooth/blueto... device/bluetooth/bluetooth_device_unittest.cc:201: #if defined(OS_MACOSX) On 2017/04/03 02:47:24, ortuno wrote: > To avoid having this #if for future platforms: > > #if defined(OS_CHROMEOS) || defined(OS_LINUX) > // ... > #else > // ... > #endif Done. https://codereview.chromium.org/2783733002/diff/60001/device/bluetooth/blueto... device/bluetooth/bluetooth_device_unittest.cc:234: #if defined(OS_MACOSX) On 2017/04/03 02:47:24, ortuno wrote: > #if !defined(OS_LINUX) && !defined(OS_CHROMEOS) Done.
lgtm
lgtm
lgtm https://codereview.chromium.org/2783733002/diff/60001/device/bluetooth/blueto... File device/bluetooth/bluetooth_device_unittest.cc (right): https://codereview.chromium.org/2783733002/diff/60001/device/bluetooth/blueto... device/bluetooth/bluetooth_device_unittest.cc:186: // Receive Advertisement with service data. On 2017/04/03 at 21:28:22, xiaoyinh wrote: > On 2017/04/03 02:47:24, ortuno wrote: > > optional: Could we add a test for when the service_data property is not valid? > > This can be done by adding a call to SimulateLowEnergyDevice(3) before > > SimulateLowEnergyDevice(1) and check that all maps and pointers are correct. > > Thanks for the suggestion. I added another device SimulateLowEnergyDevice(4) for the invalid property case, that way we could verify both valid and invalid property case upon device creation. Let me know if this makes sense. Looks great. Thanks.
The CQ bit was checked by xiaoyinh@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1491259310707930,
"parent_rev": "b434c19cb1005b7d3a7d36a6c1710bb8767d1b66", "commit_rev":
"ab0e2203cdb233620ef341cc297fc3d87ca0ddc5"}
Message was sent while issue was closed.
Description was changed from ========== Bluetooth: Add service data unit tests for chromeos This CL verifies that: 1. service data is populated correctly during device creation. 2. service data is updated correctly on property changed. BUG=688566 ========== to ========== Bluetooth: Add service data unit tests for chromeos This CL verifies that: 1. service data is populated correctly during device creation. 2. service data is updated correctly on property changed. BUG=688566 Review-Url: https://codereview.chromium.org/2783733002 Cr-Commit-Position: refs/heads/master@{#461565} Committed: https://chromium.googlesource.com/chromium/src/+/ab0e2203cdb233620ef341cc297f... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/ab0e2203cdb233620ef341cc297f...
Message was sent while issue was closed.
Findit identified this CL at revision 461565 as the culprit for failures in the build cycles as shown on: https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...
Message was sent while issue was closed.
https://codereview.chromium.org/2783733002/diff/80001/device/bluetooth/test/b... File device/bluetooth/test/bluetooth_test_mac.mm (left): https://codereview.chromium.org/2783733002/diff/80001/device/bluetooth/test/b... device/bluetooth/test/bluetooth_test_mac.mm:179: [NSData dataWithBytes:(unsigned char[]){2} length:1], Ah seems like we need to fix the test data for the following tests: BluetoothTest.AdvertisementData_ConnectionDuringDiscovery BluetoothTest.AdvertisementData_Discovery BluetoothTest.AdvertisementData_DiscoveryDuringConnection
Message was sent while issue was closed.
On 2017/04/03 23:55:51, ortuno wrote: > https://codereview.chromium.org/2783733002/diff/80001/device/bluetooth/test/b... > File device/bluetooth/test/bluetooth_test_mac.mm (left): > > https://codereview.chromium.org/2783733002/diff/80001/device/bluetooth/test/b... > device/bluetooth/test/bluetooth_test_mac.mm:179: [NSData dataWithBytes:(unsigned > char[]){2} length:1], > Ah seems like we need to fix the test data for the following tests: > > BluetoothTest.AdvertisementData_ConnectionDuringDiscovery > BluetoothTest.AdvertisementData_Discovery > BluetoothTest.AdvertisementData_DiscoveryDuringConnection So sorry about that! Should I revert this first?
Message was sent while issue was closed.
On 2017/04/03 23:57:31, xiaoyinh wrote: > So sorry about that! Should I revert this first? Yeah, let's revert!
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:80001) has been created in https://codereview.chromium.org/2792203003/ by xiaoyinh@chromium.org. The reason for reverting is: This CL broke Mac build.. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
