|
|
DescriptionBluetooth: macOS: Implement BluetoothRemoteGattCharacteristicMac::UnsubscribeFromNotifications
BUG=633191
Review-Url: https://codereview.chromium.org/2634873002
Cr-Commit-Position: refs/heads/master@{#450295}
Committed: https://chromium.googlesource.com/chromium/src/+/7c346d76dbf749c72ef9733e4befd2845fcf6935
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 #
Total comments: 30
Patch Set 6 : More unit tests #Patch Set 7 : Adding #if !defined(OS_MACOSX) #
Total comments: 4
Patch Set 8 : Correctly adding #if !defined(OS_MACOSX) #
Total comments: 2
Patch Set 9 : Adding last_notify_value for macOS #Dependent Patchsets: Messages
Total messages: 48 (28 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
jlebel@chromium.org changed reviewers: + fbeaufort@chromium.org, scheib@chromium.org
Hello Vincent, Can you review this patch? This is about the UnsubscribeFromNotifications implementation. Thanks,
LGTM
Description was changed from ========== Bluetooth: macOS: Implement BluetoothRemoteGattCharacteristicMac::UnsubscribeFromNotifications BUG=633191 ========== to ========== Bluetooth: macOS: Implement BluetoothRemoteGattCharacteristicMac::UnsubscribeFromNotifications Enabling unit tests for UnsubscribeFromNotifications() BUG=633191 ==========
jlebel@chromium.org changed reviewers: + ortuno@chromium.org
Hello Giovanni, Can you review this patch about UnsubscribeFromNotifications()? Thanks,
hmm how come we are not enabling the unit tests yet?
Description was changed from ========== Bluetooth: macOS: Implement BluetoothRemoteGattCharacteristicMac::UnsubscribeFromNotifications Enabling unit tests for UnsubscribeFromNotifications() BUG=633191 ========== to ========== Bluetooth: macOS: Implement BluetoothRemoteGattCharacteristicMac::UnsubscribeFromNotifications BUG=633191 ==========
On 2017/01/27 01:22:41, ortuno wrote: > hmm how come we are not enabling the unit tests yet? Sorry for that mistake. But talking about unittests, I didn't see any.
All tests that have StartNotifySession and StopNotifySession should pass with this patch.
The CQ bit was checked by jlebel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I updated the patch to include unit tests.
Hello Giovanni, I updated the patch to include unit tests. Jérôme,
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by jlebel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
You missed a couple of tests but we are so close! :) https://codereview.chromium.org/2634873002/diff/140001/device/bluetooth/bluet... File device/bluetooth/bluetooth_remote_gatt_characteristic_mac.mm (right): https://codereview.chromium.org/2634873002/diff/140001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_mac.mm:310: DCHECK(![GetCBCharacteristic() isNotifying]); I think you might need || error here. Otherwise if a stop notifications requests fails and you are still subscribed to notifications this DCHECK would fail. 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:1013: #if defined(OS_ANDROID) || defined(OS_WIN) Enable this test as well. https://codereview.chromium.org/2634873002/diff/140001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1035: #if defined(OS_ANDROID) || defined(OS_WIN) Enable this test as well. https://codereview.chromium.org/2634873002/diff/140001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1198: #if !defined(OS_MACOSX) You can remove this #if now. https://codereview.chromium.org/2634873002/diff/140001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1252: // Tests StartNotifySession completing before chrome objects are deleted. Also enable this test. https://codereview.chromium.org/2634873002/diff/140001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1408: #if defined(OS_ANDROID) Also enable this one https://codereview.chromium.org/2634873002/diff/140001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1432: #if defined(OS_ANDROID) Also enable this test. https://codereview.chromium.org/2634873002/diff/140001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1456: #if defined(OS_ANDROID) You forgot to enable this test. https://codereview.chromium.org/2634873002/diff/140001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1579: #if defined(OS_ANDROID) Can you mention why this is not enabled on macOS? e.g. // 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:1604: // Tests StopNotifySession success on a characteristic that enabled Notify & Can you mention why this is not enabled on macOS? e.g. // 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:1713: // TODO(624017) enable for macosx nit: remove the TODO. https://codereview.chromium.org/2634873002/diff/140001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1866: #if defined(OS_ANDROID) You are missing this one.
On 2017/02/09 00:34:48, ortuno wrote: > You missed a couple of tests but we are so close! :) > > https://codereview.chromium.org/2634873002/diff/140001/device/bluetooth/bluet... > File device/bluetooth/bluetooth_remote_gatt_characteristic_mac.mm (right): > > https://codereview.chromium.org/2634873002/diff/140001/device/bluetooth/bluet... > device/bluetooth/bluetooth_remote_gatt_characteristic_mac.mm:310: > DCHECK(![GetCBCharacteristic() isNotifying]); > I think you might need || error here. Otherwise if a stop notifications requests > fails and you are still subscribed to notifications this DCHECK would fail. > > 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:1013: #if > defined(OS_ANDROID) || defined(OS_WIN) > Enable this test as well. > > https://codereview.chromium.org/2634873002/diff/140001/device/bluetooth/bluet... > device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1035: #if > defined(OS_ANDROID) || defined(OS_WIN) > Enable this test as well. > > https://codereview.chromium.org/2634873002/diff/140001/device/bluetooth/bluet... > device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1198: #if > !defined(OS_MACOSX) > You can remove this #if now. > > https://codereview.chromium.org/2634873002/diff/140001/device/bluetooth/bluet... > device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1252: // Tests > StartNotifySession completing before chrome objects are deleted. > Also enable this test. > > https://codereview.chromium.org/2634873002/diff/140001/device/bluetooth/bluet... > device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1408: #if > defined(OS_ANDROID) > Also enable this one > > https://codereview.chromium.org/2634873002/diff/140001/device/bluetooth/bluet... > device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1432: #if > defined(OS_ANDROID) > Also enable this test. > > https://codereview.chromium.org/2634873002/diff/140001/device/bluetooth/bluet... > device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1456: #if > defined(OS_ANDROID) > You forgot to enable this test. > > https://codereview.chromium.org/2634873002/diff/140001/device/bluetooth/bluet... > device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1579: #if > defined(OS_ANDROID) > Can you mention why this is not enabled on macOS? e.g. > > // 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:1604: // Tests > StopNotifySession success on a characteristic that enabled Notify & > Can you mention why this is not enabled on macOS? e.g. > > // 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:1713: // > TODO(624017) enable for macosx > nit: remove the TODO. > > https://codereview.chromium.org/2634873002/diff/140001/device/bluetooth/bluet... > device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1866: #if > defined(OS_ANDROID) > You are missing this one. Sorry for the missing tests.
https://codereview.chromium.org/2634873002/diff/140001/device/bluetooth/bluet... File device/bluetooth/bluetooth_remote_gatt_characteristic_mac.mm (right): https://codereview.chromium.org/2634873002/diff/140001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_mac.mm:310: DCHECK(![GetCBCharacteristic() isNotifying]); On 2017/02/09 00:34:48, ortuno wrote: > I think you might need || error here. Otherwise if a stop notifications requests > fails and you are still subscribed to notifications this DCHECK would fail. Done. 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:1013: #if defined(OS_ANDROID) || defined(OS_WIN) On 2017/02/09 00:34:48, ortuno wrote: > Enable this test as well. Done. https://codereview.chromium.org/2634873002/diff/140001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1035: #if defined(OS_ANDROID) || defined(OS_WIN) On 2017/02/09 00:34:48, ortuno wrote: > Enable this test as well. Done. https://codereview.chromium.org/2634873002/diff/140001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1198: #if !defined(OS_MACOSX) On 2017/02/09 00:34:48, ortuno wrote: > You can remove this #if now. Done. https://codereview.chromium.org/2634873002/diff/140001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1252: // Tests StartNotifySession completing before chrome objects are deleted. On 2017/02/09 00:34:48, ortuno wrote: > Also enable this test. Done. 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 00:34:48, ortuno wrote: > Also enable this one I can't. Write is not implemented yet. 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 00:34:48, ortuno wrote: > Also enable this test. Same here. https://codereview.chromium.org/2634873002/diff/140001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1456: #if defined(OS_ANDROID) On 2017/02/09 00:34:48, ortuno wrote: > You forgot to enable this test. Done. https://codereview.chromium.org/2634873002/diff/140001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1579: #if defined(OS_ANDROID) On 2017/02/09 00:34:48, ortuno wrote: > Can you mention why this is not enabled on macOS? e.g. > > // 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:1604: // Tests StopNotifySession success on a characteristic that enabled Notify & On 2017/02/09 00:34:48, ortuno wrote: > Can you mention why this is not enabled on macOS? e.g. > > // 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:1713: // TODO(624017) enable for macosx On 2017/02/09 00:34:48, ortuno wrote: > nit: remove the TODO. 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 00:34:48, ortuno wrote: > You are missing this one. Write is still missing
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.
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.
The CQ bit was checked by jlebel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2634873002/diff/180001/device/bluetooth/bluet... File device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc (right): https://codereview.chromium.org/2634873002/diff/180001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1414: // macOS: Not applicable. CoreBluetooth exposes -[CBPeripheral Ah sorry to be confusing. I meant that only the part that checks for the descriptor should be surrounded by an #if defined. Similar to what we do here: https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_remote_gatt_c... https://codereview.chromium.org/2634873002/diff/180001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1433: EXPECT_EQ(2, gatt_write_descriptor_attempts_); Only these four statements should be surrounded by the #if.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #8 (id:200001) has been deleted
On 2017/02/09 22:46:28, ortuno wrote: > https://codereview.chromium.org/2634873002/diff/180001/device/bluetooth/bluet... > File device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc (right): > > https://codereview.chromium.org/2634873002/diff/180001/device/bluetooth/bluet... > device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1414: // > macOS: Not applicable. CoreBluetooth exposes -[CBPeripheral > Ah sorry to be confusing. I meant that only the part that checks for the > descriptor should be surrounded by an #if defined. Similar to what we do here: > https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_remote_gatt_c... > > https://codereview.chromium.org/2634873002/diff/180001/device/bluetooth/bluet... > device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1433: > EXPECT_EQ(2, gatt_write_descriptor_attempts_); > Only these four statements should be surrounded by the #if. I guess it should be good now?
https://codereview.chromium.org/2634873002/diff/220001/device/bluetooth/bluet... File device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc (right): https://codereview.chromium.org/2634873002/diff/220001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1447: EXPECT_EQ(2, gatt_notify_characteristic_attempts_); Great idea! Could you also do the "last_notify_value_" part in this patch? i.e.: EXPECT_TRUE(last_set_notify_value_);
The CQ bit was checked by jlebel@chromium.org to run a CQ dry run
https://codereview.chromium.org/2634873002/diff/180001/device/bluetooth/bluet... File device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc (right): https://codereview.chromium.org/2634873002/diff/180001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1414: // macOS: Not applicable. CoreBluetooth exposes -[CBPeripheral On 2017/02/09 22:46:27, ortuno wrote: > Ah sorry to be confusing. I meant that only the part that checks for the > descriptor should be surrounded by an #if defined. Similar to what we do here: > https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_remote_gatt_c... > Done. https://codereview.chromium.org/2634873002/diff/180001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1433: EXPECT_EQ(2, gatt_write_descriptor_attempts_); On 2017/02/09 22:46:27, ortuno wrote: > Only these four statements should be surrounded by the #if. Done. https://codereview.chromium.org/2634873002/diff/220001/device/bluetooth/bluet... File device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc (right): https://codereview.chromium.org/2634873002/diff/220001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1447: EXPECT_EQ(2, gatt_notify_characteristic_attempts_); On 2017/02/13 03:43:47, ortuno wrote: > Great idea! Could you also do the "last_notify_value_" part in this patch? i.e.: > > EXPECT_TRUE(last_set_notify_value_); Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM! :) :)
The CQ bit was checked by jlebel@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from scheib@chromium.org Link to the patchset: https://codereview.chromium.org/2634873002/#ps240001 (title: "Adding last_notify_value for macOS")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 240001, "attempt_start_ts": 1487062220568740, "parent_rev": "f0056e78a01fe12008eec015c18f28c766d3cf58", "commit_rev": "7c346d76dbf749c72ef9733e4befd2845fcf6935"}
Message was sent while issue was closed.
Description was changed from ========== Bluetooth: macOS: Implement BluetoothRemoteGattCharacteristicMac::UnsubscribeFromNotifications BUG=633191 ========== to ========== Bluetooth: macOS: Implement BluetoothRemoteGattCharacteristicMac::UnsubscribeFromNotifications BUG=633191 Review-Url: https://codereview.chromium.org/2634873002 Cr-Commit-Position: refs/heads/master@{#450295} Committed: https://chromium.googlesource.com/chromium/src/+/7c346d76dbf749c72ef9733e4bef... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:240001) as https://chromium.googlesource.com/chromium/src/+/7c346d76dbf749c72ef9733e4bef... |