Bluetooth: Mac: implementation for start notification

Adding implementation start characteristic notification
BluetoothRemoteGattCharacteristicMac, and adding unit tests, mock and
simulation.

Stop notification will be done in a next patch, crbug/624019

BUG=609068
Committed: https://crrev.com/9db202163870c27fdcca79eb95e75a610e8f8a29
Cr-Commit-Position: refs/heads/master@{#402811}

[jlebel, 2016-06-27 17:33:58]

Patchset #2 (id:20001) has been deleted

[jlebel, 2016-06-27 17:42:28]

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

[commit-bot: I haz the power, 2016-06-27 17:43:07]

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:53:05]

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

[commit-bot: I haz the power, 2016-06-27 18:53:07]

Dry run: 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-28 15:03:54]

Description was changed from

==========
Implementation for notification on macOS

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

to

==========
Adding implementation start characteristic notification
BluetoothRemoteGattCharacteristicMac, and adding unit tests, mock and
simulation.

Stop notification will be done in a next patch.

BUG=609068
==========

[jlebel, 2016-06-28 15:05:03]

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

[commit-bot: I haz the power, 2016-06-28 15:05:26]

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

[jlebel, 2016-06-28 15:06:23]

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

[jlebel, 2016-06-28 15:06:23]

Hello Giovanni,

Can you review my implementation for characteristic notification on macOS? On
start has been implemented. Stop will be done later.

Thanks,

[ortuno, 2016-06-28 16:24:50]

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

https://codereview.chromium.org/2094633003/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_mac.h:64: // Updates the
|value_| and send notify the mac adapter for the new value
nit: Usually the "|" are used when referring to a function argument.

https://codereview.chromium.org/2094633003/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_mac.h:76: // Returns true
if the characteristic is notifiable or indicatable.
nit: ... if the characteristic supports notifications or indications.

https://codereview.chromium.org/2094633003/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_mac.h:77: bool
IsNotifiableOrIndicatable() const;
nit: SupportsNotificationsOrIndications.

https://codereview.chromium.org/2094633003/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_mac.h:99: // Vector stores
StartNotifySession request callbacks.
nit: Remove "Vector". The type is easy to see already.

https://codereview.chromium.org/2094633003/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_mac.h:100:
std::vector<std::pair<NotifySessionCallback, ErrorCallback>>
nit optional: make the pair a typedef like
https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_remote_gatt_c...

https://codereview.chromium.org/2094633003/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_mac.h:103: bool
gatt_event_registeration_in_progress_;
nit: change this to start_notifications_in_progress or similar. In windows it's
called gatt_event_registration because the API to enable notifications is called
BluetoothGattRegisterEvent.

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

https://codereview.chromium.org/2094633003/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_mac.mm:236: }
We don't expect both conditions to fail right? Could you add a VLOG if both
conditions fail?

https://codereview.chromium.org/2094633003/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_mac.mm:274:
std::vector<std::pair<NotifySessionCallback, ErrorCallback>> callbacks;
nit: call this reentrant_safe_callbacks

https://codereview.chromium.org/2094633003/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_mac.mm:291:
callback.first.Run(base::WrapUnique(
This should be:

base::MakeUnique<BluetoothGattNotifySessionMac>(weak_ptr_factory.GetWeakPtr());

https://cs.chromium.org/chromium/src/base/memory/ptr_util.h?sq=package:chromi...

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

https://codereview.chromium.org/2094633003/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:89: #if
!defined(OS_MACOSX)
Move this right before expected_descriptors_count = 0;

https://codereview.chromium.org/2094633003/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:90: //
TODO(jlebel) Need to implement descriptors.
I don't think we need the TODO as we don't need to mess with descriptors for
enabling notifications on macOS. Something like the following should suffice:

macOS: Not applicable. CoreBluetooth exposes setNotifyValue:forCharacteristic:
which handles all interactions with the CCC descriptor.

https://codereview.chromium.org/2094633003/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:129: //
TODO(jlebel) Need to implement descriptors.
No need for the TODO. Change to match the same comment as above.

https://codereview.chromium.org/2094633003/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:352: // macOS:
Not applicable: CoreBluetooth doesn't support synchronous API.
I think you meant to use the comment on the following test :)

https://codereview.chromium.org/2094633003/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:926: //
Characteristic Configuration descriptor.
Add a TODO to enable this test on macOS if we get access to the CCC descriptor.
If you don't know if we get access to the CCC descriptor then open an issue and
add a TODO saying that we need to investigate more before enabling this test on
macOS.

https://codereview.chromium.org/2094633003/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:946: //
StartNotifySession fails if the characteristic has multiple Client
Same as above. Add TODO.

https://codereview.chromium.org/2094633003/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:992: // macOS:
Not applicable: CoreBluetooth doesn't support synchronous API.
I think this should be: macOS. Not applicable: No need to write to the
descriptor manually. setNotifyValue:forCharacteristic: takes care of it.

https://codereview.chromium.org/2094633003/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1051: //
TODO(jlebel) Need to implement descriptors.
nit: open/point to an issue.

https://codereview.chromium.org/2094633003/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1089: //
TODO(jlebel) Need to implement descriptors.
nit: same here, point to an issue.

https://codereview.chromium.org/2094633003/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1120: #if
!defined(OS_MACOSX)
You don't need this one since this test doesn't apply to macOS.

https://codereview.chromium.org/2094633003/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1195: //
TODO(jlebel) Need to implement descriptors.
Remove since this is not enabled on macOS yet.

https://codereview.chromium.org/2094633003/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1236: #if
!defined(OS_MACOSX)
Remove since this is not enabled on macOS yet.

[commit-bot: I haz the power, 2016-06-28 16:43:54]

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

[commit-bot: I haz the power, 2016-06-28 16:43:55]

Dry run: Try jobs failed on following builders:
  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-28 17:47:32]

