bluetooth: mac: write characteristic implementation

bluetooth: mac: write characteristic implementation

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

BUG=609067
Committed: https://crrev.com/4e58d3d7f3befa079ab00b65b9ffc7dd9291d44b
Cr-Commit-Position: refs/heads/master@{#402367}

[jlebel, 2016-06-22 19:01:07]

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:01:39]

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

[jlebel, 2016-06-22 19:27:47]

Patchset #2 (id:20001) has been deleted

[jlebel, 2016-06-22 19:29:09]

Description was changed from

==========
Write characteristics on macOS

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

to

==========
bluetooth: mac: write characteristic implementation

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

BUG=609067
==========

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

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

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

Dry run: Try jobs failed on following builders:
  ios-device on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
  ios-simulator on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)

[jlebel, 2016-06-22 19:36:23]

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:37:01]

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

[jlebel, 2016-06-22 19:39:15]

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

[jlebel, 2016-06-22 19:39:16]

Hello Giovanni,

Can you review the write implementation?

Thanks,

[ortuno, 2016-06-22 21:37:46]

Looking good. Just a couple of small issues.

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

https://codereview.chromium.org/2074563002/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_mac.h:85: bool
characteristic_value_write_in_progress_;
I don't think you are using this anywhere?

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

https://codereview.chromium.org/2074563002/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_mac.mm:189:
DidWriteValue(nil);
We still want this function to be async so you need to use PostTask

https://codereview.chromium.org/2074563002/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_mac.mm:217: if
(!characteristic_value_read_or_write_in_progress_) {
Please add a comment explaining when would this function get called if neither
read nor write are in progress.

https://codereview.chromium.org/2074563002/diff/40001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc (right):

https://codereview.chromium.org/2074563002/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:341: #endif 
// defined(OS_ANDROID) || defined(OS_WIN)
Add "|| defined(OS_MACOSX)"

https://codereview.chromium.org/2074563002/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:691: // Tests
WriteRemoteCharacteristic synchronous error.
Add a comment explaining why this test doesn't apply to macOS e.g. "This test
doesn't apply to macOS because the CoreBluetooth API does not return anything
synchronously when writing to a characteristic.

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

https://codereview.chromium.org/2074563002/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_service_mac.h:60: // Called by the
BluetoothRemoteGattServiceMac instance when the
You mean BluetoothLowEnergyDeviceMac instance?

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

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

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

Dry run: This issue passed the CQ dry run.

[jlebel, 2016-06-23 18:33:01]

Hello Giovanni,

I fixed you comments.

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

https://codereview.chromium.org/2074563002/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_mac.h:85: bool
characteristic_value_write_in_progress_;
On 2016/06/22 21:37:45, ortuno wrote:
> I don't think you are using this anywhere?

Done.

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

https://codereview.chromium.org/2074563002/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_mac.mm:189:
DidWriteValue(nil);
On 2016/06/22 21:37:45, ortuno wrote:
> We still want this function to be async so you need to use PostTask

Done.

https://codereview.chromium.org/2074563002/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_mac.mm:217: if
(!characteristic_value_read_or_write_in_progress_) {
On 2016/06/22 21:37:45, ortuno wrote:
> Please add a comment explaining when would this function get called if neither
> read nor write are in progress.

I'm not sure when it would be the case. For me it close to be a DCHECK(), unless
a buggy device can send 2 confirmations of the same write (same for the read).

https://codereview.chromium.org/2074563002/diff/40001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc (right):

https://codereview.chromium.org/2074563002/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:341: #endif 
// defined(OS_ANDROID) || defined(OS_WIN)
On 2016/06/22 21:37:45, ortuno wrote:
> Add "|| defined(OS_MACOSX)"

Done.

https://codereview.chromium.org/2074563002/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:691: // Tests
WriteRemoteCharacteristic synchronous error.
On 2016/06/22 21:37:45, ortuno wrote:
> Add a comment explaining why this test doesn't apply to macOS e.g. "This test
> doesn't apply to macOS because the CoreBluetooth API does not return anything
> synchronously when writing to a characteristic.

Done.

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

https://codereview.chromium.org/2074563002/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_service_mac.h:60: // Called by the
BluetoothRemoteGattServiceMac instance when the
On 2016/06/22 21:37:45, ortuno wrote:
> You mean BluetoothLowEnergyDeviceMac instance?

Done.

[jlebel, 2016-06-23 18:54:10]

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

[commit-bot: I haz the power, 2016-06-23 18:54:34]

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

[commit-bot: I haz the power, 2016-06-23 19:16:04]

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

[commit-bot: I haz the power, 2016-06-23 19:16:05]

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

[ortuno, 2016-06-23 19:21:16]

lgtm with comment fix

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

https://codereview.chromium.org/2074563002/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_mac.mm:217: if
(!characteristic_value_read_or_write_in_progress_) {
On 2016/06/23 at 18:33:01, jlebel wrote:
> On 2016/06/22 21:37:45, ortuno wrote:
> > Please add a comment explaining when would this function get called if
neither
> > read nor write are in progress.
> 
> I'm not sure when it would be the case. For me it close to be a DCHECK(),
unless a buggy device can send 2 confirmations of the same write (same for the
read).

Now that I think about it we have the same line in Android in case of a buggy
device sending two confirmations. Could you add the bit about the buggy device
to the comment?

https://codereview.chromium.org/2074563002/diff/60001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc (right):

https://codereview.chromium.org/2074563002/diff/60001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:693:
synchronously when writing to a characteristic
Fix comment :)

[jlebel, 2016-06-23 19:27:56]

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

[jlebel, 2016-06-23 19:28:30]

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

https://codereview.chromium.org/2074563002/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_mac.mm:217: if
(!characteristic_value_read_or_write_in_progress_) {
On 2016/06/23 19:21:16, ortuno wrote:
> On 2016/06/23 at 18:33:01, jlebel wrote:
> > On 2016/06/22 21:37:45, ortuno wrote:
> > > Please add a comment explaining when would this function get called if
> neither
> > > read nor write are in progress.
> > 
> > I'm not sure when it would be the case. For me it close to be a DCHECK(),
> unless a buggy device can send 2 confirmations of the same write (same for the
> read).
> 
> Now that I think about it we have the same line in Android in case of a buggy
> device sending two confirmations. Could you add the bit about the buggy device
> to the comment?

I'm not an expert in the BLE protocol, I don't known if it is possible. But I
did copy this pattern from Android or windows. I don't know if the macOS would
catch it or not.

[commit-bot: I haz the power, 2016-06-23 19:28:41]

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

[commit-bot: I haz the power, 2016-06-23 22:08:41]

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

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

Dry run: This issue passed the CQ dry run.

[msarda, 2016-06-27 11:41:36]

LGTM with a suggestion.

https://codereview.chromium.org/2074563002/diff/100001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_remote_gatt_characteristic_mac.h (right):

https://codereview.chromium.org/2074563002/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_mac.h:13: typedef
NS_ENUM(NSInteger, CBCharacteristicWriteType);
Why is this typedef needed? Could CoreBluetooth header be included here to avoid
the redefinition of this enum?

If this is dependent on a build version, then you may use something like:
#if defined(MAC_OS_X_VERSION_10_9) && \
    MAC_OS_X_VERSION_MIN_REQUIRED > MAC_OS_X_VERSION_10_9
#include <CoreBluetooth/CoreBluetooth.h>
#else
@class CBCharacteristic;
typedef NS_ENUM(NSInteger, CBCharacteristicWriteType);
#endif

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

https://codereview.chromium.org/2074563002/diff/100001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_remote_gatt_characteristic_mac.h (right):

https://codereview.chromium.org/2074563002/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_mac.h:13: typedef
NS_ENUM(NSInteger, CBCharacteristicWriteType);
On 2016/06/27 11:41:36, msarda wrote:
> Why is this typedef needed? Could CoreBluetooth header be included here to
avoid
> the redefinition of this enum?
> 
> If this is dependent on a build version, then you may use something like:
> #if defined(MAC_OS_X_VERSION_10_9) && \
>     MAC_OS_X_VERSION_MIN_REQUIRED > MAC_OS_X_VERSION_10_9
> #include <CoreBluetooth/CoreBluetooth.h>
> #else
> @class CBCharacteristic;
> typedef NS_ENUM(NSInteger, CBCharacteristicWriteType);
> #endif

Done.

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

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

[commit-bot: I haz the power, 2016-06-27 12:21:05]

Dry run: 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 12:23:09]

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

[commit-bot: I haz the power, 2016-06-27 12:23:10]

Dry run: Try jobs failed on following builders:
  ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)

[jlebel, 2016-06-27 12:43:46]

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

[commit-bot: I haz the power, 2016-06-27 12:43:57]

Dry run: 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 12:45:51]

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

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

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[jlebel, 2016-06-27 15:40:03]

