Basic infrastructure for the Bluetooth API.

Basic infrastructure for the Bluetooth API.

This CL adds NIY stubs for most methods, just providing a few basic
status methods and events.

BUG=119336
TEST=Tested manually via a simple extension.  Tests will come as the bluetooth stack gains mocks.


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

[bryeung, 2012-03-21 20:21:40]

Here is a (mostly stub) implementation of the bluetooth api. Can you guys have a
look?

[asargent_no_longer_on_chrome, 2012-03-21 21:19:57]

https://chromiumcodereview.appspot.com/9822001/diff/2001/chrome/browser/exten...
File chrome/browser/extensions/api/bluetooth/bluetooth_api.cc (right):

https://chromiumcodereview.appspot.com/9822001/diff/2001/chrome/browser/exten...
chrome/browser/extensions/api/bluetooth/bluetooth_api.cc:26: SendResponse(true);
Since you can synchronously answer the question on one spin of the message loop,
BluetoothIsAvailableFunction should probably inherit from SyncExtensionFunction
instead of AsyncExtensionFunction, and you can skip the call to SendResponse
here.

(Unless of course you expect other implementations to have to post a task to
another thread or something to be able to answer the question).

https://chromiumcodereview.appspot.com/9822001/diff/2001/chrome/browser/exten...
chrome/browser/extensions/api/bluetooth/bluetooth_api.cc:30: bool
BluetoothIsPoweredFunction::RunImpl() {
same thing here

https://chromiumcodereview.appspot.com/9822001/diff/2001/chrome/common/extens...
File chrome/common/extensions/api/experimental.bluetooth.idl (right):

https://chromiumcodereview.appspot.com/9822001/diff/2001/chrome/common/extens...
chrome/common/extensions/api/experimental.bluetooth.idl:35: callback
BooleanCallback = void (boolean result);
On an email thread I think Mihai had made the suggestion: "Rather than a
separate result parameter to callbacks, the normal pattern is to signal errors
via chrome.extension.lastError (set via the error_ member in ExtensionFunction)"

So you could probably get rid of BooleanCallback, and just have:

 // Reports failures via chrome.extension.lastError.
 callback ResultCallback = void();

and use that in multiple functions below.

https://chromiumcodereview.appspot.com/9822001/diff/2001/chrome/common/extens...
chrome/common/extensions/api/experimental.bluetooth.idl:73: static void
disconnect(Socket socket, ResultCallback callback);
Would it make sense for the callback to be optional for this method? I can
imagine some scenarios where a caller doesn't care about handling disconnect
failure.

https://chromiumcodereview.appspot.com/9822001/diff/2001/chrome/common/extens...
chrome/common/extensions/api/experimental.bluetooth.idl:95: DOMString address,
OutOfBandPairingData data, ResultCallback callback);
same question here - can callback be optional?

[bryeung, 2012-03-21 21:34:13]

Thanks!  I think I've taken care of all of that now.

https://chromiumcodereview.appspot.com/9822001/diff/2001/chrome/browser/exten...
File chrome/browser/extensions/api/bluetooth/bluetooth_api.cc (right):

https://chromiumcodereview.appspot.com/9822001/diff/2001/chrome/browser/exten...
chrome/browser/extensions/api/bluetooth/bluetooth_api.cc:26: SendResponse(true);
On 2012/03/21 21:19:58, Antony Sargent wrote:
> Since you can synchronously answer the question on one spin of the message
loop,
> BluetoothIsAvailableFunction should probably inherit from
SyncExtensionFunction
> instead of AsyncExtensionFunction, and you can skip the call to SendResponse
> here.
> 
> (Unless of course you expect other implementations to have to post a task to
> another thread or something to be able to answer the question). 

Thanks for the clarification: done.

https://chromiumcodereview.appspot.com/9822001/diff/2001/chrome/browser/exten...
chrome/browser/extensions/api/bluetooth/bluetooth_api.cc:30: bool
BluetoothIsPoweredFunction::RunImpl() {
On 2012/03/21 21:19:58, Antony Sargent wrote:
> same thing here

Done.

https://chromiumcodereview.appspot.com/9822001/diff/2001/chrome/common/extens...
File chrome/common/extensions/api/experimental.bluetooth.idl (right):

https://chromiumcodereview.appspot.com/9822001/diff/2001/chrome/common/extens...
chrome/common/extensions/api/experimental.bluetooth.idl:35: callback
BooleanCallback = void (boolean result);
On 2012/03/21 21:19:58, Antony Sargent wrote:
> On an email thread I think Mihai had made the suggestion: "Rather than a
> separate result parameter to callbacks, the normal pattern is to signal errors
> via chrome.extension.lastError (set via the error_ member in
ExtensionFunction)"
> 
> So you could probably get rid of BooleanCallback, and just have:
> 
>  // Reports failures via chrome.extension.lastError.
>  callback ResultCallback = void();
> 
> and use that in multiple functions below.

Oops!  Thanks for the reminder: I had meant to do that but forgot while figuring
out how the rest of the IDL stuff works.  Thanks.

https://chromiumcodereview.appspot.com/9822001/diff/2001/chrome/common/extens...
chrome/common/extensions/api/experimental.bluetooth.idl:73: static void
disconnect(Socket socket, ResultCallback callback);
On 2012/03/21 21:19:58, Antony Sargent wrote:
> Would it make sense for the callback to be optional for this method? I can
> imagine some scenarios where a caller doesn't care about handling disconnect
> failure.

I changed all ResultCallback callbacks to be optional.

https://chromiumcodereview.appspot.com/9822001/diff/2001/chrome/common/extens...
chrome/common/extensions/api/experimental.bluetooth.idl:95: DOMString address,
OutOfBandPairingData data, ResultCallback callback);
On 2012/03/21 21:19:58, Antony Sargent wrote:
> same question here - can callback be optional?

