Chromium Code Reviews
Help | Chromium Project | Sign in
(5)

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 months, 1 week ago by jlebel
Modified:
2 months, 1 week ago
CC:
chromium-reviews, mac-reviews_chromium.org, ortuno+watch_chromium.org, scheib+watch_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

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/+/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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+291 lines, -77 lines) Patch
M device/bluetooth/bluetooth_remote_gatt_characteristic.cc View 1 2 2 chunks +3 lines, -4 lines 0 comments Download
M device/bluetooth/bluetooth_remote_gatt_characteristic_mac.h View 1 2 3 1 chunk +4 lines, -1 line 0 comments Download
M device/bluetooth/bluetooth_remote_gatt_characteristic_mac.mm View 1 2 3 4 5 4 chunks +23 lines, -9 lines 0 comments Download
M device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc View 1 2 3 4 5 6 7 8 24 chunks +212 lines, -60 lines 0 comments Download
M device/bluetooth/test/bluetooth_test_mac.h View 1 2 3 4 5 6 7 8 3 chunks +9 lines, -1 line 0 comments Download
M device/bluetooth/test/bluetooth_test_mac.mm View 1 2 3 4 5 6 7 8 2 chunks +19 lines, -1 line 0 comments Download
M device/bluetooth/test/mock_bluetooth_cbcharacteristic_mac.h View 1 2 3 1 chunk +2 lines, -0 lines 0 comments Download
M device/bluetooth/test/mock_bluetooth_cbcharacteristic_mac.mm View 1 2 3 1 chunk +17 lines, -0 lines 0 comments Download
M device/bluetooth/test/mock_bluetooth_cbperipheral_mac.mm View 1 2 3 4 5 6 7 8 1 chunk +2 lines, -1 line 0 comments Download
Commit queue not available (can’t edit this change).

Dependent Patchsets:

Messages

Total messages: 48 (28 generated)
jlebel
Hello Vincent, Can you review this patch? This is about the UnsubscribeFromNotifications implementation. Thanks,
3 months, 1 week ago (2017-01-16 10:57:05 UTC) #5
scheib
LGTM
3 months, 1 week ago (2017-01-17 18:35:18 UTC) #6
jlebel
Hello Giovanni, Can you review this patch about UnsubscribeFromNotifications()? Thanks,
3 months ago (2017-01-26 23:29:35 UTC) #9
ortuno
hmm how come we are not enabling the unit tests yet?
3 months ago (2017-01-27 01:22:41 UTC) #10
jlebel
On 2017/01/27 01:22:41, ortuno wrote: > hmm how come we are not enabling the unit ...
3 months ago (2017-01-27 09:50:11 UTC) #12
ortuno
All tests that have StartNotifySession and StopNotifySession should pass with this patch.
2 months, 4 weeks ago (2017-01-27 20:40:23 UTC) #13
jlebel
I updated the patch to include unit tests.
2 months, 2 weeks ago (2017-02-08 20:49:50 UTC) #16
jlebel
Hello Giovanni, I updated the patch to include unit tests. Jérôme,
2 months, 2 weeks ago (2017-02-08 20:50:30 UTC) #17
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 ...
2 months, 2 weeks 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 ...
2 months, 2 weeks 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 ...
2 months, 2 weeks 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 ...
2 months, 2 weeks ago (2017-02-09 03:07:03 UTC) #27
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 ...
2 months, 2 weeks ago (2017-02-09 21:17:38 UTC) #28
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 ...
2 months, 2 weeks ago (2017-02-09 22:46:28 UTC) #31
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 > ...
2 months, 1 week 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 ...
2 months, 1 week ago (2017-02-13 03:43:48 UTC) #36
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 ...
2 months, 1 week ago (2017-02-13 19:22:22 UTC) #38
ortuno
LGTM! :) :)
2 months, 1 week ago (2017-02-14 01:47:39 UTC) #42
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
2 months, 1 week ago (2017-02-14 08:50:36 UTC) #45
commit-bot: I haz the power
2 months, 1 week 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...
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld cc6ac46