Decouple bluetooth_event_router from extension_system.

Decouple bluetooth_event_router from extension_system.

I put bluetooth_event_router on its own module, and implemented
EventRouter::Observer so that BluetoothEventRouter can listen to bluetooth
events.

I mostly followed Yoyo's example: https://chromiumcodereview.appspot.com/11351004/ to move bluetooth_event_router on its own module.


BUG=135470
TBR=sky@chromium.org

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=167534

[youngki, 2012-11-12 18:56:15]

Hi Yoyo,

Could you review this CL? This basically follows your ProcessesAPI CL and moves
BluetoothEventRouter on its own BluetoothAPI module.

We need this CL so that we can have BluetoothEventRouter listen to bluetooth
events through EventRouter::Observer, on my other CL:
https://chromiumcodereview.appspot.com/11368145/

[Yoyo Zhou, 2012-11-12 20:03:56]

https://chromiumcodereview.appspot.com/11360200/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/api/bluetooth/bluetooth_api.cc (right):

https://chromiumcodereview.appspot.com/11360200/diff/1/chrome/browser/extensi...
chrome/browser/extensions/api/bluetooth/bluetooth_api.cc:118: NOTIMPLEMENTED();
I don't understand. Can you explain what this is for?

By the way, ProcessesAPI is slightly unusual. The intended pattern is: if you
want to start the event router once there is a bluetooth API listener, then:

- Register with EventRouter in the API constructor
- Unregister in API::Shutdown
- In OnListenerAdded, start the event router, and also unregister
- Don't need to implement OnListenerRemoved

And the possible lazy start in bluetooth_event_router() still looks necessary
because BluetoothEventRouter is also used by API function calls defined in this
file. (Ideally, those would be attached to BluetoothAPI instead. But that's a
less important cleanup.)

If, on the other hand, you need to start the event router at startup, you should
do that in the constructor and you don't need to observe EventRouter at all.

[bryeung, 2012-11-12 20:44:39]

https://chromiumcodereview.appspot.com/11360200/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/api/bluetooth/bluetooth_api.cc (right):

