Change getDevices to use a DeviceCallback.

Change getDevices to use a DeviceCallback.

This modifies the getDevices method of the bluetooth Extensions API to
accept a DeviceCallback, which will be called for every matching device.

This will allow more thorough, long-running searches (involving SDP
queries) to be performed without slowing down access to matching devices
that can be returned quickly.

TEST=api test updated
BUG=134545
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=156348

[bryeung, 2012-09-07 18:06:57]

asargent: for the extensions changes
keybuk: for the bluetooth changes

[bryeung, 2012-09-07 18:06:59]

asargent: for the extensions changes
keybuk: for the bluetooth changes

[keybuk, 2012-09-10 17:58:19]

lgtm

[bryeung, 2012-09-11 15:16:19]

Antony: do you have bandwidth for this review?

[asargent_no_longer_on_chrome, 2012-09-11 17:17:30]

Yes, looking now. Apologies for delay - I got backed up on reviews.

On Tue, Sep 11, 2012 at 8:16 AM, <bryeung@chromium.org> wrote:

> Antony: do you have bandwidth for this review?
>
>
https://chromiumcodereview.**appspot.com/10915148/<https://chromiumcodereview...
>

[asargent_no_longer_on_chrome, 2012-09-11 17:42:56]

LGTM w/ one problem in event listening you should fix

http://codereview.chromium.org/10915148/diff/1/chrome/renderer/resources/exte...
File
chrome/renderer/resources/extensions/experimental.bluetooth_custom_bindings.js
(right):

http://codereview.chromium.org/10915148/diff/1/chrome/renderer/resources/exte...
chrome/renderer/resources/extensions/experimental.bluetooth_custom_bindings.js:71:
chromeHidden.bluetooth.deviceSearchResultHandler);
It looks like your intention is to only handle a single outstanding request to
getDevices at a time from a given script context, but you don't seem to enforce
that here, so you can clobber the reference to the outstanding call's listener,
and then in setCustomCallback you'll remove the wrong one. 

I think you want to check if deviceSearchResultHandler is null and return an
error if it isn't. 

Alternatively, if you wanted to allow multiple outstanding calls to getDevices
(does the C++ code support that?) you could keep a map of references to
listeners keyed by request id. I believe the request id is hung off the request
object that your setHandleRequest and setCustomCallback gets called with.

[bryeung, 2012-09-11 18:37:30]

http://codereview.chromium.org/10915148/diff/1/chrome/renderer/resources/exte...
File
chrome/renderer/resources/extensions/experimental.bluetooth_custom_bindings.js
(right):

http://codereview.chromium.org/10915148/diff/1/chrome/renderer/resources/exte...
chrome/renderer/resources/extensions/experimental.bluetooth_custom_bindings.js:71:
chromeHidden.bluetooth.deviceSearchResultHandler);
On 2012/09/11 17:42:56, Antony Sargent wrote:
> It looks like your intention is to only handle a single outstanding request to
> getDevices at a time from a given script context, but you don't seem to
enforce
> that here, so you can clobber the reference to the outstanding call's
listener,
> and then in setCustomCallback you'll remove the wrong one. 
> 
> I think you want to check if deviceSearchResultHandler is null and return an
> error if it isn't. 
> 
> Alternatively, if you wanted to allow multiple outstanding calls to getDevices
> (does the C++ code support that?) you could keep a map of references to
> listeners keyed by request id. I believe the request id is hung off the
request
> object that your setHandleRequest and setCustomCallback gets called with. 

Great catch.

I'm fine with disallowing concurrent calls to these methods.  I've updated the
code and added a test for the getDevices case.

[commit-bot: I haz the power, 2012-09-11 18:38:23]

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

[commit-bot: I haz the power, 2012-09-11 20:36:05]

Try job failure for 10915148-7001 (previous was lost) (retry) on linux_chromeos
for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2012-09-12 15:54:55]

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

[commit-bot: I haz the power, 2012-09-12 19:13:07]

Change committed as 156348