Issue 2634873002: Bluetooth: macOS: Implement BluetoothRemoteGattCharacteristicMac::UnsubscribeFromNotifications

jlebel

Patchset #1 (id:1) has been deleted

3 years, 11 months ago (2017-01-16 01:11:26 UTC) #1

jlebel

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

3 years, 11 months ago (2017-01-16 10:57:04 UTC) #4

jlebel

Hello Vincent, Can you review this patch? This is about the UnsubscribeFromNotifications implementation. Thanks,

3 years, 11 months ago (2017-01-16 10:57:05 UTC) #5

jlebel

Description was changed from ========== Bluetooth: macOS: Implement BluetoothRemoteGattCharacteristicMac::UnsubscribeFromNotifications BUG=633191 ========== to ========== Bluetooth: macOS: ...

3 years, 11 months ago (2017-01-26 23:27:37 UTC) #7

jlebel

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

3 years, 11 months ago (2017-01-26 23:29:35 UTC) #8

jlebel

Hello Giovanni, Can you review this patch about UnsubscribeFromNotifications()? Thanks,

3 years, 11 months ago (2017-01-26 23:29:35 UTC) #9

jlebel

Description was changed from ========== Bluetooth: macOS: Implement BluetoothRemoteGattCharacteristicMac::UnsubscribeFromNotifications Enabling unit tests for UnsubscribeFromNotifications() BUG=633191 ...

3 years, 10 months ago (2017-01-27 09:49:38 UTC) #11

jlebel

On 2017/01/27 01:22:41, ortuno wrote: > hmm how come we are not enabling the unit ...

3 years, 10 months ago (2017-01-27 09:50:11 UTC) #12

ortuno

All tests that have StartNotifySession and StopNotifySession should pass with this patch.

3 years, 10 months ago (2017-01-27 20:40:23 UTC) #13

jlebel

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

3 years, 10 months ago (2017-02-08 20:03:26 UTC) #14

commit-bot: I haz the power

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

3 years, 10 months ago (2017-02-08 20:04:25 UTC) #15

jlebel

Hello Giovanni, I updated the patch to include unit tests. Jérôme,

3 years, 10 months ago (2017-02-08 20:50:30 UTC) #17

commit-bot: I haz the power

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

3 years, 10 months ago (2017-02-08 21:05:10 UTC) #18

commit-bot: I haz the power

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_ng/builds/384387)

3 years, 10 months ago (2017-02-08 21:05:11 UTC) #19

jlebel

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

3 years, 10 months ago (2017-02-08 21:19:57 UTC) #20

commit-bot: I haz the power

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

3 years, 10 months ago (2017-02-08 21:20:30 UTC) #21

commit-bot: I haz the power

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

3 years, 10 months ago (2017-02-08 23:24:53 UTC) #22

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

3 years, 10 months ago (2017-02-08 23:24:54 UTC) #23

ortuno

You missed a couple of tests but we are so close! :) https://codereview.chromium.org/2634873002/diff/140001/device/bluetooth/bluetooth_remote_gatt_characteristic_mac.mm File device/bluetooth/bluetooth_remote_gatt_characteristic_mac.mm ...

3 years, 10 months ago (2017-02-09 00:34:48 UTC) #24

jlebel

On 2017/02/09 00:34:48, ortuno wrote: > You missed a couple of tests but we are ...

3 years, 10 months ago (2017-02-09 01:19:38 UTC) #25

jlebel

https://codereview.chromium.org/2634873002/diff/140001/device/bluetooth/bluetooth_remote_gatt_characteristic_mac.mm File device/bluetooth/bluetooth_remote_gatt_characteristic_mac.mm (right): https://codereview.chromium.org/2634873002/diff/140001/device/bluetooth/bluetooth_remote_gatt_characteristic_mac.mm#newcode310 device/bluetooth/bluetooth_remote_gatt_characteristic_mac.mm:310: DCHECK(![GetCBCharacteristic() isNotifying]); On 2017/02/09 00:34:48, ortuno wrote: > I ...

3 years, 10 months ago (2017-02-09 01:20:18 UTC) #26

ortuno

lgtm with some more tests https://codereview.chromium.org/2634873002/diff/140001/device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc File device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc (right): https://codereview.chromium.org/2634873002/diff/140001/device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc#newcode1408 device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1408: #if defined(OS_ANDROID) On 2017/02/09 ...

3 years, 10 months ago (2017-02-09 03:07:03 UTC) #27

lgtm with some more tests

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