Description was changed from

==========
Adding implementation start characteristic notification
BluetoothRemoteGattCharacteristicMac, and adding unit tests, mock and
simulation.

Stop notification will be done in a next patch.

BUG=609068
==========

to

==========
Adding implementation start characteristic notification
BluetoothRemoteGattCharacteristicMac, and adding unit tests, mock and
simulation.

Stop notification will be done in a next patch, crbug/624019

BUG=609068
==========

[jlebel, 2016-06-28 18:09:25]

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

[jlebel, 2016-06-28 18:09:53]

Hello Giovanni,

I've fixed my patch.

Thanks,

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

https://codereview.chromium.org/2094633003/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_mac.h:64: // Updates the
|value_| and send notify the mac adapter for the new value
On 2016/06/28 16:24:50, ortuno wrote:
> nit: Usually the "|" are used when referring to a function argument.

Done.

https://codereview.chromium.org/2094633003/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_mac.h:76: // Returns true
if the characteristic is notifiable or indicatable.
On 2016/06/28 16:24:49, ortuno wrote:
> nit: ... if the characteristic supports notifications or indications.

Done.

https://codereview.chromium.org/2094633003/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_mac.h:77: bool
IsNotifiableOrIndicatable() const;
On 2016/06/28 16:24:49, ortuno wrote:
> nit: SupportsNotificationsOrIndications.

Done.

https://codereview.chromium.org/2094633003/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_mac.h:99: // Vector stores
StartNotifySession request callbacks.
On 2016/06/28 16:24:49, ortuno wrote:
> nit: Remove "Vector". The type is easy to see already.

Done.

https://codereview.chromium.org/2094633003/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_mac.h:100:
std::vector<std::pair<NotifySessionCallback, ErrorCallback>>
On 2016/06/28 16:24:49, ortuno wrote:
> nit optional: make the pair a typedef like
>
https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_remote_gatt_c...

Done.

https://codereview.chromium.org/2094633003/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_mac.h:103: bool
gatt_event_registeration_in_progress_;
On 2016/06/28 16:24:49, ortuno wrote:
> nit: change this to start_notifications_in_progress or similar. In windows
it's
> called gatt_event_registration because the API to enable notifications is
called
> BluetoothGattRegisterEvent.

Done.

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

https://codereview.chromium.org/2094633003/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_mac.mm:236: }
On 2016/06/28 16:24:50, ortuno wrote:
> We don't expect both conditions to fail right? Could you add a VLOG if both
> conditions fail?

Done.

https://codereview.chromium.org/2094633003/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_mac.mm:274:
std::vector<std::pair<NotifySessionCallback, ErrorCallback>> callbacks;
On 2016/06/28 16:24:50, ortuno wrote:
> nit: call this reentrant_safe_callbacks

Done.

