Adding the last test related with connect/disconnect

Adding the last test related with connect/disconnect.

This CL adds better support with error code and NSError conversion.

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

[scheib, 2016-02-10 23:48:16]

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

[scheib, 2016-02-10 23:48:16]

https://codereview.chromium.org/1685963003/diff/60001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_adapter_mac.mm (right):

https://codereview.chromium.org/1685963003/diff/60001/device/bluetooth/blueto...
device/bluetooth/bluetooth_adapter_mac.mm:617: switch
((BluetoothDevice::ConnectErrorCode)error.code) {
I don't think this static cast will work. The BluetoothDevice error enums aren't
given GATT values. At quick look: NSError doesn't seem to define in any way what
the error values will be, in which case we should histogram them and learn over
time what is experienced, or guess they are GATT error codes and use hard coded
values. See
https://code.google.com/p/chromium/codesearch#chromium/src/device/bluetooth/b...

[jlebel, 2016-02-11 00:03:43]

https://codereview.chromium.org/1685963003/diff/60001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_adapter_mac.mm (right):

https://codereview.chromium.org/1685963003/diff/60001/device/bluetooth/blueto...
device/bluetooth/bluetooth_adapter_mac.mm:617: switch
((BluetoothDevice::ConnectErrorCode)error.code) {
On 2016/02/10 23:48:16, scheib wrote:
> I don't think this static cast will work. The BluetoothDevice error enums
aren't
> given GATT values. At quick look: NSError doesn't seem to define in any way
what
> the error values will be, in which case we should histogram them and learn
over
> time what is experienced, or guess they are GATT error codes and use hard
coded
> values. See
>
https://code.google.com/p/chromium/codesearch#chromium/src/device/bluetooth/b...

I'm not sure to understand. This switch is executed only if the domain is ours :
kErrorDomain. So if it is ours, the NSError that we have, can only be created in
|BluetoothLowEnergyDeviceMac*
BluetoothAdapterMac::GetBluetoothLowEnergyDeviceMac(CBPeripheral* peripheral)|.
Therefore the code is definitely a |BluetoothDevice::ConnectErrorCode|.

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

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

[msarda, 2016-02-11 13:48:08]

I think your CL description does not match the style used for CL description in
Chromium. I would expect this to be something like:

"Adding the last test related with connect/disconnect.

This CL adds better support with error code and NSError conversion.

BUG=<Add a bug number>"

I also do not see anything related to the test for connect / disconnect, so I do
not really understand what this CL wants to achieve.

Apart from that, this CL is fine from Objective-C perspective.

[jlebel, 2016-02-19 10:17:17]

Description was changed from

==========
Adding the last test related with connect/disconnect
Adding better support with error code and NSError conversion.

BUG=
==========

to

==========
Adding the last test related with connect/disconnect
This CL adds better support with error code and NSError conversion.

BUG=
==========

[msarda, 2016-02-22 14:02:09]

On 2016/02/19 11:03:06, jlebel wrote:

I think your CL description does not match the style used for CL description in
Chromium. I would expect this to be something like:

"Adding the last test related with connect/disconnect.

This CL adds better support with error code and NSError conversion.

BUG=<Add a bug number>"

I also do not see anything related to the test for connect / disconnect, so I do
not really understand what this CL wants to achieve.

Apart from that, this CL is fine from Objective-C perspective.

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

LGTM with nits for Objective-C code. Please fix the CL description as explained
in my previous comment.

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

https://codereview.chromium.org/1685963003/diff/100001/device/bluetooth/test/...
device/bluetooth/test/bluetooth_test_mac.mm:211: MockCBPeripheral*
mockPeripheral = static_cast<MockCBPeripheral*>(peripheral);
Use ObjcCast instead of static_cast for Objective-C objects.

[scheib, 2016-03-02 05:33:23]

Please add a BUG number in the change description.

https://codereview.chromium.org/1685963003/diff/60001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_adapter_mac.mm (right):

https://codereview.chromium.org/1685963003/diff/60001/device/bluetooth/blueto...
device/bluetooth/bluetooth_adapter_mac.mm:617: switch
((BluetoothDevice::ConnectErrorCode)error.code) {
On 2016/02/11 00:03:43, jlebel wrote:
> On 2016/02/10 23:48:16, scheib wrote:
> > I don't think this static cast will work. The BluetoothDevice error enums
> aren't
> > given GATT values. At quick look: NSError doesn't seem to define in any way
> what
> > the error values will be, in which case we should histogram them and learn
> over
> > time what is experienced, or guess they are GATT error codes and use hard
> coded
> > values. See
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/device/bluetooth/b...
> 
> I'm not sure to understand. This switch is executed only if the domain is ours
:
> kErrorDomain. So if it is ours, the NSError that we have, can only be created
in
> |BluetoothLowEnergyDeviceMac*
> BluetoothAdapterMac::GetBluetoothLowEnergyDeviceMac(CBPeripheral*
peripheral)|.
> Therefore the code is definitely a |BluetoothDevice::ConnectErrorCode|.

OK, I didn't see or properly understand the NSError domain until you pointed
this out. Some thoughts:

DCHECK only for assertions that aren't checked by code. This is a bit odd, read
up on it here:
http://dev.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREACHED-

We don't need the switch statement. ConnectErrorCode has
NUM_CONNECT_ERROR_CODES, so you can do a if check that error.code is >= 0 and <
NUM_CONNECT_ERROR_CODES and then do a
static_cast<BluetoothDevice::ConnectErrorCode>(error.code).

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

https://codereview.chromium.org/1685963003/diff/120001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_mac.h:112: static NSError*
GetNSErrorFromConnectErrorCode(
Because these work with an enum that is defined in BluetoothDevice wouldn't the
static helper methods be better located on BluetoothDeviceMac?

[jlebel, 2016-03-02 15:31:40]

Description was changed from

==========
Adding the last test related with connect/disconnect
This CL adds better support with error code and NSError conversion.

BUG=
==========

to

==========
Adding the last test related with connect/disconnect.

This CL adds better support with error code and NSError conversion.

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

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

https://codereview.chromium.org/1685963003/diff/60001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_adapter_mac.mm (right):

https://codereview.chromium.org/1685963003/diff/60001/device/bluetooth/blueto...
device/bluetooth/bluetooth_adapter_mac.mm:617: switch
((BluetoothDevice::ConnectErrorCode)error.code) {
On 2016/03/02 05:33:22, scheib wrote:
> On 2016/02/11 00:03:43, jlebel wrote:
> > On 2016/02/10 23:48:16, scheib wrote:
> > > I don't think this static cast will work. The BluetoothDevice error enums
> > aren't
> > > given GATT values. At quick look: NSError doesn't seem to define in any
way
> > what
> > > the error values will be, in which case we should histogram them and learn
> > over
> > > time what is experienced, or guess they are GATT error codes and use hard
> > coded
> > > values. See
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/device/bluetooth/b...
> > 
> > I'm not sure to understand. This switch is executed only if the domain is
ours
> :
> > kErrorDomain. So if it is ours, the NSError that we have, can only be
created
> in
> > |BluetoothLowEnergyDeviceMac*
> > BluetoothAdapterMac::GetBluetoothLowEnergyDeviceMac(CBPeripheral*
> peripheral)|.
> > Therefore the code is definitely a |BluetoothDevice::ConnectErrorCode|.
> 
> OK, I didn't see or properly understand the NSError domain until you pointed
> this out. Some thoughts:
> 
> DCHECK only for assertions that aren't checked by code. This is a bit odd,
read
> up on it here:
>
http://dev.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREACHED-
> 
> We don't need the switch statement. ConnectErrorCode has
> NUM_CONNECT_ERROR_CODES, so you can do a if check that error.code is >= 0 and
<
> NUM_CONNECT_ERROR_CODES and then do a
> static_cast<BluetoothDevice::ConnectErrorCode>(error.code).

Yes, of course!

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

https://codereview.chromium.org/1685963003/diff/120001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_mac.h:112: static NSError*
GetNSErrorFromConnectErrorCode(
On 2016/03/02 05:33:22, scheib wrote:
> Because these work with an enum that is defined in BluetoothDevice wouldn't
the
> static helper methods be better located on BluetoothDeviceMac?

Done.

[scheib, 2016-03-02 16:23:21]

LGTM

https://codereview.chromium.org/1685963003/diff/140001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_device_mac.mm (right):

https://codereview.chromium.org/1685963003/diff/140001/device/bluetooth/bluet...
device/bluetooth/bluetooth_device_mac.mm:34: DCHECK(false);
Remove the dcheck.
https://www.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREAC...

[jlebel, 2016-03-15 18:00:44]

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

[commit-bot: I haz the power, 2016-03-15 18:01:09]

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

[jlebel, 2016-03-15 18:03:00]

The CQ bit was unchecked by jlebel@chromium.org

[jlebel, 2016-03-16 17:06:00]

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

[commit-bot: I haz the power, 2016-03-16 17:06:24]

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

[jlebel, 2016-03-16 17:08:08]

The CQ bit was unchecked by jlebel@chromium.org

[jlebel, 2016-03-16 22:55:26]

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

[commit-bot: I haz the power, 2016-03-16 22:55:48]

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

[commit-bot: I haz the power, 2016-03-16 23:24:01]

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

[commit-bot: I haz the power, 2016-03-16 23:24:04]

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

[jlebel, 2016-03-29 09:33:33]

The CQ bit was checked by jlebel@chromium.org

[jlebel, 2016-03-29 09:33:33]

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/1685963003/#ps240001
(title: "adding DEVICE_BLUETOOTH_EXPORT to BluetoothDeviceMac")

[commit-bot: I haz the power, 2016-03-29 09:33:43]

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

[commit-bot: I haz the power, 2016-03-29 10:30:38]

Description was changed from

==========
Adding the last test related with connect/disconnect.

This CL adds better support with error code and NSError conversion.

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

to

==========
Adding the last test related with connect/disconnect.

This CL adds better support with error code and NSError conversion.

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

[commit-bot: I haz the power, 2016-03-29 10:30:40]

Committed patchset #13 (id:240001)

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

Description was changed from

==========
Adding the last test related with connect/disconnect.

This CL adds better support with error code and NSError conversion.

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

to

==========
Adding the last test related with connect/disconnect.

This CL adds better support with error code and NSError conversion.

BUG=520774

Committed: https://crrev.com/15f9964b8ff56961f10d34bf39715a5cc457f869
Cr-Commit-Position: refs/heads/master@{#383698}
==========

[commit-bot: I haz the power, 2016-03-29 10:32:09]

Patchset 13 (id:??) landed as
https://crrev.com/15f9964b8ff56961f10d34bf39715a5cc457f869
Cr-Commit-Position: refs/heads/master@{#383698}