Implementing GATT connection/disconnect on OS X.

Implementing GATT connection/disconnect on OS X.

BUG=520774
Committed: https://crrev.com/a58fd7578542a34fe30bf56ae52b884ad71c9061
Cr-Commit-Position: refs/heads/master@{#380951}

[François Beaufort, 2015-12-22 07:38:17]

beaufort.francois@gmail.com changed reviewers:
+ beaufort.francois@gmail.com

[François Beaufort, 2015-12-22 07:38:17]

Nits...

https://codereview.chromium.org/1538173003/diff/1/device/bluetooth/bluetooth_...
File device/bluetooth/bluetooth_adapter_mac.h (right):

https://codereview.chromium.org/1538173003/diff/1/device/bluetooth/bluetooth_...
device/bluetooth/bluetooth_adapter_mac.h:104: void
CreateGattConnection(BluetoothLowEnergyDeviceMac* deviceMac);
s/deviceMac/device_mac

https://codereview.chromium.org/1538173003/diff/1/device/bluetooth/bluetooth_...
device/bluetooth/bluetooth_adapter_mac.h:106: void
DisconnectGatt(BluetoothLowEnergyDeviceMac* deviceMac);
s/deviceMac/device_mac

[scheib, 2016-01-20 17:59:29]

Description was changed from

==========
Implementing GATT connection/disconnect on OS X.

BUG=520774
==========

to

==========
Implementing GATT connection/disconnect on OS X.

BUG=520774
==========

[scheib, 2016-01-20 17:59:29]

scheib@chromium.org changed reviewers:
+ scheib@chromium.org

[scheib, 2016-01-27 01:09:03]

Generally looking good for implementation. Tests, we discussed and I've started
to fix some of the documentation for them:

https://codereview.chromium.org/1619173002/

https://docs.google.com/document/d/1mBipxn1sJu6jMqP0RQZpkYXC1
     o601bzLCpCxwTA2yGA/edit?usp=sharing

https://codereview.chromium.org/1538173003/diff/40001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_adapter_mac.h (right):

https://codereview.chromium.org/1538173003/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_adapter_mac.h:100: void
CreateGattConnection(BluetoothLowEnergyDeviceMac* device_mac);
Comment group of methods. I see that these provide support for the
BluetoothLowEnergyDeviceMac objects to request Core Bluetooth to do things and
CB to route back to the device objects.

https://codereview.chromium.org/1538173003/diff/40001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_low_energy_device_mac.mm (right):

https://codereview.chromium.org/1538173003/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_low_energy_device_mac.mm:230: void
BluetoothLowEnergyDeviceMac::GattConnected() {
Please match declaration and definition order. (move this to after
GetPeripheralState, or change .h ordering)

[jlebel, 2016-02-10 13:38:51]

Hello Vincent,

My patch is done. So I implemented connect and disconnect and now the mac goes
through all tests related to connect and disconnect only.

My next patch will be about discovering services (and maybe characteristics in
the same patch).

Thanks,

https://codereview.chromium.org/1538173003/diff/1/device/bluetooth/bluetooth_...
File device/bluetooth/bluetooth_adapter_mac.h (right):

https://codereview.chromium.org/1538173003/diff/1/device/bluetooth/bluetooth_...
device/bluetooth/bluetooth_adapter_mac.h:104: void
CreateGattConnection(BluetoothLowEnergyDeviceMac* deviceMac);
On 2015/12/22 07:38:17, François Beaufort wrote:
> s/deviceMac/device_mac

Done.

https://codereview.chromium.org/1538173003/diff/1/device/bluetooth/bluetooth_...
device/bluetooth/bluetooth_adapter_mac.h:106: void
DisconnectGatt(BluetoothLowEnergyDeviceMac* deviceMac);
On 2015/12/22 07:38:17, François Beaufort wrote:
> s/deviceMac/device_mac

Done.

https://codereview.chromium.org/1538173003/diff/40001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_adapter_mac.h (right):

https://codereview.chromium.org/1538173003/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_adapter_mac.h:100: void
CreateGattConnection(BluetoothLowEnergyDeviceMac* device_mac);
On 2016/01/27 01:09:03, scheib wrote:
> Comment group of methods. I see that these provide support for the
> BluetoothLowEnergyDeviceMac objects to request Core Bluetooth to do things and
> CB to route back to the device objects.

Done.

https://codereview.chromium.org/1538173003/diff/40001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_low_energy_device_mac.mm (right):

https://codereview.chromium.org/1538173003/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_low_energy_device_mac.mm:230: void
BluetoothLowEnergyDeviceMac::GattConnected() {
On 2016/01/27 01:09:03, scheib wrote:
> Please match declaration and definition order. (move this to after
> GetPeripheralState, or change .h ordering)

Done.

[scheib, 2016-02-10 18:59:46]

It would also be good to have a Chromium reviewer with more  Objective C
experience - mine is weak. Do you have anyone local you could add to this
review? Else, I can dig up a MTV reviewer.

https://codereview.chromium.org/1538173003/diff/200001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter_mac.h (right):

https://codereview.chromium.org/1538173003/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_mac.h:103: // Close the GATT connection by
calling CoreBluetooth APIs.
Closes https://google.github.io/styleguide/cppguide.html#Function_Comments

https://codereview.chromium.org/1538173003/diff/200001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter_mac.mm (right):

https://codereview.chromium.org/1538173003/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_mac.mm:96: // Need to make sure the devices
are destroyed before CBCentralManage being
CBCentralManager is released

https://codereview.chromium.org/1538173003/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_mac.mm:586: // TODO(jlebel): Need to convert
the error.
Please make this a TODO(http://crbug.com/1111111) and have that issue block some
appropriate parent issue that eventually blocks either
https://code.google.com/p/chromium/issues/detail?id=364359 or
https://code.google.com/p/chromium/issues/detail?id=518942

https://codereview.chromium.org/1538173003/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_mac.mm:592: std::string device_address =
This code appears three times, refactor to a helper method named e.g.
GetDeviceOrDisconnectPeripheral.

https://codereview.chromium.org/1538173003/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_mac.mm:603: // TODO(jlebel): Need to pass the
error.
ditto

https://codereview.chromium.org/1538173003/diff/200001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_low_energy_device_mac.h (right):

https://codereview.chromium.org/1538173003/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_low_energy_device_mac.h:110: BluetoothAdapterMac*
GetBluetoothAdapterMac();
GetMacAdapter (this will now match a patch just landing
https://codereview.chromium.org/1681853003/)

https://codereview.chromium.org/1538173003/diff/200001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_low_energy_device_mac.mm (right):

https://codereview.chromium.org/1538173003/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_low_energy_device_mac.mm:46: if
(this->IsGattConnected()) {
if (IsGattConnected())

https://codereview.chromium.org/1538173003/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_low_energy_device_mac.mm:49:
adapter->DisconnectGatt(this);
GetMacAdapter()->DisconnectGatt(this);

https://codereview.chromium.org/1538173003/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_low_energy_device_mac.mm:233:
adapter->CreateGattConnection(this);
GetMacAdapter()->CreateGattConnection(this);

https://codereview.chromium.org/1538173003/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_low_energy_device_mac.mm:240:
adapter->DisconnectGatt(this);
GetMacAdapter()->CreateGattConnection(this);

https://codereview.chromium.org/1538173003/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_low_energy_device_mac.mm:267: return adapter;
return static_cast<BluetoothAdapterMac*>(this->adapter_);

https://codereview.chromium.org/1538173003/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_low_energy_device_mac.mm:283: this->DidConnectGatt();
DidConnectGatt();

https://codereview.chromium.org/1538173003/diff/200001/device/bluetooth/test/...
File device/bluetooth/test/bluetooth_test_mac.h (right):

https://codereview.chromium.org/1538173003/diff/200001/device/bluetooth/test/...
device/bluetooth/test/bluetooth_test_mac.h:48: void* mock_central_manager_ =
NULL;
Could we use a scoped object from src/base/mac/ here (scoped_nsobject?), so that
release doesn't need to be called manually?

https://codereview.chromium.org/1538173003/diff/200001/device/bluetooth/test/...
File device/bluetooth/test/bluetooth_test_mac.mm (right):

https://codereview.chromium.org/1538173003/diff/200001/device/bluetooth/test/...
device/bluetooth/test/bluetooth_test_mac.mm:64:
[(MockCentralManager*)mock_central_manager_ release];
Use static_cast instead of C-Style casts.
https://google.github.io/styleguide/cppguide.html#Casting Here and in many other
places.

https://codereview.chromium.org/1538173003/diff/200001/device/bluetooth/test/...
device/bluetooth/test/bluetooth_test_mac.mm:162: CBPeripheral* peripheral =
(CBPeripheral*)[[MockCBPeripheral alloc]
scoped object?

https://codereview.chromium.org/1538173003/diff/200001/device/bluetooth/test/...
device/bluetooth/test/bluetooth_test_mac.mm:224: // TODO(jlebel): Need to
convert the connect error code into NSError
ditto: crbug.com

[jlebel, 2016-02-10 21:20:08]

Hello Vincent,

I've updated my patch.

Thanks,

https://codereview.chromium.org/1538173003/diff/200001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter_mac.h (right):

https://codereview.chromium.org/1538173003/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_mac.h:103: // Close the GATT connection by
calling CoreBluetooth APIs.
On 2016/02/10 18:59:45, scheib wrote:
> Closes https://google.github.io/styleguide/cppguide.html#Function_Comments

Done.

https://codereview.chromium.org/1538173003/diff/200001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter_mac.mm (right):

https://codereview.chromium.org/1538173003/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_mac.mm:96: // Need to make sure the devices
are destroyed before CBCentralManage being
On 2016/02/10 18:59:45, scheib wrote:
> CBCentralManager is released

Done.

https://codereview.chromium.org/1538173003/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_mac.mm:586: // TODO(jlebel): Need to convert
the error.
On 2016/02/10 18:59:45, scheib wrote:
> Please make this a TODO(http://crbug.com/1111111) and have that issue block
some
> appropriate parent issue that eventually blocks either
> https://code.google.com/p/chromium/issues/detail?id=364359 or
> https://code.google.com/p/chromium/issues/detail?id=518942

Done.

https://codereview.chromium.org/1538173003/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_mac.mm:592: std::string device_address =
On 2016/02/10 18:59:45, scheib wrote:
> This code appears three times, refactor to a helper method named e.g.
> GetDeviceOrDisconnectPeripheral.

Done.

https://codereview.chromium.org/1538173003/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_mac.mm:603: // TODO(jlebel): Need to pass the
error.
On 2016/02/10 18:59:45, scheib wrote:
> ditto

Done.

https://codereview.chromium.org/1538173003/diff/200001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_low_energy_device_mac.h (right):

https://codereview.chromium.org/1538173003/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_low_energy_device_mac.h:110: BluetoothAdapterMac*
GetBluetoothAdapterMac();
On 2016/02/10 18:59:45, scheib wrote:
> GetMacAdapter (this will now match a patch just landing
> https://codereview.chromium.org/1681853003/)

Done.

https://codereview.chromium.org/1538173003/diff/200001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_low_energy_device_mac.mm (right):

https://codereview.chromium.org/1538173003/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_low_energy_device_mac.mm:46: if
(this->IsGattConnected()) {
On 2016/02/10 18:59:46, scheib wrote:
> if (IsGattConnected())

Done.

https://codereview.chromium.org/1538173003/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_low_energy_device_mac.mm:49:
adapter->DisconnectGatt(this);
On 2016/02/10 18:59:46, scheib wrote:
> GetMacAdapter()->DisconnectGatt(this);

Done.

https://codereview.chromium.org/1538173003/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_low_energy_device_mac.mm:233:
adapter->CreateGattConnection(this);
On 2016/02/10 18:59:45, scheib wrote:
> GetMacAdapter()->CreateGattConnection(this);

Done.

https://codereview.chromium.org/1538173003/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_low_energy_device_mac.mm:240:
adapter->DisconnectGatt(this);
On 2016/02/10 18:59:46, scheib wrote:
> GetMacAdapter()->CreateGattConnection(this);

Done.

https://codereview.chromium.org/1538173003/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_low_energy_device_mac.mm:267: return adapter;
On 2016/02/10 18:59:46, scheib wrote:
> return static_cast<BluetoothAdapterMac*>(this->adapter_);

Done.

https://codereview.chromium.org/1538173003/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_low_energy_device_mac.mm:283: this->DidConnectGatt();
On 2016/02/10 18:59:46, scheib wrote:
> DidConnectGatt();

Done.

https://codereview.chromium.org/1538173003/diff/200001/device/bluetooth/test/...
File device/bluetooth/test/bluetooth_test_mac.h (right):

https://codereview.chromium.org/1538173003/diff/200001/device/bluetooth/test/...
device/bluetooth/test/bluetooth_test_mac.h:48: void* mock_central_manager_ =
NULL;
On 2016/02/10 18:59:46, scheib wrote:
> Could we use a scoped object from src/base/mac/ here (scoped_nsobject?), so
that
> release doesn't need to be called manually?

I can't include "base/mac/scoped_nsobject.h" because "bluetooth_test_mac.h" is
included by device/bluetooth/bluetooth_gatt_service_unittest.cc:14.
That's why I used "void *" as the type.

https://codereview.chromium.org/1538173003/diff/200001/device/bluetooth/test/...
File device/bluetooth/test/bluetooth_test_mac.mm (right):

https://codereview.chromium.org/1538173003/diff/200001/device/bluetooth/test/...
device/bluetooth/test/bluetooth_test_mac.mm:64:
[(MockCentralManager*)mock_central_manager_ release];
On 2016/02/10 18:59:46, scheib wrote:
> Use static_cast instead of C-Style casts.
> https://google.github.io/styleguide/cppguide.html#Casting Here and in many
other
> places.

Done.

https://codereview.chromium.org/1538173003/diff/200001/device/bluetooth/test/...
device/bluetooth/test/bluetooth_test_mac.mm:162: CBPeripheral* peripheral =
(CBPeripheral*)[[MockCBPeripheral alloc]
On 2016/02/10 18:59:46, scheib wrote:
> scoped object?

Done.

https://codereview.chromium.org/1538173003/diff/200001/device/bluetooth/test/...
device/bluetooth/test/bluetooth_test_mac.mm:224: // TODO(jlebel): Need to
convert the connect error code into NSError
On 2016/02/10 18:59:46, scheib wrote:
> ditto: http://crbug.com

Done.

[jlebel, 2016-02-11 09:15:11]

jlebel@chromium.org changed reviewers:
+ msarda@chromium.org

[fbeaufortchromium, 2016-02-11 10:26:01]

fbeaufort@chromium.org changed reviewers:
+ fbeaufort@chromium.org

[fbeaufortchromium, 2016-02-11 10:26:02]

Hey Jerome, here are my two cents.

https://codereview.chromium.org/1538173003/diff/220001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter_mac.mm (right):

https://codereview.chromium.org/1538173003/diff/220001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_mac.mm:96: // Need to make sure the devices
are destroyed before CBCentralManage is
s/CBCentralManage/CBCentralManager

https://codereview.chromium.org/1538173003/diff/220001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_mac.mm:466:
GetBluetoothLowEnergyDeviceMac(peripheral);
Was the comment line below removed by accident?

// Try to find device from |devices_| with key |device_address|,

Or should we simply remove the comment?

https://codereview.chromium.org/1538173003/diff/220001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_mac.mm:576: // TODO(http://crbug.com/585894):
Need to convert the error.
Can we VLOG the error there?

[msarda, 2016-02-11 10:55:39]

https://codereview.chromium.org/1538173003/diff/220001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter_mac.h (right):

https://codereview.chromium.org/1538173003/diff/220001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_mac.h:120: CBCentralManager*
GetCentralManagerForTesting();
Why is this method protected and not private? Do you override it in tests?

https://codereview.chromium.org/1538173003/diff/220001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter_mac.mm (right):

https://codereview.chromium.org/1538173003/diff/220001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_mac.mm:96: // Need to make sure the devices
are destroyed before CBCentralManage is
I would kill this comment and just do what the comment says:
devices_clear();
low_energy_central_manager_.release();

https://codereview.chromium.org/1538173003/diff/220001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_mac.mm:559: // Try to find device from
|devices_| with key |device_address|,
s/find device/find the device
s/,/.
s/if has no entry in the map, disconnect the peripheral./If there is no entry in
the map then disconnect the peripheral.

https://codereview.chromium.org/1538173003/diff/220001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_mac.mm:572: if (!device_mac) {
This code is duplicated in 3 functions. Consider refactoring it to a helper
method.

https://codereview.chromium.org/1538173003/diff/220001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_mac.mm:596: // Try to find device from
|devices_| with key |device_address|,
I do not understand the comment. Where do you disconnect the peripheral?

https://codereview.chromium.org/1538173003/diff/220001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter_mac_unittest.mm (right):

https://codereview.chromium.org/1538173003/diff/220001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_mac_unittest.mm:122: [mock_central_manager_
release];
In Chromium, you should not call release on an objective-c objects directly. Use
instead base::scope_nsobject

https://codereview.chromium.org/1538173003/diff/220001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_low_energy_device_mac.h (right):

https://codereview.chromium.org/1538173003/diff/220001/device/bluetooth/bluet...
device/bluetooth/bluetooth_low_energy_device_mac.h:118: // Called by the adapter
when CoreBluetooth calls CBCentralManager's delegate.
I think this comment is too vague. What do you mean by "calls CBCentralManager's
delegate"? What method is called on the CBCentralManager's delegate (any
methond)?

Also in general, do not explain the internals of the implementation, but what
this method is supposed to do.

https://codereview.chromium.org/1538173003/diff/220001/device/bluetooth/bluet...
device/bluetooth/bluetooth_low_energy_device_mac.h:119: void GattConnected();
I think the tendency in Chrome is to use function names like OnSomeEvent when
SomeEvent occurs. However if you prefer the objective-C style like Did (as you
use above), please be consistent and start this method with Did (maybe
DidConnectGatt, DidEstablishGattConnection etc).

https://codereview.chromium.org/1538173003/diff/220001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_low_energy_device_mac.mm (right):

https://codereview.chromium.org/1538173003/diff/220001/device/bluetooth/bluet...
device/bluetooth/bluetooth_low_energy_device_mac.mm:273: void
BluetoothLowEnergyDeviceMac::GattConnected() {
This is a private method and I do not see where it is actually called.

https://codereview.chromium.org/1538173003/diff/220001/device/bluetooth/test/...
File device/bluetooth/test/bluetooth_test_mac.h (right):

https://codereview.chromium.org/1538173003/diff/220001/device/bluetooth/test/...
device/bluetooth/test/bluetooth_test_mac.h:48: void* mock_central_manager_ =
NULL;
Should this be scoped_nsobject<MockCentralManager>?

https://codereview.chromium.org/1538173003/diff/220001/device/bluetooth/test/...
File device/bluetooth/test/bluetooth_test_mac.mm (right):

https://codereview.chromium.org/1538173003/diff/220001/device/bluetooth/test/...
device/bluetooth/test/bluetooth_test_mac.mm:85: mock_central_manager_ =
[[MockCentralManager alloc] init];
Use scoped_nsobject to avoid having to release this object manually.

https://codereview.chromium.org/1538173003/diff/220001/device/bluetooth/test/...
device/bluetooth/test/bluetooth_test_mac.mm:197: MockCBPeripheral*
mockPeripheral = (MockCBPeripheral*)peripheral;
Use ObjCCast or ObjCCastStrict from
https://code.google.com/p/chromium/codesearch#chromium/src/base/mac/foundatio...

https://codereview.chromium.org/1538173003/diff/220001/device/bluetooth/test/...
device/bluetooth/test/bluetooth_test_mac.mm:199: CBCentralManager*
centralManager =
Here and everywhere below:
static_cast on Objective-C is not the best solution. Prefer ObjCCast or
ObjCCastStrict.

https://codereview.chromium.org/1538173003/diff/220001/device/bluetooth/test/...
device/bluetooth/test/bluetooth_test_mac.mm:206: BluetoothLowEnergyDeviceMac*
lowEnergyDeviceMac =
Cast

https://codereview.chromium.org/1538173003/diff/220001/device/bluetooth/test/...
File device/bluetooth/test/mock_bluetooth_cbperipheral_mac.h (right):

https://codereview.chromium.org/1538173003/diff/220001/device/bluetooth/test/...
device/bluetooth/test/mock_bluetooth_cbperipheral_mac.h:14: // value of this
class can change.
I do not understand this comment. What is the "value of this class"? Is the
class changing? Or its fields?

https://codereview.chromium.org/1538173003/diff/220001/device/bluetooth/test/...
device/bluetooth/test/mock_bluetooth_cbperipheral_mac.h:15: @interface
MockCBPeripheral : NSObject
Be very careful about using objects as mocks. As it will not respond correctly
to calls like isKindOfClass and isMemberOfClass

https://codereview.chromium.org/1538173003/diff/220001/device/bluetooth/test/...
device/bluetooth/test/mock_bluetooth_cbperipheral_mac.h:17: @property(readonly)
CBPeripheralState state;
Why are the other properties non-atomic and this one is not? Do you actually
synchronize the access to this property?

https://codereview.chromium.org/1538173003/diff/220001/device/bluetooth/test/...
File device/bluetooth/test/mock_bluetooth_cbperipheral_mac.mm (right):

https://codereview.chromium.org/1538173003/diff/220001/device/bluetooth/test/...
device/bluetooth/test/mock_bluetooth_cbperipheral_mac.mm:24: [_name release];
Never call release. Use scoped_nsobject and implement the accessor methods. Or
use ObjCPropertyReleaser
https://code.google.com/p/chromium/codesearch#chromium/src/base/mac/objc_prop...

https://codereview.chromium.org/1538173003/diff/220001/device/bluetooth/test/...
device/bluetooth/test/mock_bluetooth_cbperipheral_mac.mm:29: -
(void)setStateForTesting:(CBPeripheralState)state {
Optional: This is a mock object. It is not really necessary to have a
"ForTesting" in the name of this function as this object is by definition used
only for testing.

[jlebel, 2016-02-19 11:02:36]

Hello Mihai,

All comments have been addressed.

https://codereview.chromium.org/1538173003/diff/220001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter_mac.h (right):

https://codereview.chromium.org/1538173003/diff/220001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_mac.h:120: CBCentralManager*
GetCentralManagerForTesting();
On 2016/02/11 10:55:38, msarda wrote:
> Why is this method protected and not private? Do you override it in tests?

Done.

https://codereview.chromium.org/1538173003/diff/220001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter_mac.mm (right):

https://codereview.chromium.org/1538173003/diff/220001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_mac.mm:96: // Need to make sure the devices
are destroyed before CBCentralManage is
On 2016/02/11 10:55:38, msarda wrote:
> I would kill this comment and just do what the comment says:
> devices_clear();
> low_energy_central_manager_.release();

This is really important to do it explicitly. We could think this line is
useless since it will be done anyway once the destroy method is done. But there
is an issue for the disconnect.
Doing the 2 lines that you suggested looks like it is useless too.  But it is
important.
I don't mind adding low_energy_central_manager_.release();

https://codereview.chromium.org/1538173003/diff/220001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_mac.mm:466:
GetBluetoothLowEnergyDeviceMac(peripheral);
On 2016/02/11 10:26:02, fbeaufortchromium wrote:
> Was the comment line below removed by accident?
> 
> // Try to find device from |devices_| with key |device_address|,
> 
> Or should we simply remove the comment?

Done.

https://codereview.chromium.org/1538173003/diff/220001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_mac.mm:559: // Try to find device from
|devices_| with key |device_address|,
On 2016/02/11 10:55:38, msarda wrote:
> s/find device/find the device
> s/,/.
> s/if has no entry in the map, disconnect the peripheral./If there is no entry
in
> the map then disconnect the peripheral.

Done.

https://codereview.chromium.org/1538173003/diff/220001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_mac.mm:572: if (!device_mac) {
On 2016/02/11 10:55:38, msarda wrote:
> This code is duplicated in 3 functions. Consider refactoring it to a helper
> method.

I'm not sure we will gain a lot of lines by adding an helper. I will have
problems to find the right name, and the |if| will still be needed.

https://codereview.chromium.org/1538173003/diff/220001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_mac.mm:576: // TODO(http://crbug.com/585894):
Need to convert the error.
On 2016/02/11 10:26:02, fbeaufortchromium wrote:
> Can we VLOG the error there?

Done.

https://codereview.chromium.org/1538173003/diff/220001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_mac.mm:596: // Try to find device from
|devices_| with key |device_address|,
On 2016/02/11 10:55:38, msarda wrote:
> I do not understand the comment. Where do you disconnect the peripheral?

Done.

https://codereview.chromium.org/1538173003/diff/220001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter_mac_unittest.mm (right):

https://codereview.chromium.org/1538173003/diff/220001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_mac_unittest.mm:122: [mock_central_manager_
release];
On 2016/02/11 10:55:38, msarda wrote:
> In Chromium, you should not call release on an objective-c objects directly.
Use
> instead base::scope_nsobject

Done.

https://codereview.chromium.org/1538173003/diff/220001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_low_energy_device_mac.h (right):

https://codereview.chromium.org/1538173003/diff/220001/device/bluetooth/bluet...
device/bluetooth/bluetooth_low_energy_device_mac.h:118: // Called by the adapter
when CoreBluetooth calls CBCentralManager's delegate.
On 2016/02/11 10:55:38, msarda wrote:
> I think this comment is too vague. What do you mean by "calls
CBCentralManager's
> delegate"? What method is called on the CBCentralManager's delegate (any
> methond)?
> 
> Also in general, do not explain the internals of the implementation, but what
> this method is supposed to do.

Done.

https://codereview.chromium.org/1538173003/diff/220001/device/bluetooth/bluet...
device/bluetooth/bluetooth_low_energy_device_mac.h:119: void GattConnected();
On 2016/02/11 10:55:39, msarda wrote:
> I think the tendency in Chrome is to use function names like OnSomeEvent when
> SomeEvent occurs. However if you prefer the objective-C style like Did (as you
> use above), please be consistent and start this method with Did (maybe
> DidConnectGatt, DidEstablishGattConnection etc).

Done.

https://codereview.chromium.org/1538173003/diff/220001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_low_energy_device_mac.mm (right):

https://codereview.chromium.org/1538173003/diff/220001/device/bluetooth/bluet...
device/bluetooth/bluetooth_low_energy_device_mac.mm:273: void
BluetoothLowEnergyDeviceMac::GattConnected() {
On 2016/02/11 10:55:39, msarda wrote:
> This is a private method and I do not see where it is actually called.

Done.

https://codereview.chromium.org/1538173003/diff/220001/device/bluetooth/test/...
File device/bluetooth/test/bluetooth_test_mac.h (right):

https://codereview.chromium.org/1538173003/diff/220001/device/bluetooth/test/...
device/bluetooth/test/bluetooth_test_mac.h:48: void* mock_central_manager_ =
NULL;
On 2016/02/11 10:55:39, msarda wrote:
> Should this be scoped_nsobject<MockCentralManager>?

This file is included in c++ file.

https://codereview.chromium.org/1538173003/diff/220001/device/bluetooth/test/...
File device/bluetooth/test/bluetooth_test_mac.mm (right):

https://codereview.chromium.org/1538173003/diff/220001/device/bluetooth/test/...
device/bluetooth/test/bluetooth_test_mac.mm:85: mock_central_manager_ =
[[MockCentralManager alloc] init];
On 2016/02/11 10:55:39, msarda wrote:
> Use scoped_nsobject to avoid having to release this object manually.

I can't use objective-c type in this header file.

https://codereview.chromium.org/1538173003/diff/220001/device/bluetooth/test/...
device/bluetooth/test/bluetooth_test_mac.mm:199: CBCentralManager*
centralManager =
On 2016/02/11 10:55:39, msarda wrote:
> Here and everywhere below:
> static_cast on Objective-C is not the best solution. Prefer ObjCCast or
> ObjCCastStrict.

Done.

https://codereview.chromium.org/1538173003/diff/220001/device/bluetooth/test/...
device/bluetooth/test/bluetooth_test_mac.mm:206: BluetoothLowEnergyDeviceMac*
lowEnergyDeviceMac =
On 2016/02/11 10:55:39, msarda wrote:
> Cast

Done.

https://codereview.chromium.org/1538173003/diff/220001/device/bluetooth/test/...
File device/bluetooth/test/mock_bluetooth_cbperipheral_mac.h (right):

https://codereview.chromium.org/1538173003/diff/220001/device/bluetooth/test/...
device/bluetooth/test/mock_bluetooth_cbperipheral_mac.h:14: // value of this
class can change.
On 2016/02/11 10:55:39, msarda wrote:
> I do not understand this comment. What is the "value of this class"? Is the
> class changing? Or its fields?

This is a mock. To mock CBPeripheral class, for unit tests.
I don't know how to change the value of state using OCMock.

https://codereview.chromium.org/1538173003/diff/220001/device/bluetooth/test/...
device/bluetooth/test/mock_bluetooth_cbperipheral_mac.h:15: @interface
MockCBPeripheral : NSObject
On 2016/02/11 10:55:39, msarda wrote:
> Be very careful about using objects as mocks. As it will not respond correctly
> to calls like isKindOfClass and isMemberOfClass

Acknowledged.

https://codereview.chromium.org/1538173003/diff/220001/device/bluetooth/test/...
device/bluetooth/test/mock_bluetooth_cbperipheral_mac.h:17: @property(readonly)
CBPeripheralState state;
On 2016/02/11 10:55:39, msarda wrote:
> Why are the other properties non-atomic and this one is not? Do you actually
> synchronize the access to this property?

Done.

https://codereview.chromium.org/1538173003/diff/220001/device/bluetooth/test/...
File device/bluetooth/test/mock_bluetooth_cbperipheral_mac.mm (right):

https://codereview.chromium.org/1538173003/diff/220001/device/bluetooth/test/...
device/bluetooth/test/mock_bluetooth_cbperipheral_mac.mm:24: [_name release];
On 2016/02/11 10:55:39, msarda wrote:
> Never call release. Use scoped_nsobject and implement the accessor methods. Or
> use ObjCPropertyReleaser
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/mac/objc_prop...

Done.

https://codereview.chromium.org/1538173003/diff/220001/device/bluetooth/test/...
device/bluetooth/test/mock_bluetooth_cbperipheral_mac.mm:29: -
(void)setStateForTesting:(CBPeripheralState)state {
On 2016/02/11 10:55:39, msarda wrote:
> Optional: This is a mock object. It is not really necessary to have a
> "ForTesting" in the name of this function as this object is by definition used
> only for testing.

Done.

[msarda, 2016-02-22 14:01:06]

https://codereview.chromium.org/1538173003/diff/220001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter_mac.mm (right):

https://codereview.chromium.org/1538173003/diff/220001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_mac.mm:466:
GetBluetoothLowEnergyDeviceMac(peripheral);
On 2016/02/19 11:02:35, jlebel wrote:
> On 2016/02/11 10:26:02, fbeaufortchromium wrote:
> > Was the comment line below removed by accident?
> > 
> > // Try to find device from |devices_| with key |device_address|,
> > 
> > Or should we simply remove the comment?
> 
> Done.

This was not Done. Please put back the line.

https://codereview.chromium.org/1538173003/diff/220001/device/bluetooth/test/...
File device/bluetooth/test/bluetooth_test_mac.h (right):

https://codereview.chromium.org/1538173003/diff/220001/device/bluetooth/test/...
device/bluetooth/test/bluetooth_test_mac.h:48: void* mock_central_manager_ =
NULL;
On 2016/02/19 11:02:35, jlebel wrote:
> On 2016/02/11 10:55:39, msarda wrote:
> > Should this be scoped_nsobject<MockCentralManager>?
> 
> This file is included in c++ file.

Ok, I see. The thing other folks do in this case is something like what is done
in
https://code.google.com/p/chromium/codesearch#chromium/src/base/strings/sys_s...

So in your case, this would be:
#if defined(OS_MACOSX)
#include <CoreBluetooth/CoreBluetooth.h>
#ifdef __OBJC__
@class CBCentralManager;
#else
class CBCentralManager;
#endif
#endif  // OS_MACOSX
CBCentralManager* mock_central_manager_;

https://codereview.chromium.org/1538173003/diff/220001/device/bluetooth/test/...
File device/bluetooth/test/bluetooth_test_mac.mm (right):

https://codereview.chromium.org/1538173003/diff/220001/device/bluetooth/test/...
device/bluetooth/test/bluetooth_test_mac.mm:199: CBCentralManager*
centralManager =
On 2016/02/19 11:02:35, jlebel wrote:
> On 2016/02/11 10:55:39, msarda wrote:
> > Here and everywhere below:
> > static_cast on Objective-C is not the best solution. Prefer ObjCCast or
> > ObjCCastStrict.
> 
> Done.

This was not Done.

https://codereview.chromium.org/1538173003/diff/220001/device/bluetooth/test/...
File device/bluetooth/test/mock_bluetooth_cbperipheral_mac.h (right):

https://codereview.chromium.org/1538173003/diff/220001/device/bluetooth/test/...
device/bluetooth/test/mock_bluetooth_cbperipheral_mac.h:14: // value of this
class can change.
On 2016/02/19 11:02:36, jlebel wrote:
> On 2016/02/11 10:55:39, msarda wrote:
> > I do not understand this comment. What is the "value of this class"? Is the
> > class changing? Or its fields?
> 
> This is a mock. To mock CBPeripheral class, for unit tests.
> I don't know how to change the value of state using OCMock.

I think with OCMock, you could do something like:
  OCMStub([_peripheralMock state])
      .andDo(^(NSInvocation *invocation) {
        [invocation setReturnValue:&_peripheralState];
      });
I think using OCMock is better in a long run, but I will leave you and the owner
of this code to decide.

https://codereview.chromium.org/1538173003/diff/220001/device/bluetooth/test/...
device/bluetooth/test/mock_bluetooth_cbperipheral_mac.h:15: @interface
MockCBPeripheral : NSObject
On 2016/02/19 11:02:36, jlebel wrote:
> On 2016/02/11 10:55:39, msarda wrote:
> > Be very careful about using objects as mocks. As it will not respond
correctly
> > to calls like isKindOfClass and isMemberOfClass
> 
> Acknowledged.

Why does this object subclass NSObject and not CBPeripheral?

https://codereview.chromium.org/1538173003/diff/240001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter_mac.mm (right):

https://codereview.chromium.org/1538173003/diff/240001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_mac.mm:96: // Need to make sure the devices
are destroyed before CBCentralManage is
Side note: Note that simply clearing the devices vector does not necessarily
mean that the devices are destroyed. The code below only ensures that the list
of devices is cleared.

The first par "Need to make sure" does not bring anything to the readability of
this comment. 

Also, the the class |CBCentralManager| is not destroyed, but the object
|low_energy_central_manager_| is.

Please rephrase to something like:
"Clear |devices_| before  |low_energy_central_manager_| is released."

https://codereview.chromium.org/1538173003/diff/240001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter_mac_unittest.mm (right):

https://codereview.chromium.org/1538173003/diff/240001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_mac_unittest.mm:122:
static_cast<CBCentralManager*>(mock_central_manager_.get());
Do not use static_cast for Objective-C objects. Use base::ObjcCast

https://codereview.chromium.org/1538173003/diff/240001/device/bluetooth/test/...
File device/bluetooth/test/bluetooth_test_mac.mm (right):

https://codereview.chromium.org/1538173003/diff/240001/device/bluetooth/test/...
device/bluetooth/test/bluetooth_test_mac.mm:86: MockCentralManager*
mock_central_manager =
Do not use static cast for Objective-C objects. Use base::ObjcCast.

https://codereview.chromium.org/1538173003/diff/240001/device/bluetooth/test/...
device/bluetooth/test/bluetooth_test_mac.mm:197: MockCBPeripheral*
mockPeripheral = static_cast<MockCBPeripheral*>(peripheral);
https://code.google.com/p/chromium/codesearch#chromium/src/base/mac/foundatio...

https://codereview.chromium.org/1538173003/diff/240001/device/bluetooth/test/...
device/bluetooth/test/bluetooth_test_mac.mm:209: MockCBPeripheral*
mockPeripheral = static_cast<MockCBPeripheral*>(peripheral);
Do not use static_cast for Objective-C objects. Use 
https://code.google.com/p/chromium/codesearch#chromium/src/base/mac/foundatio...

https://codereview.chromium.org/1538173003/diff/240001/device/bluetooth/test/...
File device/bluetooth/test/mock_bluetooth_cbperipheral_mac.h (right):

https://codereview.chromium.org/1538173003/diff/240001/device/bluetooth/test/...
device/bluetooth/test/mock_bluetooth_cbperipheral_mac.h:14: // value of this
class can change.
I still do not understand what "value of this class" refers to. Please be more
specific.  Maybe something like:
// This class mocks the behavior of a CBPeripheral.
// Note: Using real mock objects was preferred instead of the
// standard mocking using OCMock because ...

https://codereview.chromium.org/1538173003/diff/240001/device/bluetooth/test/...
File device/bluetooth/test/mock_bluetooth_central_manager_mac.h (right):

https://codereview.chromium.org/1538173003/diff/240001/device/bluetooth/test/...
device/bluetooth/test/mock_bluetooth_central_manager_mac.h:14: // Class to mock
a CBCentralManager. Cannot use a OCMockObject because mocking
It looks like this file existed before your CL, so you may choose to ignore my
comments in this file. I make them as I think in the long run this would make
tests easier to maintain.

Could you try doing a cast to to CBCentralManager * in your tests and using
OCMock instead of using these test objects?

https://codereview.chromium.org/1538173003/diff/240001/device/bluetooth/test/...
device/bluetooth/test/mock_bluetooth_central_manager_mac.h:19: @interface
MockCentralManager : NSObject
I would expect this MockCentralManager to extend CBCentralManager.

[jlebel, 2016-02-24 16:54:55]

I updated patch.

https://codereview.chromium.org/1538173003/diff/240001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter_mac.mm (right):

https://codereview.chromium.org/1538173003/diff/240001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_mac.mm:96: // Need to make sure the devices
are destroyed before CBCentralManage is
On 2016/02/22 14:01:06, msarda wrote:
> Side note: Note that simply clearing the devices vector does not necessarily
> mean that the devices are destroyed. The code below only ensures that the list
> of devices is cleared.
> 
> The first par "Need to make sure" does not bring anything to the readability
of
> this comment. 
> 
> Also, the the class |CBCentralManager| is not destroyed, but the object
> |low_energy_central_manager_| is.
> 
> Please rephrase to something like:
> "Clear |devices_| before  |low_energy_central_manager_| is released."

Done.

https://codereview.chromium.org/1538173003/diff/240001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter_mac_unittest.mm (right):

https://codereview.chromium.org/1538173003/diff/240001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_mac_unittest.mm:122:
static_cast<CBCentralManager*>(mock_central_manager_.get());
On 2016/02/22 14:01:06, msarda wrote:
> Do not use static_cast for Objective-C objects. Use base::ObjcCast

It is not possible, this is a mock.

https://codereview.chromium.org/1538173003/diff/240001/device/bluetooth/test/...
File device/bluetooth/test/bluetooth_test_mac.mm (right):

https://codereview.chromium.org/1538173003/diff/240001/device/bluetooth/test/...
device/bluetooth/test/bluetooth_test_mac.mm:86: MockCentralManager*
mock_central_manager =
On 2016/02/22 14:01:06, msarda wrote:
> Do not use static cast for Objective-C objects. Use base::ObjcCast.

Done.

https://codereview.chromium.org/1538173003/diff/240001/device/bluetooth/test/...
device/bluetooth/test/bluetooth_test_mac.mm:197: MockCBPeripheral*
mockPeripheral = static_cast<MockCBPeripheral*>(peripheral);
On 2016/02/22 14:01:06, msarda wrote:
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/mac/foundatio...

Done.

https://codereview.chromium.org/1538173003/diff/240001/device/bluetooth/test/...
device/bluetooth/test/bluetooth_test_mac.mm:209: MockCBPeripheral*
mockPeripheral = static_cast<MockCBPeripheral*>(peripheral);
On 2016/02/22 14:01:06, msarda wrote:
> Do not use static_cast for Objective-C objects. Use 
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/mac/foundatio...

Done.

https://codereview.chromium.org/1538173003/diff/240001/device/bluetooth/test/...
File device/bluetooth/test/mock_bluetooth_cbperipheral_mac.h (right):

https://codereview.chromium.org/1538173003/diff/240001/device/bluetooth/test/...
device/bluetooth/test/mock_bluetooth_cbperipheral_mac.h:14: // value of this
class can change.
On 2016/02/22 14:01:06, msarda wrote:
> I still do not understand what "value of this class" refers to. Please be more
> specific.  Maybe something like:
> // This class mocks the behavior of a CBPeripheral.
> // Note: Using real mock objects was preferred instead of the
> // standard mocking using OCMock because ...
> 

Done.

https://codereview.chromium.org/1538173003/diff/240001/device/bluetooth/test/...
File device/bluetooth/test/mock_bluetooth_central_manager_mac.h (right):

https://codereview.chromium.org/1538173003/diff/240001/device/bluetooth/test/...
device/bluetooth/test/mock_bluetooth_central_manager_mac.h:14: // Class to mock
a CBCentralManager. Cannot use a OCMockObject because mocking
On 2016/02/22 14:01:06, msarda wrote:
> It looks like this file existed before your CL, so you may choose to ignore my
> comments in this file. I make them as I think in the long run this would make
> tests easier to maintain.
> 
> Could you try doing a cast to to CBCentralManager * in your tests and using
> OCMock instead of using these test objects?

Acknowledged.

https://codereview.chromium.org/1538173003/diff/240001/device/bluetooth/test/...
device/bluetooth/test/mock_bluetooth_central_manager_mac.h:19: @interface
MockCentralManager : NSObject
On 2016/02/22 14:01:06, msarda wrote:
> I would expect this MockCentralManager to extend CBCentralManager.

Acknowledged.

[scheib, 2016-03-01 17:38:17]

https://codereview.chromium.org/1538173003/diff/200001/device/bluetooth/test/...
File device/bluetooth/test/bluetooth_test_mac.h (right):

https://codereview.chromium.org/1538173003/diff/200001/device/bluetooth/test/...
device/bluetooth/test/bluetooth_test_mac.h:48: void* mock_central_manager_ =
NULL;
On 2016/02/10 21:20:08, jlebel wrote:
> On 2016/02/10 18:59:46, scheib wrote:
> > Could we use a scoped object from src/base/mac/ here (scoped_nsobject?), so
> that
> > release doesn't need to be called manually?
> 
> I can't include "base/mac/scoped_nsobject.h" because "bluetooth_test_mac.h" is
> included by device/bluetooth/bluetooth_gatt_service_unittest.cc:14.
> That's why I used "void *" as the type.
> 
> 

Use #if defined(OS_MACOSX) to guard the include as necessary.

https://codereview.chromium.org/1538173003/diff/280001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter_mac.mm (right):

https://codereview.chromium.org/1538173003/diff/280001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_mac.mm:107: // Set
low_energy_central_manager_ to nil so no devices will try to use
to use it
       ^^
is owned by
      ^^

[jlebel, 2016-03-01 21:44:45]

https://codereview.chromium.org/1538173003/diff/200001/device/bluetooth/test/...
File device/bluetooth/test/bluetooth_test_mac.h (right):

https://codereview.chromium.org/1538173003/diff/200001/device/bluetooth/test/...
device/bluetooth/test/bluetooth_test_mac.h:48: void* mock_central_manager_ =
NULL;
On 2016/03/01 17:38:17, scheib wrote:
> On 2016/02/10 21:20:08, jlebel wrote:
> > On 2016/02/10 18:59:46, scheib wrote:
> > > Could we use a scoped object from src/base/mac/ here (scoped_nsobject?),
so
> > that
> > > release doesn't need to be called manually?
> > 
> > I can't include "base/mac/scoped_nsobject.h" because "bluetooth_test_mac.h"
is
> > included by device/bluetooth/bluetooth_gatt_service_unittest.cc:14.
> > That's why I used "void *" as the type.
> > 
> > 
> 
> Use #if defined(OS_MACOSX) to guard the include as necessary. 

The issue is not OS X vs other platform. The issue is that this file is included
inside c++ files (like bluetooth_gatt_service_unittest.cc), even on OS X.

https://codereview.chromium.org/1538173003/diff/280001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter_mac.mm (right):

https://codereview.chromium.org/1538173003/diff/280001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_mac.mm:107: // Set
low_energy_central_manager_ to nil so no devices will try to use
On 2016/03/01 17:38:17, scheib wrote:
> to use it
>        ^^
> is owned by
>       ^^

Done.

[msarda, 2016-03-02 10:31:48]

LGTM with some nits.

https://codereview.chromium.org/1538173003/diff/300001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter_mac.mm (right):

https://codereview.chromium.org/1538173003/diff/300001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_mac.mm:106: }
Based on our discussion from last week, the devices_ vector should be cleared
here.  That would avoid them trying to use this BluetoothAdapterMac after it was
destroyed.

https://codereview.chromium.org/1538173003/diff/300001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter_mac_unittest.mm (right):

https://codereview.chromium.org/1538173003/diff/300001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_mac_unittest.mm:122:
(CBCentralManager*)mock_central_manager_.get();
If you cannot use ObjCast here, prefer using static cast. This should be what
you had before:
static_cast<CBCentralManager*>(mock_central_manager_.get())

https://codereview.chromium.org/1538173003/diff/300001/device/bluetooth/test/...
File device/bluetooth/test/bluetooth_test_mac.mm (right):

https://codereview.chromium.org/1538173003/diff/300001/device/bluetooth/test/...
device/bluetooth/test/bluetooth_test_mac.mm:196: MockCBPeripheral*
mockPeripheral = (MockCBPeripheral*)peripheral;
Here and everywhere in this file.

Sorry for this cast mess, I think my comments were not clear. If you cannot use
ObjcCast, prefer using static_cast.

[jlebel, 2016-03-02 15:34:17]

https://codereview.chromium.org/1538173003/diff/300001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter_mac.mm (right):

https://codereview.chromium.org/1538173003/diff/300001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_mac.mm:106: }
On 2016/03/02 10:31:48, msarda wrote:
> Based on our discussion from last week, the devices_ vector should be cleared
> here.  That would avoid them trying to use this BluetoothAdapterMac after it
was
> destroyed.

Done.

https://codereview.chromium.org/1538173003/diff/300001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter_mac_unittest.mm (right):

https://codereview.chromium.org/1538173003/diff/300001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_mac_unittest.mm:122:
(CBCentralManager*)mock_central_manager_.get();
On 2016/03/02 10:31:48, msarda wrote:
> If you cannot use ObjCast here, prefer using static cast. This should be what
> you had before:
> static_cast<CBCentralManager*>(mock_central_manager_.get())

To avoid any future problem, I prefer to subclass -[MockCentralManager
isKindOfClass:] and -[MockCentralManager isMemberOfClass:]
Done.

https://codereview.chromium.org/1538173003/diff/300001/device/bluetooth/test/...
File device/bluetooth/test/bluetooth_test_mac.mm (right):

https://codereview.chromium.org/1538173003/diff/300001/device/bluetooth/test/...
device/bluetooth/test/bluetooth_test_mac.mm:196: MockCBPeripheral*
mockPeripheral = (MockCBPeripheral*)peripheral;
On 2016/03/02 10:31:48, msarda wrote:
> Here and everywhere in this file.
> 
> Sorry for this cast mess, I think my comments were not clear. If you cannot
use
> ObjcCast, prefer using static_cast.

Done.

[scheib, 2016-03-02 16:28:42]

LGTM with optional suggestion:

https://codereview.chromium.org/1538173003/diff/320001/device/bluetooth/test/...
File device/bluetooth/test/bluetooth_test_mac.mm (right):

https://codereview.chromium.org/1538173003/diff/320001/device/bluetooth/test/...
device/bluetooth/test/bluetooth_test_mac.mm:33: MockCentralManager*
central_manager() { return mock_central_manager_.get(); };
'get' would also be an OK name and shorter.

[jlebel, 2016-03-03 23:01:25]

The CQ bit was checked by jlebel@chromium.org

[jlebel, 2016-03-03 23:01:26]

The patchset sent to the CQ was uploaded after l-g-t-m from msarda@chromium.org,
scheib@chromium.org
Link to the patchset: https://codereview.chromium.org/1538173003/#ps360001
(title: "Changing central_manager() to get()")

[commit-bot: I haz the power, 2016-03-03 23:02:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1538173003/360001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1538173003/360001

[commit-bot: I haz the power, 2016-03-03 23:08:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-03 23:08:33]

Try jobs failed on following builders:
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[jlebel, 2016-03-08 21:20:22]

The CQ bit was checked by jlebel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-08 21:20:42]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1538173003/370001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1538173003/370001

[commit-bot: I haz the power, 2016-03-08 21:25:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-08 21:25:09]

Dry run: Try jobs failed on following builders:
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[jlebel, 2016-03-09 21:29:02]

The CQ bit was checked by jlebel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-09 21:29:38]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1538173003/390001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1538173003/390001

[commit-bot: I haz the power, 2016-03-09 21:36:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-09 21:36:28]

Dry run: Try jobs failed on following builders:
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[jlebel, 2016-03-11 21:49:46]

The CQ bit was checked by jlebel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-11 21:50:24]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1538173003/510001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1538173003/510001

[jlebel, 2016-03-11 21:50:27]

The CQ bit was unchecked by jlebel@chromium.org

[jlebel, 2016-03-11 21:50:41]

The CQ bit was checked by jlebel@chromium.org to run a CQ dry run

[jlebel, 2016-03-11 21:50:51]

The CQ bit was unchecked by jlebel@chromium.org

[jlebel, 2016-03-14 09:25:03]

The CQ bit was checked by jlebel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-14 09:25:11]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1538173003/510001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1538173003/510001

[commit-bot: I haz the power, 2016-03-14 10:27:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-14 10:27:09]

Dry run: This issue passed the CQ dry run.

[jlebel, 2016-03-14 10:28:44]

The CQ bit was checked by jlebel@chromium.org

[jlebel, 2016-03-14 10:28:45]

The patchset sent to the CQ was uploaded after l-g-t-m from scheib@chromium.org,
msarda@chromium.org
Link to the patchset: https://codereview.chromium.org/1538173003/#ps510001
(title: "Skip tests if Bluetooth Low Energy is not available ")

[commit-bot: I haz the power, 2016-03-14 10:28:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1538173003/510001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1538173003/510001

[commit-bot: I haz the power, 2016-03-14 10:33:36]

Description was changed from

==========
Implementing GATT connection/disconnect on OS X.

BUG=520774
==========

to

==========
Implementing GATT connection/disconnect on OS X.

BUG=520774
==========

[commit-bot: I haz the power, 2016-03-14 10:33:38]

Committed patchset #27 (id:510001)

[commit-bot: I haz the power, 2016-03-14 10:35:35]

Description was changed from

==========
Implementing GATT connection/disconnect on OS X.

BUG=520774
==========

to

==========
Implementing GATT connection/disconnect on OS X.

BUG=520774

Committed: https://crrev.com/a58fd7578542a34fe30bf56ae52b884ad71c9061
Cr-Commit-Position: refs/heads/master@{#380951}
==========

[commit-bot: I haz the power, 2016-03-14 10:35:36]

Patchset 27 (id:??) landed as
https://crrev.com/a58fd7578542a34fe30bf56ae52b884ad71c9061
Cr-Commit-Position: refs/heads/master@{#380951}