Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1948763003:
Adding support for service scan on OS X (Closed)
|
Created:
4 years, 7 months ago by jlebel Modified:
4 years, 7 months ago CC:
chromium-reviews, erikchen, ortuno+watch_chromium.org, scheib+watch_chromium.org, Nico Base URL:
https://chromium.googlesource.com/chromium/src.git@cleanup Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSupport for scanning services, update and remove services.
Adding classes:
+ BluetoothLowEnergyPeripheralDelegate
+ BluetoothRemoteGattServiceMac
+ MockBluetoothCBServiceMac
Adding tests for the gatt services.
BUG=603183
Committed: https://crrev.com/c4b0978fd188d5eed6b8804ec39455bee19011eb
Cr-Commit-Position: refs/heads/master@{#392490}
Patch Set 1 #Patch Set 2 : Removing error for OS X when getting gatt services #Patch Set 3 : Removing error for OS X when getting gatt services #Patch Set 4 : Merge #Patch Set 5 : New name for BluetoothAdapterMac::StringWithCBUUID() #Patch Set 6 : Tests should be skipped if BLE is not supported #Patch Set 7 : Adding disconnect with error #
Total comments: 104
Patch Set 8 : Fixes #Patch Set 9 : Fixes #
Total comments: 22
Patch Set 10 : Fixing comments #Messages
Total messages: 50 (22 generated)
Description was changed from ========== Adding support for service scan on OS X BUG=603183 ========== to ========== Support for scanning services, update and remove services. Adding classes: + BluetoothLowEnergyPeripheralDelegate + BluetoothRemoteGattServiceMac + MockBluetoothCBServiceMac Adding tests for the gatt services. BUG=603183 ==========
jlebel@chromium.org changed reviewers: + fbeaufort@chromium.org, ortuno@chromium.org, scheib@chromium.org
Hello, Can you review this patch? Service scanning is now implemented with tests enabled. Thanks Jérôme,
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/patch-status/1948763003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1948763003/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by ortuno@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1948763003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1948763003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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/patch-status/1948763003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1948763003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on 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 to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1948763003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1948763003/80001
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/patch-status/1948763003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1948763003/100001
The CQ bit was checked by jlebel@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1948763003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1948763003/120001
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
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/patch-status/1948763003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1948763003/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/bluet... File device/bluetooth/bluetooth_low_energy_device_mac.h (right): https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_low_energy_device_mac.h:96: // Methods for BluetoothLowEnergyPeripheralBridge. nit: Methods used by BluetoothLowEnergyPeripheralBridge. https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_low_energy_device_mac.h:132: base::scoped_nsobject<BluetoothLowEnergyPeripheralDelegate> nit: Add comment for this member. https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/bluet... File device/bluetooth/bluetooth_low_energy_device_mac.mm (right): https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_low_energy_device_mac.mm:235: [GetPeripheral() discoverServices:nil]; Could this be called from BluetoothAdapterMac::DidConnectPeripheral? That way you don't need to override this function. https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_low_energy_device_mac.mm:244: if (error) { I think for now let's just VLOG it or UMA it to match other platforms. I opened an issue to decide what's the behavior we want. I'm leaning towards this though. http://crbug.com/609844 https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_low_energy_device_mac.mm:253: gatt_service = new BluetoothRemoteGattServiceMac(this, cb_service, true); To make the code easier to read, you should comment bools and ints whose meaning is not clear, in this case: true /* is_primary */ https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_low_energy_device_mac.mm:254: gatt_services_.add( nit: I would save the result from here and then DCHECK the value was actually inserted. https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_low_energy_device_mac.mm:256: std::unique_ptr<BluetoothRemoteGattService>(gatt_service)); nit: maybe you can just use base::WrapUnique() instead of std::unique_ptr<....>. https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_low_energy_device_mac.mm:257: adapter_->NotifyGattServiceAdded(gatt_service); It's a bit dangerous to be passing the raw pointer here because adding the unique_ptr to the map could have deleted the object. But I think you are good if you DCHECK that the value was actually added above. Also fyi, Chrome OS calls this after all characteristics and descriptors have been discovered for the service but Windows and Bluez do it as soon as the service is discovered. I opened an issue so that we can define what the correct behavior is http://crbug.com/609642. You don't need to do anything for this patch though. https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_low_energy_device_mac.mm:261: adapter_->NotifyGattServicesDiscovered(this); As discussed, this should be called only after all services/characteristics/descriptors have been discovered. But I do think it's OK to call it from here until we add support for the other objects. I would add a TODO to remove this so that we don't forget. https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_low_energy_device_mac.mm:264: void BluetoothLowEnergyDeviceMac::DidModifyServices( Does this get called when disconnecting? https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_low_energy_device_mac.mm:270: if ([GetPeripheral().services containsObject:cb_service]) { Based on [1] it seems we can no longer use the *any* of the services in the invalidatedServices list. So I think you need to call NotifyGattServiceRemoved for each of the services in invalidatedServices. Also keep in mind that when you call NotifyGattServiceRemoved you need to pass in a pointer to the service and that service can no longer be in the map. You can look at the Linux code to see how they achieve that.[2] [1] https://developer.apple.com/library/ios/documentation/CoreBluetooth/Reference...: [2] https://code.google.com/p/chromium/codesearch#chromium/src/device/bluetooth/b... Edit https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_low_energy_device_mac.mm:319: return NULL; nit: return nullptr; https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_low_energy_device_mac.mm:333: } You could also swap it with a local map and erase from there: GattServiceMap gatt_services_swapped; gatt_services_swapped.swap(gatt_services_); for (const auto& pair : gatt_services_swapped) { gatt_services_swapped.take_and_erase(pair.first); } https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/bluet... File device/bluetooth/bluetooth_low_energy_peripheral_delegate.h (right): https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_low_energy_peripheral_delegate.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. nit: 2016 :) https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_low_energy_peripheral_delegate.h:5: #ifndef DEVICE_BLUETOOTH_BLUETOOTH_LOW_ENERGY_CENTRAL_MANAGER_DELEGATE_H_ nit: Wrong ifndef https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/bluet... File device/bluetooth/bluetooth_low_energy_peripheral_delegate.mm (right): https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_low_energy_peripheral_delegate.mm:1: // Copyright 2015 The Chromium Authors. All rights reserved. nit: 2016 https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_low_energy_peripheral_delegate.mm:14: // classes. Out of curiosity: why do we need this class? Why can't BluetoothLowEnergyDeviceMac implement CBPeripheralDelegate's methods? Also why can't the BluetoothLowEnergyPeripheralDelegate call directly into BluetoothLowEnergyDeviceMac? https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_low_energy_peripheral_delegate.mm:20: virtual ~BluetoothLowEnergyPeripheralBridge() {} Why do you need the virtual here? https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/bluet... File device/bluetooth/bluetooth_remote_gatt_service_mac.h (right): https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_service_mac.h:1: // Copyright 2014 The Chromium Authors. All rights reserved. nit: 2016 https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_service_mac.h:29: bool primary); nit: is_primary https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_service_mac.h:54: void Unregister(const base::Closure& callback, You don't need these two functions. BluetoothGattService was split in two: BluetoothRemoteGattService and BluetoothLocalGattService. These two functions ended up in BluetoothLocalGattService. https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_service_mac.h:63: BluetoothLowEnergyDeviceMac* bluetooth_device_mac_; Please add a comment that mentions that this is the device that owns this BluetoothRemoteGattService. https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_service_mac.h:64: base::scoped_nsobject<CBService> service_; Also add a comment for this variable. https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/bluet... File device/bluetooth/bluetooth_remote_gatt_service_mac.mm (right): https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_service_mac.mm:4: nit: #include <vector> https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_service_mac.mm:19: bool primary) nit: is_primary https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_service_mac.mm:21: service_(service, base::scoped_policy::RETAIN), This might be my Objective-C ignorance showing, but is passing ownership of CBService the right thing to do? You get the CBService* from CBPeripheral:services and I don't see anywhere in the documentation that says that you are getting ownership of these pointers. https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_service_mac.mm:29: const std::string string_uuid( I think we should create and store the identifier when we construct the object. GetIdentifier is used a lot and we don't want to be creating and copying a string every time someone calls GetIdentifier. https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_service_mac.mm:37: return BluetoothAdapterMac::BluetoothUUIDWithCBUUID([service_.get() UUID]); I think we should also save the UUID. https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_service_mac.mm:52: return characteristics; nit: I think you can just do: return std::vector<BluetoothRemoteGattCharacteristic*>(); https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_service_mac.mm:59: return included_services; Same here. https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_service_mac.mm:66: return nil; nit: return nullptr; https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_service_mac.mm:74: void BluetoothRemoteGattServiceMac::Unregister( As mentioned in the .h, you no longer need these two functions. https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/bluet... File device/bluetooth/bluetooth_remote_gatt_service_unittest.cc (right): https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_service_unittest.cc:94: #if defined(OS_ANDROID) || defined(OS_WIN) || defined(OS_MACOSX) Let's not enable this yet. The NOTIMPLEMENTED in the method will pollute the logs. https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/test/... File device/bluetooth/test/bluetooth_test_mac.mm (right): https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/test/... device/bluetooth/test/bluetooth_test_mac.mm:121: mock_peripheral.get().bluetoothTestMac = this; Why do you need to do this? https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/test/... device/bluetooth/test/bluetooth_test_mac.mm:234: [services addObject:cb_service_uuid]; Does the ownership of cb_service_uuid get transfered to the services array? https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/test/... device/bluetooth/test/bluetooth_test_mac.mm:252: [peripheral_mock removeServiceWithCBUUID:cb_service_uuid]; I don't think we should use the UUID to remove services since there could be two services with the same UUID https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/test/... File device/bluetooth/test/mock_bluetooth_cbperipheral_mac.h (right): https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/test/... device/bluetooth/test/mock_bluetooth_cbperipheral_mac.h:16: } nit: add new line. https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/test/... File device/bluetooth/test/mock_bluetooth_cbperipheral_mac.mm (right): https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/test/... device/bluetooth/test/mock_bluetooth_cbperipheral_mac.mm:19: NSMutableArray* _services; How does this array get destroyed? https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/test/... device/bluetooth/test/mock_bluetooth_cbperipheral_mac.mm:27: @synthesize delegate = _delegate; Why do you need to manually synthesize these, I thought the default was _propertyName? https://developer.apple.com/library/mac/documentation/Cocoa/Conceptual/Progra... https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/test/... device/bluetooth/test/mock_bluetooth_cbperipheral_mac.mm:93: MockCBService* service = Who manages the lifetime of this object? Does it's ownership get transferred to the _services array? https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/test/... device/bluetooth/test/mock_bluetooth_cbperipheral_mac.mm:100: [_delegate peripheral:self.peripheral didDiscoverServices:error]; What is this called? I've never seen this pattern in Objective-C and I can't find any docs about it. No need to change the CL I'm just curious what this is. https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/test/... device/bluetooth/test/mock_bluetooth_cbperipheral_mac.mm:113: #pragma clang diagnostic push Can you comment why you need this? https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/test/... File device/bluetooth/test/mock_bluetooth_cbservice_mac.h (right): https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/test/... device/bluetooth/test/mock_bluetooth_cbservice_mac.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. nit: 2016 :) https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/test/... device/bluetooth/test/mock_bluetooth_cbservice_mac.h:5: #ifndef DEVICE_BLUETOOTH_MOCK_BLUETOOTH_CBSERVICE_MAC_H_ I think the correct ifndef should be: DEVICE_BLUETOOTH_TEST_MOCK_BLUETOOTH_CBSERVICE_MAC_H_ https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/test/... device/bluetooth/test/mock_bluetooth_cbservice_mac.h:16: @property(readonly, nonatomic) CBUUID* UUID; Shouldn't the service own the UUID rather than keeping a pointer to it? https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/test/... device/bluetooth/test/mock_bluetooth_cbservice_mac.h:20: - (instancetype)initWithCBUUID:(CBUUID*)uuid primary:(BOOL)isPrimary; nit: isPrimary:(BOOL)isPrimary https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/test/... File device/bluetooth/test/mock_bluetooth_cbservice_mac.mm (right): https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/test/... device/bluetooth/test/mock_bluetooth_cbservice_mac.mm:1: // Copyright 2015 The Chromium Authors. All rights reserved. nit: 2016
Thanks for the review. https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/bluet... File device/bluetooth/bluetooth_low_energy_device_mac.h (right): https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_low_energy_device_mac.h:96: // Methods for BluetoothLowEnergyPeripheralBridge. On 2016/05/06 16:03:32, ortuno wrote: > nit: Methods used by BluetoothLowEnergyPeripheralBridge. Done. https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_low_energy_device_mac.h:132: base::scoped_nsobject<BluetoothLowEnergyPeripheralDelegate> On 2016/05/06 16:03:32, ortuno wrote: > nit: Add comment for this member. Done. https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/bluet... File device/bluetooth/bluetooth_low_energy_device_mac.mm (right): https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_low_energy_device_mac.mm:235: [GetPeripheral() discoverServices:nil]; On 2016/05/06 16:03:33, ortuno wrote: > Could this be called from BluetoothAdapterMac::DidConnectPeripheral? That way > you don't need to override this function. Yes, of course, it can be done there. But it doesn't be logical. This is a call to deal with the peripheral, that's why I wanted to do in BluetoothLowEnergyDeviceMac. https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_low_energy_device_mac.mm:244: if (error) { On 2016/05/06 16:03:33, ortuno wrote: > I think for now let's just VLOG it or UMA it to match other platforms. I opened > an issue to decide what's the behavior we want. I'm leaning towards this though. > http://crbug.com/609844 Done. https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_low_energy_device_mac.mm:253: gatt_service = new BluetoothRemoteGattServiceMac(this, cb_service, true); On 2016/05/06 16:03:33, ortuno wrote: > To make the code easier to read, you should comment bools and ints whose meaning > is not clear, in this case: > > true /* is_primary */ Done. https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_low_energy_device_mac.mm:254: gatt_services_.add( On 2016/05/06 16:03:33, ortuno wrote: > nit: I would save the result from here and then DCHECK the value was actually > inserted. a DCHECK like: DCHECK(GetPeripheralIdentifier(cb_service == gatt_service); ? https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_low_energy_device_mac.mm:256: std::unique_ptr<BluetoothRemoteGattService>(gatt_service)); On 2016/05/06 16:03:33, ortuno wrote: > nit: maybe you can just use base::WrapUnique() instead of std::unique_ptr<....>. Done. https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_low_energy_device_mac.mm:257: adapter_->NotifyGattServiceAdded(gatt_service); On 2016/05/06 16:03:33, ortuno wrote: > It's a bit dangerous to be passing the raw pointer here because adding the > unique_ptr to the map could have deleted the object. But I think you are good if > you DCHECK that the value was actually added above. > > Also fyi, Chrome OS calls this after all characteristics and descriptors have > been discovered for the service but Windows and Bluez do it as soon as the > service is discovered. I opened an issue so that we can define what the correct > behavior is http://crbug.com/609642. You don't need to do anything for this > patch though. It is nicer to create a service so I can track which services has fully discovered and which one are still pending. So I would definitely prefer the windows/bluez version. https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_low_energy_device_mac.mm:261: adapter_->NotifyGattServicesDiscovered(this); On 2016/05/06 16:03:33, ortuno wrote: > As discussed, this should be called only after all > services/characteristics/descriptors have been discovered. But I do think it's > OK to call it from here until we add support for the other objects. I would add > a TODO to remove this so that we don't forget. Done. https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_low_energy_device_mac.mm:264: void BluetoothLowEnergyDeviceMac::DidModifyServices( On 2016/05/06 16:03:33, ortuno wrote: > Does this get called when disconnecting? No. It is called when the services are modified. https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_low_energy_device_mac.mm:270: if ([GetPeripheral().services containsObject:cb_service]) { On 2016/05/06 16:03:33, ortuno wrote: > Based on [1] it seems we can no longer use the *any* of the services in the > invalidatedServices list. So I think you need to call NotifyGattServiceRemoved > for each of the services in invalidatedServices. > > Also keep in mind that when you call NotifyGattServiceRemoved you need to pass > in a pointer to the service and that service can no longer be in the map. You > can look at the Linux code to see how they achieve that.[2] > > [1] > https://developer.apple.com/library/ios/documentation/CoreBluetooth/Reference...: > [2] > https://code.google.com/p/chromium/codesearch#chromium/src/device/bluetooth/b... > Edit Done. https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_low_energy_device_mac.mm:319: return NULL; On 2016/05/06 16:03:33, ortuno wrote: > nit: return nullptr; Done. https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_low_energy_device_mac.mm:333: } On 2016/05/06 16:03:33, ortuno wrote: > You could also swap it with a local map and erase from there: > > > GattServiceMap gatt_services_swapped; > gatt_services_swapped.swap(gatt_services_); > for (const auto& pair : gatt_services_swapped) { > gatt_services_swapped.take_and_erase(pair.first); > } I have a crash on the second service removed. https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/bluet... File device/bluetooth/bluetooth_low_energy_peripheral_delegate.h (right): https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_low_energy_peripheral_delegate.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2016/05/06 16:03:33, ortuno wrote: > nit: 2016 :) Done. https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_low_energy_peripheral_delegate.h:5: #ifndef DEVICE_BLUETOOTH_BLUETOOTH_LOW_ENERGY_CENTRAL_MANAGER_DELEGATE_H_ On 2016/05/06 16:03:33, ortuno wrote: > nit: Wrong ifndef Done. https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/bluet... File device/bluetooth/bluetooth_low_energy_peripheral_delegate.mm (right): https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_low_energy_peripheral_delegate.mm:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2016/05/06 16:03:33, ortuno wrote: > nit: 2016 Done. https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_low_energy_peripheral_delegate.mm:14: // classes. On 2016/05/06 16:03:33, ortuno wrote: > Out of curiosity: why do we need this class? Why can't > BluetoothLowEnergyDeviceMac implement CBPeripheralDelegate's methods? Also why > can't the BluetoothLowEnergyPeripheralDelegate call directly into > BluetoothLowEnergyDeviceMac? I'm following the pattern that already exist for BluetoothLowEnergyCentralManagerDelegate. I'm guessing the benefit of this complex pattern is to have protected methods in BluetoothLowEnergyDeviceMac that can be called by the friend BluetoothLowEnergyPeripheralBridge. If you don't mind moving the methods from private to public, I can remove this class. And I will do the same in the other class (in another patch). https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_low_energy_peripheral_delegate.mm:20: virtual ~BluetoothLowEnergyPeripheralBridge() {} On 2016/05/06 16:03:33, ortuno wrote: > Why do you need the virtual here? Done. https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/bluet... File device/bluetooth/bluetooth_remote_gatt_service_mac.h (right): https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_service_mac.h:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2016/05/06 16:03:33, ortuno wrote: > nit: 2016 Done. https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_service_mac.h:29: bool primary); On 2016/05/06 16:03:33, ortuno wrote: > nit: is_primary Done. https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_service_mac.h:54: void Unregister(const base::Closure& callback, On 2016/05/06 16:03:33, ortuno wrote: > You don't need these two functions. BluetoothGattService was split in two: > BluetoothRemoteGattService and BluetoothLocalGattService. These two functions > ended up in BluetoothLocalGattService. Done. https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_service_mac.h:63: BluetoothLowEnergyDeviceMac* bluetooth_device_mac_; On 2016/05/06 16:03:33, ortuno wrote: > Please add a comment that mentions that this is the device that owns this > BluetoothRemoteGattService. Done. https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_service_mac.h:64: base::scoped_nsobject<CBService> service_; On 2016/05/06 16:03:33, ortuno wrote: > Also add a comment for this variable. Done. https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/bluet... File device/bluetooth/bluetooth_remote_gatt_service_mac.mm (right): https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_service_mac.mm:4: On 2016/05/06 16:03:34, ortuno wrote: > nit: #include <vector> Done. https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_service_mac.mm:19: bool primary) On 2016/05/06 16:03:34, ortuno wrote: > nit: is_primary Done. https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_service_mac.mm:21: service_(service, base::scoped_policy::RETAIN), On 2016/05/06 16:03:34, ortuno wrote: > This might be my Objective-C ignorance showing, but is passing ownership of > CBService the right thing to do? You get the CBService* from > CBPeripheral:services and I don't see anywhere in the documentation that says > that you are getting ownership of these pointers. BluetoothRemoteGattServiceMac needs CBService instance. Therefore it has to retain it. It is not an ownership. BluetoothRemoteGattServiceMac instance has also to find out when the CBService instance should not be used but this is another question. https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_service_mac.mm:29: const std::string string_uuid( On 2016/05/06 16:03:33, ortuno wrote: > I think we should create and store the identifier when we construct the object. > GetIdentifier is used a lot and we don't want to be creating and copying a > string every time someone calls GetIdentifier. Done. https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_service_mac.mm:37: return BluetoothAdapterMac::BluetoothUUIDWithCBUUID([service_.get() UUID]); On 2016/05/06 16:03:34, ortuno wrote: > I think we should also save the UUID. Done. https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_service_mac.mm:52: return characteristics; On 2016/05/06 16:03:34, ortuno wrote: > nit: I think you can just do: > > return std::vector<BluetoothRemoteGattCharacteristic*>(); Done. https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_service_mac.mm:59: return included_services; On 2016/05/06 16:03:34, ortuno wrote: > Same here. Done. https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_service_mac.mm:66: return nil; On 2016/05/06 16:03:34, ortuno wrote: > nit: return nullptr; Done. https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_service_mac.mm:74: void BluetoothRemoteGattServiceMac::Unregister( On 2016/05/06 16:03:33, ortuno wrote: > As mentioned in the .h, you no longer need these two functions. Done. https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/bluet... File device/bluetooth/bluetooth_remote_gatt_service_unittest.cc (right): https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_service_unittest.cc:94: #if defined(OS_ANDROID) || defined(OS_WIN) || defined(OS_MACOSX) On 2016/05/06 16:03:34, ortuno wrote: > Let's not enable this yet. The NOTIMPLEMENTED in the method will pollute the > logs. Done. https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/test/... File device/bluetooth/test/bluetooth_test_mac.mm (right): https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/test/... device/bluetooth/test/bluetooth_test_mac.mm:121: mock_peripheral.get().bluetoothTestMac = this; On 2016/05/06 16:03:34, ortuno wrote: > Why do you need to do this? I need to do this so the mock peripheral can call: BluetoothTestMac::OnFakeBluetoothDeviceConnectGattCalled() and BluetoothTestMac::OnFakeBluetoothGattDisconnect(). https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/test/... device/bluetooth/test/bluetooth_test_mac.mm:234: [services addObject:cb_service_uuid]; On 2016/05/06 16:03:34, ortuno wrote: > Does the ownership of cb_service_uuid get transfered to the services array? There is no real ownership in objective-c. The |services| is going to retain |cb_service_uuid|. So the instance will be kept alive at least as long as |services| needs it. The instance of CBUUID has been created without calling explicitely |alloc|. Therefore the retain count value is set to 1, and will be decreased automatically by 1 when this method will be finished. So the retain count is set to 1 after |[CBUUID UUIDWithString:@(uuid.c_str())]|. Then it is increased after |[services addObject:cb_service_uuid]|. And after this method, the retain count value is will be set back to 1. https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/test/... device/bluetooth/test/bluetooth_test_mac.mm:252: [peripheral_mock removeServiceWithCBUUID:cb_service_uuid]; On 2016/05/06 16:03:34, ortuno wrote: > I don't think we should use the UUID to remove services since there could be two > services with the same UUID Done. https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/test/... File device/bluetooth/test/mock_bluetooth_cbperipheral_mac.h (right): https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/test/... device/bluetooth/test/mock_bluetooth_cbperipheral_mac.h:16: } On 2016/05/06 16:03:34, ortuno wrote: > nit: add new line. I can't. |git cl format| removes it. https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/test/... File device/bluetooth/test/mock_bluetooth_cbperipheral_mac.mm (right): https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/test/... device/bluetooth/test/mock_bluetooth_cbperipheral_mac.mm:19: NSMutableArray* _services; On 2016/05/06 16:03:34, ortuno wrote: > How does this array get destroyed? Done. https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/test/... device/bluetooth/test/mock_bluetooth_cbperipheral_mac.mm:27: @synthesize delegate = _delegate; On 2016/05/06 16:03:34, ortuno wrote: > Why do you need to manually synthesize these, I thought the default was > _propertyName? > https://developer.apple.com/library/mac/documentation/Cocoa/Conceptual/Progra... Unfortunately Chrome is not using ARC yet. Therefore synthesize has to be done explicitly. https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/test/... device/bluetooth/test/mock_bluetooth_cbperipheral_mac.mm:93: MockCBService* service = On 2016/05/06 16:03:34, ortuno wrote: > Who manages the lifetime of this object? Does it's ownership get transferred to > the _services array? Done. https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/test/... device/bluetooth/test/mock_bluetooth_cbperipheral_mac.mm:100: [_delegate peripheral:self.peripheral didDiscoverServices:error]; On 2016/05/06 16:03:34, ortuno wrote: > What is this called? I've never seen this pattern in Objective-C and I can't > find any docs about it. No need to change the CL I'm just curious what this is. What pattern are you talking about? "self.peripheral"? https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/test/... device/bluetooth/test/mock_bluetooth_cbperipheral_mac.mm:113: #pragma clang diagnostic push On 2016/05/06 16:03:34, ortuno wrote: > Can you comment why you need this? Done. https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/test/... File device/bluetooth/test/mock_bluetooth_cbservice_mac.h (right): https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/test/... device/bluetooth/test/mock_bluetooth_cbservice_mac.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2016/05/06 16:03:34, ortuno wrote: > nit: 2016 :) Done. https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/test/... device/bluetooth/test/mock_bluetooth_cbservice_mac.h:5: #ifndef DEVICE_BLUETOOTH_MOCK_BLUETOOTH_CBSERVICE_MAC_H_ On 2016/05/06 16:03:34, ortuno wrote: > I think the correct ifndef should be: > > DEVICE_BLUETOOTH_TEST_MOCK_BLUETOOTH_CBSERVICE_MAC_H_ Done. https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/test/... device/bluetooth/test/mock_bluetooth_cbservice_mac.h:16: @property(readonly, nonatomic) CBUUID* UUID; On 2016/05/06 16:03:34, ortuno wrote: > Shouldn't the service own the UUID rather than keeping a pointer to it? Done. https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/test/... device/bluetooth/test/mock_bluetooth_cbservice_mac.h:20: - (instancetype)initWithCBUUID:(CBUUID*)uuid primary:(BOOL)isPrimary; On 2016/05/06 16:03:34, ortuno wrote: > nit: isPrimary:(BOOL)isPrimary That's not the usual way to name methods in Obj-C: CBMutableService: - (id)initWithType:(nullable CBUUID *)UUID primary:(BOOL)isPrimary; https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/test/... File device/bluetooth/test/mock_bluetooth_cbservice_mac.mm (right): https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/test/... device/bluetooth/test/mock_bluetooth_cbservice_mac.mm:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2016/05/06 16:03:35, ortuno wrote: > nit: 2016 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/patch-status/1948763003/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1948763003/160001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/bluet... File device/bluetooth/bluetooth_low_energy_device_mac.mm (right): https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_low_energy_device_mac.mm:235: [GetPeripheral() discoverServices:nil]; On 2016/05/07 at 00:16:10, jlebel wrote: > On 2016/05/06 16:03:33, ortuno wrote: > > Could this be called from BluetoothAdapterMac::DidConnectPeripheral? That way > > you don't need to override this function. > > Yes, of course, it can be done there. But it doesn't be logical. This is a call to deal with the peripheral, that's why I wanted to do in BluetoothLowEnergyDeviceMac. Ah, good point. Up to you. https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_low_energy_device_mac.mm:254: gatt_services_.add( On 2016/05/07 at 00:16:11, jlebel wrote: > On 2016/05/06 16:03:33, ortuno wrote: > > nit: I would save the result from here and then DCHECK the value was actually > > inserted. > > a DCHECK like: > DCHECK(GetPeripheralIdentifier(cb_service == gatt_service); > ? More like: auto result_iter = gatt_services_.add( gatt_service->GetIdentifier(), base::WrapUnique(gatt_service)); DCHECK(result_iter->second); https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_low_energy_device_mac.mm:333: } On 2016/05/07 at 00:16:11, jlebel wrote: > On 2016/05/06 16:03:33, ortuno wrote: > > You could also swap it with a local map and erase from there: > > > > > > GattServiceMap gatt_services_swapped; > > gatt_services_swapped.swap(gatt_services_); > > for (const auto& pair : gatt_services_swapped) { > > gatt_services_swapped.take_and_erase(pair.first); > > } > > I have a crash on the second service removed. Hmm really? I just tried it with the tests and it seemed to work. Did you have any problems with real devices? https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/bluet... File device/bluetooth/bluetooth_low_energy_peripheral_delegate.mm (right): https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_low_energy_peripheral_delegate.mm:14: // classes. On 2016/05/07 at 00:16:11, jlebel wrote: > On 2016/05/06 16:03:33, ortuno wrote: > > Out of curiosity: why do we need this class? Why can't > > BluetoothLowEnergyDeviceMac implement CBPeripheralDelegate's methods? Also why > > can't the BluetoothLowEnergyPeripheralDelegate call directly into > > BluetoothLowEnergyDeviceMac? > > I'm following the pattern that already exist for BluetoothLowEnergyCentralManagerDelegate. I'm guessing the benefit of this complex pattern is to have protected methods in BluetoothLowEnergyDeviceMac that can be called by the friend BluetoothLowEnergyPeripheralBridge. > > If you don't mind moving the methods from private to public, I can remove this class. And I will do the same in the other class (in another patch). It's OK. I just wanted to know the background for this pattern. https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/bluet... File device/bluetooth/bluetooth_remote_gatt_service_mac.mm (right): https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_service_mac.mm:21: service_(service, base::scoped_policy::RETAIN), On 2016/05/07 at 00:16:11, jlebel wrote: > On 2016/05/06 16:03:34, ortuno wrote: > > This might be my Objective-C ignorance showing, but is passing ownership of > > CBService the right thing to do? You get the CBService* from > > CBPeripheral:services and I don't see anywhere in the documentation that says > > that you are getting ownership of these pointers. > > BluetoothRemoteGattServiceMac needs CBService instance. Therefore it has to retain it. It is not an ownership. BluetoothRemoteGattServiceMac instance has also to find out when the CBService instance should not be used but this is another question. Ah I see. Thanks for the explanation. https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/test/... File device/bluetooth/test/bluetooth_test_mac.mm (right): https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/test/... device/bluetooth/test/bluetooth_test_mac.mm:121: mock_peripheral.get().bluetoothTestMac = this; On 2016/05/07 at 00:16:12, jlebel wrote: > On 2016/05/06 16:03:34, ortuno wrote: > > Why do you need to do this? > > I need to do this so the mock peripheral can call: > BluetoothTestMac::OnFakeBluetoothDeviceConnectGattCalled() and BluetoothTestMac::OnFakeBluetoothGattDisconnect(). Ah ok. https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/test/... device/bluetooth/test/bluetooth_test_mac.mm:234: [services addObject:cb_service_uuid]; On 2016/05/07 at 00:16:12, jlebel wrote: > On 2016/05/06 16:03:34, ortuno wrote: > > Does the ownership of cb_service_uuid get transfered to the services array? > > There is no real ownership in objective-c. The |services| is going to retain |cb_service_uuid|. So the instance will be kept alive at least as long as |services| needs it. > The instance of CBUUID has been created without calling explicitely |alloc|. Therefore the retain count value is set to 1, and will be decreased automatically by 1 when this method will be finished. > So the retain count is set to 1 after |[CBUUID UUIDWithString:@(uuid.c_str())]|. Then it is increased after |[services addObject:cb_service_uuid]|. And after this method, the retain count value is will be set back to 1. Got it. Thanks for the explanation! https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/test/... File device/bluetooth/test/mock_bluetooth_cbperipheral_mac.mm (right): https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/test/... device/bluetooth/test/mock_bluetooth_cbperipheral_mac.mm:27: @synthesize delegate = _delegate; On 2016/05/07 at 00:16:12, jlebel wrote: > On 2016/05/06 16:03:34, ortuno wrote: > > Why do you need to manually synthesize these, I thought the default was > > _propertyName? > > https://developer.apple.com/library/mac/documentation/Cocoa/Conceptual/Progra... > > Unfortunately Chrome is not using ARC yet. Therefore synthesize has to be done explicitly. Ah. I see. https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/test/... device/bluetooth/test/mock_bluetooth_cbperipheral_mac.mm:100: [_delegate peripheral:self.peripheral didDiscoverServices:error]; On 2016/05/07 at 00:16:12, jlebel wrote: > On 2016/05/06 16:03:34, ortuno wrote: > > What is this called? I've never seen this pattern in Objective-C and I can't > > find any docs about it. No need to change the CL I'm just curious what this is. > > What pattern are you talking about? "self.peripheral"? I guess I'm not sure what the second and third parts mean. I usually see the following: [object function:parameter] But here I see: [object name:parameter function:parameter] https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/test/... File device/bluetooth/test/mock_bluetooth_cbservice_mac.h (right): https://codereview.chromium.org/1948763003/diff/120001/device/bluetooth/test/... device/bluetooth/test/mock_bluetooth_cbservice_mac.h:20: - (instancetype)initWithCBUUID:(CBUUID*)uuid primary:(BOOL)isPrimary; On 2016/05/07 at 00:16:12, jlebel wrote: > On 2016/05/06 16:03:34, ortuno wrote: > > nit: isPrimary:(BOOL)isPrimary > > That's not the usual way to name methods in Obj-C: > > CBMutableService: > - (id)initWithType:(nullable CBUUID *)UUID primary:(BOOL)isPrimary; Ah ok. https://codereview.chromium.org/1948763003/diff/160001/device/bluetooth/bluet... File device/bluetooth/bluetooth_low_energy_device_mac.mm (right): https://codereview.chromium.org/1948763003/diff/160001/device/bluetooth/bluet... device/bluetooth/bluetooth_low_energy_device_mac.mm:247: DisconnectGatt(); Let's not disconnect to match the other platforms. https://codereview.chromium.org/1948763003/diff/160001/device/bluetooth/bluet... device/bluetooth/bluetooth_low_energy_device_mac.mm:262: // characteristics has been found. s/has/have/ https://codereview.chromium.org/1948763003/diff/160001/device/bluetooth/bluet... File device/bluetooth/bluetooth_remote_gatt_service_mac.h (right): https://codereview.chromium.org/1948763003/diff/160001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_service_mac.h:51: // bluetooth_device_mac_ is the own of instances of this class. s/is the own of/owns https://codereview.chromium.org/1948763003/diff/160001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_service_mac.h:53: // This instance manages services_ (service from CoreBluetooth). I think this would be better: A service from CBPeripheral.services. https://codereview.chromium.org/1948763003/diff/160001/device/bluetooth/bluet... File device/bluetooth/bluetooth_remote_gatt_service_mac.mm (right): https://codereview.chromium.org/1948763003/diff/160001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_service_mac.mm:25: const std::string string_uuid( You already have a BluetoothUUID so you can just get a std::string from uuid_.canonical_value() and a c_str() from there. https://codereview.chromium.org/1948763003/diff/160001/device/bluetooth/bluet... File device/bluetooth/bluetooth_remote_gatt_service_unittest.cc (right): https://codereview.chromium.org/1948763003/diff/160001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_service_unittest.cc:115: #endif // defined(OS_ANDROID) || defined(OS_WIN) || defined(OS_MACOSX) You forgot to delete it from the comment :) https://codereview.chromium.org/1948763003/diff/160001/device/bluetooth/test/... File device/bluetooth/test/mock_bluetooth_cbperipheral_mac.mm (right): https://codereview.chromium.org/1948763003/diff/160001/device/bluetooth/test/... device/bluetooth/test/mock_bluetooth_cbperipheral_mac.mm:100: [_services addObject:service.get().service]; Will this make it so that the service is retained by _services? https://codereview.chromium.org/1948763003/diff/160001/device/bluetooth/test/... File device/bluetooth/test/mock_bluetooth_cbservice_mac.h (right): https://codereview.chromium.org/1948763003/diff/160001/device/bluetooth/test/... device/bluetooth/test/mock_bluetooth_cbservice_mac.h:16: @property(readonly, nonatomic) CBUUID* UUID; nit: I think this should be lower case: https://google.github.io/styleguide/objcguide.xml?showone=Variable_Names#Vari...
thakis, erikchen: could you double-check that the suppression in https://codereview.chromium.org/1948763003/diff/160001/device/bluetooth/test/... is correct?
erikchen@chromium.org changed reviewers: + erikchen@chromium.org
https://codereview.chromium.org/1948763003/diff/160001/device/bluetooth/test/... File device/bluetooth/test/mock_bluetooth_cbperipheral_mac.mm (right): https://codereview.chromium.org/1948763003/diff/160001/device/bluetooth/test/... device/bluetooth/test/mock_bluetooth_cbperipheral_mac.mm:19: NSMutableArray* _services; used a scoped_nsobject? https://codereview.chromium.org/1948763003/diff/160001/device/bluetooth/test/... device/bluetooth/test/mock_bluetooth_cbperipheral_mac.mm:114: // -[CBPeripheralDelegate peripheral:didModifyServices:] is only available Ah, you should probably just do: if ([_delegate respondsToSelector(...)]) [_delegate performSelector:...] or perhaps DCHECK(_delegate respondsToSelector(...) [ _delegate performSelector:...] Chrome is not compiled with 10.8, in any interpretation.
https://codereview.chromium.org/1948763003/diff/160001/device/bluetooth/test/... File device/bluetooth/test/mock_bluetooth_cbperipheral_mac.mm (right): https://codereview.chromium.org/1948763003/diff/160001/device/bluetooth/test/... device/bluetooth/test/mock_bluetooth_cbperipheral_mac.mm:114: // -[CBPeripheralDelegate peripheral:didModifyServices:] is only available On 2016/05/09 at 20:00:08, erikchen wrote: > Ah, you should probably just do: > > if ([_delegate respondsToSelector(...)]) > [_delegate performSelector:...] > > or perhaps > DCHECK(_delegate respondsToSelector(...) > [ _delegate performSelector:...] > > Chrome is not compiled with 10.8, in any interpretation. Can confirm that this works: DCHECK([_delegate respondsToSelector:@selector(peripheral:didModifyServices:)]); [ _delegate performSelector:@selector(peripheral:didModifyServices:) withObject:self.peripheral withObject:@[ serviceToRemove ]];
Thanks for your comments. https://codereview.chromium.org/1948763003/diff/160001/device/bluetooth/bluet... File device/bluetooth/bluetooth_low_energy_device_mac.mm (right): https://codereview.chromium.org/1948763003/diff/160001/device/bluetooth/bluet... device/bluetooth/bluetooth_low_energy_device_mac.mm:247: DisconnectGatt(); On 2016/05/09 19:54:01, ortuno wrote: > Let's not disconnect to match the other platforms. Done. https://codereview.chromium.org/1948763003/diff/160001/device/bluetooth/bluet... device/bluetooth/bluetooth_low_energy_device_mac.mm:262: // characteristics has been found. On 2016/05/09 19:54:01, ortuno wrote: > s/has/have/ Done. https://codereview.chromium.org/1948763003/diff/160001/device/bluetooth/bluet... File device/bluetooth/bluetooth_remote_gatt_service_mac.h (right): https://codereview.chromium.org/1948763003/diff/160001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_service_mac.h:51: // bluetooth_device_mac_ is the own of instances of this class. On 2016/05/09 19:54:01, ortuno wrote: > s/is the own of/owns Done. https://codereview.chromium.org/1948763003/diff/160001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_service_mac.h:53: // This instance manages services_ (service from CoreBluetooth). On 2016/05/09 19:54:01, ortuno wrote: > I think this would be better: A service from CBPeripheral.services. Done. https://codereview.chromium.org/1948763003/diff/160001/device/bluetooth/bluet... File device/bluetooth/bluetooth_remote_gatt_service_mac.mm (right): https://codereview.chromium.org/1948763003/diff/160001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_service_mac.mm:25: const std::string string_uuid( On 2016/05/09 19:54:01, ortuno wrote: > You already have a BluetoothUUID so you can just get a std::string from > uuid_.canonical_value() and a c_str() from there. Done. https://codereview.chromium.org/1948763003/diff/160001/device/bluetooth/bluet... File device/bluetooth/bluetooth_remote_gatt_service_unittest.cc (right): https://codereview.chromium.org/1948763003/diff/160001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_service_unittest.cc:115: #endif // defined(OS_ANDROID) || defined(OS_WIN) || defined(OS_MACOSX) On 2016/05/09 19:54:01, ortuno wrote: > You forgot to delete it from the comment :) Done. https://codereview.chromium.org/1948763003/diff/160001/device/bluetooth/test/... File device/bluetooth/test/mock_bluetooth_cbperipheral_mac.mm (right): https://codereview.chromium.org/1948763003/diff/160001/device/bluetooth/test/... device/bluetooth/test/mock_bluetooth_cbperipheral_mac.mm:19: NSMutableArray* _services; On 2016/05/09 20:00:08, erikchen wrote: > used a scoped_nsobject? Done. https://codereview.chromium.org/1948763003/diff/160001/device/bluetooth/test/... device/bluetooth/test/mock_bluetooth_cbperipheral_mac.mm:100: [_services addObject:service.get().service]; On 2016/05/09 19:54:01, ortuno wrote: > Will this make it so that the service is retained by _services? yes. https://codereview.chromium.org/1948763003/diff/160001/device/bluetooth/test/... device/bluetooth/test/mock_bluetooth_cbperipheral_mac.mm:114: // -[CBPeripheralDelegate peripheral:didModifyServices:] is only available On 2016/05/09 20:00:08, erikchen wrote: > Ah, you should probably just do: > > if ([_delegate respondsToSelector(...)]) > [_delegate performSelector:...] > > or perhaps > DCHECK(_delegate respondsToSelector(...) > [ _delegate performSelector:...] > > Chrome is not compiled with 10.8, in any interpretation. > Done. https://codereview.chromium.org/1948763003/diff/160001/device/bluetooth/test/... device/bluetooth/test/mock_bluetooth_cbperipheral_mac.mm:114: // -[CBPeripheralDelegate peripheral:didModifyServices:] is only available On 2016/05/09 20:25:36, ortuno wrote: > On 2016/05/09 at 20:00:08, erikchen wrote: > > Ah, you should probably just do: > > > > if ([_delegate respondsToSelector(...)]) > > [_delegate performSelector:...] > > > > or perhaps > > DCHECK(_delegate respondsToSelector(...) > > [ _delegate performSelector:...] > > > > Chrome is not compiled with 10.8, in any interpretation. > > Can confirm that this works: > > DCHECK([_delegate respondsToSelector:@selector(peripheral:didModifyServices:)]); > [ _delegate performSelector:@selector(peripheral:didModifyServices:) > withObject:self.peripheral withObject:@[ serviceToRemove ]]; Done. https://codereview.chromium.org/1948763003/diff/160001/device/bluetooth/test/... File device/bluetooth/test/mock_bluetooth_cbservice_mac.h (right): https://codereview.chromium.org/1948763003/diff/160001/device/bluetooth/test/... device/bluetooth/test/mock_bluetooth_cbservice_mac.h:16: @property(readonly, nonatomic) CBUUID* UUID; On 2016/05/09 19:54:01, ortuno wrote: > nit: I think this should be lower case: > https://google.github.io/styleguide/objcguide.xml?showone=Variable_Names#Vari... Apple always abbreviation in upper case, like -[CBPeripheral UUID]. But I can change it if you prefer.
lgtm!
The CQ bit was checked by ortuno@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1948763003/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1948763003/180001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by jlebel@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1948763003/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1948763003/180001
Message was sent while issue was closed.
Description was changed from ========== Support for scanning services, update and remove services. Adding classes: + BluetoothLowEnergyPeripheralDelegate + BluetoothRemoteGattServiceMac + MockBluetoothCBServiceMac Adding tests for the gatt services. BUG=603183 ========== to ========== Support for scanning services, update and remove services. Adding classes: + BluetoothLowEnergyPeripheralDelegate + BluetoothRemoteGattServiceMac + MockBluetoothCBServiceMac Adding tests for the gatt services. BUG=603183 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Support for scanning services, update and remove services. Adding classes: + BluetoothLowEnergyPeripheralDelegate + BluetoothRemoteGattServiceMac + MockBluetoothCBServiceMac Adding tests for the gatt services. BUG=603183 ========== to ========== Support for scanning services, update and remove services. Adding classes: + BluetoothLowEnergyPeripheralDelegate + BluetoothRemoteGattServiceMac + MockBluetoothCBServiceMac Adding tests for the gatt services. BUG=603183 Committed: https://crrev.com/c4b0978fd188d5eed6b8804ec39455bee19011eb Cr-Commit-Position: refs/heads/master@{#392490} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/c4b0978fd188d5eed6b8804ec39455bee19011eb Cr-Commit-Position: refs/heads/master@{#392490} |