https://codereview.chromium.org/2634873002/diff/140001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1408: #if
defined(OS_ANDROID)
On 2017/02/09 01:20:18, jlebel wrote:
> On 2017/02/09 00:34:48, ortuno wrote:
> > Also enable this one
> 
> I can't. Write is not implemented yet.

This is testing that:

1. We wrote the correct value to the descriptor.
2. The session is marked as inactive.
3. That the characteristic is no longer notifying.

(1) doesn't apply to macOS because CoreBluetooth handles writing to the
descriptor, so we don't really need write here.

Can you surround the descriptor check in an #if !defined(OS_MACOS) and explain
why that doesn't apply to macOS:

    // macOS: Not applicable. CoreBluetooth exposes -[CBPeripheral
    // setNotifyValue:forCharacteristic:] which handles all interactions with
    // the CCC descriptor.

https://codereview.chromium.org/2634873002/diff/140001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1432: #if
defined(OS_ANDROID)
On 2017/02/09 01:20:18, jlebel wrote:
> On 2017/02/09 00:34:48, ortuno wrote:
> > Also enable this test.
> 
> Same here.

Similarly here this tests that:

1. The correct value was written to the descriptor.
2. That the notification is no longer active.

(1) doesn't apply to macOS but (2) does. So do the same as above and surround
the non-mac part in an #if and mention why we do this:

    // macOS: Not applicable. CoreBluetooth exposes -[CBPeripheral
    // setNotifyValue:forCharacteristic:] which handles all interactions with
    // the CCC descriptor.

https://codereview.chromium.org/2634873002/diff/140001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1866: #if
defined(OS_ANDROID)
On 2017/02/09 01:20:18, jlebel wrote:
> On 2017/02/09 00:34:48, ortuno wrote:
> > You are missing this one.
> 
> Write is still missing

Same here. Surround the part that checks for the descriptor being written in an
#if and explain why:

    // macOS: Not applicable. CoreBluetooth exposes -[CBPeripheral
    // setNotifyValue:forCharacteristic:] which handles all interactions with
    // the CCC descriptor.

jlebel

https://codereview.chromium.org/2634873002/diff/140001/device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc File device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc (right): https://codereview.chromium.org/2634873002/diff/140001/device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc#newcode1408 device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1408: #if defined(OS_ANDROID) On 2017/02/09 03:07:03, ortuno wrote: > On ...

3 years, 10 months ago (2017-02-09 21:17:38 UTC) #28

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

https://codereview.chromium.org/2634873002/diff/140001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1408: #if
defined(OS_ANDROID)
On 2017/02/09 03:07:03, ortuno wrote:
> On 2017/02/09 01:20:18, jlebel wrote:
> > On 2017/02/09 00:34:48, ortuno wrote:
> > > Also enable this one
> > 
> > I can't. Write is not implemented yet.
> 
> This is testing that:
> 
> 1. We wrote the correct value to the descriptor.
> 2. The session is marked as inactive.
> 3. That the characteristic is no longer notifying.
> 
> (1) doesn't apply to macOS because CoreBluetooth handles writing to the
> descriptor, so we don't really need write here.
> 
> Can you surround the descriptor check in an #if !defined(OS_MACOS) and explain
> why that doesn't apply to macOS:
> 
>     // macOS: Not applicable. CoreBluetooth exposes -[CBPeripheral
>     // setNotifyValue:forCharacteristic:] which handles all interactions with
>     // the CCC descriptor.

Done.

https://codereview.chromium.org/2634873002/diff/140001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1432: #if
defined(OS_ANDROID)
On 2017/02/09 03:07:03, ortuno wrote:
> On 2017/02/09 01:20:18, jlebel wrote:
> > On 2017/02/09 00:34:48, ortuno wrote:
> > > Also enable this test.
> > 
> > Same here.
> 
> Similarly here this tests that:
> 
> 1. The correct value was written to the descriptor.
> 2. That the notification is no longer active.
> 
> (1) doesn't apply to macOS but (2) does. So do the same as above and surround
> the non-mac part in an #if and mention why we do this:
> 
>     // macOS: Not applicable. CoreBluetooth exposes -[CBPeripheral
>     // setNotifyValue:forCharacteristic:] which handles all interactions with
>     // the CCC descriptor.

Done.

https://codereview.chromium.org/2634873002/diff/140001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1866: #if
defined(OS_ANDROID)
On 2017/02/09 03:07:03, ortuno wrote:
> On 2017/02/09 01:20:18, jlebel wrote:
> > On 2017/02/09 00:34:48, ortuno wrote:
> > > You are missing this one.
> > 
> > Write is still missing
> 
> Same here. Surround the part that checks for the descriptor being written in
an
> #if and explain why:
> 
>     // macOS: Not applicable. CoreBluetooth exposes -[CBPeripheral
>     // setNotifyValue:forCharacteristic:] which handles all interactions with
>     // the CCC descriptor.