Done.

[Mihai Parparita -not on Chrome, 2012-03-21 21:48:52]

https://chromiumcodereview.appspot.com/9822001/diff/2001/chrome/browser/exten...
File chrome/browser/extensions/api/bluetooth/bluetooth_api.cc (right):

https://chromiumcodereview.appspot.com/9822001/diff/2001/chrome/browser/exten...
chrome/browser/extensions/api/bluetooth/bluetooth_api.cc:23: return false;
You have a DCHECK for adapter not being null in BluetoothEventRouter, so do you
really want to code defensively for that scenario?

Per the style guide
(http://dev.chromium.org/developers/coding-style#TOC-CHECK-vs-DCHECK-and-NOTRE...),
it's better to crash for DCHECK failures, rather than trying to recover.

https://chromiumcodereview.appspot.com/9822001/diff/2001/chrome/common/extens...
File chrome/common/extensions/api/experimental.bluetooth.idl (right):

https://chromiumcodereview.appspot.com/9822001/diff/2001/chrome/common/extens...
chrome/common/extensions/api/experimental.bluetooth.idl:7: [nodoc] namespace
experimental.bluetooth {
Antony: Is there an IDL equivalent to the "platforms" option in the JSON schema
(http://code.google.com/codesearch#OAMlx_jo-ck/src/chrome/common/extensions/ap...

[asargent_no_longer_on_chrome, 2012-03-21 23:49:15]

> Antony: Is there an IDL equivalent to the "platforms" option in the JSON
schema

Not yet, but it should be easy to add. I created crbug.com/119398 to track this.

[bryeung, 2012-03-22 00:37:04]

Thanks Mihai: everything addressed I think.

https://chromiumcodereview.appspot.com/9822001/diff/2001/chrome/browser/exten...
File chrome/browser/extensions/api/bluetooth/bluetooth_api.cc (right):

https://chromiumcodereview.appspot.com/9822001/diff/2001/chrome/browser/exten...
chrome/browser/extensions/api/bluetooth/bluetooth_api.cc:23: return false;
On 2012/03/21 21:48:52, Mihai Parparita wrote:
> You have a DCHECK for adapter not being null in BluetoothEventRouter, so do
you
> really want to code defensively for that scenario?
> 
> Per the style guide
>
(http://dev.chromium.org/developers/coding-style#TOC-CHECK-vs-DCHECK-and-NOTRE...),
> it's better to crash for DCHECK failures, rather than trying to recover.

Thank you for pointing this out.  I removed the if statement here and below.

https://chromiumcodereview.appspot.com/9822001/diff/2001/chrome/common/extens...
File chrome/common/extensions/api/experimental.bluetooth.idl (right):

https://chromiumcodereview.appspot.com/9822001/diff/2001/chrome/common/extens...
chrome/common/extensions/api/experimental.bluetooth.idl:7: [nodoc] namespace
experimental.bluetooth {
On 2012/03/21 21:48:52, Mihai Parparita wrote:
> Antony: Is there an IDL equivalent to the "platforms" option in the JSON
schema
>
(http://code.google.com/codesearch#OAMlx_jo-ck/src/chrome/common/extensions/ap...
> 

I believe I've set this up to NIY for now.  Are you okay with this going in
as-is and being revised when the feature is available?

I've added a TODO.

[Mihai Parparita -not on Chrome, 2012-03-22 15:33:11]

LGTM

https://chromiumcodereview.appspot.com/9822001/diff/3015/chrome/common/extens...
File chrome/common/extensions/api/experimental.bluetooth.idl (right):

https://chromiumcodereview.appspot.com/9822001/diff/3015/chrome/common/extens...
chrome/common/extensions/api/experimental.bluetooth.idl:6: // TODO(bryeung):
mark this API as ChromeOS only (see crbug.com/119398).
This is fine.

[commit-bot: I haz the power, 2012-03-22 15:38:26]

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

[commit-bot: I haz the power, 2012-03-22 16:38:26]

Change committed as 128227