https://codereview.chromium.org/2094633003/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_mac.mm:291:
callback.first.Run(base::WrapUnique(
On 2016/06/28 16:24:50, ortuno wrote:
> This should be:
> 
>
base::MakeUnique<BluetoothGattNotifySessionMac>(weak_ptr_factory.GetWeakPtr());
> 
>
https://cs.chromium.org/chromium/src/base/memory/ptr_util.h?sq=package:chromi...

Done.

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

https://codereview.chromium.org/2094633003/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:89: #if
!defined(OS_MACOSX)
On 2016/06/28 16:24:50, ortuno wrote:
> Move this right before expected_descriptors_count = 0;

Done.

https://codereview.chromium.org/2094633003/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:90: //
TODO(jlebel) Need to implement descriptors.
On 2016/06/28 16:24:50, ortuno wrote:
> I don't think we need the TODO as we don't need to mess with descriptors for
> enabling notifications on macOS. Something like the following should suffice:
> 
> macOS: Not applicable. CoreBluetooth exposes setNotifyValue:forCharacteristic:
> which handles all interactions with the CCC descriptor.

Done.

https://codereview.chromium.org/2094633003/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:129: //
TODO(jlebel) Need to implement descriptors.
On 2016/06/28 16:24:50, ortuno wrote:
> No need for the TODO. Change to match the same comment as above.

Done.

https://codereview.chromium.org/2094633003/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:352: // macOS:
Not applicable: CoreBluetooth doesn't support synchronous API.
On 2016/06/28 16:24:50, ortuno wrote:
> I think you meant to use the comment on the following test :)

Done.

https://codereview.chromium.org/2094633003/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:926: //
Characteristic Configuration descriptor.
On 2016/06/28 16:24:50, ortuno wrote:
> Add a TODO to enable this test on macOS if we get access to the CCC
descriptor.
> If you don't know if we get access to the CCC descriptor then open an issue
and
> add a TODO saying that we need to investigate more before enabling this test
on
> macOS.

Done.

https://codereview.chromium.org/2094633003/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:946: //
StartNotifySession fails if the characteristic has multiple Client
On 2016/06/28 16:24:50, ortuno wrote:
> Same as above. Add TODO.

Done.

https://codereview.chromium.org/2094633003/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:992: // macOS:
Not applicable: CoreBluetooth doesn't support synchronous API.
On 2016/06/28 16:24:50, ortuno wrote:
> I think this should be: macOS. Not applicable: No need to write to the
> descriptor manually. setNotifyValue:forCharacteristic: takes care of it.

Done.

https://codereview.chromium.org/2094633003/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1051: //
TODO(jlebel) Need to implement descriptors.
On 2016/06/28 16:24:50, ortuno wrote:
> nit: open/point to an issue.

Done.

https://codereview.chromium.org/2094633003/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1089: //
TODO(jlebel) Need to implement descriptors.
On 2016/06/28 16:24:50, ortuno wrote:
> nit: same here, point to an issue.

Done.

https://codereview.chromium.org/2094633003/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1120: #if
!defined(OS_MACOSX)
On 2016/06/28 16:24:50, ortuno wrote:
> You don't need this one since this test doesn't apply to macOS.

Done.

https://codereview.chromium.org/2094633003/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1195: //
TODO(jlebel) Need to implement descriptors.
On 2016/06/28 16:24:50, ortuno wrote:
> Remove since this is not enabled on macOS yet.

Done.

https://codereview.chromium.org/2094633003/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1236: #if
!defined(OS_MACOSX)
On 2016/06/28 16:24:50, ortuno wrote:
> Remove since this is not enabled on macOS yet.

Done.

[commit-bot: I haz the power, 2016-06-28 18:10:02]

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

[ortuno, 2016-06-28 19:18:58]

lgtm! bar some minor grammar issues

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

https://codereview.chromium.org/2094633003/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_mac.h:64: // Updates the
value_ and send notify the mac adapter for the new value
grammar nit: Updates value_ and notifies the adapter of the new value. (Don't
forget '.' at the end of the sentence).

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

https://codereview.chromium.org/2094633003/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_mac.mm:240:
"notification";
grammar nit: "." at the end of sentence.

https://codereview.chromium.org/2094633003/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_mac.mm:256: VLOG(1) <<
"Write notification while no write operation pending";
grammar nit: missing colon.

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

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

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

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

[jlebel, 2016-06-28 21:02:59]

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

[commit-bot: I haz the power, 2016-06-28 21:03:46]

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

[jlebel, 2016-06-28 21:06:15]

Patchset #6 (id:120001) has been deleted

[jlebel, 2016-06-28 21:09:02]

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

[jlebel, 2016-06-28 21:09:34]

Hello Giovanni

Thanks for your comments.

Jérôme,

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

https://codereview.chromium.org/2094633003/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_mac.h:64: // Updates the
value_ and send notify the mac adapter for the new value
On 2016/06/28 19:18:58, ortuno wrote:
> grammar nit: Updates value_ and notifies the adapter of the new value. (Don't
> forget '.' at the end of the sentence).

Done.

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

https://codereview.chromium.org/2094633003/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_mac.mm:240:
"notification";
On 2016/06/28 19:18:58, ortuno wrote:
> grammar nit: "." at the end of sentence.

Done.

https://codereview.chromium.org/2094633003/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_mac.mm:256: VLOG(1) <<
"Write notification while no write operation pending";
On 2016/06/28 19:18:58, ortuno wrote:
> grammar nit: missing colon.

Done.

[commit-bot: I haz the power, 2016-06-28 21:09:47]

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

[scheib, 2016-06-28 21:26:04]

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

[commit-bot: I haz the power, 2016-06-29 00:46:59]

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

[commit-bot: I haz the power, 2016-06-29 00:47:01]

Dry run: This issue passed the CQ dry run.

[msarda, 2016-06-29 07:57:39]

LGTM with nits.

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

https://codereview.chromium.org/2094633003/diff/140001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_mac.h:64: // Updates
value_ and notifies the adapter of the new value.
s/value_/|value_|

https://codereview.chromium.org/2094633003/diff/140001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_mac.h:70: // session has
been started or failed.
I do not understand what "failed" means: does this mean that the notify session
failed to start or that a running session fails at some point?

Maybe:
s/failed/failed to be started

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

https://codereview.chromium.org/2094633003/diff/140001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_mac.mm:219: //
notification received.
s/received/is received

https://codereview.chromium.org/2094633003/diff/140001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_mac.mm:226: <<
error.domain.UTF8String << ", error code: " << error.code;
Concert NSString to std::string objects using base:SysNSStringToUTF8 function:
https://cs.chromium.org/chromium/src/base/strings/sys_string_conversions.h?q=...

This means:
s/error.domain.UTF8String/base:SysNSStringToUTF8(error.domain)

[jlebel, 2016-06-29 08:05:57]

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

https://codereview.chromium.org/2094633003/diff/140001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_mac.h:64: // Updates
value_ and notifies the adapter of the new value.
On 2016/06/29 07:57:39, msarda wrote:
> s/value_/|value_|

i've been asked to remove the ||.

https://codereview.chromium.org/2094633003/diff/140001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_mac.h:70: // session has
been started or failed.
On 2016/06/29 07:57:39, msarda wrote:
> I do not understand what "failed" means: does this mean that the notify
session
> failed to start or that a running session fails at some point?
> 
> Maybe:
> s/failed/failed to be started

Done.

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

https://codereview.chromium.org/2094633003/diff/140001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_mac.mm:219: //
notification received.
On 2016/06/29 07:57:39, msarda wrote:
> s/received/is received

Done.

https://codereview.chromium.org/2094633003/diff/140001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_mac.mm:226: <<
error.domain.UTF8String << ", error code: " << error.code;
On 2016/06/29 07:57:39, msarda wrote:
> Concert NSString to std::string objects using base:SysNSStringToUTF8 function:
>
https://cs.chromium.org/chromium/src/base/strings/sys_string_conversions.h?q=...
> 
> This means:
> s/error.domain.UTF8String/base:SysNSStringToUTF8(error.domain)

Done.

[jlebel, 2016-06-29 08:06:04]

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

[commit-bot: I haz the power, 2016-06-29 08:06:19]

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-29 11:12:18]

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

