Bluetooth Extension API: Add a discovery API

Bluetooth Extension API: Add a discovery API

BUG=132597
TEST=simple test added
(events testing coming in a future CL: crbug.com/132616)


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

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=142666

[asargent_no_longer_on_chrome, 2012-06-14 18:23:55]

http://codereview.chromium.org/10536159/diff/2001/chrome/browser/chromeos/ext...
File chrome/browser/chromeos/extensions/bluetooth_event_router.cc (right):

http://codereview.chromium.org/10536159/diff/2001/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/bluetooth_event_router.cc:101:
profile_->GetExtensionEventRouter()->DispatchEventToRenderers(
If I have 2 platform apps installed that both have added a listener for this
event, and only one of them calls startDiscovery, does this mean that both apps
will start getting their listener functions fired? If so, for the one that
hasn't called startDiscovery that seems kind of strange.

We also have the DispatchEventToExtension method which might help you avoid that
problem, but then you'll probably need to add bookkeeping for which Extension
objects are currently discovering vs. not.

http://codereview.chromium.org/10536159/diff/2001/chrome/common/extensions/ap...
File chrome/common/extensions/api/experimental_bluetooth.idl (right):

http://codereview.chromium.org/10536159/diff/2001/chrome/common/extensions/ap...
chrome/common/extensions/api/experimental_bluetooth.idl:108: static void
startDiscovery();
FYI, another approach we could take is to have this function take a parameter
which is an event listener function, instead of having a "global" event of
onDeviceDiscovered.

See the onEvent option of the tts.speak method, or the onclick option of the
contextMenus.create method, for examples of this style.

http://code.google.com/chrome/extensions/tts.html#method-speak
http://code.google.com/chrome/extensions/contextMenus.html#method-create


I don't think you necessarily need to change this, but perhaps keep it in mind
and think about whether it would make writing code against the API a little
easier/clearer or not. For example, compare these 2  code snippets:

chrome.bluetooth.startDiscovery({onDiscovered:function(device) {
  // do something with device
  chrome.bluetooth.stopDiscovery();
}});


versus:


chrome.bluetooth.onDeviceDiscovered.addListener(function(device) {
  // do something with device
  chrome.bluetooth.stopDiscovery();  
})
chrome.bluetooth.startDiscovery();

http://codereview.chromium.org/10536159/diff/2001/chrome/common/extensions/ap...
chrome/common/extensions/api/experimental_bluetooth.idl:111: static void
stopDiscovery();
nit: startDiscovery and stopDiscovery should probably both take a optional
ResultCallback parameter. It looks like you send back an indication of error in
some cases in the .cc file.

[bryeung, 2012-06-14 19:06:44]

Passing the event listener is actually how I wanted to do this, but I
made the assumption it wasn't possible without doing enough research
first.  Thanks for the pointers!

I'll fix this up to use that method.

Bryan

On Thu, Jun 14, 2012 at 2:23 PM,  <asargent@chromium.org> wrote:
>
>
http://codereview.chromium.org/10536159/diff/2001/chrome/browser/chromeos/ext...
> File chrome/browser/chromeos/extensions/bluetooth_event_router.cc
> (right):
>
>
http://codereview.chromium.org/10536159/diff/2001/chrome/browser/chromeos/ext...
> chrome/browser/chromeos/extensions/bluetooth_event_router.cc:101:
> profile_->GetExtensionEventRouter()->DispatchEventToRenderers(
> If I have 2 platform apps installed that both have added a listener for
> this event, and only one of them calls startDiscovery, does this mean
> that both apps will start getting their listener functions fired? If so,
> for the one that hasn't called startDiscovery that seems kind of
> strange.
>
> We also have the DispatchEventToExtension method which might help you
> avoid that problem, but then you'll probably need to add bookkeeping for
> which Extension objects are currently discovering vs. not.
>
>
http://codereview.chromium.org/10536159/diff/2001/chrome/common/extensions/ap...
> File chrome/common/extensions/api/experimental_bluetooth.idl (right):
>
>
http://codereview.chromium.org/10536159/diff/2001/chrome/common/extensions/ap...
> chrome/common/extensions/api/experimental_bluetooth.idl:108: static void
> startDiscovery();
> FYI, another approach we could take is to have this function take a
> parameter which is an event listener function, instead of having a
> "global" event of onDeviceDiscovered.
>
> See the onEvent option of the tts.speak method, or the onclick option of
> the contextMenus.create method, for examples of this style.
>
> http://code.google.com/chrome/extensions/tts.html#method-speak
> http://code.google.com/chrome/extensions/contextMenus.html#method-create
>
>
> I don't think you necessarily need to change this, but perhaps keep it
> in mind and think about whether it would make writing code against the
> API a little easier/clearer or not. For example, compare these 2  code
> snippets:
>
> chrome.bluetooth.startDiscovery({onDiscovered:function(device) {
>  // do something with device
>  chrome.bluetooth.stopDiscovery();
> }});
>
>
> versus:
>
>
> chrome.bluetooth.onDeviceDiscovered.addListener(function(device) {
>  // do something with device
>  chrome.bluetooth.stopDiscovery();
> })
> chrome.bluetooth.startDiscovery();
>
>
http://codereview.chromium.org/10536159/diff/2001/chrome/common/extensions/ap...
> chrome/common/extensions/api/experimental_bluetooth.idl:111: static void
> stopDiscovery();
> nit: startDiscovery and stopDiscovery should probably both take a
> optional ResultCallback parameter. It looks like you send back an
> indication of error in some cases in the .cc file.
>
> http://codereview.chromium.org/10536159/

[bryeung, 2012-06-15 06:52:07]

I haven't been able to test this yet (my build for my chromebook crashes at the
login screen), but hopefully this isn't too far off the mark.

[asargent_no_longer_on_chrome, 2012-06-15 19:59:08]

http://codereview.chromium.org/10536159/diff/5017/chrome/browser/chromeos/ext...
File chrome/browser/chromeos/extensions/bluetooth_event_router.cc (right):

http://codereview.chromium.org/10536159/diff/5017/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/bluetooth_event_router.cc:102:
profile_->GetExtensionEventRouter()->DispatchEventToRenderers(
In thinking about this a little more, it's still kind of expensive to dispatch
an event to all extension renderers in the whole system.

It's probably ok to land as-is for now, but can you file a follow-up bug to come
back and at a minimum restrict this to just platform apps, but ideally just send
to the extension renderers that are actually currently doing discovery?

http://codereview.chromium.org/10536159/diff/5017/chrome/common/extensions/ap...
File chrome/common/extensions/api/experimental_bluetooth.idl (right):

http://codereview.chromium.org/10536159/diff/5017/chrome/common/extensions/ap...
chrome/common/extensions/api/experimental_bluetooth.idl:197: static void
onDeviceDiscovered_(Device device);
I think that since this doesn't need to be visible to consumers of the API, you
don't need to list it in the IDL file. You can create an arbitrarily named event
to listen for in your custom_bindings.js by doing:

chromeHidden.bluetooth.onDeviceDiscovered =
  new chrome.Event("bluetooth.onDeviceDiscovered");


...

chromeHidden.bluetooth.onDeviceDiscovered.addListener(function(...) {...})

[asargent_no_longer_on_chrome, 2012-06-15 20:19:24]

LGTM assuming comment about event is fixed (if for some reason that proves
difficult, I think it would even be ok to land as-is with a TODO and bug filed
to remove it)

[bryeung, 2012-06-16 22:20:40]

The suggestion to hide the event worked like a charm, and I've been able to test
this on device now.

I'm going to commit with the TODO to fix the dispatch to be more
specific/efficient.

Thanks!

http://codereview.chromium.org/10536159/diff/5017/chrome/browser/chromeos/ext...
File chrome/browser/chromeos/extensions/bluetooth_event_router.cc (right):

http://codereview.chromium.org/10536159/diff/5017/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/bluetooth_event_router.cc:102:
profile_->GetExtensionEventRouter()->DispatchEventToRenderers(
On 2012/06/15 19:59:08, Antony Sargent wrote:
> In thinking about this a little more, it's still kind of expensive to dispatch
> an event to all extension renderers in the whole system.
> 
> It's probably ok to land as-is for now, but can you file a follow-up bug to
come
> back and at a minimum restrict this to just platform apps, but ideally just
send
> to the extension renderers that are actually currently doing discovery?

Added a TODO.  Will fix soon.

[commit-bot: I haz the power, 2012-06-16 22:22:43]

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

[commit-bot: I haz the power, 2012-06-17 01:00:38]

Change committed as 142623