Done.

jlebel

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

3 years, 10 months ago (2017-02-09 22:30:27 UTC) #29

commit-bot: I haz the power

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

3 years, 10 months ago (2017-02-09 22:31:02 UTC) #30

ortuno

https://codereview.chromium.org/2634873002/diff/180001/device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc File device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc (right): https://codereview.chromium.org/2634873002/diff/180001/device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc#newcode1414 device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1414: // macOS: Not applicable. CoreBluetooth exposes -[CBPeripheral Ah sorry ...

3 years, 10 months ago (2017-02-09 22:46:28 UTC) #31

commit-bot: I haz the power

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

3 years, 10 months ago (2017-02-10 00:04:41 UTC) #32

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

3 years, 10 months ago (2017-02-10 00:04:42 UTC) #33

jlebel

On 2017/02/09 22:46:28, ortuno wrote: > https://codereview.chromium.org/2634873002/diff/180001/device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc > File device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc (right): > > https://codereview.chromium.org/2634873002/diff/180001/device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc#newcode1414 > ...

3 years, 10 months ago (2017-02-13 00:39:36 UTC) #35

ortuno

https://codereview.chromium.org/2634873002/diff/220001/device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc File device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc (right): https://codereview.chromium.org/2634873002/diff/220001/device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc#newcode1447 device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1447: EXPECT_EQ(2, gatt_notify_characteristic_attempts_); Great idea! Could you also do the ...

3 years, 10 months ago (2017-02-13 03:43:48 UTC) #36

jlebel

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

3 years, 10 months ago (2017-02-13 19:21:50 UTC) #37

jlebel

https://codereview.chromium.org/2634873002/diff/180001/device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc File device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc (right): https://codereview.chromium.org/2634873002/diff/180001/device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc#newcode1414 device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1414: // macOS: Not applicable. CoreBluetooth exposes -[CBPeripheral On 2017/02/09 ...

3 years, 10 months ago (2017-02-13 19:22:22 UTC) #38

commit-bot: I haz the power

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

3 years, 10 months ago (2017-02-13 19:22:37 UTC) #39

commit-bot: I haz the power

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

3 years, 10 months ago (2017-02-13 20:57:59 UTC) #40

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

3 years, 10 months ago (2017-02-13 20:58:00 UTC) #41

jlebel

The patchset sent to the CQ was uploaded after l-g-t-m from scheib@chromium.org Link to the ...

3 years, 10 months ago (2017-02-14 08:50:21 UTC) #44

commit-bot: I haz the power

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

3 years, 10 months ago (2017-02-14 08:50:36 UTC) #45

commit-bot: I haz the power

CQ is committing da patch. Bot data: {"patchset_id": 240001, "attempt_start_ts": 1487062220568740, "parent_rev": "f0056e78a01fe12008eec015c18f28c766d3cf58", "commit_rev": "7c346d76dbf749c72ef9733e4befd2845fcf6935"}

3 years, 10 months ago (2017-02-14 08:54:45 UTC) #46

commit-bot: I haz the power

Description was changed from ========== Bluetooth: macOS: Implement BluetoothRemoteGattCharacteristicMac::UnsubscribeFromNotifications BUG=633191 ========== to ========== Bluetooth: macOS: ...

3 years, 10 months ago (2017-02-14 08:55:42 UTC) #47

commit-bot: I haz the power

3 years, 10 months ago (2017-02-14 08:55:44 UTC) #48

Message was sent while issue was closed.

Committed patchset #9 (id:240001) as
https://chromium.googlesource.com/chromium/src/+/7c346d76dbf749c72ef9733e4bef...

Issue 2634873002: Bluetooth: macOS: Implement BluetoothRemoteGattCharacteristicMac::UnsubscribeFromNotifications (Closed)

Description

Patch Set 1 #

Patch Set 2 : Make sure UnsubscribeFromNotifications() is used #

Patch Set 3 : Better implementation #

Patch Set 4 : Adding unit tests #

Patch Set 5 : Fixing tests #

Patch Set 6 : More unit tests #

Patch Set 7 : Adding #if !defined(OS_MACOSX) #

Patch Set 8 : Correctly adding #if !defined(OS_MACOSX) #

Patch Set 9 : Adding last_notify_value for macOS #

Messages