Implement chrome.experimental.bluetooth.connect

Implement chrome.experimental.bluetooth.connect

BUG=119473
TEST=my demo app works with this change


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

[bryeung, 2012-05-12 00:27:00]

Antony: do you have bandwidth for this one?

[asargent_no_longer_on_chrome, 2012-05-12 16:36:05]

Overall looks fine with one small potential bug.

Do you have any tests for this API yet? We don't necessarily need to hold up
this CL for that, but we really want to have them the closer we get to having
the API be supported.

http://codereview.chromium.org/10381120/diff/1/chrome/browser/chromeos/extens...
File chrome/browser/chromeos/extensions/bluetooth_event_router.cc (right):

http://codereview.chromium.org/10381120/diff/1/chrome/browser/chromeos/extens...
chrome/browser/chromeos/extensions/bluetooth_event_router.cc:42:
socket_map_[next_socket_id_++] = socket;
shouldn't this be [return_id] instead of [next_socket_id_++]?

http://codereview.chromium.org/10381120/diff/1/chrome/browser/chromeos/extens...
File chrome/browser/chromeos/extensions/bluetooth_event_router.h (right):

http://codereview.chromium.org/10381120/diff/1/chrome/browser/chromeos/extens...
chrome/browser/chromeos/extensions/bluetooth_event_router.h:30: // ReleaseSocket
is called for the socket.  Returns an id for the socket.
Questions to think about for possible future work beyond this CL: If the
app/extension that registered the socket gets disabled or uninstalled, should we
automatically release the socket? Also, what if more than one app/extension
tries to open a socket to the same device? Does the bluetooth code handle that
for us or do we need to do something in the extensions system?

[bryeung, 2012-05-14 17:20:27]

Agreed about tests: I'll sync up with Scott about the testing infrastructure
work for Bluetooth and offer to lend him a hand if he needs it.

http://codereview.chromium.org/10381120/diff/1/chrome/browser/chromeos/extens...
File chrome/browser/chromeos/extensions/bluetooth_event_router.cc (right):

http://codereview.chromium.org/10381120/diff/1/chrome/browser/chromeos/extens...
chrome/browser/chromeos/extensions/bluetooth_event_router.cc:42:
socket_map_[next_socket_id_++] = socket;
On 2012/05/12 16:36:05, Antony Sargent wrote:
> shouldn't this be [return_id] instead of [next_socket_id_++]?

Yikes!  Good catch.

http://codereview.chromium.org/10381120/diff/1/chrome/browser/chromeos/extens...
File chrome/browser/chromeos/extensions/bluetooth_event_router.h (right):

http://codereview.chromium.org/10381120/diff/1/chrome/browser/chromeos/extens...
chrome/browser/chromeos/extensions/bluetooth_event_router.h:30: // ReleaseSocket
is called for the socket.  Returns an id for the socket.
On 2012/05/12 16:36:05, Antony Sargent wrote:
> Questions to think about for possible future work beyond this CL: If the
> app/extension that registered the socket gets disabled or uninstalled, should
we
> automatically release the socket? Also, what if more than one app/extension
> tries to open a socket to the same device? Does the bluetooth code handle that
> for us or do we need to do something in the extensions system?

Is there a "canonical" (or just known good) example somewhere of code to release
resources upon disable/uninstall?

I think it's probably best if we let the underlying Bluetooth system handle
multiple requests for the same service.

[bryeung, 2012-05-15 15:38:39]

Antony: I'd like to spend next week working on testing infrastructure and paying
off some debt around bluetooth.

For now, I'd like to get this (and the upcoming read/write) CL landed without
tests for the Monday sprint demo.

Are you okay with that?

[asargent_no_longer_on_chrome, 2012-05-15 16:01:18]

LGTM

Sorry for latency - I forgot to circle back around and respond to your comments.

https://chromiumcodereview.appspot.com/10381120/diff/1/chrome/browser/chromeo...
File chrome/browser/chromeos/extensions/bluetooth_event_router.h (right):

https://chromiumcodereview.appspot.com/10381120/diff/1/chrome/browser/chromeo...
chrome/browser/chromeos/extensions/bluetooth_event_router.h:30: // ReleaseSocket
is called for the socket.  Returns an id for the socket.
On 2012/05/14 17:20:28, bryeung wrote:
> On 2012/05/12 16:36:05, Antony Sargent wrote:
> > Questions to think about for possible future work beyond this CL: If the
> > app/extension that registered the socket gets disabled or uninstalled,
should
> we
> > automatically release the socket? Also, what if more than one app/extension
> > tries to open a socket to the same device? Does the bluetooth code handle
that
> > for us or do we need to do something in the extensions system?
> 
> Is there a "canonical" (or just known good) example somewhere of code to
release
> resources upon disable/uninstall?

One common pattern is to have the class which holds references to the resources
that need to be released listen for the EXTENSION_UNLOADED notification. See
ExtensionMenuManager::ExtensionMenuManager and ExtensionMenuManager::Observe for
an example of this.


> 
> I think it's probably best if we let the underlying Bluetooth system handle
> multiple requests for the same service.

Ok, sounds good to me as long as the bluetooth system is designed to handle
this.

[commit-bot: I haz the power, 2012-05-15 17:19:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bryeung@chromium.org/10381120/3002

[commit-bot: I haz the power, 2012-05-15 19:02:30]

Change committed as 137196