Bluetooth: macOS: Replacing BluetoothRemoteGattCharacteristicMac::StartNotifySession() by Bluetooth…

Bluetooth: macOS: Replacing BluetoothRemoteGattCharacteristicMac::StartNotifySession() by BluetoothRemoteGattCharacteristicMac::SubscribeToNotifications().

BUG=633191
Review-Url: https://codereview.chromium.org/2599303002
Cr-Commit-Position: refs/heads/master@{#443820}
Committed: https://chromium.googlesource.com/chromium/src/+/230ba260170c6d7ab48a72a5f0eea01be95740f3

patch from issue 2599303002 at patchset 120001 (http://crrev.com/2599303002#ps120001)

Review-Url: https://codereview.chromium.org/2632253003
Cr-Commit-Position: refs/heads/master@{#444899}
Committed: https://chromium.googlesource.com/chromium/src/+/b06715722249119894485b423b3bdeda2c18d60b

[jlebel, 2017-01-17 00:09:30]

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

[jlebel, 2017-01-17 00:09:31]

Hello Giovanni,

I've updated this patch with your comments. But I still have the same issue with
the BluetoothRemoteGattCharacteristicTest.StartNotifySession being blocked in:
BluetoothRemoteGattCharacteristic::~BluetoothRemoteGattCharacteristic()

If you have time, I would enjoy some help. Thanks.

Jérôme,

[ortuno, 2017-01-18 01:22:03]

I think you found a bug :o

Line [1] should be:

base::Bind(&BluetoothRemoteGattCharacteristic::CancelStopNotifySession,
           GetWeakPtr(), callback));

instead of just callback.


I'm not sure why this is not showing up on other platforms D:

[1]
https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_remote_gatt_c...

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

https://codereview.chromium.org/2632253003/diff/20001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_mac.mm:93: if
(!subscribe_to_notification_callback_.second.is_null()) {
Do any tests fail if you remove this? I think the code in
~BluetoothRemoteGattCharacteristic should handle running the error callback.

[jlebel, 2017-01-18 14:42:32]

Do you want me to fix the infinite loop bug in this patch?

I can't compile your suggestion:
../../device/bluetooth/bluetooth_remote_gatt_characteristic.cc:234:39: error: no
matching constructor for initialization of
'device::BluetoothRemoteGattCharacteristic::NotifySessionCommand'
  NotifySessionCommand* command = new NotifySessionCommand(
                                      ^
../../device/bluetooth/bluetooth_remote_gatt_characteristic.cc:46:58: note:
candidate constructor not viable: no known conversion from
'Callback<MakeUnboundRunType<void
(device::BluetoothRemoteGattCharacteristic::*)(base::Callback<void (),
base::internal::CopyMode::Copyable, base::internal::RepeatMode::Repeating>),
base::WeakPtr<device::BluetoothRemoteGattCharacteristic>, const
base::Callback<void (), base::internal::CopyMode::Copyable,
base::internal::RepeatMode::Repeating> &>>' to 'const Callback<void
(device::BluetoothRemoteGattCharacteristic::NotifySessionCommand::Type,
device::BluetoothRemoteGattCharacteristic::NotifySessionCommand::Result,
BluetoothRemoteGattService::GattErrorCode)>' for 1st argument
BluetoothRemoteGattCharacteristic::NotifySessionCommand::NotifySessionCommand(
                                                         ^
../../device/bluetooth/bluetooth_remote_gatt_characteristic.h:176:9: note:
candidate constructor (the implicit copy constructor) not viable: requires 1
argument, but 2 were provided
  class NotifySessionCommand {

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

https://codereview.chromium.org/2632253003/diff/20001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_mac.mm:93: if
(!subscribe_to_notification_callback_.second.is_null()) {
On 2017/01/18 01:22:02, ortuno wrote:
> Do any tests fail if you remove this? I think the code in
> ~BluetoothRemoteGattCharacteristic should handle running the error callback.

This code can be removed without breaking unit tests.

[ortuno, 2017-01-19 07:30:11]

See http://crbug.com/682583 for the complete code. Yes let's fix it in this
patch but let's keep the issue opened so I can add some tests for it.

[jlebel, 2017-01-19 08:37:39]

Hello Giovanni,

Sorry, I saw this comment after the bug... So I created a patch and rebased this
patch to other one. I guess since I've done the work, let's keep it that way, if
it is ok with you.

Jérôme,

[jlebel, 2017-01-19 13:32:47]

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

[commit-bot: I haz the power, 2017-01-19 13:33:00]

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, 2017-01-19 14:57:11]

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

[commit-bot: I haz the power, 2017-01-19 14:57:12]

Dry run: This issue passed the CQ dry run.

[ortuno, 2017-01-19 22:29:20]

lgtm

[jlebel, 2017-01-20 00:16:25]

The CQ bit was checked by jlebel@chromium.org

[commit-bot: I haz the power, 2017-01-20 00:17:22]

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, 2017-01-20 00:52:38]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1484871385667660,
"parent_rev": "34215edf2e52ccf0e9fd52135afd1addde2c7356", "commit_rev":
"b06715722249119894485b423b3bdeda2c18d60b"}

[commit-bot: I haz the power, 2017-01-20 00:53:11]

Description was changed from

==========
Bluetooth: macOS: Replacing
BluetoothRemoteGattCharacteristicMac::StartNotifySession() by
BluetoothRemoteGattCharacteristicMac::SubscribeToNotifications().

BUG=633191

Review-Url: https://codereview.chromium.org/2599303002
Cr-Commit-Position: refs/heads/master@{#443820}
Committed:
https://chromium.googlesource.com/chromium/src/+/230ba260170c6d7ab48a72a5f0ee...

patch from issue 2599303002 at patchset 120001
(http://crrev.com/2599303002#ps120001)
==========

to

==========
Bluetooth: macOS: Replacing
BluetoothRemoteGattCharacteristicMac::StartNotifySession() by
BluetoothRemoteGattCharacteristicMac::SubscribeToNotifications().

BUG=633191

Review-Url: https://codereview.chromium.org/2599303002
Cr-Commit-Position: refs/heads/master@{#443820}
Committed:
https://chromium.googlesource.com/chromium/src/+/230ba260170c6d7ab48a72a5f0ee...

patch from issue 2599303002 at patchset 120001
(http://crrev.com/2599303002#ps120001)

Review-Url: https://codereview.chromium.org/2632253003
Cr-Commit-Position: refs/heads/master@{#444899}
Committed:
https://chromium.googlesource.com/chromium/src/+/b06715722249119894485b423b3b...
==========

[commit-bot: I haz the power, 2017-01-20 00:53:14]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/b06715722249119894485b423b3b...