Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
791763005:
Added bluetooth LE support on Mac platform (Closed)
DescriptionAdded bluetooth LE support on Mac platform
BUG=449682
Committed: https://crrev.com/42f79f8b08a99190627f3a67e86a378987104a4a
Cr-Commit-Position: refs/heads/master@{#315190}
Patch Set 1 : #Patch Set 2 : #
Total comments: 48
Patch Set 3 : feedback. #
Total comments: 53
Patch Set 4 : #Patch Set 5 : #
Total comments: 4
Patch Set 6 : #
Total comments: 4
Patch Set 7 : #
Total comments: 2
Patch Set 8 : #
Total comments: 2
Patch Set 9 : #Patch Set 10 : #Patch Set 11 : #Patch Set 12 : #Patch Set 13 : #Patch Set 14 : #Messages
Total messages: 50 (19 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
dvh@chromium.org changed reviewers: + armansito@chromium.org
Hi Armansito, Please take a look. Thanks!
https://codereview.chromium.org/791763005/diff/120001/device/bluetooth/blueto... File device/bluetooth/bluetooth_low_energy_device_mac.h (left): https://codereview.chromium.org/791763005/diff/120001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_device_mac.h:8: #import <IOBluetooth/IOBluetooth.h> Does importing IOBluetooth.h on Mac also include the CoreBluetooth.h header? Or don't you have to explicitly import that as well? https://codereview.chromium.org/791763005/diff/120001/device/bluetooth/blueto... File device/bluetooth/bluetooth_low_energy_discovery_manager_mac.h (right): https://codereview.chromium.org/791763005/diff/120001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_discovery_manager_mac.h:5: #ifndef DEVICE_BLUETOOTH_BLUETOOTH_LOW_ENERGY_DISCOVER_MANAGER_MAC_H_ The header is named *_discovery_manager_* but everything else is named "discover manager" (without the 'y'). Please name this class *DiscoveryManager* and update the header guards. https://codereview.chromium.org/791763005/diff/120001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_discovery_manager_mac.h:26: class BluetoothLowEnergyDiscoverManagerMac { This class should inherit from BluetoothDiscoveryManagerMac. The idea is that you would then call BluetoothDiscoveryManagerMac::CreateLowEnergy() (which you would add), which would then return a BluetoothLowEnergyDiscoveryManagerMac. https://codereview.chromium.org/791763005/diff/120001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_discovery_manager_mac.h:57: NSDictionary* advertisementData, int rssi); nit: new line after this method https://codereview.chromium.org/791763005/diff/120001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_discovery_manager_mac.h:71: // Observer interested in notifications from us. nit: Please add a new line after member declarations if the following member declaration is preceded by a comment. https://codereview.chromium.org/791763005/diff/120001/device/bluetooth/blueto... File device/bluetooth/bluetooth_low_energy_discovery_manager_mac.mm (right): https://codereview.chromium.org/791763005/diff/120001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_discovery_manager_mac.mm:1: #include "device/bluetooth/bluetooth_low_energy_discovery_manager_mac.h" Is this file missing the copyright header? https://codereview.chromium.org/791763005/diff/120001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_discovery_manager_mac.mm:21: virtual void DiscoveredPeripheral(CBPeripheral *peripheral, nit: This should read "CBPeripheral* peripheral" (no space before '*') per style-guide. Here and everywhere else. https://codereview.chromium.org/791763005/diff/120001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_discovery_manager_mac.mm:48: self = [super init]; nit: check if init worked here (i.e. set the members inside if (self) { ... }) https://codereview.chromium.org/791763005/diff/120001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_discovery_manager_mac.mm:102: } Please add new lines where you can to make the code more readable. Here and elsewhere. https://codereview.chromium.org/791763005/diff/120001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_discovery_manager_mac.mm:105: if (services_uuids_.size() > 0) { nit: if (!services_uuids_.empty()) { https://codereview.chromium.org/791763005/diff/120001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_discovery_manager_mac.mm:120: [manager_ stopScan]; Don't call this if |discovering_| is true? https://codereview.chromium.org/791763005/diff/120001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_discovery_manager_mac.mm:127: BluetoothLowEnergyDeviceMac *device = It seems like this is being unnecessarily created and deleted if there's only an update for an existing device. Move this into the corresponding if-block. https://codereview.chromium.org/791763005/diff/120001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_discovery_manager_mac.mm:132: if (iter != devices_.end()) { Do an early return and get rid of the else. I'd actually turn this if statement around, so if (iter == device_.end()) { BluetoothLowEnergyDeviceMac* device = new BluetoothLowE...; devices_.insert(...); observer_->DeviceFound(device); return; } BluetoothLowEnergyDeviceMac* old_device = ...; ... https://codereview.chromium.org/791763005/diff/120001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_discovery_manager_mac.mm:161: [[CBCentralManager alloc] initWithDelegate:bridge_ Will this code compile at all in older versions? Will we need to add something to base/mac/sdk_forward_declarations.*?
https://codereview.chromium.org/791763005/diff/120001/device/bluetooth/blueto... File device/bluetooth/bluetooth_low_energy_device_mac.h (right): https://codereview.chromium.org/791763005/diff/120001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_device_mac.h:26: nit: We usually don't add new lines between override declarations. I would add one big block of overrides preceded by a comment saying "// BluetoothDevice overrides". https://codereview.chromium.org/791763005/diff/120001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_device_mac.h:103: base::scoped_nsobject<CBPeripheral> peripheral_; nit: Add new lines between member declarations here. https://codereview.chromium.org/791763005/diff/120001/device/bluetooth/blueto... File device/bluetooth/bluetooth_low_energy_device_mac.mm (right): https://codereview.chromium.org/791763005/diff/120001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_device_mac.mm:1: #include "device/bluetooth/bluetooth_low_energy_device_mac.h" Copyright header? https://codereview.chromium.org/791763005/diff/120001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_device_mac.mm:9: namespace { nit: new line here. https://codereview.chromium.org/791763005/diff/120001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_device_mac.mm:52: } nit: new line. https://codereview.chromium.org/791763005/diff/120001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_device_mac.mm:53: } nit: Closing brace for namespace should read "} // namespace". https://codereview.chromium.org/791763005/diff/120001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_device_mac.mm:85: return std::string([uuidString UTF8String]); nit: Remove else since you already have an early return. https://codereview.chromium.org/791763005/diff/120001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_device_mac.mm:138: const ConnectionInfoCallback& callback) {} Add NOTIMPLEMENTED() ?
Most of the issues have been addresses + some questions. https://codereview.chromium.org/791763005/diff/120001/device/bluetooth/blueto... File device/bluetooth/bluetooth_low_energy_device_mac.h (left): https://codereview.chromium.org/791763005/diff/120001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_device_mac.h:8: #import <IOBluetooth/IOBluetooth.h> On 2015/01/24 17:59:37, armansito wrote: > Does importing IOBluetooth.h on Mac also include the CoreBluetooth.h header? Or > don't you have to explicitly import that as well? Sure, it will import CoreBluetooth automatically for us. I can't import CoreBluetooth directly on Mac. https://codereview.chromium.org/791763005/diff/120001/device/bluetooth/blueto... File device/bluetooth/bluetooth_low_energy_device_mac.h (right): https://codereview.chromium.org/791763005/diff/120001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_device_mac.h:26: On 2015/01/24 18:07:09, armansito wrote: > nit: We usually don't add new lines between override declarations. I would add > one big block of overrides preceded by a comment saying "// BluetoothDevice > overrides". Done. https://codereview.chromium.org/791763005/diff/120001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_device_mac.h:103: base::scoped_nsobject<CBPeripheral> peripheral_; On 2015/01/24 18:07:09, armansito wrote: > nit: Add new lines between member declarations here. Done. https://codereview.chromium.org/791763005/diff/120001/device/bluetooth/blueto... File device/bluetooth/bluetooth_low_energy_device_mac.mm (right): https://codereview.chromium.org/791763005/diff/120001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_device_mac.mm:1: #include "device/bluetooth/bluetooth_low_energy_device_mac.h" On 2015/01/24 18:07:09, armansito wrote: > Copyright header? Done. https://codereview.chromium.org/791763005/diff/120001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_device_mac.mm:9: namespace { On 2015/01/24 18:07:09, armansito wrote: > nit: new line here. Done. https://codereview.chromium.org/791763005/diff/120001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_device_mac.mm:53: } On 2015/01/24 18:07:10, armansito wrote: > nit: Closing brace for namespace should read "} // namespace". Done. https://codereview.chromium.org/791763005/diff/120001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_device_mac.mm:85: return std::string([uuidString UTF8String]); On 2015/01/24 18:07:09, armansito wrote: > nit: Remove else since you already have an early return. Done. https://codereview.chromium.org/791763005/diff/120001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_device_mac.mm:138: const ConnectionInfoCallback& callback) {} On 2015/01/24 18:07:10, armansito wrote: > Add NOTIMPLEMENTED() ? Done. https://codereview.chromium.org/791763005/diff/120001/device/bluetooth/blueto... File device/bluetooth/bluetooth_low_energy_discovery_manager_mac.h (right): https://codereview.chromium.org/791763005/diff/120001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_discovery_manager_mac.h:5: #ifndef DEVICE_BLUETOOTH_BLUETOOTH_LOW_ENERGY_DISCOVER_MANAGER_MAC_H_ On 2015/01/24 17:59:37, armansito wrote: > The header is named *_discovery_manager_* but everything else is named "discover > manager" (without the 'y'). Please name this class *DiscoveryManager* and update > the header guards. Done. https://codereview.chromium.org/791763005/diff/120001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_discovery_manager_mac.h:26: class BluetoothLowEnergyDiscoverManagerMac { On 2015/01/24 17:59:37, armansito wrote: > This class should inherit from BluetoothDiscoveryManagerMac. The idea is that > you would then call BluetoothDiscoveryManagerMac::CreateLowEnergy() (which you > would add), which would then return a BluetoothLowEnergyDiscoveryManagerMac. Does it make sense to inherit from BluetoothDiscoveryManagerMac if CreateLowEnergy() returns an explicit instance of BluetoothLowEnergyDiscoveryManagerMac? The alternative is for CreateLowEnergy() to return a BluetoothDiscoveryManagerMac. Though, this option doesn't work great since BluetoothDiscoveryManagerMac::observer works with IOBluetooth objects whereas low energy alternative works with BluetoothLowEnergyDeviceMac. Let me know if you have some specific suggestions. https://codereview.chromium.org/791763005/diff/120001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_discovery_manager_mac.h:57: NSDictionary* advertisementData, int rssi); On 2015/01/24 17:59:38, armansito wrote: > nit: new line after this method Done. https://codereview.chromium.org/791763005/diff/120001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_discovery_manager_mac.h:71: // Observer interested in notifications from us. On 2015/01/24 17:59:37, armansito wrote: > nit: Please add a new line after member declarations if the following member > declaration is preceded by a comment. Done. https://codereview.chromium.org/791763005/diff/120001/device/bluetooth/blueto... File device/bluetooth/bluetooth_low_energy_discovery_manager_mac.mm (right): https://codereview.chromium.org/791763005/diff/120001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_discovery_manager_mac.mm:1: #include "device/bluetooth/bluetooth_low_energy_discovery_manager_mac.h" On 2015/01/24 17:59:38, armansito wrote: > Is this file missing the copyright header? Done. https://codereview.chromium.org/791763005/diff/120001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_discovery_manager_mac.mm:21: virtual void DiscoveredPeripheral(CBPeripheral *peripheral, On 2015/01/24 17:59:38, armansito wrote: > nit: This should read "CBPeripheral* peripheral" (no space before '*') per > style-guide. Here and everywhere else. Done. https://codereview.chromium.org/791763005/diff/120001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_discovery_manager_mac.mm:48: self = [super init]; On 2015/01/24 17:59:38, armansito wrote: > nit: check if init worked here (i.e. set the members inside if (self) { ... }) Done. https://codereview.chromium.org/791763005/diff/120001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_discovery_manager_mac.mm:102: } On 2015/01/24 17:59:38, armansito wrote: > Please add new lines where you can to make the code more readable. Here and > elsewhere. Done. https://codereview.chromium.org/791763005/diff/120001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_discovery_manager_mac.mm:105: if (services_uuids_.size() > 0) { On 2015/01/24 17:59:38, armansito wrote: > nit: if (!services_uuids_.empty()) { Done. https://codereview.chromium.org/791763005/diff/120001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_discovery_manager_mac.mm:120: [manager_ stopScan]; On 2015/01/24 17:59:38, armansito wrote: > Don't call this if |discovering_| is true? Done. https://codereview.chromium.org/791763005/diff/120001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_discovery_manager_mac.mm:127: BluetoothLowEnergyDeviceMac *device = On 2015/01/24 17:59:38, armansito wrote: > It seems like this is being unnecessarily created and deleted if there's only an > update for an existing device. Move this into the corresponding if-block. The main goal here is to be able to lookup any existing device using device->GetIdentifier(). Do you have something in mind about how to improve this? https://codereview.chromium.org/791763005/diff/120001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_discovery_manager_mac.mm:132: if (iter != devices_.end()) { On 2015/01/24 17:59:38, armansito wrote: > Do an early return and get rid of the else. I'd actually turn this if statement > around, so > > if (iter == device_.end()) { > BluetoothLowEnergyDeviceMac* device = new BluetoothLowE...; > devices_.insert(...); > observer_->DeviceFound(device); > return; > } > > BluetoothLowEnergyDeviceMac* old_device = ...; > ... Done. https://codereview.chromium.org/791763005/diff/120001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_discovery_manager_mac.mm:161: [[CBCentralManager alloc] initWithDelegate:bridge_ On 2015/01/24 17:59:38, armansito wrote: > Will this code compile at all in older versions? Will we need to add something > to base/mac/sdk_forward_declarations.*? The build system of Chromium will build using a recent enough version of the Mac OS X SDK. Even if the build is running on old version of Mac OS X, it will still build. Do we need to support old version of the Mac OS X SDK?
armansito@, could you take a look? Thanks!
https://codereview.chromium.org/791763005/diff/120001/device/bluetooth/blueto... File device/bluetooth/bluetooth_low_energy_device_mac.h (left): https://codereview.chromium.org/791763005/diff/120001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_device_mac.h:8: #import <IOBluetooth/IOBluetooth.h> On 2015/01/27 00:14:55, dvh wrote: > On 2015/01/24 17:59:37, armansito wrote: > > Does importing IOBluetooth.h on Mac also include the CoreBluetooth.h header? > Or > > don't you have to explicitly import that as well? > > Sure, it will import CoreBluetooth automatically for us. I can't import > CoreBluetooth directly on Mac. OK, I just vaguely remember having to directly import the CoreBluetooth.h header from underneath IOBluetooth (since it's nested in there somewhere). Overall sgtm. https://codereview.chromium.org/791763005/diff/120001/device/bluetooth/blueto... File device/bluetooth/bluetooth_low_energy_discovery_manager_mac.h (right): https://codereview.chromium.org/791763005/diff/120001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_discovery_manager_mac.h:26: class BluetoothLowEnergyDiscoverManagerMac { On 2015/01/27 00:14:55, dvh wrote: > On 2015/01/24 17:59:37, armansito wrote: > > This class should inherit from BluetoothDiscoveryManagerMac. The idea is that > > you would then call BluetoothDiscoveryManagerMac::CreateLowEnergy() (which you > > would add), which would then return a BluetoothLowEnergyDiscoveryManagerMac. > > Does it make sense to inherit from BluetoothDiscoveryManagerMac if > CreateLowEnergy() returns an explicit instance of > BluetoothLowEnergyDiscoveryManagerMac? > > The alternative is for CreateLowEnergy() to return a > BluetoothDiscoveryManagerMac. Though, this option doesn't work great since > BluetoothDiscoveryManagerMac::observer works with IOBluetooth objects whereas > low energy alternative works with BluetoothLowEnergyDeviceMac. > > Let me know if you have some specific suggestions. No, I like keeping the LE stuff separate from BluetoothDiscoveryManagerMac, we get this nice isolation from it. The inheritance is mostly organizational. We don't make use of any polymorphism here but I think its nice to say that these two DiscoveryManager classes conform to the same interface. https://codereview.chromium.org/791763005/diff/120001/device/bluetooth/blueto... File device/bluetooth/bluetooth_low_energy_discovery_manager_mac.mm (right): https://codereview.chromium.org/791763005/diff/120001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_discovery_manager_mac.mm:127: BluetoothLowEnergyDeviceMac *device = On 2015/01/27 00:14:55, dvh wrote: > On 2015/01/24 17:59:38, armansito wrote: > > It seems like this is being unnecessarily created and deleted if there's only > an > > update for an existing device. Move this into the corresponding if-block. > > The main goal here is to be able to lookup any existing device using > device->GetIdentifier(). > Do you have something in mind about how to improve this? It seems strange to me to construct a whole object just to get its identifier, which you then conditionally delete later. Since you're mostly interested in the identifier and you already have a pointer to the CBPeripheral, maybe its better to just define a helper that does the conversion and then call that same helper from BluetoothLowEnergyDeviceMac::GetIdentifier()? https://codereview.chromium.org/791763005/diff/120001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_discovery_manager_mac.mm:161: [[CBCentralManager alloc] initWithDelegate:bridge_ On 2015/01/27 00:14:56, dvh wrote: > On 2015/01/24 17:59:38, armansito wrote: > > Will this code compile at all in older versions? Will we need to add something > > to base/mac/sdk_forward_declarations.*? > > The build system of Chromium will build using a recent enough version of the Mac > OS X SDK. > Even if the build is running on old version of Mac OS X, it will still build. > > Do we need to support old version of the Mac OS X SDK? I don't directly work on Chrome for Mac, so as long as you're confident that this won't be built on older versions I'm OK with it. Maybe a get a reviewer from that side confirm this as well? https://codereview.chromium.org/791763005/diff/140001/device/bluetooth/blueto... File device/bluetooth/bluetooth_low_energy_device_mac.h (right): https://codereview.chromium.org/791763005/diff/140001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_device_mac.h:30: // BluetoothDevice override. nit: s/override/overrides/
dvh@chromium.org changed reviewers: + groby@chromium.org
> https://codereview.chromium.org/791763005/diff/120001/device/bluetooth/blueto... > device/bluetooth/bluetooth_low_energy_discovery_manager_mac.mm:161: > [[CBCentralManager alloc] initWithDelegate:bridge_ > On 2015/01/27 00:14:56, dvh wrote: > > On 2015/01/24 17:59:38, armansito wrote: > > > Will this code compile at all in older versions? Will we need to add > something > > > to base/mac/sdk_forward_declarations.*? > > > > The build system of Chromium will build using a recent enough version of the > Mac > > OS X SDK. > > Even if the build is running on old version of Mac OS X, it will still build. > > > > Do we need to support old version of the Mac OS X SDK? > > I don't directly work on Chrome for Mac, so as long as you're confident that > this won't be built on older versions I'm OK with it. Maybe a get a reviewer > from that side confirm this as well? adding groby@ as reviewer to confirm that Chrome for Mac won't build with old versions of the SDK.
> No, I like keeping the LE stuff separate from BluetoothDiscoveryManagerMac, we > get this nice isolation from it. The inheritance is mostly organizational. We > don't make use of any polymorphism here but I think its nice to say that these > two DiscoveryManager classes conform to the same interface. Would it be ok to have the following since we can't reuse the same observer? BluetoothDiscoveryManagerMac::CreateLowEnergy(BluetoothDevice::UUIDList services_uuids, LowEnergyObserver * observer); then move BluetoothDiscoveryManagerMac::Observer to BluetoothDiscoveryManagerMac::LowEnergyObserver
avi@chromium.org changed reviewers: + avi@chromium.org - groby@chromium.org
I would highly recommend looking in base/. There are lots of utilities in there that you need to use to commit acceptable Chromium code. Secondly, where are your trybots? It's clear upon visual inspection that this won't compile. I ran across this CL randomly, but I'm taking it for Mac review. https://codereview.chromium.org/791763005/diff/140001/device/bluetooth/blueto... File device/bluetooth/bluetooth_low_energy_device_mac.h (right): https://codereview.chromium.org/791763005/diff/140001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_device_mac.h:8: #if TARGET_IPHONE_SIMULATOR || TARGET_OS_IPHONE You want to use the Chromium OS_ defines. Do not use random system defines. https://codereview.chromium.org/791763005/diff/140001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_device_mac.h:93: } Add a blank space here. Why did you remove the "//namespace device" comment? Style requires it. https://codereview.chromium.org/791763005/diff/140001/device/bluetooth/blueto... File device/bluetooth/bluetooth_low_energy_device_mac.mm (right): https://codereview.chromium.org/791763005/diff/140001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_device_mac.mm:11: using namespace device; This doesn't work, and if you tried to compile you'd get a zillion errors. You need to open the device namespace, not say "using". https://codereview.chromium.org/791763005/diff/140001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_device_mac.mm:59: } // namespace device this does not end a namespace "device"; it ends an anonymous namespace https://codereview.chromium.org/791763005/diff/140001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_device_mac.mm:94: return std::string([uuidString UTF8String]); SysNSStringToUTF8 https://codereview.chromium.org/791763005/diff/140001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_device_mac.mm:102: CFDataGetLength(data)); Huh? SysCFStringRefToUTF8. https://codereview.chromium.org/791763005/diff/140001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_device_mac.mm:231: return std::string((const char*)[data bytes], [data length]); SysNSStringToUTF8 https://codereview.chromium.org/791763005/diff/140001/device/bluetooth/blueto... File device/bluetooth/bluetooth_low_energy_discovery_manager_mac.h (right): https://codereview.chromium.org/791763005/diff/140001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_discovery_manager_mac.h:8: #if TARGET_IPHONE_SIMULATOR || TARGET_OS_IPHONE ditto https://codereview.chromium.org/791763005/diff/140001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_discovery_manager_mac.h:15: no blank line https://codereview.chromium.org/791763005/diff/140001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_discovery_manager_mac.h:25: // This class will scan for bluetooth LE device on Mac. Bluetooth is capitalized https://codereview.chromium.org/791763005/diff/140001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_discovery_manager_mac.h:35: virtual void DeviceUpdated(BluetoothLowEnergyDeviceMac* device) = 0; This interface _must_ have a virtual destructor. https://codereview.chromium.org/791763005/diff/140001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_discovery_manager_mac.h:60: // The device discovery can really be started when bluetooth is powered on. Bluetooth is capitalized. https://codereview.chromium.org/791763005/diff/140001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_discovery_manager_mac.h:63: // been started and if the bluetooth is powered and then really start the Capital B. https://codereview.chromium.org/791763005/diff/140001/device/bluetooth/blueto... File device/bluetooth/bluetooth_low_energy_discovery_manager_mac.mm (right): https://codereview.chromium.org/791763005/diff/140001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_discovery_manager_mac.mm:8: no blank space https://codereview.chromium.org/791763005/diff/140001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_discovery_manager_mac.mm:11: using namespace device; this doesn't look necessary https://codereview.chromium.org/791763005/diff/140001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_discovery_manager_mac.mm:61: delete delegate_; nonono Use a scoped_ptr for the ivar. _Never_ manually delete; if you need to manually delete, you are doing things wrong. https://codereview.chromium.org/791763005/diff/140001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_discovery_manager_mac.mm:117: for (unsigned int i = 0; i < services_uuids_.size(); i++) { for (auto& uuid : services_uuids_) ? https://codereview.chromium.org/791763005/diff/140001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_discovery_manager_mac.mm:119: stringWithUTF8String:services_uuids_[i].canonical_value().c_str()]; SysUTF8ToNSString https://codereview.chromium.org/791763005/diff/140001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_discovery_manager_mac.mm:144: std::map<const std::string, BluetoothLowEnergyDeviceMac*>::iterator iter = auto https://codereview.chromium.org/791763005/diff/140001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_discovery_manager_mac.mm:149: std::pair<const std::string, BluetoothLowEnergyDeviceMac*>( make_pair https://codereview.chromium.org/791763005/diff/140001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_discovery_manager_mac.mm:156: delete device; NEVER DELETE. Use scoped_ptr instead. https://codereview.chromium.org/791763005/diff/140001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_discovery_manager_mac.mm:164: return new BluetoothLowEnergyDiscoveryManagerMac(observer); If this returns a new object, it might make sense to make it return a scoped_ptr. https://codereview.chromium.org/791763005/diff/140001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_discovery_manager_mac.mm:176: [[CBCentralManager alloc] initWithDelegate:bridge_ You _will_ need to add this to base/mac/sdk_forward_declarations.*. Chrome builds all the way back to the 10.6 SDK. https://codereview.chromium.org/791763005/diff/140001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_discovery_manager_mac.mm:189: devices_.clear(); Look in stl_util.h and replace the entire contents of this function with STLDeleteValues.
groby@chromium.org changed reviewers: + groby@chromium.org
Sorry for completely missing this... https://codereview.chromium.org/791763005/diff/120001/device/bluetooth/blueto... File device/bluetooth/bluetooth_low_energy_discovery_manager_mac.mm (right): https://codereview.chromium.org/791763005/diff/120001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_discovery_manager_mac.mm:161: [[CBCentralManager alloc] initWithDelegate:bridge_ On 2015/01/29 04:37:18, armansito wrote: > On 2015/01/27 00:14:56, dvh wrote: > > On 2015/01/24 17:59:38, armansito wrote: > > > Will this code compile at all in older versions? Will we need to add > something > > > to base/mac/sdk_forward_declarations.*? > > > > The build system of Chromium will build using a recent enough version of the > Mac > > OS X SDK. > > Even if the build is running on old version of Mac OS X, it will still build. > > > > Do we need to support old version of the Mac OS X SDK? > > I don't directly work on Chrome for Mac, so as long as you're confident that > this won't be built on older versions I'm OK with it. Maybe a get a reviewer > from that side confirm this as well? Unless I'm completely misinformed, we're currently building against the 10.6 SDK. See http://crbug.com/335325 for the ongoing effort to move away from that.
Updated. Please take a look. Thanks! https://codereview.chromium.org/791763005/diff/140001/device/bluetooth/blueto... File device/bluetooth/bluetooth_low_energy_device_mac.h (right): https://codereview.chromium.org/791763005/diff/140001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_device_mac.h:8: #if TARGET_IPHONE_SIMULATOR || TARGET_OS_IPHONE On 2015/01/29 05:25:02, Avi wrote: > You want to use the Chromium OS_ defines. Do not use random system defines. Done. https://codereview.chromium.org/791763005/diff/140001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_device_mac.h:30: // BluetoothDevice override. On 2015/01/29 04:37:19, armansito wrote: > nit: s/override/overrides/ Done. https://codereview.chromium.org/791763005/diff/140001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_device_mac.h:93: } On 2015/01/29 05:25:02, Avi wrote: > Add a blank space here. Why did you remove the "//namespace device" comment? > Style requires it. Done. https://codereview.chromium.org/791763005/diff/140001/device/bluetooth/blueto... File device/bluetooth/bluetooth_low_energy_device_mac.mm (right): https://codereview.chromium.org/791763005/diff/140001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_device_mac.mm:11: using namespace device; On 2015/01/29 05:25:02, Avi wrote: > This doesn't work, and if you tried to compile you'd get a zillion errors. You > need to open the device namespace, not say "using". I'm not sure to understand what you mean here. Could you provide further details? https://codereview.chromium.org/791763005/diff/140001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_device_mac.mm:59: } // namespace device On 2015/01/29 05:25:02, Avi wrote: > this does not end a namespace "device"; it ends an anonymous namespace Done. https://codereview.chromium.org/791763005/diff/140001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_device_mac.mm:94: return std::string([uuidString UTF8String]); On 2015/01/29 05:25:02, Avi wrote: > SysNSStringToUTF8 Done. https://codereview.chromium.org/791763005/diff/140001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_device_mac.mm:102: CFDataGetLength(data)); On 2015/01/29 05:25:02, Avi wrote: > Huh? SysCFStringRefToUTF8. Done. https://codereview.chromium.org/791763005/diff/140001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_device_mac.mm:231: return std::string((const char*)[data bytes], [data length]); On 2015/01/29 05:25:02, Avi wrote: > SysNSStringToUTF8 Done. https://codereview.chromium.org/791763005/diff/140001/device/bluetooth/blueto... File device/bluetooth/bluetooth_low_energy_discovery_manager_mac.h (right): https://codereview.chromium.org/791763005/diff/140001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_discovery_manager_mac.h:8: #if TARGET_IPHONE_SIMULATOR || TARGET_OS_IPHONE On 2015/01/29 05:25:02, Avi wrote: > ditto Done. https://codereview.chromium.org/791763005/diff/140001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_discovery_manager_mac.h:15: On 2015/01/29 05:25:02, Avi wrote: > no blank line Done. https://codereview.chromium.org/791763005/diff/140001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_discovery_manager_mac.h:25: // This class will scan for bluetooth LE device on Mac. On 2015/01/29 05:25:02, Avi wrote: > Bluetooth is capitalized Done. https://codereview.chromium.org/791763005/diff/140001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_discovery_manager_mac.h:35: virtual void DeviceUpdated(BluetoothLowEnergyDeviceMac* device) = 0; On 2015/01/29 05:25:02, Avi wrote: > This interface _must_ have a virtual destructor. is BluetoothDiscoveryManagerMac::Observer wrong as well? https://codereview.chromium.org/791763005/diff/140001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_discovery_manager_mac.h:60: // The device discovery can really be started when bluetooth is powered on. On 2015/01/29 05:25:02, Avi wrote: > Bluetooth is capitalized. Done. https://codereview.chromium.org/791763005/diff/140001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_discovery_manager_mac.h:63: // been started and if the bluetooth is powered and then really start the On 2015/01/29 05:25:02, Avi wrote: > Capital B. Done. https://codereview.chromium.org/791763005/diff/140001/device/bluetooth/blueto... File device/bluetooth/bluetooth_low_energy_discovery_manager_mac.mm (right): https://codereview.chromium.org/791763005/diff/140001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_discovery_manager_mac.mm:8: On 2015/01/29 05:25:03, Avi wrote: > no blank space Done. https://codereview.chromium.org/791763005/diff/140001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_discovery_manager_mac.mm:11: using namespace device; On 2015/01/29 05:25:02, Avi wrote: > this doesn't look necessary It will let me refer BluetoothLowEnergyDiscoveryManagerMac with BluetoothLowEnergyDiscoveryManagerMac instead of device:: BluetoothLowEnergyDiscoveryManagerMac. https://codereview.chromium.org/791763005/diff/140001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_discovery_manager_mac.mm:61: delete delegate_; On 2015/01/29 05:25:03, Avi wrote: > nonono > > Use a scoped_ptr for the ivar. _Never_ manually delete; if you need to manually > delete, you are doing things wrong. Done. https://codereview.chromium.org/791763005/diff/140001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_discovery_manager_mac.mm:117: for (unsigned int i = 0; i < services_uuids_.size(); i++) { On 2015/01/29 05:25:03, Avi wrote: > for (auto& uuid : services_uuids_) ? Done. https://codereview.chromium.org/791763005/diff/140001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_discovery_manager_mac.mm:119: stringWithUTF8String:services_uuids_[i].canonical_value().c_str()]; On 2015/01/29 05:25:03, Avi wrote: > SysUTF8ToNSString Done. https://codereview.chromium.org/791763005/diff/140001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_discovery_manager_mac.mm:144: std::map<const std::string, BluetoothLowEnergyDeviceMac*>::iterator iter = On 2015/01/29 05:25:03, Avi wrote: > auto Done. https://codereview.chromium.org/791763005/diff/140001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_discovery_manager_mac.mm:149: std::pair<const std::string, BluetoothLowEnergyDeviceMac*>( On 2015/01/29 05:25:02, Avi wrote: > make_pair Done. https://codereview.chromium.org/791763005/diff/140001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_discovery_manager_mac.mm:156: delete device; On 2015/01/29 05:25:03, Avi wrote: > NEVER DELETE. Use scoped_ptr instead. Acknowledged. https://codereview.chromium.org/791763005/diff/140001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_discovery_manager_mac.mm:164: return new BluetoothLowEnergyDiscoveryManagerMac(observer); On 2015/01/29 05:25:03, Avi wrote: > If this returns a new object, it might make sense to make it return a > scoped_ptr. see BluetoothDiscoveryManagerMac::CreateClassic(). https://codereview.chromium.org/791763005/diff/140001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_discovery_manager_mac.mm:176: [[CBCentralManager alloc] initWithDelegate:bridge_ On 2015/01/29 05:25:03, Avi wrote: > You _will_ need to add this to base/mac/sdk_forward_declarations.*. Chrome > builds all the way back to the 10.6 SDK. Done. https://codereview.chromium.org/791763005/diff/140001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_discovery_manager_mac.mm:189: devices_.clear(); On 2015/01/29 05:25:03, Avi wrote: > Look in stl_util.h and replace the entire contents of this function with > STLDeleteValues. Done.
https://codereview.chromium.org/791763005/diff/140001/device/bluetooth/blueto... File device/bluetooth/bluetooth_low_energy_device_mac.mm (right): https://codereview.chromium.org/791763005/diff/140001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_device_mac.mm:11: using namespace device; On 2015/01/30 22:45:45, dvh wrote: > On 2015/01/29 05:25:02, Avi wrote: > > This doesn't work, and if you tried to compile you'd get a zillion errors. You > > need to open the device namespace, not say "using". > > I'm not sure to understand what you mean here. > Could you provide further details? Do not use "using" in this way. First, it's forbidden by the style guide. http://google-styleguide.googlecode.com/svn/trunk/cppguide.html says: "You may not use a using-directive to make all names from a namespace available." Second, just open namespace device { here and close it at the bottom. You put BluetoothLowEnergyDeviceMac into the "device" namespace in the header file. Put it into that namespace in this file. https://codereview.chromium.org/791763005/diff/140001/device/bluetooth/blueto... File device/bluetooth/bluetooth_low_energy_discovery_manager_mac.h (right): https://codereview.chromium.org/791763005/diff/140001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_discovery_manager_mac.h:35: virtual void DeviceUpdated(BluetoothLowEnergyDeviceMac* device) = 0; On 2015/01/30 22:45:45, dvh wrote: > On 2015/01/29 05:25:02, Avi wrote: > > This interface _must_ have a virtual destructor. > > is BluetoothDiscoveryManagerMac::Observer wrong as well? Yes, it is. The style guide in "Interfaces" http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Interfaces says: "To make sure all implementations of the interface can be destroyed correctly, the interface must also declare a virtual destructor." If you could take care of BluetoothDiscoveryManagerMac::Observer, that would be great. https://codereview.chromium.org/791763005/diff/140001/device/bluetooth/blueto... File device/bluetooth/bluetooth_low_energy_discovery_manager_mac.mm (right): https://codereview.chromium.org/791763005/diff/140001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_discovery_manager_mac.mm:11: using namespace device; On 2015/01/30 22:45:46, dvh wrote: > On 2015/01/29 05:25:02, Avi wrote: > > this doesn't look necessary > > It will let me refer BluetoothLowEnergyDiscoveryManagerMac with > BluetoothLowEnergyDiscoveryManagerMac instead of device:: > BluetoothLowEnergyDiscoveryManagerMac. If you want to access the class without the device::, use a using-declaration, like using device::BluetoothLowEnergyDiscoveryManagerMac; You are never allowed to use a using-directive, such as you do here. (That's prohibited by the style guide, as mentioned earlier.) https://codereview.chromium.org/791763005/diff/180001/device/bluetooth/blueto... File device/bluetooth/bluetooth_low_energy_device_mac.mm (right): https://codereview.chromium.org/791763005/diff/180001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_device_mac.mm:60: } Put a blank line before this. Also, you need to mark it as // namespace https://codereview.chromium.org/791763005/diff/180001/device/bluetooth/blueto... File device/bluetooth/bluetooth_low_energy_discovery_manager_mac.mm (right): https://codereview.chromium.org/791763005/diff/180001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_discovery_manager_mac.mm:64: } You don't need to override -dealloc if all you're doing is calling through.
Updated. Please take a look again. Thanks! https://codereview.chromium.org/791763005/diff/180001/device/bluetooth/blueto... File device/bluetooth/bluetooth_low_energy_device_mac.mm (right): https://codereview.chromium.org/791763005/diff/180001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_device_mac.mm:60: } On 2015/01/30 23:18:31, Avi wrote: > Put a blank line before this. Also, you need to mark it as > > // namespace Done. https://codereview.chromium.org/791763005/diff/180001/device/bluetooth/blueto... File device/bluetooth/bluetooth_low_energy_discovery_manager_mac.mm (right): https://codereview.chromium.org/791763005/diff/180001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_discovery_manager_mac.mm:64: } On 2015/01/30 23:18:31, Avi wrote: > You don't need to override -dealloc if all you're doing is calling through. Done.
lgtm https://codereview.chromium.org/791763005/diff/200001/device/bluetooth/blueto... File device/bluetooth/bluetooth_discovery_manager_mac.h (right): https://codereview.chromium.org/791763005/diff/200001/device/bluetooth/blueto... device/bluetooth/bluetooth_discovery_manager_mac.h:22: virtual ~Observer() {} nit: put this in a protected section https://codereview.chromium.org/791763005/diff/200001/device/bluetooth/blueto... File device/bluetooth/bluetooth_low_energy_discovery_manager_mac.h (right): https://codereview.chromium.org/791763005/diff/200001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_discovery_manager_mac.h:31: same here
groby@, Could you take an other look? Thanks. armansito@, Could you take a look at the questions I have and reply? Thanks!
LGTM w/ tiny nit (Caveat - I'm not an owner, so mostly a syntactical scan) https://codereview.chromium.org/791763005/diff/220001/device/bluetooth/blueto... File device/bluetooth/bluetooth_low_energy_device_mac.mm (right): https://codereview.chromium.org/791763005/diff/220001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_device_mac.mm:53: if ([uuid respondsToSelector:@selector(UUIDString)]) { Please add a TODO: that this can be removed once we move to 10.10. (Yes, it's a long way out - but it'll be a bit harder to miss once we get there)
On 2015/02/02 19:21:12, groby wrote: > LGTM w/ tiny nit > > (Caveat - I'm not an owner, so mostly a syntactical scan) > > https://codereview.chromium.org/791763005/diff/220001/device/bluetooth/blueto... > File device/bluetooth/bluetooth_low_energy_device_mac.mm (right): > > https://codereview.chromium.org/791763005/diff/220001/device/bluetooth/blueto... > device/bluetooth/bluetooth_low_energy_device_mac.mm:53: if ([uuid > respondsToSelector:@selector(UUIDString)]) { > Please add a TODO: that this can be removed once we move to 10.10. (Yes, it's a > long way out - but it'll be a bit harder to miss once we get there) Even if we moved to 10.10 build, Chrome would still run on old platforms (< 10.10), then, the method would still not be available even if it builds and it would fail when calling it.
Sorry, I meant once we move to a 10.10 minimum SDK. Yes, I know that will be a while :)
Looks a lot better, lgtm with one nit. https://codereview.chromium.org/791763005/diff/240001/device/bluetooth/blueto... File device/bluetooth/bluetooth_low_energy_device_mac.h (right): https://codereview.chromium.org/791763005/diff/240001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_device_mac.h:28: virtual int GetRSSI() const; nit: Does this need to be virtual?
New patchsets have been uploaded after l-g-t-m from armansito@chromium.org
https://codereview.chromium.org/791763005/diff/200001/device/bluetooth/blueto... File device/bluetooth/bluetooth_discovery_manager_mac.h (right): https://codereview.chromium.org/791763005/diff/200001/device/bluetooth/blueto... device/bluetooth/bluetooth_discovery_manager_mac.h:22: virtual ~Observer() {} On 2015/01/31 02:22:38, Avi wrote: > nit: put this in a protected section Done. https://codereview.chromium.org/791763005/diff/200001/device/bluetooth/blueto... File device/bluetooth/bluetooth_low_energy_discovery_manager_mac.h (right): https://codereview.chromium.org/791763005/diff/200001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_discovery_manager_mac.h:31: On 2015/01/31 02:22:38, Avi wrote: > same here Done. https://codereview.chromium.org/791763005/diff/220001/device/bluetooth/blueto... File device/bluetooth/bluetooth_low_energy_device_mac.mm (right): https://codereview.chromium.org/791763005/diff/220001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_device_mac.mm:53: if ([uuid respondsToSelector:@selector(UUIDString)]) { On 2015/02/02 19:21:12, groby wrote: > Please add a TODO: that this can be removed once we move to 10.10. (Yes, it's a > long way out - but it'll be a bit harder to miss once we get there) Done. https://codereview.chromium.org/791763005/diff/240001/device/bluetooth/blueto... File device/bluetooth/bluetooth_low_energy_device_mac.h (right): https://codereview.chromium.org/791763005/diff/240001/device/bluetooth/blueto... device/bluetooth/bluetooth_low_energy_device_mac.h:28: virtual int GetRSSI() const; On 2015/02/05 19:54:12, armansito wrote: > nit: Does this need to be virtual? Acknowledged.
The CQ bit was checked by dvh@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/791763005/260001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by dvh@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/791763005/280001
The CQ bit was unchecked by commit-bot@chromium.org
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 dvh@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/791763005/300001
The CQ bit was unchecked by dvh@chromium.org
The CQ bit was checked by dvh@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/791763005/320001
The CQ bit was unchecked by commit-bot@chromium.org
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 dvh@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/791763005/340001
Message was sent while issue was closed.
Committed patchset #13 (id:340001)
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/42f79f8b08a99190627f3a67e86a378987104a4a Cr-Commit-Position: refs/heads/master@{#315190}
Message was sent while issue was closed.
A revert of this CL (patchset #13 id:340001) has been created in https://codereview.chromium.org/871843009/ by ccameron@chromium.org. The reason for reverting is: Breaks compile: http://build.chromium.org/p/chromium.mac/builders/Mac%20Builder/builds/17750 ../../device/bluetooth/bluetooth_low_energy_discovery_manager_mac.mm:119:30: error: class method '+UUIDWithString:' not found (return type defaults to 'id') [-Werror,-Wobjc-method-access] CBUUID* uuid = [CBUUID UUIDWithString:uuidString]; ^~~~~~~~~~~~~~ ../../device/bluetooth/bluetooth_low_energy_device_mac.mm:82:39: error: use of undeclared identifier 'CBAdvertisementDataIsConnectable' [advertisementData objectForKey:CBAdvertisementDataIsConnectable]; ^ ../../device/bluetooth/bluetooth_low_energy_device_mac.mm:85:39: error: use of undeclared identifier 'CBAdvertisementDataServiceDataKey' [advertisementData objectForKey:CBAdvertisementDataServiceDataKey]; ^ 2 errors generated.. |
