Read characteristic implementation on macOS

bluetooth: mac: read characteristic implementation

Adding implementing read characteristic in BluetoothRemoteGattCharacteristicMac, and adding unit tests, mock and simulation.

BUG=609066
Committed: https://crrev.com/e223e929756507526187534fdaf0d18e78550cb3
Cr-Commit-Position: refs/heads/master@{#402182}

[jlebel, 2016-06-10 23:13:20]

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

[commit-bot: I haz the power, 2016-06-10 23:13:52]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2052513002/40001

[commit-bot: I haz the power, 2016-06-11 01:07:32]

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

[commit-bot: I haz the power, 2016-06-11 01:07:33]

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

[jlebel, 2016-06-13 15:17:08]

Description was changed from

==========
Read characteristic on OS X.

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

to

==========
Read characteristic on OS X.

BUG=609066
==========

[jlebel, 2016-06-13 15:17:28]

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

[commit-bot: I haz the power, 2016-06-13 15:17:40]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2052513002/80001

[commit-bot: I haz the power, 2016-06-13 15:27:02]

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

[commit-bot: I haz the power, 2016-06-13 15:27:03]

Dry run: Try jobs failed on following builders:
  cast_shell_android on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[jlebel, 2016-06-13 15:54:30]

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

[commit-bot: I haz the power, 2016-06-13 15:54:44]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2052513002/100001

[commit-bot: I haz the power, 2016-06-13 16:40:00]

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

[commit-bot: I haz the power, 2016-06-13 16:40:01]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[jlebel, 2016-06-15 16:37:37]

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

[commit-bot: I haz the power, 2016-06-15 16:38:07]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2052513002/120001

[commit-bot: I haz the power, 2016-06-15 16:41:09]

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

[commit-bot: I haz the power, 2016-06-15 16:41:10]

Dry run: Try jobs failed on following builders:
  ios-simulator on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[jlebel, 2016-06-15 23:54:24]

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

[commit-bot: I haz the power, 2016-06-15 23:54:49]

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

[jlebel, 2016-06-16 12:29:07]

Description was changed from

==========
Read characteristic on OS X.

BUG=609066
==========

to

==========
bluetooth: mac: read characteristic implementation

Adding implementing read characteristic in BluetoothRemoteGattCharacteristicMac,
and adding unit tests, mock and simulation.

BUG=609066
==========

[jlebel, 2016-06-16 12:31:43]

jlebel@chromium.org changed reviewers:
+ fbeaufort@chromium.org, msarda@chromium.org, ortuno@chromium.org,
scheib@chromium.org

[jlebel, 2016-06-16 12:31:44]

Hello,

I've implemented read for characteristic in macOS. Can you review my code?

Thanks,

[jlebel, 2016-06-16 12:45:07]

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

[commit-bot: I haz the power, 2016-06-16 12:45:16]

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

[commit-bot: I haz the power, 2016-06-16 14:32:13]

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

[commit-bot: I haz the power, 2016-06-16 14:32:14]

Dry run: This issue passed the CQ dry run.

[ortuno, 2016-06-16 18:22:19]

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

https://codereview.chromium.org/2052513002/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_low_energy_peripheral_delegate.mm:80:
bridge_->DidReadValue(characteristic, error);
We can't assume this a is Read Value result because
didUpdateValueForCharacteristic is also called for notifications :(

So you have to have some sort of logic to decide if you should call a readValue
callback or if you should just call GattCharacteristicChanged. I think this
should be done the Characteristic level. So the bridge's, device's and service's
methods should be called DidUpdateValueForCharacteristic. The characteristic
method should just be called DidUpdateValue.

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

https://codereview.chromium.org/2052513002/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_mac.mm:139:
error_callback.Run(BluetoothRemoteGattService::GATT_ERROR_NOT_PERMITTED);
ReadRemoteCharacteristic should always be async. Post a task to call
error_callback.

https://codereview.chromium.org/2052513002/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_mac.mm:143:
error_callback.Run(BluetoothRemoteGattService::GATT_ERROR_IN_PROGRESS);
Same here, post a task.

https://codereview.chromium.org/2052513002/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_mac.mm:174: const uint8_t*
buffer = static_cast<const uint8_t*>(nsdata_value.bytes);
nit: Shouldn't you use [nsdata_value bytes] since NSData is an objective-c
class?

https://codereview.chromium.org/2052513002/diff/200001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc (right):

https://codereview.chromium.org/2052513002/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:313:
EXPECT_EQ(1, gatt_read_characteristic_attempts_);
We should also check that GattCharacteristicChanged was called. You will
probably need and #if for Android, not sure if you need one for Windows. Same
applies for all other tests.

https://codereview.chromium.org/2052513002/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:339: #if
defined(OS_ANDROID) || defined(OS_WIN)
As mentioned in email I think this test can be enabled.

https://codereview.chromium.org/2052513002/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:601: }
Also check that GattCharacteristicChanged hasn't been called.

https://codereview.chromium.org/2052513002/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:625: // Tests
ReadRemoteCharacteristic synchronous error.
Add a comment here for why this doesn't apply to mac e.g. "CoreBluetooth's
function to read a characteristic's value doesn't return any error
synchronously."

[jlebel, 2016-06-20 19:03:54]

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

[commit-bot: I haz the power, 2016-06-20 19:04:26]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2052513002/220001

[commit-bot: I haz the power, 2016-06-20 20:23:06]

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

[commit-bot: I haz the power, 2016-06-20 20:23:07]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

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

Patchset #14 (id:260001) has been deleted

[jlebel, 2016-06-22 17:12:08]

Patchset #15 (id:300001) has been deleted

[jlebel, 2016-06-22 17:12:18]

Patchset #14 (id:280001) has been deleted

[jlebel, 2016-06-22 17:12:27]

Patchset #13 (id:240001) has been deleted

[jlebel, 2016-06-22 17:12:39]

Patchset #12 (id:220001) has been deleted

[jlebel, 2016-06-22 17:57:04]

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

[commit-bot: I haz the power, 2016-06-22 17:57:41]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2052513002/380001

[jlebel, 2016-06-22 18:03:59]

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

https://codereview.chromium.org/2052513002/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_low_energy_peripheral_delegate.mm:80:
bridge_->DidReadValue(characteristic, error);
On 2016/06/16 18:22:18, ortuno wrote:
> We can't assume this a is Read Value result because
> didUpdateValueForCharacteristic is also called for notifications :(
> 
> So you have to have some sort of logic to decide if you should call a
readValue
> callback or if you should just call GattCharacteristicChanged. I think this
> should be done the Characteristic level. So the bridge's, device's and
service's
> methods should be called DidUpdateValueForCharacteristic. The characteristic
> method should just be called DidUpdateValue.

Done.

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

https://codereview.chromium.org/2052513002/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_mac.mm:139:
error_callback.Run(BluetoothRemoteGattService::GATT_ERROR_NOT_PERMITTED);
On 2016/06/16 18:22:18, ortuno wrote:
> ReadRemoteCharacteristic should always be async. Post a task to call
> error_callback.

Can I denounce windows code?
https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_remote_gatt_c...

Should I create a bug for windows?

https://codereview.chromium.org/2052513002/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_mac.mm:143:
error_callback.Run(BluetoothRemoteGattService::GATT_ERROR_IN_PROGRESS);
On 2016/06/16 18:22:18, ortuno wrote:
> Same here, post a task.

Done.

https://codereview.chromium.org/2052513002/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_mac.mm:174: const uint8_t*
buffer = static_cast<const uint8_t*>(nsdata_value.bytes);
On 2016/06/16 18:22:18, ortuno wrote:
> nit: Shouldn't you use [nsdata_value bytes] since NSData is an objective-c
> class?

Calling [nsdata_value bytes] or doing |nsdata_value.bytes|, it is exactly the
same for Objective-C. Both call the method -[NSData bytes].

Since -[NSData bytes] is declared as a property, .bytes is more appropriate. The
syntaxe . has been created for the @property.

https://codereview.chromium.org/2052513002/diff/200001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc (right):

https://codereview.chromium.org/2052513002/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:313:
EXPECT_EQ(1, gatt_read_characteristic_attempts_);
On 2016/06/16 18:22:19, ortuno wrote:
> We should also check that GattCharacteristicChanged was called. You will
> probably need and #if for Android, not sure if you need one for Windows. Same
> applies for all other tests.

Done in with: https://codereview.chromium.org/2085293002

https://codereview.chromium.org/2052513002/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:339: #if
defined(OS_ANDROID) || defined(OS_WIN)
On 2016/06/16 18:22:19, ortuno wrote:
> As mentioned in email I think this test can be enabled.

Done.

https://codereview.chromium.org/2052513002/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:601: }
On 2016/06/16 18:22:19, ortuno wrote:
> Also check that GattCharacteristicChanged hasn't been called.

Done.

https://codereview.chromium.org/2052513002/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:625: // Tests
ReadRemoteCharacteristic synchronous error.
On 2016/06/16 18:22:19, ortuno wrote:
> Add a comment here for why this doesn't apply to mac e.g. "CoreBluetooth's
> function to read a characteristic's value doesn't return any error
> synchronously."

Done.

[ortuno, 2016-06-22 18:08:15]

lgtm!

[jlebel, 2016-06-22 19:41:45]

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

[commit-bot: I haz the power, 2016-06-22 19:42:19]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2052513002/420001

[commit-bot: I haz the power, 2016-06-22 21:21:22]

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

[commit-bot: I haz the power, 2016-06-22 21:21:24]

Dry run: This issue passed the CQ dry run.

[msarda, 2016-06-27 11:52:47]

LGTM with nits.

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

https://codereview.chromium.org/2052513002/diff/420001/device/bluetooth/bluet...
device/bluetooth/bluetooth_device_mac.mm:22: // TODO(http://crbug.com/585894):
Need to convert the error.
It looks like the error is converted here. Does this TODO mean that the "error
code" needs to be converted?

https://codereview.chromium.org/2052513002/diff/420001/device/bluetooth/bluet...
device/bluetooth/bluetooth_device_mac.mm:60: DCHECK(false);
Use NOTREACHED() (equivalent with DCHECK(false) but with a better name).

https://codereview.chromium.org/2052513002/diff/420001/device/bluetooth/bluet...
device/bluetooth/bluetooth_device_mac.mm:63: // TODO(http://crbug.com/619595):
Need to convert the error.
I do not understand this TODO. It looks like the error is converted already.

https://codereview.chromium.org/2052513002/diff/420001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_remote_gatt_characteristic_mac.mm (right):

https://codereview.chromium.org/2052513002/diff/420001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_mac.mm:144:
BluetoothRemoteGattService::GATT_ERROR_IN_PROGRESS));
I'm wondering if GATT_ERROR_IN_PROGRESS is the right error code if the
characteristic is not readable. Would GATT_ERROR_NOT_SUPPORTED be a better
error?

[jlebel, 2016-06-27 12:09:20]

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

https://codereview.chromium.org/2052513002/diff/420001/device/bluetooth/bluet...
device/bluetooth/bluetooth_device_mac.mm:22: // TODO(http://crbug.com/585894):
Need to convert the error.
On 2016/06/27 11:52:47, msarda wrote:
> It looks like the error is converted here. Does this TODO mean that the "error
> code" needs to be converted?

It would be nicer to use a CBError code.

https://codereview.chromium.org/2052513002/diff/420001/device/bluetooth/bluet...
device/bluetooth/bluetooth_device_mac.mm:60: DCHECK(false);
On 2016/06/27 11:52:47, msarda wrote:
> Use NOTREACHED() (equivalent with DCHECK(false) but with a better name).

Done.

https://codereview.chromium.org/2052513002/diff/420001/device/bluetooth/bluet...
device/bluetooth/bluetooth_device_mac.mm:63: // TODO(http://crbug.com/619595):
Need to convert the error.
On 2016/06/27 11:52:47, msarda wrote:
> I do not understand this TODO. It looks like the error is converted already.

Updating the comment to:
Need to convert the error code from CoreBluetooth to a GattErrorCode value

https://codereview.chromium.org/2052513002/diff/420001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_remote_gatt_characteristic_mac.mm (right):

https://codereview.chromium.org/2052513002/diff/420001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_mac.mm:144:
BluetoothRemoteGattService::GATT_ERROR_IN_PROGRESS));
On 2016/06/27 11:52:47, msarda wrote:
> I'm wondering if GATT_ERROR_IN_PROGRESS is the right error code if the
> characteristic is not readable. Would GATT_ERROR_NOT_SUPPORTED be a better
> error?

