Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2599303002:
Bluetooth: macOS: BluetoothRemoteGattCharacteristicMac::SubscribeToNotifications (Closed)
DescriptionBluetooth: 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 Set 1 #Patch Set 2 : Update #Patch Set 3 : Fix from 2595373003 #Patch Set 4 : Bluetooth: macOS: Replacing BluetoothRemoteGattCharacteristicMac::StartNotifySession() by Bluetooth… #
Total comments: 9
Messages
Total messages: 27 (20 generated)
Description was changed from ========== Bluetooth: macOS: Implement BluetoothRemoteGattCharacteristicMac::SubscribeToNotifications and UnsubscribeFromNotifications. Also remove the override of StartNotifySession so that the platform-independent version is used. BUG=633191 ========== to ========== Bluetooth: macOS: BluetoothRemoteGattCharacteristicMac::SubscribeToNotifications. BUG=633191 ==========
Patchset #3 (id:40001) has been deleted
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:60001) has been deleted
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: 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_...)
Patchset #2 (id:80001) has been deleted
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.
Description was changed from ========== Bluetooth: macOS: BluetoothRemoteGattCharacteristicMac::SubscribeToNotifications. BUG=633191 ========== to ========== Bluetooth: macOS: Replacing BluetoothRemoteGattCharacteristicMac::StartNotifySession() by BluetoothRemoteGattCharacteristicMac::SubscribeToNotifications(). BUG=633191 ==========
jlebel@chromium.org changed reviewers: + fbeaufort@chromium.org, scheib@chromium.org
Hello Vincent, Can you review my patch? Thanks,
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/2599303002/#ps120001 (title: "Fix from 2595373003")
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": 120001, "attempt_start_ts": 1484517530431340,
"parent_rev": "266a86c94d34f3c7c3d22ee00624636b801f80d3", "commit_rev":
"230ba260170c6d7ab48a72a5f0eea01be95740f3"}
Message was sent while issue was closed.
Description was changed from ========== Bluetooth: macOS: Replacing BluetoothRemoteGattCharacteristicMac::StartNotifySession() by BluetoothRemoteGattCharacteristicMac::SubscribeToNotifications(). BUG=633191 ========== 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... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:120001) as https://chromium.googlesource.com/chromium/src/+/230ba260170c6d7ab48a72a5f0ee...
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:120001) has been created in https://codereview.chromium.org/2638753002/ by pkalinnikov@chromium.org. The reason for reverting is: 6 BluetoothRemoteGattCharacteristicTest's, e.g., BluetoothRemoteGattCharacteristicTest.GattCharacteristicValueChanged, are getting time out on "Mac10.10 Tests" and "Mac10.11 Tests" builders..
Message was sent while issue was closed.
ortuno@chromium.org changed reviewers: + ortuno@chromium.org
Message was sent while issue was closed.
I think we can simplify things a bit. Left some comments. https://codereview.chromium.org/2599303002/diff/140001/device/bluetooth/bluet... File device/bluetooth/bluetooth_remote_gatt_characteristic_mac.h (right): https://codereview.chromium.org/2599303002/diff/140001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_mac.h:132: std::unique_ptr<BluetoothRemoteGattDescriptorMac>> I don't think we need a vector anymore. The StartNotifySession code should make sure that there is only a single call to start notifications. A single PendingStartNotifyCall should be enough. https://codereview.chromium.org/2599303002/diff/140001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_mac.h:134: base::WeakPtrFactory<BluetoothRemoteGattCharacteristicMac> weak_ptr_factory_; No need for this anymore. StartNotifySession will make sure there is only one call to start notifications at the same time. https://codereview.chromium.org/2599303002/diff/140001/device/bluetooth/bluet... File device/bluetooth/bluetooth_remote_gatt_characteristic_mac.mm (right): https://codereview.chromium.org/2599303002/diff/140001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_mac.mm:215: if (IsNotifying()) { No need for this check. This is already handled by the StartNotifySession code: https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_remote_gatt_c... https://codereview.chromium.org/2599303002/diff/140001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_mac.mm:223: if (!SupportsNotificationsOrIndications()) { Same here this is already handled by StartNotifySession: https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_remote_gatt_c... https://codereview.chromium.org/2599303002/diff/140001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_mac.mm:231: start_notify_session_callbacks_.push_back( We should just DCHECK that the pair is empty.
Message was sent while issue was closed.
Fix in crrev.com/2632253003 https://codereview.chromium.org/2599303002/diff/140001/device/bluetooth/bluet... File device/bluetooth/bluetooth_remote_gatt_characteristic_mac.h (right): https://codereview.chromium.org/2599303002/diff/140001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_mac.h:132: std::unique_ptr<BluetoothRemoteGattDescriptorMac>> On 2017/01/16 22:56:26, ortuno (OOO until Jan 16th) wrote: > I don't think we need a vector anymore. The StartNotifySession code should make > sure that there is only a single call to start notifications. A single > PendingStartNotifyCall should be enough. Done. https://codereview.chromium.org/2599303002/diff/140001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_mac.h:134: base::WeakPtrFactory<BluetoothRemoteGattCharacteristicMac> weak_ptr_factory_; On 2017/01/16 22:56:26, ortuno (OOO until Jan 16th) wrote: > No need for this anymore. StartNotifySession will make sure there is only one > call to start notifications at the same time. Done. https://codereview.chromium.org/2599303002/diff/140001/device/bluetooth/bluet... File device/bluetooth/bluetooth_remote_gatt_characteristic_mac.mm (right): https://codereview.chromium.org/2599303002/diff/140001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_mac.mm:215: if (IsNotifying()) { On 2017/01/16 22:56:26, ortuno (OOO until Jan 16th) wrote: > No need for this check. This is already handled by the StartNotifySession code: > > https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_remote_gatt_c... Done. https://codereview.chromium.org/2599303002/diff/140001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_mac.mm:223: if (!SupportsNotificationsOrIndications()) { On 2017/01/16 22:56:26, ortuno (OOO until Jan 16th) wrote: > Same here this is already handled by StartNotifySession: > > https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_remote_gatt_c... Done. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