[commit-bot: I haz the power, 2016-06-29 11:12:19]

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-29 12:52:37]

The CQ bit was checked by jlebel@chromium.org

[jlebel, 2016-06-29 12:52:38]

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/2094633003/#ps160001
(title: "Addressing msarda's comments")

[commit-bot: I haz the power, 2016-06-29 12:52:58]

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-29 15:04:03]

Description was changed from

==========
Adding implementation start characteristic notification
BluetoothRemoteGattCharacteristicMac, and adding unit tests, mock and
simulation.

Stop notification will be done in a next patch, crbug/624019

BUG=609068
==========

to

==========
Adding implementation start characteristic notification
BluetoothRemoteGattCharacteristicMac, and adding unit tests, mock and
simulation.

Stop notification will be done in a next patch, crbug/624019

BUG=609068
==========

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

Committed patchset #7 (id:160001)

[commit-bot: I haz the power, 2016-06-29 15:07:31]

Description was changed from

==========
Adding implementation start characteristic notification
BluetoothRemoteGattCharacteristicMac, and adding unit tests, mock and
simulation.

Stop notification will be done in a next patch, crbug/624019

BUG=609068
==========

to

==========
Adding implementation start characteristic notification
BluetoothRemoteGattCharacteristicMac, and adding unit tests, mock and
simulation.

Stop notification will be done in a next patch, crbug/624019

BUG=609068

Committed: https://crrev.com/9db202163870c27fdcca79eb95e75a610e8f8a29
Cr-Commit-Position: refs/heads/master@{#402811}
==========

[commit-bot: I haz the power, 2016-06-29 15:07:32]

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