Done.

[jlebel, 2016-06-27 12:29:58]

The CQ bit was checked by jlebel@chromium.org

[jlebel, 2016-06-27 12:29:59]

The patchset sent to the CQ was uploaded after l-g-t-m from ortuno@chromium.org,
msarda@chromium.org
Link to the patchset: https://codereview.chromium.org/2052513002/#ps440001
(title: "Addressing msarda's comment")

[commit-bot: I haz the power, 2016-06-27 12:30:14]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-27 13:09:36]

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

[commit-bot: I haz the power, 2016-06-27 13:09:37]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jlebel, 2016-06-27 15:04:13]

The CQ bit was checked by jlebel@chromium.org

[commit-bot: I haz the power, 2016-06-27 15:04:38]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-27 15:23:09]

Description was changed from

==========
bluetooth: mac: read characteristic implementation

Adding implementing read characteristic in BluetoothRemoteGattCharacteristicMac,
and adding unit tests, mock and simulation.

BUG=609066
==========

to

==========
bluetooth: mac: read characteristic implementation

Adding implementing read characteristic in BluetoothRemoteGattCharacteristicMac,
and adding unit tests, mock and simulation.

BUG=609066
==========

[commit-bot: I haz the power, 2016-06-27 15:23:11]

Committed patchset #18 (id:440001)

[commit-bot: I haz the power, 2016-06-27 15:26:02]

Description was changed from

==========
bluetooth: mac: read characteristic implementation

Adding implementing read characteristic in BluetoothRemoteGattCharacteristicMac,
and adding unit tests, mock and simulation.

BUG=609066
==========

to

==========
bluetooth: mac: read characteristic implementation

Adding implementing read characteristic in BluetoothRemoteGattCharacteristicMac,
and adding unit tests, mock and simulation.

BUG=609066

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

[commit-bot: I haz the power, 2016-06-27 15:26:04]

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