Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2321693002:
arc/bluetooth: Save BluetoothGattConnection objects (Closed)
|
Created:
4 years, 3 months ago by Eric Caruso Modified:
4 years, 3 months ago CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, hidehiko+watch_chromium.org, lhchavez+watch_chromium.org, yusukes+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionarc/bluetooth: Save BluetoothGattConnection objects
When we make outbound GATT connections, we were immediately
letting them expire in OnGattConnected, which meant we would
disconnect remote devices as soon as they were connected to us.
BUG=644792
TEST=make GATT connections from inside the container and
verify that they stay connected, disconnect and verify that
the device is disconnected
Committed: https://crrev.com/fcca36a57736c9f17c30607904a4e73b95461744
Cr-Commit-Position: refs/heads/master@{#417097}
Patch Set 1 #
Total comments: 3
Patch Set 2 : lhchavez@ nits #
Total comments: 4
Patch Set 3 : replacing emplace with operator[] to make builder happy #Patch Set 4 : steel@ nits #
Messages
Total messages: 25 (12 generated)
Description was changed from ========== arc/bluetooth: Save BluetoothGattConnection objects When we make outbound GATT connections, we were immediately letting them expire in OnGattConnected, which meant we would disconnect remote devices as soon as they were connected to us. BUG=644792 TEST=make GATT connections from inside the container and verify that they stay connected, disconnect and verify that the device is disconnected ========== to ========== arc/bluetooth: Save BluetoothGattConnection objects When we make outbound GATT connections, we were immediately letting them expire in OnGattConnected, which meant we would disconnect remote devices as soon as they were connected to us. BUG=644792 TEST=make GATT connections from inside the container and verify that they stay connected, disconnect and verify that the device is disconnected ==========
ejcaruso@chromium.org changed reviewers: + lhchavez@chromium.org, steel@chromium.org
PTAL, thanks.
lgtm with nits https://codereview.chromium.org/2321693002/diff/1/components/arc/bluetooth/ar... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2321693002/diff/1/components/arc/bluetooth/ar... components/arc/bluetooth/arc_bluetooth_bridge.cc:947: gatt_connections_.emplace(addr->To<std::string>(), std::move(connection)); nit: DCHECK(CalledOnValidThread()); https://codereview.chromium.org/2321693002/diff/1/components/arc/bluetooth/ar... components/arc/bluetooth/arc_bluetooth_bridge.cc:959: auto it = gatt_connections_.find(addr->To<std::string>()); nit: DCHECK(CalledOnValidThread()); https://codereview.chromium.org/2321693002/diff/1/components/arc/bluetooth/ar... File components/arc/bluetooth/arc_bluetooth_bridge.h (right): https://codereview.chromium.org/2321693002/diff/1/components/arc/bluetooth/ar... components/arc/bluetooth/arc_bluetooth_bridge.h:392: gatt_connections_; nit: Add a comment that we are holding these just to avoid immediate expiration.
Thanks!
The CQ bit was checked by ejcaruso@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lhchavez@chromium.org Link to the patchset: https://codereview.chromium.org/2321693002/#ps20001 (title: "lhchavez@ nits")
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
Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by ejcaruso@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lhchavez@chromium.org Link to the patchset: https://codereview.chromium.org/2321693002/#ps40001 (title: "replacing emplace with operator[] to make builder happy")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2321693002/diff/20001/components/arc/bluetoot... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2321693002/diff/20001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:948: gatt_connections_.emplace(addr->To<std::string>(), std::move(connection)); can't use map::emplace https://codereview.chromium.org/2321693002/diff/20001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:948: gatt_connections_.emplace(addr->To<std::string>(), std::move(connection)); Also, don't expect to "not" have the address in this map. It is possible to not get a disconnected event and again get a connnected event. https://codereview.chromium.org/2321693002/diff/20001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:962: if (it == gatt_connections_.end()) nit: {} around multiline
The CQ bit was unchecked by ejcaruso@chromium.org
https://codereview.chromium.org/2321693002/diff/20001/components/arc/bluetoot... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2321693002/diff/20001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:948: gatt_connections_.emplace(addr->To<std::string>(), std::move(connection)); On 2016/09/07 21:31:39, Rahul Chaturvedi wrote: > can't use map::emplace I switched this out for operator[] which will cause the old std::unique_ptr to be destroyed if this happens.
Thanks.
The CQ bit was checked by ejcaruso@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lhchavez@chromium.org Link to the patchset: https://codereview.chromium.org/2321693002/#ps60001 (title: "steel@ nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
bt lgtm
Message was sent while issue was closed.
Description was changed from ========== arc/bluetooth: Save BluetoothGattConnection objects When we make outbound GATT connections, we were immediately letting them expire in OnGattConnected, which meant we would disconnect remote devices as soon as they were connected to us. BUG=644792 TEST=make GATT connections from inside the container and verify that they stay connected, disconnect and verify that the device is disconnected ========== to ========== arc/bluetooth: Save BluetoothGattConnection objects When we make outbound GATT connections, we were immediately letting them expire in OnGattConnected, which meant we would disconnect remote devices as soon as they were connected to us. BUG=644792 TEST=make GATT connections from inside the container and verify that they stay connected, disconnect and verify that the device is disconnected ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== arc/bluetooth: Save BluetoothGattConnection objects When we make outbound GATT connections, we were immediately letting them expire in OnGattConnected, which meant we would disconnect remote devices as soon as they were connected to us. BUG=644792 TEST=make GATT connections from inside the container and verify that they stay connected, disconnect and verify that the device is disconnected ========== to ========== arc/bluetooth: Save BluetoothGattConnection objects When we make outbound GATT connections, we were immediately letting them expire in OnGattConnected, which meant we would disconnect remote devices as soon as they were connected to us. BUG=644792 TEST=make GATT connections from inside the container and verify that they stay connected, disconnect and verify that the device is disconnected Committed: https://crrev.com/fcca36a57736c9f17c30607904a4e73b95461744 Cr-Commit-Position: refs/heads/master@{#417097} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/fcca36a57736c9f17c30607904a4e73b95461744 Cr-Commit-Position: refs/heads/master@{#417097} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
