Delayed adding BluetoothEventRouter as BluetoothAdapter observer until when the

Delayed adding BluetoothEventRouter as BluetoothAdapter observer until when the
bluetooth API calls and events are actually made.

The background of this change is related to the windows implementation of BluetoothAdapter (https://chromiumcodereview.appspot.com/11345037/) where we decided to use polling approach to detect the bluetooth adapter state. In order to limit the polling, we'd like to poll when there are observers to bluetooth adapter, and that's why we'd like to delay registering observers until it's necessary.


BUG=135470

[youngki, 2012-11-02 19:54:24]

Scott, could you take a look at this CL before I show it to @miket, the owner of
chrome/browser/extensions/ ?

[keybuk, 2012-11-05 17:51:38]

could you have bryeung (the author of this code) look at it instead?

[youngki, 2012-11-05 18:28:29]

I cc'd Bryan on this review. Hopefully he can catch this review after he comes
back from the vacation.

Mike, I looked into notification framework as we discussed over the other email
thread, and unfortunately all the notification frameworks I investigated were
not suitable for detecting windows bluetooth state. I investigated
network_change_notifier and talked with its owner and decided this should not be
used for detecting bluetooth state. Also after investigating Windows SDK and
talking with @girard (local Windows expert in wat office), Windows notification
framework needs a dedicated thread to listen to the state.

So after discussion with Bryan we decided to go with polling approach, but we
don't want to poll when bluetooth isn't necessary. So we are trying to limit
polling by adding bluetooth observer until it's necessary. BluetoothAdapterWin
will only poll when there are any observers.

Could you take a look at this CL? I added bluetooth_event_router_unittest just
to test the functionalities I added. I am not sure if any other unittest is
needed, but let me know if I should add more testing.

[miket_OOO, 2012-11-06 02:04:09]

LGTM with comments.

http://codereview.chromium.org/11369055/diff/1007/chrome/browser/extensions/b...
File chrome/browser/extensions/bluetooth_event_router.cc (right):

http://codereview.chromium.org/11369055/diff/1007/chrome/browser/extensions/b...
chrome/browser/extensions/bluetooth_event_router.cc:53:
adapter_->AddObserver(this);
Braces not needed.

I might answer this question for myself later in the review, but is it OK to
repeatedly add the observer if the caller calls GetMutableAdapter more than
once?

http://codereview.chromium.org/11369055/diff/1007/chrome/browser/extensions/b...
chrome/browser/extensions/bluetooth_event_router.cc:81: const std::string&
event_name) {
this method could be const.

http://codereview.chromium.org/11369055/diff/1007/chrome/browser/extensions/b...
chrome/browser/extensions/bluetooth_event_router.cc:91:
adapter_->AddObserver(this);
Would it be correct, or more elegant, to call GetMutableAdapter() here instead
of doing the get() check & AddObserver()?

http://codereview.chromium.org/11369055/diff/1007/chrome/browser/extensions/b...
File chrome/browser/extensions/bluetooth_event_router_unittest.cc (right):

http://codereview.chromium.org/11369055/diff/1007/chrome/browser/extensions/b...
chrome/browser/extensions/bluetooth_event_router_unittest.cc:16: const char*
kNonBluetoothEventName = "Non bluetooth event name";
add vertical whitespace to balance the namespace {

http://codereview.chromium.org/11369055/diff/1007/chrome/browser/extensions/b...
chrome/browser/extensions/bluetooth_event_router_unittest.cc:46:
EXPECT_EQ(mock_adapter_, router_.GetMutableAdapter());
More on earlier comment: it seems that if you run this line twice, you'll hit
http://code.google.com/searchframe#OAMlx_jo-ck/src/base/observer_list.h&exact....
If it doesn't make sense for someone to call it twice in the real world, then
never mind.

http://codereview.chromium.org/11369055/diff/1007/device/bluetooth/bluetooth_...
File device/bluetooth/bluetooth_adapter_chromeos.cc (right):

http://codereview.chromium.org/11369055/diff/1007/device/bluetooth/bluetooth_...
device/bluetooth/bluetooth_adapter_chromeos.cc:54: if
(!observers_.HasObserver(observer))
Aha. Would it make sense to move this check into GetMutableAdapter() and
preserve the expectation that a given observer can be added only once? One might
make the argument that it's the caller's job, not the AddObserver(), to make
sure not to add twice. It's not a huge deal, but it feels like an inconsistency.

[youngki, 2012-11-06 16:18:54]

Adding Scott (@sky) for chrome/OWNERS.

Scott, could you review chrome/chrome_test.gypi? I added a new unittest:
bluetooth_event_router_unittest.cc.

https://chromiumcodereview.appspot.com/11369055/diff/1007/chrome/browser/exte...
File chrome/browser/extensions/bluetooth_event_router.cc (right):

https://chromiumcodereview.appspot.com/11369055/diff/1007/chrome/browser/exte...
chrome/browser/extensions/bluetooth_event_router.cc:53:
adapter_->AddObserver(this);
On 2012/11/06 02:04:09, miket wrote:
> Braces not needed.
> 
> I might answer this question for myself later in the review, but is it OK to
> repeatedly add the observer if the caller calls GetMutableAdapter more than
> once?

Done.

https://chromiumcodereview.appspot.com/11369055/diff/1007/chrome/browser/exte...
chrome/browser/extensions/bluetooth_event_router.cc:81: const std::string&
event_name) {
On 2012/11/06 02:04:09, miket wrote:
> this method could be const.

This method is actually static, so no need to be const.

https://chromiumcodereview.appspot.com/11369055/diff/1007/chrome/browser/exte...
chrome/browser/extensions/bluetooth_event_router.cc:91:
adapter_->AddObserver(this);
On 2012/11/06 02:04:09, miket wrote:
> Would it be correct, or more elegant, to call GetMutableAdapter() here instead
> of doing the get() check & AddObserver()?

Hm.. Although they do the same thing, we are not trying to get the mutable
adapter here; I think I will keep this implementation.

Also giving a second thought, I reverted my change on GetMutableAdapter(). It
looks wrong to add an observer when the purpose of the function is just to
return the GetMutableAdapter(). Also BluetoothEventRouter needs to add observers
only when the bluetooth event listeners are added anyways.

https://chromiumcodereview.appspot.com/11369055/diff/1007/chrome/browser/exte...
File chrome/browser/extensions/bluetooth_event_router_unittest.cc (right):

https://chromiumcodereview.appspot.com/11369055/diff/1007/chrome/browser/exte...
chrome/browser/extensions/bluetooth_event_router_unittest.cc:16: const char*
kNonBluetoothEventName = "Non bluetooth event name";
On 2012/11/06 02:04:09, miket wrote:
> add vertical whitespace to balance the namespace {

Done.

https://chromiumcodereview.appspot.com/11369055/diff/1007/chrome/browser/exte...
chrome/browser/extensions/bluetooth_event_router_unittest.cc:46:
EXPECT_EQ(mock_adapter_, router_.GetMutableAdapter());
On 2012/11/06 02:04:09, miket wrote:
> More on earlier comment: it seems that if you run this line twice, you'll hit
>
http://code.google.com/searchframe#OAMlx_jo-ck/src/base/observer_list.h&exact....
> If it doesn't make sense for someone to call it twice in the real world, then
> never mind.

Added a test case to make sure that AddObserver is called only once even if the
multiple bluetooth event listeners are added.

https://chromiumcodereview.appspot.com/11369055/diff/1007/device/bluetooth/bl...
File device/bluetooth/bluetooth_adapter_chromeos.cc (right):

https://chromiumcodereview.appspot.com/11369055/diff/1007/device/bluetooth/bl...
device/bluetooth/bluetooth_adapter_chromeos.cc:54: if
(!observers_.HasObserver(observer))
On 2012/11/06 02:04:09, miket wrote:
> Aha. Would it make sense to move this check into GetMutableAdapter() and
> preserve the expectation that a given observer can be added only once? One
might
> make the argument that it's the caller's job, not the AddObserver(), to make
> sure not to add twice. It's not a huge deal, but it feels like an
inconsistency.

Done. Reverted this file and this check is now done by bluetooth_event_router.

[sky, 2012-11-06 16:48:42]

LGTM

[youngki, 2012-11-14 13:28:09]

The replacement of this CL submitted as
https://chromiumcodereview.appspot.com/11360200/

Closing this issue.