Patchset #7 (id:140001) has been deleted

[jlebel, 2016-06-27 16:50:06]

Patchset #7 (id:160001) has been deleted

[jlebel, 2016-06-27 16:51:32]

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

[commit-bot: I haz the power, 2016-06-27 16:51:52]

Dry run: 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 18:38:56]

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

[commit-bot: I haz the power, 2016-06-27 18:38:58]

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

[jlebel, 2016-06-28 00:26:59]

The CQ bit was checked by jlebel@chromium.org

[jlebel, 2016-06-28 00:26: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/2074563002/#ps180001
(title: "Merge from top of tree")

[commit-bot: I haz the power, 2016-06-28 00:28:27]

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-28 01:43:39]

Description was changed from

==========
bluetooth: mac: write characteristic implementation

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

BUG=609067
==========

to

==========
bluetooth: mac: write characteristic implementation

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

BUG=609067
==========

[commit-bot: I haz the power, 2016-06-28 01:43:40]

Committed patchset #7 (id:180001)

[commit-bot: I haz the power, 2016-06-28 01:45:22]

Description was changed from

==========
bluetooth: mac: write characteristic implementation

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

BUG=609067
==========

to

==========
bluetooth: mac: write characteristic implementation

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

BUG=609067

Committed: https://crrev.com/4e58d3d7f3befa079ab00b65b9ffc7dd9291d44b
Cr-Commit-Position: refs/heads/master@{#402367}
==========

[commit-bot: I haz the power, 2016-06-28 01:45:23]

Patchset 7 (id:??) landed as
https://crrev.com/4e58d3d7f3befa079ab00b65b9ffc7dd9291d44b
Cr-Commit-Position: refs/heads/master@{#402367}