Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2641133003:
Bluetooth: macOS: Adding counter for service discovery callbacks. (Closed)
DescriptionBluetooth: macOS: Adding counter for service discovery callbacks.
Each time -[CBPeripheral discoverServices:] is called, BluetoothLowEnergyDeviceMac::DiscoverPrimaryServices()
has to be called for the device to complete.
BUG=697246
Review-Url: https://codereview.chromium.org/2641133003
Cr-Commit-Position: refs/heads/master@{#455002}
Committed: https://chromium.googlesource.com/chromium/src/+/0d7b7b5667f623683dcc42b6c60da2274ab1eb26
Patch Set 1 #Patch Set 2 : Unit test #Patch Set 3 : Adding logs #
Total comments: 9
Patch Set 4 : Better test #
Total comments: 8
Patch Set 5 : Better tests #Patch Set 6 : Resistant to bad device #
Total comments: 4
Patch Set 7 : Splitting test #
Total comments: 12
Patch Set 8 : . #Patch Set 9 : . #Patch Set 10 : Adding BluetoothTestMac::SimulateDidDiscoverServices() #
Total comments: 6
Patch Set 11 : Adding comment #Dependent Patchsets: Messages
Total messages: 44 (24 generated)
Patchset #1 (id:1) has been deleted
jlebel@chromium.org changed reviewers: + ortuno@chromium.org
Hello Giovanni, Can you review this patch related to have an exact match between -[CBPeripheral discoverServices:] calls, and BluetoothLowEnergyDeviceMac::DiscoverPrimaryServices() callback? Thanks,
The CQ bit was checked by jlebel@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.
Description was changed from ========== Bluetooth: macOS: Adding counter for service discovery callbacks. Each time -[CBPeripheral discoverServices:] is called, BluetoothLowEnergyDeviceMac::DiscoverPrimaryServices() has to be called for the device to complete. BUG= ========== to ========== Bluetooth: macOS: Adding counter for service discovery callbacks. Each time -[CBPeripheral discoverServices:] is called, BluetoothLowEnergyDeviceMac::DiscoverPrimaryServices() has to be called for the device to complete. BUG=681414 ==========
https://codereview.chromium.org/2641133003/diff/60001/device/bluetooth/blueto... File device/bluetooth/bluetooth_low_energy_device_mac.mm (right): https://codereview.chromium.org/2641133003/diff/60001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_device_mac.mm:218: DCHECK(discovery_pending_count_ >= 0); Looking at the code it seems we are calling DiscoverCharacteristics twice because we are getting two calls to DidDiscoverPrimaryServices. Is that correct? If so then I think this patch introduces another DCHECK that will fail: DCHECK(discovery_pending_count_ >= 0);: 1. DiscoverPrimaryServices(); // discovery_pending_count_ is now 1 2. DidDiscoverPrimaryServices(); // discovery_pending_count_ is now 0. 3. DidDiscoverPrimaryServices(); // discovery_pending_count_ is now -1 and DCHECK fails.
https://codereview.chromium.org/2641133003/diff/60001/device/bluetooth/blueto... File device/bluetooth/bluetooth_low_energy_device_mac.mm (right): https://codereview.chromium.org/2641133003/diff/60001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_device_mac.mm:218: DCHECK(discovery_pending_count_ >= 0); On 2017/01/22 20:39:14, ortuno wrote: > Looking at the code it seems we are calling DiscoverCharacteristics twice > because we are getting two calls to DidDiscoverPrimaryServices. Is that correct? > If so then I think this patch introduces another DCHECK that will fail: > DCHECK(discovery_pending_count_ >= 0);: > > 1. DiscoverPrimaryServices(); // discovery_pending_count_ is now 1 > 2. DidDiscoverPrimaryServices(); // discovery_pending_count_ is now 0. > 3. DidDiscoverPrimaryServices(); // discovery_pending_count_ is now -1 and > DCHECK fails. discovery_pending_count_ should never be -1. Do you want me to change the line 219 to: if (discovery_pending_count_ <= 0) {
https://codereview.chromium.org/2641133003/diff/60001/device/bluetooth/blueto... File device/bluetooth/bluetooth_low_energy_device_mac.mm (right): https://codereview.chromium.org/2641133003/diff/60001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_device_mac.mm:218: DCHECK(discovery_pending_count_ >= 0); On 2017/01/22 at 21:23:33, jlebel wrote: > On 2017/01/22 20:39:14, ortuno wrote: > > Looking at the code it seems we are calling DiscoverCharacteristics twice > > because we are getting two calls to DidDiscoverPrimaryServices. Is that correct? > > If so then I think this patch introduces another DCHECK that will fail: > > DCHECK(discovery_pending_count_ >= 0);: > > > > 1. DiscoverPrimaryServices(); // discovery_pending_count_ is now 1 > > 2. DidDiscoverPrimaryServices(); // discovery_pending_count_ is now 0. > > 3. DidDiscoverPrimaryServices(); // discovery_pending_count_ is now -1 and > > DCHECK fails. > > discovery_pending_count_ should never be -1. Do you want me to change the line 219 to: > if (discovery_pending_count_ <= 0) { I'm not sure that solves the problem. For example: 1. DiscoverPrimaryServices(); // discovery_pending_count_ is now 1 2. DidDiscoverPrimaryServices(); // discovery_pending_count_ is now 0. 3. DidDiscoverPrimaryServices(); // discovery_pending_count_ is now -1 and DCHECK fails. // Device disconnects and connects again. 4. DiscoverPrimaryServices(); // discovery_pending_count_ is now 0. 5. DidDiscoverPrimaryServices(); // discovery_pending_count_ is now -1 and DiscoverCharacteristics is called. 6. DidDiscoverPrimaryServices(); // discovery_pending_count_ is now -2 and DiscoverCharacteristics is called again. What about just using a boolean and checking it at the beginning of the method: DiscoverPrimaryServices() { pending_services_discovery_ = true; // initiate service discovery. } DidDiscoverPrimaryServices() { // Check for error and print if (!pending_services_discovery_) LOG(WARNING) << "Incorrect DidDiscoverPrimaryServices call." return; pending_services_discovery_ = false; // ... }
https://codereview.chromium.org/2641133003/diff/60001/device/bluetooth/blueto... File device/bluetooth/bluetooth_device_unittest.cc (right): https://codereview.chromium.org/2641133003/diff/60001/device/bluetooth/blueto... device/bluetooth/bluetooth_device_unittest.cc:403: TEST_F(BluetoothTest, DoubleDidModifyServicesEvent) { This is a good test for the case in which we get two DidModifyServices but if I understood you correctly this morning we need a test for when DidConnectGatt and DidModifyServices are called one right after the other. So: // Starts one discovery procedure. SimulateGattConnection() // Starts another discovery procedure. SimulateGattServicesChanged(...); SimulateGattServicesDiscovered(...); SimulateGattServicesDiscovered(...); https://codereview.chromium.org/2641133003/diff/60001/device/bluetooth/blueto... device/bluetooth/bluetooth_device_unittest.cc:429: SimulateGattServicesDiscovered(device, {} /* services */); Is there any way we can test that DiscoveryCharacteristics was not called for the first SimulateGattServicesDiscovered? https://codereview.chromium.org/2641133003/diff/60001/device/bluetooth/blueto... File device/bluetooth/bluetooth_low_energy_device_mac.h (right): https://codereview.chromium.org/2641133003/diff/60001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_device_mac.h:146: // Increases each time -[CBPeripheral discoverServices:] is called, and nit optional: I would move this comment to DiscoverPrimaryServices.
https://codereview.chromium.org/2641133003/diff/60001/device/bluetooth/blueto... File device/bluetooth/bluetooth_device_unittest.cc (right): https://codereview.chromium.org/2641133003/diff/60001/device/bluetooth/blueto... device/bluetooth/bluetooth_device_unittest.cc:403: TEST_F(BluetoothTest, DoubleDidModifyServicesEvent) { On 2017/01/23 06:12:38, ortuno wrote: > This is a good test for the case in which we get two DidModifyServices but if I > understood you correctly this morning we need a test for when DidConnectGatt and > DidModifyServices are called one right after the other. > > So: > > // Starts one discovery procedure. > SimulateGattConnection() > // Starts another discovery procedure. > SimulateGattServicesChanged(...); > > SimulateGattServicesDiscovered(...); > SimulateGattServicesDiscovered(...); Done. https://codereview.chromium.org/2641133003/diff/60001/device/bluetooth/blueto... device/bluetooth/bluetooth_device_unittest.cc:429: SimulateGattServicesDiscovered(device, {} /* services */); On 2017/01/23 06:12:38, ortuno wrote: > Is there any way we can test that DiscoveryCharacteristics was not called for > the first SimulateGattServicesDiscovered? I guess the simplest way to do that would be to add a counter in BluetoothTestMac, and make the MockCBPeripheral increment the counter. MockCBPeripheral instance already know about the BluetoothTestMac instance. So it would not be too hard. But it doesn't go exactly like the observer pattern that already exist. Would it work for you with this simple way?
https://codereview.chromium.org/2641133003/diff/60001/device/bluetooth/blueto... File device/bluetooth/bluetooth_device_unittest.cc (right): https://codereview.chromium.org/2641133003/diff/60001/device/bluetooth/blueto... device/bluetooth/bluetooth_device_unittest.cc:429: SimulateGattServicesDiscovered(device, {} /* services */); On 2017/01/23 at 18:49:38, jlebel wrote: > On 2017/01/23 06:12:38, ortuno wrote: > > Is there any way we can test that DiscoveryCharacteristics was not called for > > the first SimulateGattServicesDiscovered? > > I guess the simplest way to do that would be to add a counter in BluetoothTestMac, and make the MockCBPeripheral increment the counter. MockCBPeripheral instance already know about the BluetoothTestMac instance. So it would not be too hard. But it doesn't go exactly like the observer pattern that already exist. > Would it work for you with this simple way? SGTM https://codereview.chromium.org/2641133003/diff/80001/device/bluetooth/blueto... File device/bluetooth/bluetooth_device_unittest.cc (right): https://codereview.chromium.org/2641133003/diff/80001/device/bluetooth/blueto... device/bluetooth/bluetooth_device_unittest.cc:399: // Tests that receiving 2 notifications in a row from macOS that services has Add why this doesn't apply to Android: // Android: This test doesn't apply to Android because there is no services changed event that could arrive during a discovery procedure. https://codereview.chromium.org/2641133003/diff/80001/device/bluetooth/blueto... device/bluetooth/bluetooth_device_unittest.cc:403: // device changed notification should be sent. I think this should be: the characteristic discovery procedure should be started. https://codereview.chromium.org/2641133003/diff/80001/device/bluetooth/blueto... device/bluetooth/bluetooth_device_unittest.cc:426: SimulateGattServicesDiscovered(device, {} /* services */); Could we add a service here and a different one below and make sure both are there after discovery has completed? https://codereview.chromium.org/2641133003/diff/80001/device/bluetooth/blueto... File device/bluetooth/bluetooth_low_energy_device_mac.mm (right): https://codereview.chromium.org/2641133003/diff/80001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_device_mac.mm:348: [GetPeripheral() discoverServices:nil]; nit: Move this to the end of the function.
Patchset #5 (id:100001) has been deleted
https://codereview.chromium.org/2641133003/diff/80001/device/bluetooth/blueto... File device/bluetooth/bluetooth_device_unittest.cc (right): https://codereview.chromium.org/2641133003/diff/80001/device/bluetooth/blueto... device/bluetooth/bluetooth_device_unittest.cc:399: // Tests that receiving 2 notifications in a row from macOS that services has On 2017/01/23 20:44:02, ortuno wrote: > Add why this doesn't apply to Android: > > // Android: This test doesn't apply to Android because there is no services > changed event that could arrive during a discovery procedure. Done. https://codereview.chromium.org/2641133003/diff/80001/device/bluetooth/blueto... device/bluetooth/bluetooth_device_unittest.cc:403: // device changed notification should be sent. On 2017/01/23 20:44:02, ortuno wrote: > I think this should be: the characteristic discovery procedure should be > started. Done. https://codereview.chromium.org/2641133003/diff/80001/device/bluetooth/blueto... device/bluetooth/bluetooth_device_unittest.cc:426: SimulateGattServicesDiscovered(device, {} /* services */); On 2017/01/23 20:44:02, ortuno wrote: > Could we add a service here and a different one below and make sure both are > there after discovery has completed? Done. https://codereview.chromium.org/2641133003/diff/80001/device/bluetooth/blueto... File device/bluetooth/bluetooth_low_energy_device_mac.mm (right): https://codereview.chromium.org/2641133003/diff/80001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_device_mac.mm:348: [GetPeripheral() discoverServices:nil]; On 2017/01/23 20:44:02, ortuno wrote: > nit: Move this to the end of the function. Done.
The CQ bit was checked by jlebel@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: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by jlebel@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...
On 2017/01/26 00:36:54, jlebel wrote: > https://codereview.chromium.org/2641133003/diff/80001/device/bluetooth/blueto... > File device/bluetooth/bluetooth_device_unittest.cc (right): > > https://codereview.chromium.org/2641133003/diff/80001/device/bluetooth/blueto... > device/bluetooth/bluetooth_device_unittest.cc:399: // Tests that receiving 2 > notifications in a row from macOS that services has > On 2017/01/23 20:44:02, ortuno wrote: > > Add why this doesn't apply to Android: > > > > // Android: This test doesn't apply to Android because there is no services > > changed event that could arrive during a discovery procedure. > > Done. > > https://codereview.chromium.org/2641133003/diff/80001/device/bluetooth/blueto... > device/bluetooth/bluetooth_device_unittest.cc:403: // device changed > notification should be sent. > On 2017/01/23 20:44:02, ortuno wrote: > > I think this should be: the characteristic discovery procedure should be > > started. > > Done. > > https://codereview.chromium.org/2641133003/diff/80001/device/bluetooth/blueto... > device/bluetooth/bluetooth_device_unittest.cc:426: > SimulateGattServicesDiscovered(device, {} /* services */); > On 2017/01/23 20:44:02, ortuno wrote: > > Could we add a service here and a different one below and make sure both are > > there after discovery has completed? > > Done. > > https://codereview.chromium.org/2641133003/diff/80001/device/bluetooth/blueto... > File device/bluetooth/bluetooth_low_energy_device_mac.mm (right): > > https://codereview.chromium.org/2641133003/diff/80001/device/bluetooth/blueto... > device/bluetooth/bluetooth_low_energy_device_mac.mm:348: [GetPeripheral() > discoverServices:nil]; > On 2017/01/23 20:44:02, ortuno wrote: > > nit: Move this to the end of the function. > > Done. Are you ok with this change? https://codereview.chromium.org/2641133003/diff2/120001:140001/device/bluetoo... I update the unit test DoubleDidModifyServicesEvent to test this change.
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_TIMED_OUT, no build URL) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
https://codereview.chromium.org/2641133003/diff/140001/device/bluetooth/bluet... File device/bluetooth/bluetooth_device_unittest.cc (right): https://codereview.chromium.org/2641133003/diff/140001/device/bluetooth/bluet... device/bluetooth/bluetooth_device_unittest.cc:399: // Tests that receiving 2 notifications in a row from macOS that services has Good test! Seems like we are testing two things though: 1. Two pending discovery requests. 2. Incorrect didDiscoverServices. Do you think we can split it into two tests: TwoPendingServiceDiscoveryRequests ExtraDidDiscoverServicesCall https://codereview.chromium.org/2641133003/diff/140001/device/bluetooth/bluet... device/bluetooth/bluetooth_device_unittest.cc:432: services1.push_back(BluetoothUUID(kTestUUIDHeartRate).canonical_value()); I think we can just use kTestUUIDHeartRate since it's already in its canonical form.
https://codereview.chromium.org/2641133003/diff/140001/device/bluetooth/bluet... File device/bluetooth/bluetooth_device_unittest.cc (right): https://codereview.chromium.org/2641133003/diff/140001/device/bluetooth/bluet... device/bluetooth/bluetooth_device_unittest.cc:399: // Tests that receiving 2 notifications in a row from macOS that services has On 2017/02/27 07:01:41, ortuno wrote: > Good test! Seems like we are testing two things though: > > 1. Two pending discovery requests. > 2. Incorrect didDiscoverServices. > > Do you think we can split it into two tests: > > TwoPendingServiceDiscoveryRequests > ExtraDidDiscoverServicesCall Done. https://codereview.chromium.org/2641133003/diff/140001/device/bluetooth/bluet... device/bluetooth/bluetooth_device_unittest.cc:432: services1.push_back(BluetoothUUID(kTestUUIDHeartRate).canonical_value()); On 2017/02/27 07:01:41, ortuno wrote: > I think we can just use kTestUUIDHeartRate since it's already in its canonical > form. Done.
https://codereview.chromium.org/2641133003/diff/160001/device/bluetooth/bluet... File device/bluetooth/bluetooth_device_unittest.cc (right): https://codereview.chromium.org/2641133003/diff/160001/device/bluetooth/bluet... device/bluetooth/bluetooth_device_unittest.cc:445: EXPECT_EQ(gatt_characteristic_discovery_attempts_, 2); Shouldn't there be only one characteristic discovery attempt? https://codereview.chromium.org/2641133003/diff/160001/device/bluetooth/bluet... device/bluetooth/bluetooth_device_unittest.cc:475: EXPECT_EQ(gatt_characteristic_discovery_attempts_, 0); I'm surprised the legitimate system call doesn't result in a characteristic discovery attempt. https://codereview.chromium.org/2641133003/diff/160001/device/bluetooth/bluet... File device/bluetooth/bluetooth_low_energy_device_mac.mm (right): https://codereview.chromium.org/2641133003/diff/160001/device/bluetooth/bluet... device/bluetooth/bluetooth_low_energy_device_mac.mm:195: discovery_pending_count_ = 0; I think just return. I don't think we want to start discovery procedures because buggy events.
https://codereview.chromium.org/2641133003/diff/160001/device/bluetooth/bluet... File device/bluetooth/bluetooth_device_unittest.cc (right): https://codereview.chromium.org/2641133003/diff/160001/device/bluetooth/bluet... device/bluetooth/bluetooth_device_unittest.cc:445: EXPECT_EQ(gatt_characteristic_discovery_attempts_, 2); On 2017/02/28 02:06:50, ortuno wrote: > Shouldn't there be only one characteristic discovery attempt? We discover characteristics for kTestUUIDHeartRate and then for kTestUUIDImmediateAlert. So -[MockCBPeripehral discoverCharacteristics:forService:] is called twice. https://codereview.chromium.org/2641133003/diff/160001/device/bluetooth/bluet... device/bluetooth/bluetooth_device_unittest.cc:475: EXPECT_EQ(gatt_characteristic_discovery_attempts_, 0); On 2017/02/28 02:06:50, ortuno wrote: > I'm surprised the legitimate system call doesn't result in a characteristic > discovery attempt. We have no service, therefore, -[MockCBPeripehral discoverCharacteristics:forService:] can't be called. https://codereview.chromium.org/2641133003/diff/160001/device/bluetooth/bluet... File device/bluetooth/bluetooth_low_energy_device_mac.mm (right): https://codereview.chromium.org/2641133003/diff/160001/device/bluetooth/bluet... device/bluetooth/bluetooth_low_energy_device_mac.mm:195: discovery_pending_count_ = 0; On 2017/02/28 02:06:50, ortuno wrote: > I think just return. I don't think we want to start discovery procedures because > buggy events. ok.
lgtm with some optional suggestions. https://codereview.chromium.org/2641133003/diff/160001/device/bluetooth/bluet... File device/bluetooth/bluetooth_device_unittest.cc (right): https://codereview.chromium.org/2641133003/diff/160001/device/bluetooth/bluet... device/bluetooth/bluetooth_device_unittest.cc:433: SimulateGattServicesDiscovered(device, services1); optional: You could do: SimulateGattServicesDiscovered(device, std::vector<std::string>({kTestUUIDHeartRate})); Same below. https://codereview.chromium.org/2641133003/diff/160001/device/bluetooth/bluet... device/bluetooth/bluetooth_device_unittest.cc:445: EXPECT_EQ(gatt_characteristic_discovery_attempts_, 2); On 2017/02/28 at 23:16:18, jlebel wrote: > On 2017/02/28 02:06:50, ortuno wrote: > > Shouldn't there be only one characteristic discovery attempt? > > We discover characteristics for kTestUUIDHeartRate and then for kTestUUIDImmediateAlert. So -[MockCBPeripehral discoverCharacteristics:forService:] is called twice. Ah that makes sense. Thanks. https://codereview.chromium.org/2641133003/diff/160001/device/bluetooth/bluet... device/bluetooth/bluetooth_device_unittest.cc:475: EXPECT_EQ(gatt_characteristic_discovery_attempts_, 0); On 2017/02/28 at 23:16:18, jlebel wrote: > On 2017/02/28 02:06:50, ortuno wrote: > > I'm surprised the legitimate system call doesn't result in a characteristic > > discovery attempt. > > We have no service, therefore, -[MockCBPeripehral > discoverCharacteristics:forService:] can't be called. Ah right. optional: Add a service to make it clearer that a characteristic discovery attempt will be initiated for legitimate notifications but not for illegitimate notifications.
Description was changed from ========== Bluetooth: macOS: Adding counter for service discovery callbacks. Each time -[CBPeripheral discoverServices:] is called, BluetoothLowEnergyDeviceMac::DiscoverPrimaryServices() has to be called for the device to complete. BUG=681414 ========== to ========== Bluetooth: macOS: Adding counter for service discovery callbacks. Each time -[CBPeripheral discoverServices:] is called, BluetoothLowEnergyDeviceMac::DiscoverPrimaryServices() has to be called for the device to complete. BUG=697246 ==========
https://codereview.chromium.org/2641133003/diff/160001/device/bluetooth/bluet... File device/bluetooth/bluetooth_device_unittest.cc (right): https://codereview.chromium.org/2641133003/diff/160001/device/bluetooth/bluet... device/bluetooth/bluetooth_device_unittest.cc:433: SimulateGattServicesDiscovered(device, services1); On 2017/02/28 23:28:11, ortuno wrote: > optional: You could do: > > SimulateGattServicesDiscovered(device, > std::vector<std::string>({kTestUUIDHeartRate})); > > Same below. Done. https://codereview.chromium.org/2641133003/diff/160001/device/bluetooth/bluet... device/bluetooth/bluetooth_device_unittest.cc:475: EXPECT_EQ(gatt_characteristic_discovery_attempts_, 0); On 2017/02/28 23:28:11, ortuno wrote: > On 2017/02/28 at 23:16:18, jlebel wrote: > > On 2017/02/28 02:06:50, ortuno wrote: > > > I'm surprised the legitimate system call doesn't result in a characteristic > > > discovery attempt. > > > > We have no service, therefore, -[MockCBPeripehral > > discoverCharacteristics:forService:] can't be called. > > Ah right. optional: Add a service to make it clearer that a characteristic > discovery attempt will be initiated for legitimate notifications but not for > illegitimate notifications. If I do that, this DCHECK fails: https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_remote_gatt_s... SimulateGattServicesDiscovered() has been implemented to simulate all system call related to "DidDiscover" (services, characteristics and descriptors). So if our device already knows about a service, than this extra SimulateGattServicesDiscovered() will call -[<id>CBPeripheralDelegate peripheral:didDiscoverServices:] and then also -[id<CBPeripheralDelegate> peripheral:didDiscoverCharacteristicsForService:error:]. So I guess that's one unittest that I can create for crrev.com/2638653002
https://codereview.chromium.org/2641133003/diff/160001/device/bluetooth/bluet... File device/bluetooth/bluetooth_device_unittest.cc (right): https://codereview.chromium.org/2641133003/diff/160001/device/bluetooth/bluet... device/bluetooth/bluetooth_device_unittest.cc:475: EXPECT_EQ(gatt_characteristic_discovery_attempts_, 0); On 2017/02/28 23:52:16, jlebel wrote: > On 2017/02/28 23:28:11, ortuno wrote: > > On 2017/02/28 at 23:16:18, jlebel wrote: > > > On 2017/02/28 02:06:50, ortuno wrote: > > > > I'm surprised the legitimate system call doesn't result in a > characteristic > > > > discovery attempt. > > > > > > We have no service, therefore, -[MockCBPeripehral > > > discoverCharacteristics:forService:] can't be called. > > > > Ah right. optional: Add a service to make it clearer that a characteristic > > discovery attempt will be initiated for legitimate notifications but not for > > illegitimate notifications. > > If I do that, this DCHECK fails: > https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_remote_gatt_s... > > SimulateGattServicesDiscovered() has been implemented to simulate all system > call related to "DidDiscover" (services, characteristics and descriptors). So if > our device already knows about a service, than this extra > SimulateGattServicesDiscovered() will call -[<id>CBPeripheralDelegate > peripheral:didDiscoverServices:] and then also -[id<CBPeripheralDelegate> > peripheral:didDiscoverCharacteristicsForService:error:]. > > So I guess that's one unittest that I can create for crrev.com/2638653002 Done.
Patchset #10 (id:220001) has been deleted
Patchset #10 (id:240001) has been deleted
Patchset #10 (id:260001) has been deleted
lgtm bar one comment and suggestion for future patches. https://codereview.chromium.org/2641133003/diff/230009/device/bluetooth/bluet... File device/bluetooth/bluetooth_low_energy_device_mac.mm (right): https://codereview.chromium.org/2641133003/diff/230009/device/bluetooth/bluet... device/bluetooth/bluetooth_low_energy_device_mac.mm:232: gatt_service_mac->DiscoverCharacteristics(); Unrelated to this patch but doesn't this mean that we rediscover characteristics for all services even the ones that we know haven't changed? https://codereview.chromium.org/2641133003/diff/230009/device/bluetooth/test/... File device/bluetooth/test/bluetooth_test_mac.h (right): https://codereview.chromium.org/2641133003/diff/230009/device/bluetooth/test/... device/bluetooth/test/bluetooth_test_mac.h:92: void SimulateDidDiscoverServices(BluetoothDevice* device, Explain why this is specific to macOS e.g.: macOS is the only platform for which we need to discover each set of attributes individually so we need a method to simulate discovering each set of attributes. https://codereview.chromium.org/2641133003/diff/230009/device/bluetooth/test/... File device/bluetooth/test/bluetooth_test_mac.mm (right): https://codereview.chromium.org/2641133003/diff/230009/device/bluetooth/test/... device/bluetooth/test/bluetooth_test_mac.mm:491: void BluetoothTestMac::SimulateDidDiscoverServices( Seems like we are repeating a lot of code from SimulateGattServicesDiscovered is there any way we can reuse that code? OK to leave for future patches.
https://codereview.chromium.org/2641133003/diff/230009/device/bluetooth/bluet... File device/bluetooth/bluetooth_low_energy_device_mac.mm (right): https://codereview.chromium.org/2641133003/diff/230009/device/bluetooth/bluet... device/bluetooth/bluetooth_low_energy_device_mac.mm:232: gatt_service_mac->DiscoverCharacteristics(); On 2017/03/06 09:49:55, ortuno wrote: > Unrelated to this patch but doesn't this mean that we rediscover characteristics > for all services even the ones that we know haven't changed? Correct. I can do better. crbug.com/698858 https://codereview.chromium.org/2641133003/diff/230009/device/bluetooth/test/... File device/bluetooth/test/bluetooth_test_mac.h (right): https://codereview.chromium.org/2641133003/diff/230009/device/bluetooth/test/... device/bluetooth/test/bluetooth_test_mac.h:92: void SimulateDidDiscoverServices(BluetoothDevice* device, On 2017/03/06 09:49:55, ortuno wrote: > Explain why this is specific to macOS e.g.: > > macOS is the only platform for which we need to discover each set of attributes > individually so we need a method to simulate discovering each set of attributes. Done. https://codereview.chromium.org/2641133003/diff/230009/device/bluetooth/test/... File device/bluetooth/test/bluetooth_test_mac.mm (right): https://codereview.chromium.org/2641133003/diff/230009/device/bluetooth/test/... device/bluetooth/test/bluetooth_test_mac.mm:491: void BluetoothTestMac::SimulateDidDiscoverServices( On 2017/03/06 09:49:55, ortuno wrote: > Seems like we are repeating a lot of code from SimulateGattServicesDiscovered is > there any way we can reuse that code? OK to leave for future patches. crrev.com/2738563002
The CQ bit was checked by jlebel@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ortuno@chromium.org Link to the patchset: https://codereview.chromium.org/2641133003/#ps290001 (title: "Adding comment")
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": 290001, "attempt_start_ts": 1488838410627970,
"parent_rev": "d5fda4e9a4c9fc1a0f0a061506f41c47c78b3961", "commit_rev":
"0d7b7b5667f623683dcc42b6c60da2274ab1eb26"}
Message was sent while issue was closed.
Description was changed from ========== Bluetooth: macOS: Adding counter for service discovery callbacks. Each time -[CBPeripheral discoverServices:] is called, BluetoothLowEnergyDeviceMac::DiscoverPrimaryServices() has to be called for the device to complete. BUG=697246 ========== to ========== Bluetooth: macOS: Adding counter for service discovery callbacks. Each time -[CBPeripheral discoverServices:] is called, BluetoothLowEnergyDeviceMac::DiscoverPrimaryServices() has to be called for the device to complete. BUG=697246 Review-Url: https://codereview.chromium.org/2641133003 Cr-Commit-Position: refs/heads/master@{#455002} Committed: https://chromium.googlesource.com/chromium/src/+/0d7b7b5667f623683dcc42b6c60d... ==========
Message was sent while issue was closed.
Committed patchset #11 (id:290001) as https://chromium.googlesource.com/chromium/src/+/0d7b7b5667f623683dcc42b6c60d... |
