Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2223203002:
arc: bluetooth: Add/Remove BT observer only when ARC is ready (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: Add/Remove BT observer only when ARC is ready
The current implementation make arc_bluetooth_bridge to be
a bluetooth observer all the time. This lead to unnessary
work when ARC is not ready.
This patch changes that by register arc_bluetooth_bridge
to be a bluetooth observer when arc is ready and unregister
when arc is gone
BUG=635578
TEST=No log spam when there is no arc support
Committed: https://crrev.com/1ffc3026c8758d9464e76d8ea0fd0c542906e4cb
Cr-Commit-Position: refs/heads/master@{#410828}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Add more check #
Total comments: 7
Patch Set 3 : Fix #2 comment #
Total comments: 9
Patch Set 4 : Fix comment #Patch Set 5 : Add HasObserver #
Total comments: 6
Patch Set 6 : Fix nit in last patch #
Messages
Total messages: 27 (8 generated)
puthik@chromium.org changed reviewers: + ortuno@chromium.org
+ortuno for BT review (discussed offline) fyi: rkc
rkc@chromium.org changed reviewers: + rkc@chromium.org
https://codereview.chromium.org/2223203002/diff/1/components/arc/bluetooth/ar... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2223203002/diff/1/components/arc/bluetooth/ar... components/arc/bluetooth/arc_bluetooth_bridge.cc:216: VLOG(1) << "registering bluetooth adapter"; VLOG(1) << "Registering Bluetooth adapter."; https://codereview.chromium.org/2223203002/diff/1/components/arc/bluetooth/ar... components/arc/bluetooth/arc_bluetooth_bridge.cc:220: VLOG(1) << "no bluetooth adapter available"; VLOG(1) << "No Bluetooth adapter available."; https://codereview.chromium.org/2223203002/diff/1/components/arc/bluetooth/ar... components/arc/bluetooth/arc_bluetooth_bridge.cc:226: bluetooth_adapter_->RemoveObserver(this); You don't need the bluetooth_observer_flag_ flag. If there is no observer registered, RemoveObserver will do nothing.
lhchavez@chromium.org changed reviewers: + lhchavez@chromium.org
https://codereview.chromium.org/2223203002/diff/20001/components/arc/bluetoot... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2223203002/diff/20001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:211: // We should register ourself to bluetooth observer here. nit: s/ourself/ourselves/. Same below. https://codereview.chromium.org/2223203002/diff/20001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:237: if (bluetooth_observer_flag_ && bluetooth_adapter_) If you remove the flag like rkc@ suggested, maybe do bluetooth_adapter_.reset() here.
https://codereview.chromium.org/2223203002/diff/20001/components/arc/bluetoot... File components/arc/bluetooth/arc_bluetooth_bridge.cc (left): https://codereview.chromium.org/2223203002/diff/20001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:221: bluetooth_instance->Init(binding_.CreateInterfacePtrAndBind()); Where are we doing this now? https://codereview.chromium.org/2223203002/diff/20001/components/arc/bluetoot... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2223203002/diff/20001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:237: if (bluetooth_observer_flag_ && bluetooth_adapter_) On 2016/08/08 21:12:03, Luis Héctor Chávez wrote: > If you remove the flag like rkc@ suggested, maybe do bluetooth_adapter_.reset() > here. +1
https://codereview.chromium.org/2223203002/diff/1/components/arc/bluetooth/ar... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2223203002/diff/1/components/arc/bluetooth/ar... components/arc/bluetooth/arc_bluetooth_bridge.cc:216: VLOG(1) << "registering bluetooth adapter"; On 2016/08/08 21:08:12, rkc wrote: > VLOG(1) << "Registering Bluetooth adapter."; Done. https://codereview.chromium.org/2223203002/diff/1/components/arc/bluetooth/ar... components/arc/bluetooth/arc_bluetooth_bridge.cc:220: VLOG(1) << "no bluetooth adapter available"; On 2016/08/08 21:08:12, rkc wrote: > VLOG(1) << "No Bluetooth adapter available."; Done. https://codereview.chromium.org/2223203002/diff/1/components/arc/bluetooth/ar... components/arc/bluetooth/arc_bluetooth_bridge.cc:226: bluetooth_adapter_->RemoveObserver(this); On 2016/08/08 21:08:12, rkc wrote: > You don't need the bluetooth_observer_flag_ flag. If there is no observer > registered, RemoveObserver will do nothing. The flag is used to see that we should register observer when bluetooth_adapter_ is ready or when arc_instance is ready. https://codereview.chromium.org/2223203002/diff/20001/components/arc/bluetoot... File components/arc/bluetooth/arc_bluetooth_bridge.cc (left): https://codereview.chromium.org/2223203002/diff/20001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:221: bluetooth_instance->Init(binding_.CreateInterfacePtrAndBind()); On 2016/08/08 21:16:15, rkc wrote: > Where are we doing this now? My error. Fixed https://codereview.chromium.org/2223203002/diff/20001/components/arc/bluetoot... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2223203002/diff/20001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:211: // We should register ourself to bluetooth observer here. On 2016/08/08 21:12:03, Luis Héctor Chávez wrote: > nit: s/ourself/ourselves/. Same below. Done. https://codereview.chromium.org/2223203002/diff/20001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:237: if (bluetooth_observer_flag_ && bluetooth_adapter_) On 2016/08/08 21:16:15, rkc wrote: > On 2016/08/08 21:12:03, Luis Héctor Chávez wrote: > > If you remove the flag like rkc@ suggested, maybe do > bluetooth_adapter_.reset() > > here. > > +1 I changed to code to hold make bluetooth_adapter_ a valid pointer all the time. Or else we need to add null check for bluetooth_adapter_ to every function.
https://codereview.chromium.org/2223203002/diff/40001/components/arc/bluetoot... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2223203002/diff/40001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:212: if (!bluetooth_observer_flag_ && s/bluetooth_observer_flag_/bluetooth_adapter_->HasObserver(this)/ across all the file.
https://codereview.chromium.org/2223203002/diff/40001/components/arc/bluetoot... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2223203002/diff/40001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:182: if (BluetoothAdapterFactory::IsBluetoothAdapterAvailable()) { This is a useless check. This only checks if Bluetooth is available on this 'platform'. We won't be running ARC++ ever on a platform that doesn't support Bluetooth (currently only IOS?). https://codereview.chromium.org/2223203002/diff/40001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:231: // We should register ourselves to bluetooth observer here. This comment isn't clear. Instead, it should read something like, The Bluetooth adapter was ready before the ARC instance, hence we didn't register ourselves as an observer with it then. Since our instance is ready, we should register it now. https://codereview.chromium.org/2223203002/diff/40001/components/arc/bluetoot... File components/arc/bluetooth/arc_bluetooth_bridge.h (right): https://codereview.chromium.org/2223203002/diff/40001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.h:368: bool bluetooth_observer_flag_ = false; The correct solution is just to add a HasObserver method to BluetoothAdapter. Maintaining a flag for this shouldn't be done here.
https://codereview.chromium.org/2223203002/diff/40001/components/arc/bluetoot... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2223203002/diff/40001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:182: if (BluetoothAdapterFactory::IsBluetoothAdapterAvailable()) { On 2016/08/08 21:58:29, rkc wrote: > This is a useless check. This only checks if Bluetooth is available on this > 'platform'. We won't be running ARC++ ever on a platform that doesn't support > Bluetooth (currently only IOS?). It's out of scope of this CL so I will leave it here. https://codereview.chromium.org/2223203002/diff/40001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:212: if (!bluetooth_observer_flag_ && On 2016/08/08 21:54:16, Luis Héctor Chávez wrote: > s/bluetooth_observer_flag_/bluetooth_adapter_->HasObserver(this)/ > > across all the file. There is no HasObserver(this) for bluetooth_adapter_. https://codereview.chromium.org/2223203002/diff/40001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:231: // We should register ourselves to bluetooth observer here. On 2016/08/08 21:58:29, rkc wrote: > This comment isn't clear. Instead, it should read something like, > > The Bluetooth adapter was ready before the ARC instance, hence we didn't > register ourselves as an observer with it then. Since our instance is ready, we > should register it now. Done.
https://codereview.chromium.org/2223203002/diff/40001/components/arc/bluetoot... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2223203002/diff/40001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:212: if (!bluetooth_observer_flag_ && On 2016/08/08 22:33:32, puthik_chromium wrote: > On 2016/08/08 21:54:16, Luis Héctor Chávez wrote: > > s/bluetooth_observer_flag_/bluetooth_adapter_->HasObserver(this)/ > > > > across all the file. > > There is no HasObserver(this) for bluetooth_adapter_. Yes, it is trivial to add it though.
https://codereview.chromium.org/2223203002/diff/40001/components/arc/bluetoot... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2223203002/diff/40001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:212: if (!bluetooth_observer_flag_ && On 2016/08/08 22:36:32, rkc wrote: > On 2016/08/08 22:33:32, puthik_chromium wrote: > > On 2016/08/08 21:54:16, Luis Héctor Chávez wrote: > > > s/bluetooth_observer_flag_/bluetooth_adapter_->HasObserver(this)/ > > > > > > across all the file. > > > > There is no HasObserver(this) for bluetooth_adapter_. > > Yes, it is trivial to add it though. Done.
device/bluetooth rs lgtm, didn't look at the rest.
lgtm % comments https://codereview.chromium.org/2223203002/diff/80001/components/arc/bluetoot... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2223203002/diff/80001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:232: if (!bluetooth_adapter_->HasObserver(this) && bluetooth_adapter_) bluetooth_adapter_ && !bluetooth_adapter_->HasObserver(this) That way in case the adapter pointer is not set, we won't try to dereference it. https://codereview.chromium.org/2223203002/diff/80001/device/bluetooth/blueto... File device/bluetooth/bluetooth_adapter.cc (right): https://codereview.chromium.org/2223203002/diff/80001/device/bluetooth/blueto... device/bluetooth/bluetooth_adapter.cc:59: return observers_.HasObserver(observer); Before this line, DCHECK(observer);
lgtm with nit https://codereview.chromium.org/2223203002/diff/80001/components/arc/bluetoot... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2223203002/diff/80001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:215: bluetooth_adapter_->AddObserver(this); nit: This line needs braces due to the if not being in one line.
https://codereview.chromium.org/2223203002/diff/80001/components/arc/bluetoot... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2223203002/diff/80001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:215: bluetooth_adapter_->AddObserver(this); On 2016/08/08 23:59:20, Luis Héctor Chávez wrote: > nit: This line needs braces due to the if not being in one line. Done. https://codereview.chromium.org/2223203002/diff/80001/components/arc/bluetoot... components/arc/bluetooth/arc_bluetooth_bridge.cc:232: if (!bluetooth_adapter_->HasObserver(this) && bluetooth_adapter_) On 2016/08/08 23:53:33, rkc wrote: > bluetooth_adapter_ && !bluetooth_adapter_->HasObserver(this) > That way in case the adapter pointer is not set, we won't try to dereference it. Done. https://codereview.chromium.org/2223203002/diff/80001/device/bluetooth/blueto... File device/bluetooth/bluetooth_adapter.cc (right): https://codereview.chromium.org/2223203002/diff/80001/device/bluetooth/blueto... device/bluetooth/bluetooth_adapter.cc:59: return observers_.HasObserver(observer); On 2016/08/08 23:53:33, rkc wrote: > Before this line, > DCHECK(observer); Added. Actually, It would just return false if observer is null anyway.
The CQ bit was checked by puthik@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rkc@chromium.org, ortuno@chromium.org, lhchavez@chromium.org Link to the patchset: https://codereview.chromium.org/2223203002/#ps100001 (title: "Fix nit in last patch")
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: 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 puthik@chromium.org
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 #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== arc: bluetooth: Add/Remove BT observer only when ARC is ready The current implementation make arc_bluetooth_bridge to be a bluetooth observer all the time. This lead to unnessary work when ARC is not ready. This patch changes that by register arc_bluetooth_bridge to be a bluetooth observer when arc is ready and unregister when arc is gone BUG=635578 TEST=No log spam when there is no arc support ========== to ========== arc: bluetooth: Add/Remove BT observer only when ARC is ready The current implementation make arc_bluetooth_bridge to be a bluetooth observer all the time. This lead to unnessary work when ARC is not ready. This patch changes that by register arc_bluetooth_bridge to be a bluetooth observer when arc is ready and unregister when arc is gone BUG=635578 TEST=No log spam when there is no arc support Committed: https://crrev.com/1ffc3026c8758d9464e76d8ea0fd0c542906e4cb Cr-Commit-Position: refs/heads/master@{#410828} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/1ffc3026c8758d9464e76d8ea0fd0c542906e4cb Cr-Commit-Position: refs/heads/master@{#410828} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
