Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2189183002:
arc: bluetooth: Always acquire a NotifySession (Closed)
|
Created:
4 years, 4 months ago by puthik_chromium Modified:
4 years, 4 months ago CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, hidehiko+watch_chromium.org, lhchavez+watch_chromium.org, rkc, yusukes+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionarc: bluetooth: Always acquire a NotifySession
If Arc++ needs to keep receiving notifications it needs to
unconditionally acquire a NotifySession. The Bluetooth API
uses a ref-counting model to write to the descriptor. If
other clients drop their NotifySessions, notifications will stop.
BUG=632476
TEST=Build
Committed: https://crrev.com/3a796de6b43352fb756aa1c9c24a8bc5fc481553
Cr-Commit-Position: refs/heads/master@{#408692}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Remove unneed code #Patch Set 3 : Remove IsNotifying since it's going to be private #Messages
Total messages: 23 (11 generated)
The CQ bit was checked by puthik@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...
puthik@chromium.org changed reviewers: + ortuno@chromium.org
FYI rkc@
https://codereview.chromium.org/2189183002/diff/1/components/arc/bluetooth/ar... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2189183002/diff/1/components/arc/bluetooth/ar... components/arc/bluetooth/arc_bluetooth_bridge.cc:1128: LOG(WARNING) << __func__ << ("Notification is stopped"); I don't think this solves the problem. You are still not acquiring a NotifySession if any other clients e.g. a website, have a NotifySession. Once other clients drops their sessions notifications will stop.
https://codereview.chromium.org/2189183002/diff/1/components/arc/bluetooth/ar... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2189183002/diff/1/components/arc/bluetooth/ar... components/arc/bluetooth/arc_bluetooth_bridge.cc:1102: characteristic->StartNotifySession( The notifying session is always acquire here. https://codereview.chromium.org/2189183002/diff/1/components/arc/bluetooth/ar... components/arc/bluetooth/arc_bluetooth_bridge.cc:1128: LOG(WARNING) << __func__ << ("Notification is stopped"); On 2016/07/28 22:21:06, ortuno wrote: > I don't think this solves the problem. You are still not acquiring a > NotifySession if any other clients e.g. a website, have a NotifySession. Once > other clients drops their sessions notifications will stop. This is the Deregister function. I think delete the if (characteristic->IsNotifying()) in Register function above should solve the problem.
https://codereview.chromium.org/2189183002/diff/1/components/arc/bluetooth/ar... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2189183002/diff/1/components/arc/bluetooth/ar... components/arc/bluetooth/arc_bluetooth_bridge.cc:1126: if (!characteristic->IsNotifying()) { IsNotifying() will always be true as long as there is a client holding a NotifySession. If arc has a notify session then this will always be true. The callback for Stop will always be called so you can just call stop on your notify session. https://codereview.chromium.org/2189183002/diff/1/components/arc/bluetooth/ar... components/arc/bluetooth/arc_bluetooth_bridge.cc:1128: LOG(WARNING) << __func__ << ("Notification is stopped"); On 2016/07/28 at 22:40:31, puthik_chromium wrote: > On 2016/07/28 22:21:06, ortuno wrote: > > I don't think this solves the problem. You are still not acquiring a > > NotifySession if any other clients e.g. a website, have a NotifySession. Once > > other clients drops their sessions notifications will stop. > > This is the Deregister function. I think delete the if (characteristic->IsNotifying()) in Register function above should solve the problem. Ah my bad. You are right.
The CQ bit was checked by puthik@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 checked by puthik@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...
Will add lhchavez owner reviewer after get bluetooth l-g-t-m https://codereview.chromium.org/2189183002/diff/1/components/arc/bluetooth/ar... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2189183002/diff/1/components/arc/bluetooth/ar... components/arc/bluetooth/arc_bluetooth_bridge.cc:1126: if (!characteristic->IsNotifying()) { On 2016/07/28 22:56:00, ortuno wrote: > IsNotifying() will always be true as long as there is a client holding a > NotifySession. If arc has a notify session then this will always be true. The > callback for Stop will always be called so you can just call stop on your notify > session. Since this is always true. Changed to DCHECK.
lgtm bluetooth usage
puthik@chromium.org changed reviewers: + lhchavez@chromium.org
+lhchavez
rs-lgtm
The CQ bit was checked by puthik@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ortuno@chromium.org Link to the patchset: https://codereview.chromium.org/2189183002/#ps40001 (title: "Remove IsNotifying since it's going to be private")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== arc: bluetooth: Always acquire a NotifySession If Arc++ needs to keep receiving notifications it needs to unconditionally acquire a NotifySession. The Bluetooth API uses a ref-counting model to write to the descriptor. If other clients drop their NotifySessions, notifications will stop. BUG=632476 TEST=Build ========== to ========== arc: bluetooth: Always acquire a NotifySession If Arc++ needs to keep receiving notifications it needs to unconditionally acquire a NotifySession. The Bluetooth API uses a ref-counting model to write to the descriptor. If other clients drop their NotifySessions, notifications will stop. BUG=632476 TEST=Build Committed: https://crrev.com/3a796de6b43352fb756aa1c9c24a8bc5fc481553 Cr-Commit-Position: refs/heads/master@{#408692} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/3a796de6b43352fb756aa1c9c24a8bc5fc481553 Cr-Commit-Position: refs/heads/master@{#408692} |
