Bluetooth API: improve discovery

Bluetooth API: improve discovery

This CL: 
- eliminates unnecessary dispatches
- correctly handles devices that are discovered before interest is
registered

TEST=ran api test
BUG=133179
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=150898

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

[bryeung, 2012-07-24 15:54:53]

Antony: in light of our discussion about the other Bluetooth events, I think
this is probably a better solution for reducing device discovered event
dispatches.  What do you think?

[asargent_no_longer_on_chrome, 2012-07-24 20:29:45]

I think this should work fine, LGTM.

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

https://chromiumcodereview.appspot.com/10815072/diff/1/chrome/browser/extensi...
chrome/browser/extensions/api/bluetooth/bluetooth_api.cc:461: // finished.
Random thought- this can happen because *another* app has already started
discovery right? Would it make sense to queue up the request instead of making
the app call again on its own?

Alternatively, is it possible and would it make sense to cache the results of
the currently executing discovery, so we can deliver them to anyone who asks
while discovery is underway?

https://chromiumcodereview.appspot.com/10815072/diff/1/chrome/renderer/resour...
File
chrome/renderer/resources/extensions/experimental.bluetooth_custom_bindings.js
(right):

https://chromiumcodereview.appspot.com/10815072/diff/1/chrome/renderer/resour...
chrome/renderer/resources/extensions/experimental.bluetooth_custom_bindings.js:21:
var clearDeviceDiscoveredHandler = function() {
style nit: no need to assign this to a var - instead just write it like this:

function clearDeviceDiscoveredHandler() {
  ...
}

https://chromiumcodereview.appspot.com/10815072/diff/1/chrome/renderer/resour...
chrome/renderer/resources/extensions/experimental.bluetooth_custom_bindings.js:28:
function() {
nit: my personal preference is to leave the opening declaration of the function
on the line above this as it was before, so that the body of the function
doesn't have to indented so much.

https://chromiumcodereview.appspot.com/10815072/diff/1/chrome/renderer/resour...
chrome/renderer/resources/extensions/experimental.bluetooth_custom_bindings.js:40:
throw new Error("deviceCallback is required in the options object");
nit: the canonical way to report errors is to set lastError and call the result
callback. So at the top of the file you should add:

var lastError = require('last_error');

And then instead of throwing an Error on this line, you should instead do
something like this:

  
  if (typeof(args[args.length-1]) == "function") {
    var callback = args[args.length-1];
    lastError.set("deviceCallback is required in the options object");
    callback();
    return;
  }

[bryeung, 2012-08-03 20:08:37]

Scott: added you for OWNERS (the bluetooth_adapter mock changes)

http://codereview.chromium.org/10815072/diff/1/chrome/browser/extensions/api/...
File chrome/browser/extensions/api/bluetooth/bluetooth_api.cc (right):

http://codereview.chromium.org/10815072/diff/1/chrome/browser/extensions/api/...
chrome/browser/extensions/api/bluetooth/bluetooth_api.cc:461: // finished.
On 2012/07/24 20:29:45, Antony Sargent wrote:
> Random thought- this can happen because *another* app has already started
> discovery right? Would it make sense to queue up the request instead of making
> the app call again on its own?
> 
> Alternatively, is it possible and would it make sense to cache the results of
> the currently executing discovery, so we can deliver them to anyone who asks
> while discovery is underway?

I had originally decided against this, but your comment made me reconsider and I
think you're right: caching the devices and replaying them is what I would want
if I were a user of the API.  Fixed.

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

http://codereview.chromium.org/10815072/diff/1/chrome/renderer/resources/exte...
chrome/renderer/resources/extensions/experimental.bluetooth_custom_bindings.js:21:
var clearDeviceDiscoveredHandler = function() {
On 2012/07/24 20:29:45, Antony Sargent wrote:
> style nit: no need to assign this to a var - instead just write it like this:
> 
> function clearDeviceDiscoveredHandler() {
>   ...
> }
> 

Done.

http://codereview.chromium.org/10815072/diff/1/chrome/renderer/resources/exte...
chrome/renderer/resources/extensions/experimental.bluetooth_custom_bindings.js:28:
function() {
On 2012/07/24 20:29:45, Antony Sargent wrote:
> nit: my personal preference is to leave the opening declaration of the
function
> on the line above this as it was before, so that the body of the function
> doesn't have to indented so much.

I'd prefer to keep all of the functions indented similarly here, and the one
below doesn't fit on the first line due to arguments.  Are you okay with leaving
this as-is?

http://codereview.chromium.org/10815072/diff/1/chrome/renderer/resources/exte...
chrome/renderer/resources/extensions/experimental.bluetooth_custom_bindings.js:40:
throw new Error("deviceCallback is required in the options object");
On 2012/07/24 20:29:45, Antony Sargent wrote:
> nit: the canonical way to report errors is to set lastError and call the
result
> callback. So at the top of the file you should add:
> 
> var lastError = require('last_error');
> 
> And then instead of throwing an Error on this line, you should instead do
> something like this:
> 
>   
>   if (typeof(args[args.length-1]) == "function") {
>     var callback = args[args.length-1];
>     lastError.set("deviceCallback is required in the options object");
>     callback();
>     return;
>   }

Done.

[asargent_no_longer_on_chrome, 2012-08-03 23:35:23]

changes LGTM

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

http://codereview.chromium.org/10815072/diff/1/chrome/renderer/resources/exte...
chrome/renderer/resources/extensions/experimental.bluetooth_custom_bindings.js:28:
function() {
I think it's fine to leave it as you have it. Vi vs. emacs. Coke vs. Pepsi.
Canada vs. US.

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

http://codereview.chromium.org/10815072/diff/6001/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/bluetooth_event_router.cc:164: const
experimental_bluetooth::Device& device) {
Can you call HasEventListener on the extension event router here and early out
(avoiding building up the JSON) if no one is listening?

http://codereview.chromium.org/10815072/diff/6001/chrome/browser/chromeos/ext...
File chrome/browser/chromeos/extensions/bluetooth_event_router.h (right):

http://codereview.chromium.org/10815072/diff/6001/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/bluetooth_event_router.h:83: DeviceList;
Can this be a ScopedVector instead of a vector of linked_ptr's? 

There was a thread recently on chromium-dev where people were saying
ScopedVector is prefererable. I vaguely recall there are some situations where
ScopedVector is less convenient though, so feel free to leave the code as-is if
it isn't easy to switch out.

[bryeung, 2012-08-07 14:22:12]

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

http://codereview.chromium.org/10815072/diff/6001/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/bluetooth_event_router.cc:164: const
experimental_bluetooth::Device& device) {
On 2012/08/03 23:35:23, Antony Sargent wrote:
> Can you call HasEventListener on the extension event router here and early out
> (avoiding building up the JSON) if no one is listening?

I'd prefer to leave it out, if you don't mind.

That would require repeating the event name, which makes the code less
maintainable IMO.  Also, gdk@ has a refactoring coming soon (should land today
or tomorrow) that removes the need to build the JSON altogether.

http://codereview.chromium.org/10815072/diff/6001/chrome/browser/chromeos/ext...
File chrome/browser/chromeos/extensions/bluetooth_event_router.h (right):

http://codereview.chromium.org/10815072/diff/6001/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/bluetooth_event_router.h:83: DeviceList;
On 2012/08/03 23:35:23, Antony Sargent wrote:
> Can this be a ScopedVector instead of a vector of linked_ptr's? 
> 
> There was a thread recently on chromium-dev where people were saying
> ScopedVector is prefererable. I vaguely recall there are some situations where
> ScopedVector is less convenient though, so feel free to leave the code as-is
if
> it isn't easy to switch out.

I had missed that thread: thanks for pointing it out.  Fixed.

[asargent_no_longer_on_chrome, 2012-08-07 15:32:48]

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

http://codereview.chromium.org/10815072/diff/6001/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/bluetooth_event_router.cc:164: const
experimental_bluetooth::Device& device) {
On 2012/08/07 14:22:12, bryeung wrote:
> On 2012/08/03 23:35:23, Antony Sargent wrote:
> > Can you call HasEventListener on the extension event router here and early
out
> > (avoiding building up the JSON) if no one is listening?
> 
> I'd prefer to leave it out, if you don't mind.
> 
> That would require repeating the event name, which makes the code less
> maintainable IMO.  Also, gdk@ has a refactoring coming soon (should land today
> or tomorrow) that removes the need to build the JSON altogether.

Ok, no problem.

[bryeung, 2012-08-08 14:39:20]

Scott: friendly ping for OWNERS review.  Thanks.

[keybuk, 2012-08-09 18:15:00]

lgtm

[commit-bot: I haz the power, 2012-08-09 19:13:41]

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

[commit-bot: I haz the power, 2012-08-09 19:14:30]

Presubmit check for 10815072-18001 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit Warnings **
License must match:
.*? Copyright (\(c\) )?(2012|2011|2010|2009|2008|2007|2006|2006-2008) The
Chromium Authors\. All rights reserved\.\n.*? Use of this source code is
governed by a BSD-style license that can be\n.*? found in the LICENSE file\.(?:
\*/)?\n
Found a bad license header in these files:
  chrome/test/data/extensions/api_test/bluetooth/test_discovery_expectation.js \
 
chrome/test/data/extensions/api_test/bluetooth/test_discovery_in_progress_expectation.js

Presubmit checks took 42.8s to calculate.

[commit-bot: I haz the power, 2012-08-09 19:19:16]

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

[commit-bot: I haz the power, 2012-08-09 20:59:06]

Change committed as 150898