Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2853933002:
bluetooth: macOS: Support for extra didConnectPeripheral event from macOS. (Closed)
Descriptionbluetooth: macOS: Support for extra didConnectPeripheral event from macOS.
There is no need to discover services when receive an extra didConnectPeripheral
event from macOS.
BUG=681414
Review-Url: https://codereview.chromium.org/2853933002
Cr-Commit-Position: refs/heads/master@{#469493}
Committed: https://chromium.googlesource.com/chromium/src/+/3f28cf2cfbf216ae1309895138d9bf25aebe068f
Patch Set 1 #
Total comments: 14
Patch Set 2 : . #Patch Set 3 : Adding Android bug #
Total comments: 4
Patch Set 4 : Adding BluetoothLowEnergyDeviceMac::DidConnectPeripheral() #
Total comments: 4
Patch Set 5 : Fixing test for observer.device_changed_count() #
Total comments: 2
Patch Set 6 : Comment #Patch Set 7 : merge #
Messages
Total messages: 33 (18 generated)
Patchset #1 (id:1) has been deleted
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...
jlebel@chromium.org changed reviewers: + ortuno@chromium.org
Hello Giovanni, Can you review this patch about the double peripheralDidConnect event from macOS? Thanks,
Patchset #1 (id:20001) has been deleted
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...
Description was changed from ========== Support for extra didConnectPeripheral event from macOS. There is no need to discover services when receive an extra didConnectPeripheral event from macOS. BUG=681414 ========== to ========== bluetooth: macOS: Support for extra didConnectPeripheral event from macOS. There is no need to discover services when receive an extra didConnectPeripheral event from macOS. BUG=681414 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
https://codereview.chromium.org/2853933002/diff/40001/device/bluetooth/blueto... File device/bluetooth/bluetooth_low_energy_device_mac.mm (right): https://codereview.chromium.org/2853933002/diff/40001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_device_mac.mm:194: void BluetoothLowEnergyDeviceMac::DidConnectGatt() { Why can't we do this in DidConnectPeripheral before calling DidConnectGatt? https://codereview.chromium.org/2853933002/diff/40001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_device_mac.mm:504: const char* name_cstr = name ? name->c_str() : ""; optional. You might be able to avoid this: << name->value_or("Unnamed device") << ... https://codereview.chromium.org/2853933002/diff/40001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_device_mac.mm:508: << &device << ", " << is_gatt_connected << "\"" << name_cstr Would the following work? << &device << ", GATT Connected: " << device.IsGattConnected() << "\"" ...
If we want to match Android behavior, this patch seems a bit useless (except the log part). https://codereview.chromium.org/2853933002/diff/40001/device/bluetooth/blueto... File device/bluetooth/bluetooth_device_unittest.cc (right): https://codereview.chromium.org/2853933002/diff/40001/device/bluetooth/blueto... device/bluetooth/bluetooth_device_unittest.cc:1043: EXPECT_EQ(1, gatt_discovery_attempts_); For Android, gatt_discovery_attempts_ is 2. So each time there is a gatt connection event from the system, there is a gatt discovery. Do we want the same on macOS? That would mean a full discovery cycle for macOS. https://codereview.chromium.org/2853933002/diff/40001/device/bluetooth/blueto... File device/bluetooth/bluetooth_low_energy_device_mac.mm (right): https://codereview.chromium.org/2853933002/diff/40001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_device_mac.mm:194: void BluetoothLowEnergyDeviceMac::DidConnectGatt() { On 2017/05/02 01:24:23, ortuno wrote: > Why can't we do this in DidConnectPeripheral before calling DidConnectGatt? That's a behavior of the device, no? https://codereview.chromium.org/2853933002/diff/40001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_device_mac.mm:504: const char* name_cstr = name ? name->c_str() : ""; On 2017/05/02 01:24:23, ortuno wrote: > optional. You might be able to avoid this: > > << name->value_or("Unnamed device") << ... Done. https://codereview.chromium.org/2853933002/diff/40001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_device_mac.mm:508: << &device << ", " << is_gatt_connected << "\"" << name_cstr On 2017/05/02 01:24:23, ortuno wrote: > Would the following work? > > << &device << ", GATT Connected: " << device.IsGattConnected() << "\"" ... The result is: GATT Connected: 0 GATT Connected: 1
https://codereview.chromium.org/2853933002/diff/40001/device/bluetooth/blueto... File device/bluetooth/bluetooth_device_unittest.cc (right): https://codereview.chromium.org/2853933002/diff/40001/device/bluetooth/blueto... device/bluetooth/bluetooth_device_unittest.cc:1043: EXPECT_EQ(1, gatt_discovery_attempts_); On 2017/05/02 at 19:54:46, jlebel wrote: > For Android, gatt_discovery_attempts_ is 2. So each time there is a gatt connection event from the system, there is a gatt discovery. > > Do we want the same on macOS? That would mean a full discovery cycle for macOS. D: That's definitely a bug in Android. Just open an issue about it, add a #if defined(OS_ANDROID) and mention the issue. https://codereview.chromium.org/2853933002/diff/40001/device/bluetooth/blueto... File device/bluetooth/bluetooth_low_energy_device_mac.mm (right): https://codereview.chromium.org/2853933002/diff/40001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_device_mac.mm:194: void BluetoothLowEnergyDeviceMac::DidConnectGatt() { On 2017/05/02 at 19:54:46, jlebel wrote: > On 2017/05/02 01:24:23, ortuno wrote: > > Why can't we do this in DidConnectPeripheral before calling DidConnectGatt? > > That's a behavior of the device, no? Hmm I'm not sure I understand what you mean. DidConnectGatt is supposed to be a non-virtual cross platform function that implementations use once a device finishes connection. I think it makes sense to set connected_ before DidConnectGatt and dispatch service discovery there as well. https://codereview.chromium.org/2853933002/diff/40001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_device_mac.mm:508: << &device << ", " << is_gatt_connected << "\"" << name_cstr On 2017/05/02 at 19:54:46, jlebel wrote: > On 2017/05/02 01:24:23, ortuno wrote: > > Would the following work? > > > > << &device << ", GATT Connected: " << device.IsGattConnected() << "\"" ... > > The result is: > GATT Connected: 0 > GATT Connected: 1 I think that's good enough. But up to you.
https://codereview.chromium.org/2853933002/diff/40001/device/bluetooth/blueto... File device/bluetooth/bluetooth_device_unittest.cc (right): https://codereview.chromium.org/2853933002/diff/40001/device/bluetooth/blueto... device/bluetooth/bluetooth_device_unittest.cc:1043: EXPECT_EQ(1, gatt_discovery_attempts_); On 2017/05/02 23:36:45, ortuno wrote: > On 2017/05/02 at 19:54:46, jlebel wrote: > > For Android, gatt_discovery_attempts_ is 2. So each time there is a gatt > connection event from the system, there is a gatt discovery. > > > > Do we want the same on macOS? That would mean a full discovery cycle for > macOS. > > D: That's definitely a bug in Android. Just open an issue about it, add a #if > defined(OS_ANDROID) and mention the issue. Done. https://codereview.chromium.org/2853933002/diff/40001/device/bluetooth/blueto... File device/bluetooth/bluetooth_low_energy_device_mac.mm (right): https://codereview.chromium.org/2853933002/diff/40001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_device_mac.mm:194: void BluetoothLowEnergyDeviceMac::DidConnectGatt() { On 2017/05/02 23:36:45, ortuno wrote: > On 2017/05/02 at 19:54:46, jlebel wrote: > > On 2017/05/02 01:24:23, ortuno wrote: > > > Why can't we do this in DidConnectPeripheral before calling DidConnectGatt? > > > > That's a behavior of the device, no? > > Hmm I'm not sure I understand what you mean. DidConnectGatt is supposed to be a > non-virtual cross platform function that implementations use once a device > finishes connection. I think it makes sense to set connected_ before > DidConnectGatt and dispatch service discovery there as well. I'm not sure how to the same in BluetoothAdapterMac::DidConnectPeripheral() First I need to update |connected_|, this can be done only in BluetoothLowEnergyDeviceMac. Plus, DiscoverPrimaryServices() seems to be more logical to be called in BluetoothLowEnergyDeviceMac. https://codereview.chromium.org/2853933002/diff/40001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_device_mac.mm:508: << &device << ", " << is_gatt_connected << "\"" << name_cstr On 2017/05/02 23:36:45, ortuno wrote: > On 2017/05/02 at 19:54:46, jlebel wrote: > > On 2017/05/02 01:24:23, ortuno wrote: > > > Would the following work? > > > > > > << &device << ", GATT Connected: " << device.IsGattConnected() << "\"" ... > > > > The result is: > > GATT Connected: 0 > > GATT Connected: 1 > > I think that's good enough. But up to you. I definitely prefer with english words to be more explicit.
https://codereview.chromium.org/2853933002/diff/40001/device/bluetooth/blueto... File device/bluetooth/bluetooth_low_energy_device_mac.mm (right): https://codereview.chromium.org/2853933002/diff/40001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_device_mac.mm:194: void BluetoothLowEnergyDeviceMac::DidConnectGatt() { On 2017/05/03 at 21:16:19, jlebel wrote: > On 2017/05/02 23:36:45, ortuno wrote: > > On 2017/05/02 at 19:54:46, jlebel wrote: > > > On 2017/05/02 01:24:23, ortuno wrote: > > > > Why can't we do this in DidConnectPeripheral before calling DidConnectGatt? > > > > > > That's a behavior of the device, no? > > > > Hmm I'm not sure I understand what you mean. DidConnectGatt is supposed to be a > > non-virtual cross platform function that implementations use once a device > > finishes connection. I think it makes sense to set connected_ before > > DidConnectGatt and dispatch service discovery there as well. > > I'm not sure how to the same in BluetoothAdapterMac::DidConnectPeripheral() > First I need to update |connected_|, this can be done only in BluetoothLowEnergyDeviceMac. > Plus, DiscoverPrimaryServices() seems to be more logical to be called in BluetoothLowEnergyDeviceMac. Ah. I missed the fact that DidConnectPeripheral is in the Adapter. What do you think about having BluetoothAdapterMac just forward the event to the device. Like we do for characteristics in mac[1] or for connection events in Android[2]: // bluetooth_adapter_mac.mm void BluetoothAdapterMac::DidConnectPeripheral(CBPeripheral* peripheral) { BluetoothLowEnergyDeviceMac* device_mac = GetBluetoothLowEnergyDeviceMac(peripheral); if (!device_mac) { [low_energy_central_manager_ cancelPeripheralConnection:peripheral]; return; } device_mac->DidConnectPeripheral(); } // bluetooth_low_energy_device_mac.mm void BluetoothLowEnergyDeviceMac::DidConnectPeripheral() { const bool should_discover_services = !connected_; connected_ = true; if (should_discover_services) DiscoverPrimaryServices(); DidConnectGatt(); } This is very similar to what you propose with the added benefit of keeping DidConnectGatt() truly cross platform. [1] https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_low_energy_de... [2] https://cs.chromium.org/chromium/src/device/bluetooth/android/java/src/org/ch... https://codereview.chromium.org/2853933002/diff/80001/device/bluetooth/blueto... File device/bluetooth/bluetooth_device_unittest.cc (right): https://codereview.chromium.org/2853933002/diff/80001/device/bluetooth/blueto... device/bluetooth/bluetooth_device_unittest.cc:1031: #if !defined(OS_ANDROID) #if defined(OS_ANDROID) EXPECT_EQ(2, gatt_discovery_attempts_); #else // !defined(OS_ANDROID) EXPECT_EQ(1, gatt_discovery_attempts_); #endif // defined(OS_ANDROID) https://codereview.chromium.org/2853933002/diff/80001/device/bluetooth/blueto... device/bluetooth/bluetooth_device_unittest.cc:1032: // TODO(crbug.com/718168): on Android gatt_discovery_attempts_ is 2. // Android incorrectly starts second discovery for devices that are already connected. // TODO(crbug.com/718168): Remove once Android is fixed.
Patchset #4 (id:100001) has been deleted
https://codereview.chromium.org/2853933002/diff/80001/device/bluetooth/blueto... File device/bluetooth/bluetooth_device_unittest.cc (right): https://codereview.chromium.org/2853933002/diff/80001/device/bluetooth/blueto... device/bluetooth/bluetooth_device_unittest.cc:1031: #if !defined(OS_ANDROID) On 2017/05/04 00:31:03, ortuno wrote: > #if defined(OS_ANDROID) > EXPECT_EQ(2, gatt_discovery_attempts_); > #else // !defined(OS_ANDROID) > EXPECT_EQ(1, gatt_discovery_attempts_); > #endif // defined(OS_ANDROID) Done. https://codereview.chromium.org/2853933002/diff/80001/device/bluetooth/blueto... device/bluetooth/bluetooth_device_unittest.cc:1032: // TODO(crbug.com/718168): on Android gatt_discovery_attempts_ is 2. On 2017/05/04 00:31:03, ortuno wrote: > // Android incorrectly starts second discovery for devices that are already > connected. > // TODO(crbug.com/718168): Remove once Android is fixed. Done.
Code looks good. Just a couple of questions. https://codereview.chromium.org/2853933002/diff/120001/device/bluetooth/bluet... File device/bluetooth/bluetooth_low_energy_device_mac.mm (right): https://codereview.chromium.org/2853933002/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_low_energy_device_mac.mm:384: DidConnectGatt(); I wonder if we should even call DidConnectGatt(). If the device is connected already then we've already called it. https://codereview.chromium.org/2853933002/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_low_energy_device_mac.mm:386: DiscoverPrimaryServices(); q: Any reason why we do this after DidConnectGatt()? We do it before DidConnectGatt in Android. We'll probably refactor that and I want to make sure we match this.
https://codereview.chromium.org/2853933002/diff/120001/device/bluetooth/bluet... File device/bluetooth/bluetooth_low_energy_device_mac.mm (right): https://codereview.chromium.org/2853933002/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_low_energy_device_mac.mm:384: DidConnectGatt(); On 2017/05/04 01:26:11, ortuno wrote: > I wonder if we should even call DidConnectGatt(). If the device is connected > already then we've already called it. Done. https://codereview.chromium.org/2853933002/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_low_energy_device_mac.mm:386: DiscoverPrimaryServices(); On 2017/05/04 01:26:11, ortuno wrote: > q: Any reason why we do this after DidConnectGatt()? We do it before > DidConnectGatt in Android. We'll probably refactor that and I want to make sure > we match this. It is better to first notify that the GATT is connected, and then start to discover services.
lgtm! https://codereview.chromium.org/2853933002/diff/140001/device/bluetooth/bluet... File device/bluetooth/bluetooth_low_energy_device_mac.mm (right): https://codereview.chromium.org/2853933002/diff/140001/device/bluetooth/bluet... device/bluetooth/bluetooth_low_energy_device_mac.mm:388: // call twice, related to macOS bug. This second call should be ignored. See called twice because of a macOS bug.
https://codereview.chromium.org/2853933002/diff/140001/device/bluetooth/bluet... File device/bluetooth/bluetooth_low_energy_device_mac.mm (right): https://codereview.chromium.org/2853933002/diff/140001/device/bluetooth/bluet... device/bluetooth/bluetooth_low_energy_device_mac.mm:388: // call twice, related to macOS bug. This second call should be ignored. See On 2017/05/04 01:49:15, ortuno wrote: > called twice because of a macOS bug. Done.
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/2853933002/#ps120002 (title: "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 commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by 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/2853933002/#ps170001 (title: "merge")
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": 170001, "attempt_start_ts": 1493933419265880,
"parent_rev": "8d142c1674a4233fc63b367b1bc100ac00eaa4f7", "commit_rev":
"3f28cf2cfbf216ae1309895138d9bf25aebe068f"}
Message was sent while issue was closed.
Description was changed from ========== bluetooth: macOS: Support for extra didConnectPeripheral event from macOS. There is no need to discover services when receive an extra didConnectPeripheral event from macOS. BUG=681414 ========== to ========== bluetooth: macOS: Support for extra didConnectPeripheral event from macOS. There is no need to discover services when receive an extra didConnectPeripheral event from macOS. BUG=681414 Review-Url: https://codereview.chromium.org/2853933002 Cr-Commit-Position: refs/heads/master@{#469493} Committed: https://chromium.googlesource.com/chromium/src/+/3f28cf2cfbf216ae1309895138d9... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:170001) as https://chromium.googlesource.com/chromium/src/+/3f28cf2cfbf216ae1309895138d9... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
