Lazy-creates BluetoothAdapter in ExtensionBluetoothEventRouter (EBEE).

Lazy-creates BluetoothAdapter in ExtensionBluetoothEventRouter (EBEE).

Calling adapter() and GetMutableAdapter() creates and returns the refptr of
BluetoothAdapter, but EBEE does not hold the reference of it.

Adding BluetoothEventListener and registering BluetoothSocket creates and holds
the reference of BluetoothAdapter.

EBEE will release the reference if there is no socket and no listener.

BUG=135470

[youngki, 2012-11-08 17:36:04]

Bryan and Mike,

I decided to abandon https://chromiumcodereview.appspot.com/11369055/ after
discussion with Bryan and this is the new implementation with a design change.
Instead of delaying AddObserver, this delays holding adapter reference until
it's necessary. Also adapter() and GetMutableAdapter() does not hold the
reference to the adapter unless it already holds one.

Please take a look.

[bryeung, 2012-11-08 19:24:02]

https://chromiumcodereview.appspot.com/11368145/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/bluetooth_event_router.cc (right):

https://chromiumcodereview.appspot.com/11368145/diff/1/chrome/browser/extensi...
chrome/browser/extensions/bluetooth_event_router.cc:37: if (adapter_.get())
I think the .get is actually not necessary

https://chromiumcodereview.appspot.com/11368145/diff/1/chrome/browser/extensi...
chrome/browser/extensions/bluetooth_event_router.cc:50: if (adapter_.get())
I don't think you need the .get()

I'll stop repeating this comment now :-)

https://chromiumcodereview.appspot.com/11368145/diff/1/chrome/browser/extensi...
chrome/browser/extensions/bluetooth_event_router.cc:53: return
GetDefaultAdapter();
shouldn't this be initializing the adapter?

https://chromiumcodereview.appspot.com/11368145/diff/1/chrome/browser/extensi...
chrome/browser/extensions/bluetooth_event_router.cc:56: void
ExtensionBluetoothEventRouter::InitializeAdapter() {
is there a use case for having an adapter that has not been initialized?  Seems
like it would be safer to just move this code into GetMutableAdapater.

https://chromiumcodereview.appspot.com/11368145/diff/1/chrome/browser/extensi...
chrome/browser/extensions/bluetooth_event_router.cc:68: return
device::BluetoothAdapterFactory::DefaultAdapter();
why not put this inside of GetMutableAdapter and use that everywhere within this
class as well?

https://chromiumcodereview.appspot.com/11368145/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/bluetooth_event_router.h (right):

https://chromiumcodereview.appspot.com/11368145/diff/1/chrome/browser/extensi...
chrome/browser/extensions/bluetooth_event_router.h:34: // Initializes |adapter_|
when a bluetooth event listener is added.
Please describe when this should be called, not it's effect on the internals of
the class.

https://chromiumcodereview.appspot.com/11368145/diff/1/chrome/browser/extensi...
chrome/browser/extensions/bluetooth_event_router.h:37: // Releases |adapter_|
when a bluetooth event listener is removed.
same as above

https://chromiumcodereview.appspot.com/11368145/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/bluetooth_event_router_unittest.cc (right):

https://chromiumcodereview.appspot.com/11368145/diff/1/chrome/browser/extensi...
chrome/browser/extensions/bluetooth_event_router_unittest.cc:64: adapter =
router_.adapter();
why the double assignment here?

https://chromiumcodereview.appspot.com/11368145/diff/1/chrome/browser/extensi...
chrome/browser/extensions/bluetooth_event_router_unittest.cc:78:
.WillOnce(testing::Return());
can you just do .Times(1) here instead?

https://chromiumcodereview.appspot.com/11368145/diff/1/chrome/browser/extensi...
chrome/browser/extensions/bluetooth_event_router_unittest.cc:157:
extensions::event_names::kBluetoothOnAvailabilityChanged);
would be nice to test the removal in the other order as well, just to be
thorough

https://chromiumcodereview.appspot.com/11368145/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/event_router.cc (right):

https://chromiumcodereview.appspot.com/11368145/diff/1/chrome/browser/extensi...
chrome/browser/extensions/event_router.cc:191:
service->NotifyExtensionEventRoutersOnListenerAdded(event_name);
I don't think this is the right way of doing this.  I'm going to start an email
thread with mpcomplete@ to try and figure out what the right way is.