https://chromiumcodereview.appspot.com/11360200/diff/1/chrome/browser/extensi...
chrome/browser/extensions/api/bluetooth/bluetooth_api.cc:111: if
(!bluetooth_event_router_) {
no braces

https://chromiumcodereview.appspot.com/11360200/diff/1/chrome/browser/extensi...
chrome/browser/extensions/api/bluetooth/bluetooth_api.cc:118: NOTIMPLEMENTED();
Seems like this CL should handle the lifetime management of the DefaultAdapter
as well.

https://chromiumcodereview.appspot.com/11360200/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/api/bluetooth/bluetooth_api_factory.cc (right):

https://chromiumcodereview.appspot.com/11360200/diff/1/chrome/browser/extensi...
chrome/browser/extensions/api/bluetooth/bluetooth_api_factory.cc:47: return
true;
really?  this doesn't interfere with tests?

https://chromiumcodereview.appspot.com/11360200/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/api/bluetooth/bluetooth_api_factory.h (right):

https://chromiumcodereview.appspot.com/11360200/diff/1/chrome/browser/extensi...
chrome/browser/extensions/api/bluetooth/bluetooth_api_factory.h:15: class
BluetoothAPIFactory : public ProfileKeyedServiceFactory {
This looks like a lot of boilerplate for each API.  Is it really necessary to
repeat all of this?

[Yoyo Zhou, 2012-11-12 21:06:01]

https://chromiumcodereview.appspot.com/11360200/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/api/bluetooth/bluetooth_api_factory.cc (right):

https://chromiumcodereview.appspot.com/11360200/diff/1/chrome/browser/extensi...
chrome/browser/extensions/api/bluetooth/bluetooth_api_factory.cc:47: return
true;
On 2012/11/12 20:44:39, bryeung wrote:
> really?  this doesn't interfere with tests?

You'd have to run the tests to find out. =)

But this affects TestingProfile, which is mainly used in unit tests; extension
API tests and such use regular ProfileImpl.

https://chromiumcodereview.appspot.com/11360200/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/api/bluetooth/bluetooth_api_factory.h (right):

https://chromiumcodereview.appspot.com/11360200/diff/1/chrome/browser/extensi...
chrome/browser/extensions/api/bluetooth/bluetooth_api_factory.h:15: class
BluetoothAPIFactory : public ProfileKeyedServiceFactory {
On 2012/11/12 20:44:39, bryeung wrote:
> This looks like a lot of boilerplate for each API.  Is it really necessary to
> repeat all of this?

Unfortunately, yes, for now. I'm working on a solution but it's trapped in C++
template purgatory at the moment.

[youngki, 2012-11-13 01:14:13]

https://chromiumcodereview.appspot.com/11360200/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/api/bluetooth/bluetooth_api.cc (right):

https://chromiumcodereview.appspot.com/11360200/diff/1/chrome/browser/extensi...
chrome/browser/extensions/api/bluetooth/bluetooth_api.cc:111: if
(!bluetooth_event_router_) {
On 2012/11/12 20:44:39, bryeung wrote:
> no braces

Done.

https://chromiumcodereview.appspot.com/11360200/diff/1/chrome/browser/extensi...
chrome/browser/extensions/api/bluetooth/bluetooth_api.cc:118: NOTIMPLEMENTED();
On 2012/11/12 20:44:39, bryeung wrote:
> Seems like this CL should handle the lifetime management of the DefaultAdapter
> as well.

Okay I combined https://chromiumcodereview.appspot.com/11360200/ into this CL.
Hopefully the intention is clearer. Basically we'd like to let bluetooth event
router know when the events are added/removed.

https://chromiumcodereview.appspot.com/11360200/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/api/bluetooth/bluetooth_api_factory.cc (right):

https://chromiumcodereview.appspot.com/11360200/diff/1/chrome/browser/extensi...
chrome/browser/extensions/api/bluetooth/bluetooth_api_factory.cc:47: return
true;
On 2012/11/12 21:06:01, Yoyo Zhou wrote:
> On 2012/11/12 20:44:39, bryeung wrote:
> > really?  this doesn't interfere with tests?
> 
> You'd have to run the tests to find out. =)
> 
> But this affects TestingProfile, which is mainly used in unit tests; extension
> API tests and such use regular ProfileImpl.

BluetoothApiTest* in unit_tests all pass.

[bryeung, 2012-11-13 15:14:43]

https://chromiumcodereview.appspot.com/11360200/diff/9001/chrome/browser/exte...
File chrome/browser/extensions/api/bluetooth/bluetooth_api.h (right):

https://chromiumcodereview.appspot.com/11360200/diff/9001/chrome/browser/exte...
chrome/browser/extensions/api/bluetooth/bluetooth_api.h:39: // Convenience
method to get the BluetoothAPI for a profile.
I prefer static methods at the very start of the public block.

https://chromiumcodereview.appspot.com/11360200/diff/9001/chrome/browser/exte...
File chrome/browser/extensions/api/bluetooth/bluetooth_event_router.cc (right):

https://chromiumcodereview.appspot.com/11360200/diff/9001/chrome/browser/exte...
chrome/browser/extensions/api/bluetooth/bluetooth_event_router.cc:36:
ExtensionBluetoothEventRouter::~ExtensionBluetoothEventRouter() {
shouldn't this always release the adapter?  Seems like a bug if there are open
sockets or listeners and we're destroying this object.  You should at least put
a couple of CHECKs in here.

https://chromiumcodereview.appspot.com/11360200/diff/9001/chrome/browser/exte...
chrome/browser/extensions/api/bluetooth/bluetooth_event_router.cc:89: if
(!IsAdapterNeeded())
this check should be moved to MaybeReleaseAdapter

https://chromiumcodereview.appspot.com/11360200/diff/9001/chrome/browser/exte...
chrome/browser/extensions/api/bluetooth/bluetooth_event_router.cc:102: bool
ExtensionBluetoothEventRouter::IsAdapterNeeded() const {
no need for a separate method: roll this code into MaybeReleaseAdapter

https://chromiumcodereview.appspot.com/11360200/diff/9001/chrome/browser/exte...
chrome/browser/extensions/api/bluetooth/bluetooth_event_router.cc:103: return
num_event_listeners_ > 0 || socket_map_.size() > 0;
Actually: I don't think we need to check the socket_map_ here.  The sockets
aren't tied to the adapter at all, so there's no reason to hold the adapter
while we have sockets open.

[youngki, 2012-11-13 16:09:49]

https://chromiumcodereview.appspot.com/11360200/diff/9001/chrome/browser/exte...
File chrome/browser/extensions/api/bluetooth/bluetooth_api.h (right):

https://chromiumcodereview.appspot.com/11360200/diff/9001/chrome/browser/exte...
chrome/browser/extensions/api/bluetooth/bluetooth_api.h:39: // Convenience
method to get the BluetoothAPI for a profile.
On 2012/11/13 15:14:43, bryeung wrote:
> I prefer static methods at the very start of the public block.

Done.

https://chromiumcodereview.appspot.com/11360200/diff/9001/chrome/browser/exte...
File chrome/browser/extensions/api/bluetooth/bluetooth_event_router.cc (right):

https://chromiumcodereview.appspot.com/11360200/diff/9001/chrome/browser/exte...
chrome/browser/extensions/api/bluetooth/bluetooth_event_router.cc:36:
ExtensionBluetoothEventRouter::~ExtensionBluetoothEventRouter() {
On 2012/11/13 15:14:43, bryeung wrote:
> shouldn't this always release the adapter?  Seems like a bug if there are open
> sockets or listeners and we're destroying this object.  You should at least
put
> a couple of CHECKs in here.

Done.

https://chromiumcodereview.appspot.com/11360200/diff/9001/chrome/browser/exte...
chrome/browser/extensions/api/bluetooth/bluetooth_event_router.cc:89: if
(!IsAdapterNeeded())
On 2012/11/13 15:14:43, bryeung wrote:
> this check should be moved to MaybeReleaseAdapter

Done.

https://chromiumcodereview.appspot.com/11360200/diff/9001/chrome/browser/exte...
chrome/browser/extensions/api/bluetooth/bluetooth_event_router.cc:102: bool
ExtensionBluetoothEventRouter::IsAdapterNeeded() const {
On 2012/11/13 15:14:43, bryeung wrote:
> no need for a separate method: roll this code into MaybeReleaseAdapter

Done.

https://chromiumcodereview.appspot.com/11360200/diff/9001/chrome/browser/exte...
chrome/browser/extensions/api/bluetooth/bluetooth_event_router.cc:103: return
num_event_listeners_ > 0 || socket_map_.size() > 0;
On 2012/11/13 15:14:43, bryeung wrote:
> Actually: I don't think we need to check the socket_map_ here.  The sockets
> aren't tied to the adapter at all, so there's no reason to hold the adapter
> while we have sockets open.

Done. I removed the test of creating/removing adapter based on sockets from the
unittest as well.

[bryeung, 2012-11-13 16:16:15]

https://chromiumcodereview.appspot.com/11360200/diff/2002/chrome/browser/exte...
File chrome/browser/extensions/api/bluetooth/bluetooth_api.h (right):

https://chromiumcodereview.appspot.com/11360200/diff/2002/chrome/browser/exte...
chrome/browser/extensions/api/bluetooth/bluetooth_api.h:42:
ExtensionBluetoothEventRouter* bluetooth_event_router();
also move this about Shutdown

https://chromiumcodereview.appspot.com/11360200/diff/2002/chrome/browser/exte...
File chrome/browser/extensions/api/bluetooth/bluetooth_event_router.cc (right):

https://chromiumcodereview.appspot.com/11360200/diff/2002/chrome/browser/exte...
chrome/browser/extensions/api/bluetooth/bluetooth_event_router.cc:37:
DCHECK(socket_map_.size() == 0);
CHECK, not DCHECK please

https://chromiumcodereview.appspot.com/11360200/diff/2002/chrome/browser/exte...
chrome/browser/extensions/api/bluetooth/bluetooth_event_router.cc:55: void
ExtensionBluetoothEventRouter::InitializeAdapterIfNeeded() {
re-order methods to match header please

https://chromiumcodereview.appspot.com/11360200/diff/2002/chrome/browser/exte...
chrome/browser/extensions/api/bluetooth/bluetooth_event_router.cc:80:
InitializeAdapterIfNeeded();
we shouldn't be holding the adapter for sockets

https://chromiumcodereview.appspot.com/11360200/diff/2002/chrome/browser/exte...
chrome/browser/extensions/api/bluetooth/bluetooth_event_router.cc:89:
MaybeReleaseAdapter();
don't need this for sockets

https://chromiumcodereview.appspot.com/11360200/diff/2002/chrome/browser/exte...
chrome/browser/extensions/api/bluetooth/bluetooth_event_router.cc:108:
DCHECK(num_event_listeners_ >= 0);
CHECK (since we'll leak the adapter if this goes below 0)

https://chromiumcodereview.appspot.com/11360200/diff/2002/chrome/browser/exte...
File chrome/browser/extensions/api/bluetooth/bluetooth_event_router_unittest.cc
(right):

https://chromiumcodereview.appspot.com/11360200/diff/2002/chrome/browser/exte...
chrome/browser/extensions/api/bluetooth/bluetooth_event_router_unittest.cc:8:
#include "device/bluetooth/test/mock_bluetooth_socket.h"
you can remove all of the socket code from this now

I'd keep the mock_bluetooth_socket files and land them in a separate CL.

[youngki, 2012-11-13 17:01:30]

https://chromiumcodereview.appspot.com/11360200/diff/2002/chrome/browser/exte...
File chrome/browser/extensions/api/bluetooth/bluetooth_api.h (right):

https://chromiumcodereview.appspot.com/11360200/diff/2002/chrome/browser/exte...
chrome/browser/extensions/api/bluetooth/bluetooth_api.h:42:
ExtensionBluetoothEventRouter* bluetooth_event_router();
On 2012/11/13 16:16:15, bryeung wrote:
> also move this about Shutdown

Done.

https://chromiumcodereview.appspot.com/11360200/diff/2002/chrome/browser/exte...
File chrome/browser/extensions/api/bluetooth/bluetooth_event_router.cc (right):

https://chromiumcodereview.appspot.com/11360200/diff/2002/chrome/browser/exte...
chrome/browser/extensions/api/bluetooth/bluetooth_event_router.cc:37:
DCHECK(socket_map_.size() == 0);
On 2012/11/13 16:16:15, bryeung wrote:
> CHECK, not DCHECK please

Done.

https://chromiumcodereview.appspot.com/11360200/diff/2002/chrome/browser/exte...
chrome/browser/extensions/api/bluetooth/bluetooth_event_router.cc:55: void
ExtensionBluetoothEventRouter::InitializeAdapterIfNeeded() {
On 2012/11/13 16:16:15, bryeung wrote:
> re-order methods to match header please

Done.

https://chromiumcodereview.appspot.com/11360200/diff/2002/chrome/browser/exte...
chrome/browser/extensions/api/bluetooth/bluetooth_event_router.cc:80:
InitializeAdapterIfNeeded();
On 2012/11/13 16:16:15, bryeung wrote:
> we shouldn't be holding the adapter for sockets

Done.

https://chromiumcodereview.appspot.com/11360200/diff/2002/chrome/browser/exte...
chrome/browser/extensions/api/bluetooth/bluetooth_event_router.cc:89:
MaybeReleaseAdapter();
On 2012/11/13 16:16:15, bryeung wrote:
> don't need this for sockets

Done.

https://chromiumcodereview.appspot.com/11360200/diff/2002/chrome/browser/exte...
chrome/browser/extensions/api/bluetooth/bluetooth_event_router.cc:108:
DCHECK(num_event_listeners_ >= 0);
On 2012/11/13 16:16:15, bryeung wrote:
> CHECK (since we'll leak the adapter if this goes below 0)

Done.

https://chromiumcodereview.appspot.com/11360200/diff/2002/chrome/browser/exte...
File chrome/browser/extensions/api/bluetooth/bluetooth_event_router_unittest.cc
(right):

https://chromiumcodereview.appspot.com/11360200/diff/2002/chrome/browser/exte...
chrome/browser/extensions/api/bluetooth/bluetooth_event_router_unittest.cc:8:
#include "device/bluetooth/test/mock_bluetooth_socket.h"
On 2012/11/13 16:16:15, bryeung wrote:
> you can remove all of the socket code from this now
> 
> I'd keep the mock_bluetooth_socket files and land them in a separate CL.

Done.

[bryeung, 2012-11-13 18:22:15]

lgtm

[Yoyo Zhou, 2012-11-13 19:57:52]

LGTM, please make sure tests pass

[Elliot Glaysher, 2012-11-13 20:05:35]

profiles lgtm

[youngki, 2012-11-13 21:43:23]

Thanks for the reviews. Currently win_rel and android_clang_dbg don't pass, but
they don't seem to be broken by this CL, so I am putting this into commit queue
now.

[commit-bot: I haz the power, 2012-11-13 21:44:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/youngki@chromium.org/11360200/13001

[commit-bot: I haz the power, 2012-11-14 00:42:40]

Change committed as 167534