[youngki, 2012-11-09 05:12:52]

I will implement ExtensionBluetoothApi as discussed in the other email thread as
soon as Yoyo's CL is landed.

Meanwhile I addressed your comments. PTAL.

https://chromiumcodereview.appspot.com/11368145/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/bluetooth_event_router.cc (right):

https://chromiumcodereview.appspot.com/11368145/diff/1/chrome/browser/extensi...
chrome/browser/extensions/bluetooth_event_router.cc:37: if (adapter_.get())
On 2012/11/08 19:24:02, bryeung wrote:
> I think the .get is actually not necessary

Done.

https://chromiumcodereview.appspot.com/11368145/diff/1/chrome/browser/extensi...
chrome/browser/extensions/bluetooth_event_router.cc:50: if (adapter_.get())
On 2012/11/08 19:24:02, bryeung wrote:
> I don't think you need the .get()
> 
> I'll stop repeating this comment now :-)

Done.

https://chromiumcodereview.appspot.com/11368145/diff/1/chrome/browser/extensi...
chrome/browser/extensions/bluetooth_event_router.cc:53: return
GetDefaultAdapter();
On 2012/11/08 19:24:02, bryeung wrote:
> shouldn't this be initializing the adapter?

Maybe I am thinking too specifically on Windows Bluetooth adapter side, so let
me know if this is a bad decision for BluetoothEventRouter itself, but I don't
think we need to initialize the adapter (i.e. hold the reference to it) for the
following reasons:

1) If the adapter wasn't referenced anywhere, then this function will create an
adapter and return it with up-to-date bluetooth adapter info. So no need to hold
the reference in this case.

2) If the adapter is already referenced somewhere, then the adapter is already
likely to have an up-to-date (less than 500ms in windows circumstance) bluetooth
adapter info, so no need to hold a reference to it.

https://chromiumcodereview.appspot.com/11368145/diff/1/chrome/browser/extensi...
chrome/browser/extensions/bluetooth_event_router.cc:56: void
ExtensionBluetoothEventRouter::InitializeAdapter() {
On 2012/11/08 19:24:02, bryeung wrote:
> is there a use case for having an adapter that has not been initialized? 
Seems
> like it would be safer to just move this code into GetMutableAdapater.

This is actually an helper method to not repeat the same two lines over and
over. Note that InitializeAdapter() is also called in RegisterSocket and
OnEventListenerAdded.

https://chromiumcodereview.appspot.com/11368145/diff/1/chrome/browser/extensi...
chrome/browser/extensions/bluetooth_event_router.cc:68: return
device::BluetoothAdapterFactory::DefaultAdapter();
On 2012/11/08 19:24:02, bryeung wrote:
> why not put this inside of GetMutableAdapter and use that everywhere within
this
> class as well?

This is for unittest purpose. I want to override this method in the unittest so
that this method returns mock adapter instead.

https://chromiumcodereview.appspot.com/11368145/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/bluetooth_event_router.h (right):

https://chromiumcodereview.appspot.com/11368145/diff/1/chrome/browser/extensi...
chrome/browser/extensions/bluetooth_event_router.h:34: // Initializes |adapter_|
when a bluetooth event listener is added.
On 2012/11/08 19:24:02, bryeung wrote:
> Please describe when this should be called, not it's effect on the internals
of
> the class.

Done.

https://chromiumcodereview.appspot.com/11368145/diff/1/chrome/browser/extensi...
chrome/browser/extensions/bluetooth_event_router.h:37: // Releases |adapter_|
when a bluetooth event listener is removed.
On 2012/11/08 19:24:02, bryeung wrote:
> same as above

Done.

https://chromiumcodereview.appspot.com/11368145/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/bluetooth_event_router_unittest.cc (right):

https://chromiumcodereview.appspot.com/11368145/diff/1/chrome/browser/extensi...
chrome/browser/extensions/bluetooth_event_router_unittest.cc:64: adapter =
router_.adapter();
On 2012/11/08 19:24:02, bryeung wrote:
> why the double assignment here?

Removed.

https://chromiumcodereview.appspot.com/11368145/diff/1/chrome/browser/extensi...
chrome/browser/extensions/bluetooth_event_router_unittest.cc:78:
.WillOnce(testing::Return());
On 2012/11/08 19:24:02, bryeung wrote:
> can you just do .Times(1) here instead?

Done.

https://chromiumcodereview.appspot.com/11368145/diff/1/chrome/browser/extensi...
chrome/browser/extensions/bluetooth_event_router_unittest.cc:157:
extensions::event_names::kBluetoothOnAvailabilityChanged);
On 2012/11/08 19:24:02, bryeung wrote:
> would be nice to test the removal in the other order as well, just to be
> thorough

Done.

https://chromiumcodereview.appspot.com/11368145/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/event_router.cc (right):

https://chromiumcodereview.appspot.com/11368145/diff/1/chrome/browser/extensi...
chrome/browser/extensions/event_router.cc:191:
service->NotifyExtensionEventRoutersOnListenerAdded(event_name);
On 2012/11/08 19:24:02, bryeung wrote:
> I don't think this is the right way of doing this.  I'm going to start an
email
> thread with mpcomplete@ to try and figure out what the right way is.

I will wait for Yoyo's CL to be landed and I will make the change as discussed
in the email thread.

[miket_OOO, 2012-11-09 17:35:23]

lgtm

[bryeung, 2012-11-09 17:46:44]

I'll wait for the update after Yoyo's CL lands before finishing the review.

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

https://chromiumcodereview.appspot.com/11368145/diff/5001/chrome/browser/exte...
chrome/browser/extensions/bluetooth_event_router.cc:56: void
ExtensionBluetoothEventRouter::InitializeAdapter() {
I'd like this renamed to InitializeAdapterIfNeeded, with a check so it only
creates the adapter if there isn't one already stored.

https://chromiumcodereview.appspot.com/11368145/diff/5001/chrome/browser/exte...
chrome/browser/extensions/bluetooth_event_router.cc:57: adapter_ =
GetDefaultAdapter();
Just call GetMutableAdapter here.

https://chromiumcodereview.appspot.com/11368145/diff/5001/chrome/browser/exte...
chrome/browser/extensions/bluetooth_event_router.cc:61: void
ExtensionBluetoothEventRouter::ReleaseAdapter() {
I'd like this to renamed MaybeReleaseAdapter and the logic of IsAdapterNeeded to
be moved into this function as well.

https://chromiumcodereview.appspot.com/11368145/diff/5001/chrome/browser/exte...
chrome/browser/extensions/bluetooth_event_router.cc:68: return
device::BluetoothAdapterFactory::DefaultAdapter();
this should be moved into GetMutableAdapter

[youngki, 2012-11-12 23:25:24]

I am combining this CL into https://chromiumcodereview.appspot.com/11360200/

Meanwhile, here goes my response.

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

https://chromiumcodereview.appspot.com/11368145/diff/5001/chrome/browser/exte...
chrome/browser/extensions/bluetooth_event_router.cc:56: void
ExtensionBluetoothEventRouter::InitializeAdapter() {
On 2012/11/09 17:46:44, bryeung wrote:
> I'd like this renamed to InitializeAdapterIfNeeded, with a check so it only
> creates the adapter if there isn't one already stored.

Done.

https://chromiumcodereview.appspot.com/11368145/diff/5001/chrome/browser/exte...
chrome/browser/extensions/bluetooth_event_router.cc:57: adapter_ =
GetDefaultAdapter();
On 2012/11/09 17:46:44, bryeung wrote:
> Just call GetMutableAdapter here.

Done.

https://chromiumcodereview.appspot.com/11368145/diff/5001/chrome/browser/exte...
chrome/browser/extensions/bluetooth_event_router.cc:61: void
ExtensionBluetoothEventRouter::ReleaseAdapter() {
On 2012/11/09 17:46:44, bryeung wrote:
> I'd like this to renamed MaybeReleaseAdapter and the logic of IsAdapterNeeded
to
> be moved into this function as well.

Done.

https://chromiumcodereview.appspot.com/11368145/diff/5001/chrome/browser/exte...
chrome/browser/extensions/bluetooth_event_router.cc:68: return
device::BluetoothAdapterFactory::DefaultAdapter();
On 2012/11/09 17:46:44, bryeung wrote:
> this should be moved into GetMutableAdapter

Done.

[youngki, 2012-11-14 13:27:34]

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

Closing this issue.