API Bindings for GATT server functionality.

API Bindings for GATT server functionality.

This CL adds the stub API bindings for adding GATT server functionality
to the chrome.bluetoothLowEnergy API. This only adds stubs with no actual
functional code.

The CL does do some cleanup on the existing code (mostly with getting rid
of IWYU lint issues in several files). It also makes the changes required
to the existing code for the minor changes in the existing IDL code.

R=ortuno@chromium.org, scheib@chromium.org
BUG=606217
Committed: https://crrev.com/0ffb07da3432b430adefad9041d4a0f3f4767020
Cr-Commit-Position: refs/heads/master@{#391446}

[rkc, 2016-04-25 23:41:39]

The CQ bit was checked by rkc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-25 23:42:28]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1915243003/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1915243003/1

[rkc, 2016-04-25 23:55:37]

Remove redundant is_local field

[rkc, 2016-04-25 23:56:46]

The CQ bit was checked by rkc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-25 23:57:09]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1915243003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1915243003/20001

[rkc, 2016-04-26 02:41:56]

The CQ bit was checked by rkc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-26 02:42:22]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1915243003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1915243003/100001

[commit-bot: I haz the power, 2016-04-26 03:35:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-26 03:35:42]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Devlin, 2016-04-26 23:14:04]

rdevlin.cronin@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[Devlin, 2016-04-26 23:14:06]

Are there any tests for the new API functions?  Also, can you include a link to
the API review in the CL description?

https://codereview.chromium.org/1915243003/diff/120001/chrome/browser/extensi...
File
chrome/browser/extensions/api/bluetooth_low_energy/bluetooth_low_energy_api.h
(right):

https://codereview.chromium.org/1915243003/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/api/bluetooth_low_energy/bluetooth_low_energy_api.h:64:
// DEPRECATED: This inherits from AsynExtensionFunction, which we're trying to
typo: Async, not Asyn

https://codereview.chromium.org/1915243003/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/api/bluetooth_low_energy/bluetooth_low_energy_api.h:413:
: public BluetoothLowEnergyExtensionFunctionDeprecated {
Why the deprecated version?

https://codereview.chromium.org/1915243003/diff/120001/chrome/common/extensio...
File chrome/common/extensions/api/bluetooth_low_energy.idl (right):

https://codereview.chromium.org/1915243003/diff/120001/chrome/common/extensio...
chrome/common/extensions/api/bluetooth_low_energy.idl:24: enum ServiceResult
{success, alreadyRegistered, notRegistered};
comment these enums.

https://codereview.chromium.org/1915243003/diff/120001/chrome/common/extensio...
chrome/common/extensions/api/bluetooth_low_energy.idl:174: callback
ValueCallback = void (ArrayBuffer value);
no space after void

https://codereview.chromium.org/1915243003/diff/120001/chrome/common/extensio...
chrome/common/extensions/api/bluetooth_low_energy.idl:208: // |service| : The
service to create.
no space after |service|

https://codereview.chromium.org/1915243003/diff/120001/chrome/common/extensio...
chrome/common/extensions/api/bluetooth_low_energy.idl:345: // ID is invalid,
this call will return false.
No it won't. ;)  Also, should this be an error?

https://codereview.chromium.org/1915243003/diff/120001/chrome/common/extensio...
chrome/common/extensions/api/bluetooth_low_energy.idl:352: // ID is invalid,
this call will return false.
ditto

https://codereview.chromium.org/1915243003/diff/120001/chrome/common/extensio...
chrome/common/extensions/api/bluetooth_low_energy.idl:419: // Fired when a
connected central device requests to read the value of a
Do we ever call these events?

https://codereview.chromium.org/1915243003/diff/120001/chrome/common/extensio...
File chrome/common/extensions/docs/templates/intros/bluetooth_low_energy.html
(right):

https://codereview.chromium.org/1915243003/diff/120001/chrome/common/extensio...
chrome/common/extensions/docs/templates/intros/bluetooth_low_energy.html:3: This
API works <b>only on Android, OS X, Windows and Chrome OS</b>.
Android?  We have extensions on Android?

[rkc, 2016-04-27 00:00:39]

There was no formal API review for this but the design doc in which this API is
proposed has been approved by the Chrome OS leads. The design doc is linked on
the corresponding bug.

I'm adding tests for the functions as I implement them. The implementation +
tests for the create attribute functions are here:
https://codereview.chromium.org/1920353002/

https://codereview.chromium.org/1915243003/diff/120001/chrome/browser/extensi...
File
chrome/browser/extensions/api/bluetooth_low_energy/bluetooth_low_energy_api.h
(right):

https://codereview.chromium.org/1915243003/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/api/bluetooth_low_energy/bluetooth_low_energy_api.h:64:
// DEPRECATED: This inherits from AsynExtensionFunction, which we're trying to
On 2016/04/26 23:14:05, Devlin wrote:
> typo: Async, not Asyn

Done.

https://codereview.chromium.org/1915243003/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/api/bluetooth_low_energy/bluetooth_low_energy_api.h:413:
: public BluetoothLowEnergyExtensionFunctionDeprecated {
On 2016/04/26 23:14:05, Devlin wrote:
> Why the deprecated version?

Whoops, fixed.

https://codereview.chromium.org/1915243003/diff/120001/chrome/common/extensio...
File chrome/common/extensions/api/bluetooth_low_energy.idl (right):

https://codereview.chromium.org/1915243003/diff/120001/chrome/common/extensio...
chrome/common/extensions/api/bluetooth_low_energy.idl:24: enum ServiceResult
{success, alreadyRegistered, notRegistered};
On 2016/04/26 23:14:05, Devlin wrote:
> comment these enums.

Done.

https://codereview.chromium.org/1915243003/diff/120001/chrome/common/extensio...
chrome/common/extensions/api/bluetooth_low_energy.idl:174: callback
ValueCallback = void (ArrayBuffer value);
On 2016/04/26 23:14:05, Devlin wrote:
> no space after void

Done.

https://codereview.chromium.org/1915243003/diff/120001/chrome/common/extensio...
chrome/common/extensions/api/bluetooth_low_energy.idl:208: // |service| : The
service to create.
On 2016/04/26 23:14:06, Devlin wrote:
> no space after |service|

I think I am missing something. All other parameters in every function have a
space after the | and before the :
Any reason why this should be different?

https://codereview.chromium.org/1915243003/diff/120001/chrome/common/extensio...
chrome/common/extensions/api/bluetooth_low_energy.idl:345: // ID is invalid,
this call will return false.
On 2016/04/26 23:14:05, Devlin wrote:
> No it won't. ;)  Also, should this be an error?

Done.

https://codereview.chromium.org/1915243003/diff/120001/chrome/common/extensio...
chrome/common/extensions/api/bluetooth_low_energy.idl:352: // ID is invalid,
this call will return false.
On 2016/04/26 23:14:05, Devlin wrote:
> ditto

Done.

https://codereview.chromium.org/1915243003/diff/120001/chrome/common/extensio...
chrome/common/extensions/api/bluetooth_low_energy.idl:419: // Fired when a
connected central device requests to read the value of a
On 2016/04/26 23:14:05, Devlin wrote:
> Do we ever call these events?

Not yet, but I have CLs on the way that will be calling them.

https://codereview.chromium.org/1915243003/diff/120001/chrome/common/extensio...
File chrome/common/extensions/docs/templates/intros/bluetooth_low_energy.html
(right):

https://codereview.chromium.org/1915243003/diff/120001/chrome/common/extensio...
chrome/common/extensions/docs/templates/intros/bluetooth_low_energy.html:3: This
API works <b>only on Android, OS X, Windows and Chrome OS</b>.
On 2016/04/26 23:14:06, Devlin wrote:
> Android?  We have extensions on Android?

Done.

[Devlin, 2016-04-27 14:45:03]

lgtm from a high-level standpoint, but please wait for a btle owner to take a
look.

Thanks for converting to ExtensionFunction :)

https://codereview.chromium.org/1915243003/diff/120001/chrome/common/extensio...
File chrome/common/extensions/api/bluetooth_low_energy.idl (right):

https://codereview.chromium.org/1915243003/diff/120001/chrome/common/extensio...
chrome/common/extensions/api/bluetooth_low_energy.idl:208: // |service| : The
service to create.
On 2016/04/27 00:00:39, Rahul Chaturvedi wrote:
> On 2016/04/26 23:14:06, Devlin wrote:
> > no space after |service|
> 
> I think I am missing something. All other parameters in every function have a
> space after the | and before the :
> Any reason why this should be different?

They probably copied style from another API idl file that did it erroneously -
unfortunately style mistakes like this are often viral. ;)  We don't need to fix
all of them now (the docs should still render fine), but let's be right going
forward.

https://codereview.chromium.org/1915243003/diff/120001/chrome/common/extensio...
chrome/common/extensions/api/bluetooth_low_energy.idl:419: // Fired when a
connected central device requests to read the value of a
On 2016/04/27 00:00:39, Rahul Chaturvedi wrote:
> On 2016/04/26 23:14:05, Devlin wrote:
> > Do we ever call these events?
> 
> Not yet, but I have CLs on the way that will be calling them.

I'd rather add idl entries with support.  These generate our docs, so it's kind
of weird to have a documented but non-existent function.  And there's not really
much reason to add them separately.

https://codereview.chromium.org/1915243003/diff/140001/chrome/browser/extensi...
File
chrome/browser/extensions/api/bluetooth_low_energy/bluetooth_low_energy_api.cc
(right):

https://codereview.chromium.org/1915243003/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/api/bluetooth_low_energy/bluetooth_low_energy_api.cc:132:
void DoWorkReturnBoolCallback(const base::Callback<bool()>& callback) {
if you wanted to be extra fancy you could do something like:
<template T>
void DoWorkCallback(const T& callback) {
  DCHECK(!callback.is_null());
  callback.Run();
}

That said, is the result of bool DoWork() ever used?  If not, you could just
convert it to void and only have the one.

https://codereview.chromium.org/1915243003/diff/140001/chrome/browser/extensi...
File
chrome/browser/extensions/api/bluetooth_low_energy/bluetooth_low_energy_api.h
(right):

https://codereview.chromium.org/1915243003/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/api/bluetooth_low_energy/bluetooth_low_energy_api.h:12:
#include
"chrome/browser/extensions/api/bluetooth_low_energy/bluetooth_api_advertisement.h"
do we need the full include here, or is the forward declaration sufficient?

[rkc, 2016-04-27 20:38:56]

https://codereview.chromium.org/1915243003/diff/120001/chrome/common/extensio...
File chrome/common/extensions/api/bluetooth_low_energy.idl (right):

https://codereview.chromium.org/1915243003/diff/120001/chrome/common/extensio...
chrome/common/extensions/api/bluetooth_low_energy.idl:208: // |service| : The
service to create.
On 2016/04/27 14:45:03, Devlin wrote:
> On 2016/04/27 00:00:39, Rahul Chaturvedi wrote:
> > On 2016/04/26 23:14:06, Devlin wrote:
> > > no space after |service|
> > 
> > I think I am missing something. All other parameters in every function have
a
> > space after the | and before the :
> > Any reason why this should be different?
> 
> They probably copied style from another API idl file that did it erroneously -
> unfortunately style mistakes like this are often viral. ;)  We don't need to
fix
> all of them now (the docs should still render fine), but let's be right going
> forward.

Sure. Also fixed all of the parameters in this file at least.
Done.

https://codereview.chromium.org/1915243003/diff/120001/chrome/common/extensio...
chrome/common/extensions/api/bluetooth_low_energy.idl:419: // Fired when a
connected central device requests to read the value of a
On 2016/04/27 14:45:03, Devlin wrote:
> On 2016/04/27 00:00:39, Rahul Chaturvedi wrote:
> > On 2016/04/26 23:14:05, Devlin wrote:
> > > Do we ever call these events?
> > 
> > Not yet, but I have CLs on the way that will be calling them.
> 
> I'd rather add idl entries with support.  These generate our docs, so it's
kind
> of weird to have a documented but non-existent function.  And there's not
really
> much reason to add them separately.

This is part 1 of a 3 patch series. The third patch will definitely have code
using this. I am currently writing that code.

https://codereview.chromium.org/1915243003/diff/140001/chrome/browser/extensi...
File
chrome/browser/extensions/api/bluetooth_low_energy/bluetooth_low_energy_api.cc
(right):

https://codereview.chromium.org/1915243003/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/api/bluetooth_low_energy/bluetooth_low_energy_api.cc:132:
void DoWorkReturnBoolCallback(const base::Callback<bool()>& callback) {
On 2016/04/27 14:45:03, Devlin wrote:
> if you wanted to be extra fancy you could do something like:
> <template T>
> void DoWorkCallback(const T& callback) {
>   DCHECK(!callback.is_null());
>   callback.Run();
> }
> 
> That said, is the result of bool DoWork() ever used?  If not, you could just
> convert it to void and only have the one.

Converting DoWork to return void would require changes throughout the file. I
would rather do that (and other changes possibly, like moving them all over) in
a separate CL than bundle them all in this CL.

I do like the fancy solution though, switching to it; though we'll have to
explicitly specialize the template during the Bind call, otherwise it won't be
able to infer the type of the Functor.

Done.

https://codereview.chromium.org/1915243003/diff/140001/chrome/browser/extensi...
File
chrome/browser/extensions/api/bluetooth_low_energy/bluetooth_low_energy_api.h
(right):

https://codereview.chromium.org/1915243003/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/api/bluetooth_low_energy/bluetooth_low_energy_api.h:12:
#include
"chrome/browser/extensions/api/bluetooth_low_energy/bluetooth_api_advertisement.h"
On 2016/04/27 14:45:03, Devlin wrote:
> do we need the full include here, or is the forward declaration sufficient?

We need the full include. We refer to an enum within the class below.

[rkc, 2016-04-27 22:24:06]

Made several changes to remove callbacks from the events as per discussion with
Devlin.

[Devlin, 2016-04-27 22:30:15]

https://codereview.chromium.org/1915243003/diff/180001/chrome/common/extensio...
File chrome/common/extensions/api/bluetooth_low_energy.idl (right):

https://codereview.chromium.org/1915243003/diff/180001/chrome/common/extensio...
chrome/common/extensions/api/bluetooth_low_energy.idl:240: DOMString serviceId,
indentation

https://codereview.chromium.org/1915243003/diff/180001/chrome/common/extensio...
chrome/common/extensions/api/bluetooth_low_energy.idl:275: DOMString
characteristicId,
indentation

https://codereview.chromium.org/1915243003/diff/180001/chrome/common/extensio...
chrome/common/extensions/api/bluetooth_low_energy.idl:354: static void
registerService(DOMString serviceId,
put all arguments at equal indentation, so either
static void registerService(
    DOMString serviceId, ServiceResultCallback callback);
or
static void registerService(DOMString serviceId,
                            ServiceResultCallback callback);

https://codereview.chromium.org/1915243003/diff/180001/chrome/common/extensio...
chrome/common/extensions/api/bluetooth_low_energy.idl:398: static void
sendRequestResponse(long requestId, boolean isError,
for functions like this, it's a lot nicer to take an object parameter, e.g.
dictionary SendRequestResponseOptions {
  long requestId,
  boolean isError,
  ArrayBuffer? value,
}

static void sendRequestResponse(SendRequestResonseOptions options);

That way when we decide to add another param, it's really easy and we don't
break anyone.

https://codereview.chromium.org/1915243003/diff/180001/chrome/common/extensio...
chrome/common/extensions/api/bluetooth_low_energy.idl:399: optional ArrayBuffer
value);
ditto

https://codereview.chromium.org/1915243003/diff/180001/chrome/common/extensio...
chrome/common/extensions/api/bluetooth_low_energy.idl:441: static void
onCharacteristicReadRequest(long requestId, Device device,
argument indentation (all of these)

[rkc, 2016-04-27 22:40:23]

https://codereview.chromium.org/1915243003/diff/180001/chrome/common/extensio...
File chrome/common/extensions/api/bluetooth_low_energy.idl (right):

https://codereview.chromium.org/1915243003/diff/180001/chrome/common/extensio...
chrome/common/extensions/api/bluetooth_low_energy.idl:240: DOMString serviceId,
On 2016/04/27 22:30:15, Devlin wrote:
> indentation

Done.

https://codereview.chromium.org/1915243003/diff/180001/chrome/common/extensio...
chrome/common/extensions/api/bluetooth_low_energy.idl:275: DOMString
characteristicId,
On 2016/04/27 22:30:15, Devlin wrote:
> indentation

Done.

https://codereview.chromium.org/1915243003/diff/180001/chrome/common/extensio...
chrome/common/extensions/api/bluetooth_low_energy.idl:354: static void
registerService(DOMString serviceId,
On 2016/04/27 22:30:15, Devlin wrote:
> put all arguments at equal indentation, so either
> static void registerService(
>     DOMString serviceId, ServiceResultCallback callback);
> or
> static void registerService(DOMString serviceId,
>                             ServiceResultCallback callback);

Done.

https://codereview.chromium.org/1915243003/diff/180001/chrome/common/extensio...
chrome/common/extensions/api/bluetooth_low_energy.idl:398: static void
sendRequestResponse(long requestId, boolean isError,
On 2016/04/27 22:30:15, Devlin wrote:
> for functions like this, it's a lot nicer to take an object parameter, e.g.
> dictionary SendRequestResponseOptions {
>   long requestId,
>   boolean isError,
>   ArrayBuffer? value,
> }
> 
> static void sendRequestResponse(SendRequestResonseOptions options);
> 
> That way when we decide to add another param, it's really easy and we don't
> break anyone.

Done.

https://codereview.chromium.org/1915243003/diff/180001/chrome/common/extensio...
chrome/common/extensions/api/bluetooth_low_energy.idl:399: optional ArrayBuffer
value);
On 2016/04/27 22:30:15, Devlin wrote:
> ditto

Done.

https://codereview.chromium.org/1915243003/diff/180001/chrome/common/extensio...
chrome/common/extensions/api/bluetooth_low_energy.idl:441: static void
onCharacteristicReadRequest(long requestId, Device device,
On 2016/04/27 22:30:15, Devlin wrote:
> argument indentation (all of these)

Done.

[rkc, 2016-04-28 03:07:44]

Description was changed from

==========
API Bindings for GATT server functionality.

This CL adds the stub API bindings for adding GATT server functionality
to the chrome.bluetoothLowEnergy API. This only adds stubs with no actual
functional code.

The CL does do some cleanup on the existing code (mostly with getting rid
of IWYU lint issues in several files). It also makes the changes required
to the existing code for the minor changes in the existing IDL code.

R=ortuno@chromium.org, scheib@chromium.org
BUG=606217
==========

to

==========
API Bindings for GATT server functionality.

This CL adds the stub API bindings for adding GATT server functionality
to the chrome.bluetoothLowEnergy API. This only adds stubs with no actual
functional code.

The CL does do some cleanup on the existing code (mostly with getting rid
of IWYU lint issues in several files). It also makes the changes required
to the existing code for the minor changes in the existing IDL code.

R=ortuno@chromium.org, scheib@chromium.org
BUG=606217
==========

[rkc, 2016-04-28 03:07:44]

rkc@chromium.org changed reviewers:
+ isherman@chromium.org

[rkc, 2016-04-28 03:08:13]

Added isherman@ for owners review of the metrics changes.

[rkc, 2016-04-28 03:10:43]

The CQ bit was checked by rkc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-28 03:11:04]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1915243003/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1915243003/220001

[commit-bot: I haz the power, 2016-04-28 03:14:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-28 03:14:11]

Dry run: Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_gn...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Ilya Sherman, 2016-04-28 17:19:21]

histograms.xml lgtm

[rkc, 2016-04-28 19:38:38]

The CQ bit was checked by rkc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-28 19:38:59]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1915243003/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1915243003/240001

[commit-bot: I haz the power, 2016-04-28 20:42:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-28 20:42:10]

Dry run: Try jobs failed on following builders:
  win_clang on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[rkc, 2016-04-29 17:09:16]

The CQ bit was checked by rkc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-29 17:09:29]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1915243003/280001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1915243003/280001

[Devlin, 2016-04-29 17:13:13]

https://codereview.chromium.org/1915243003/diff/280001/chrome/browser/extensi...
File
chrome/browser/extensions/api/bluetooth_low_energy/bluetooth_low_energy_api.cc
(right):

https://codereview.chromium.org/1915243003/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/api/bluetooth_low_energy/bluetooth_low_energy_api.cc:1106:
Respond(ArgumentList(apibtle::CreateService::Results::Create(std::string())));
What's the error?

This isn't quite fine, because then the extension function never responds - it's
just left hanging.  Also, who says this isn't on Windows?  If it's not, why is
it being compiled?

https://codereview.chromium.org/1915243003/diff/280001/chrome/common/extensio...
File chrome/common/extensions/api/bluetooth_low_energy.idl (right):

https://codereview.chromium.org/1915243003/diff/280001/chrome/common/extensio...
chrome/common/extensions/api/bluetooth_low_energy.idl:449: long requestId,
Device device, Characteristic characteristic);
I'd consider making these dictionaries, too, so that it's easy to add parameters
later.  But I'll leave it up to you.

https://codereview.chromium.org/1915243003/diff/280001/chrome/common/extensio...
File chrome/common/extensions/docs/templates/intros/bluetooth_low_energy.html
(right):

https://codereview.chromium.org/1915243003/diff/280001/chrome/common/extensio...
chrome/common/extensions/docs/templates/intros/bluetooth_low_energy.html:3: This
API works <b>only on OS X, Windows and Chrome OS</b>.
This seems to imply Windows...

[commit-bot: I haz the power, 2016-04-29 18:29:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-29 18:29:39]

Dry run: This issue passed the CQ dry run.

[rkc, 2016-04-29 20:15:58]

https://codereview.chromium.org/1915243003/diff/280001/chrome/browser/extensi...
File
chrome/browser/extensions/api/bluetooth_low_energy/bluetooth_low_energy_api.cc
(right):

https://codereview.chromium.org/1915243003/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/api/bluetooth_low_energy/bluetooth_low_energy_api.cc:1106:
Respond(ArgumentList(apibtle::CreateService::Results::Create(std::string())));
On 2016/04/29 17:13:13, Devlin wrote:
> What's the error?
> 
> This isn't quite fine, because then the extension function never responds -
it's
> just left hanging.  Also, who says this isn't on Windows?  If it's not, why is
> it being compiled?

Added to the permissions, this function will only be available on
ChromeOS/Linux.

https://codereview.chromium.org/1915243003/diff/280001/chrome/common/extensio...
File chrome/common/extensions/api/bluetooth_low_energy.idl (right):

https://codereview.chromium.org/1915243003/diff/280001/chrome/common/extensio...
chrome/common/extensions/api/bluetooth_low_energy.idl:449: long requestId,
Device device, Characteristic characteristic);
On 2016/04/29 17:13:13, Devlin wrote:
> I'd consider making these dictionaries, too, so that it's easy to add
parameters
> later.  But I'll leave it up to you.

Done.

https://codereview.chromium.org/1915243003/diff/280001/chrome/common/extensio...
File chrome/common/extensions/docs/templates/intros/bluetooth_low_energy.html
(right):

https://codereview.chromium.org/1915243003/diff/280001/chrome/common/extensio...
chrome/common/extensions/docs/templates/intros/bluetooth_low_energy.html:3: This
API works <b>only on OS X, Windows and Chrome OS</b>.
On 2016/04/29 17:13:13, Devlin wrote:
> This seems to imply Windows...

This intro is for the whole API, not just the added functions.
I wouldn't even be adding this file here at all unless the presubmit didn't
absolutely require me to.

[rkc, 2016-04-30 20:31:44]

The CQ bit was checked by rkc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-30 20:31:54]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1915243003/340001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1915243003/340001

[commit-bot: I haz the power, 2016-04-30 21:11:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-30 21:11:07]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Devlin, 2016-05-02 17:04:39]

https://codereview.chromium.org/1915243003/diff/340001/chrome/browser/extensi...
File
chrome/browser/extensions/api/bluetooth_low_energy/bluetooth_low_energy_api.cc
(right):

https://codereview.chromium.org/1915243003/diff/340001/chrome/browser/extensi...
chrome/browser/extensions/api/bluetooth_low_energy/bluetooth_low_energy_api.cc:233:
if (!BluetoothManifestData::CheckLowEnergyPermitted(extension()))
This still isn't ideal, because:
a) The bluetooth api object is available to the extension, i.e.
chrome.bluetoothLowEnergy.createService() will exist.
b) This code will need to be compiled cross-platform.
c) Everyone writing a new function here needs to remember to do this.
d) It makes it a lot harder to find out where these functions are actually
supported.

I'd much rather we support this through our features files - we have them to
address exactly these cases.  It looks like CheckLowEnergyPermitted() basically
just checks a bit of the manifest - is there a reason we can't have that bit
restricted to a certain platform in the _manifest_features.json file, and depend
upon it in the api_features_.json file?

[Devlin, 2016-05-02 22:27:58]

lgtm again.  Sorry for the back and forth, and thanks for the experimentation.

https://codereview.chromium.org/1915243003/diff/360001/chrome/browser/extensi...
File
chrome/browser/extensions/api/bluetooth_low_energy/bluetooth_low_energy_api.cc
(right):

https://codereview.chromium.org/1915243003/diff/360001/chrome/browser/extensi...
chrome/browser/extensions/api/bluetooth_low_energy/bluetooth_low_energy_api.cc:1120:
Respond(ArgumentList(apibtle::CreateService::Results::Create(std::string())));
So this is because of windows evil renaming to CreateServiceW, right?  Can you
add a note of that?  Also, can you add a TODO that, ideally, we'd be able to
handle this in our feature system, but right now it's too limited?

https://codereview.chromium.org/1915243003/diff/360001/chrome/browser/extensi...
File
chrome/browser/extensions/api/bluetooth_low_energy/bluetooth_low_energy_api.h
(right):

https://codereview.chromium.org/1915243003/diff/360001/chrome/browser/extensi...
chrome/browser/extensions/api/bluetooth_low_energy/bluetooth_low_energy_api.h:21:
namespace apibtle = extensions::api::bluetooth_low_energy;
nit: this isn't typically allowed - only in .cc files where it doesn't pollute
the global namespace.

[rkc, 2016-05-02 23:25:51]

https://codereview.chromium.org/1915243003/diff/360001/chrome/browser/extensi...
File
chrome/browser/extensions/api/bluetooth_low_energy/bluetooth_low_energy_api.cc
(right):

https://codereview.chromium.org/1915243003/diff/360001/chrome/browser/extensi...
chrome/browser/extensions/api/bluetooth_low_energy/bluetooth_low_energy_api.cc:1120:
Respond(ArgumentList(apibtle::CreateService::Results::Create(std::string())));
On 2016/05/02 22:27:58, Devlin wrote:
> So this is because of windows evil renaming to CreateServiceW, right?  Can you
> add a note of that?  Also, can you add a TODO that, ideally, we'd be able to
> handle this in our feature system, but right now it's too limited?

Done.

https://codereview.chromium.org/1915243003/diff/360001/chrome/browser/extensi...
File
chrome/browser/extensions/api/bluetooth_low_energy/bluetooth_low_energy_api.h
(right):

https://codereview.chromium.org/1915243003/diff/360001/chrome/browser/extensi...
chrome/browser/extensions/api/bluetooth_low_energy/bluetooth_low_energy_api.h:21:
namespace apibtle = extensions::api::bluetooth_low_energy;
On 2016/05/02 22:27:58, Devlin wrote:
> nit: this isn't typically allowed - only in .cc files where it doesn't pollute
> the global namespace.

Done.

[rkc, 2016-05-02 23:27:04]

The CQ bit was checked by rkc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-02 23:27:34]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1915243003/380001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1915243003/380001

[commit-bot: I haz the power, 2016-05-03 00:19:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-03 00:19:16]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_clang on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[rkc, 2016-05-03 20:54:50]

The CQ bit was checked by rkc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-03 20:55:23]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1915243003/400001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1915243003/400001

[commit-bot: I haz the power, 2016-05-03 21:50:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-03 21:50:39]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[rkc, 2016-05-04 00:02:13]

mac_win_test_fix

[rkc, 2016-05-04 00:02:37]

The CQ bit was checked by rkc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-04 00:03:36]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1915243003/420001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1915243003/420001

[scheib, 2016-05-04 00:30:04]

https://codereview.chromium.org/1915243003/diff/400001/chrome/common/extensio...
File chrome/common/extensions/api/bluetooth_low_energy.idl (right):

https://codereview.chromium.org/1915243003/diff/400001/chrome/common/extensio...
chrome/common/extensions/api/bluetooth_low_energy.idl:171: // Unique ID for this
request. Use this ID when responding this request.
responding to

https://codereview.chromium.org/1915243003/diff/400001/chrome/common/extensio...
chrome/common/extensions/api/bluetooth_low_energy.idl:233: // Create a locally
hosted GATT service. This service can be registered
I don't remember if / how we can flag documentation for private only API
surfaces. It is nice to have it documented - but poor to have external devs
believe they can use it.

https://codereview.chromium.org/1915243003/diff/400001/chrome/common/extensio...
chrome/common/extensions/api/bluetooth_low_energy.idl:372: // ID is invalid, the
lastError will be set.
lastError is set, but also the ServiceResultCallback contains error responses.
Do we need both?

[rkc, 2016-05-04 00:42:44]

https://codereview.chromium.org/1915243003/diff/400001/chrome/common/extensio...
File chrome/common/extensions/api/bluetooth_low_energy.idl (right):

https://codereview.chromium.org/1915243003/diff/400001/chrome/common/extensio...
chrome/common/extensions/api/bluetooth_low_energy.idl:171: // Unique ID for this
request. Use this ID when responding this request.
On 2016/05/04 00:30:04, scheib wrote:
> responding to

Done.

https://codereview.chromium.org/1915243003/diff/400001/chrome/common/extensio...
chrome/common/extensions/api/bluetooth_low_energy.idl:233: // Create a locally
hosted GATT service. This service can be registered
On 2016/05/04 00:30:04, scheib wrote:
> I don't remember if / how we can flag documentation for private only API
> surfaces. It is nice to have it documented - but poor to have external devs
> believe they can use it.

We can [nodoc] it but then it goes away from any document generation. The
advertisement APIs are also similarly restricted, but we haven't nodoc'ed them.
We should follow the same pattern for now; I don't see a reason why in M53 we
wouldn't just open these APIs up for everyone on Chrome OS/Linux.

https://codereview.chromium.org/1915243003/diff/400001/chrome/common/extensio...
chrome/common/extensions/api/bluetooth_low_energy.idl:372: // ID is invalid, the
lastError will be set.
On 2016/05/04 00:30:04, scheib wrote:
> lastError is set, but also the ServiceResultCallback contains error responses.
> Do we need both?

We won't get a service error since we won't even try to register a service is a
user gives an invalid service ID. There would be no point. Instead, we use the
usual mechanism to let the user know that the service ID itself is invalid.

[scheib, 2016-05-04 01:26:32]

https://codereview.chromium.org/1915243003/diff/400001/chrome/common/extensio...
File chrome/common/extensions/api/bluetooth_low_energy.idl (right):

https://codereview.chromium.org/1915243003/diff/400001/chrome/common/extensio...
chrome/common/extensions/api/bluetooth_low_energy.idl:233: // Create a locally
hosted GATT service. This service can be registered
On 2016/05/04 00:42:44, Rahul Chaturvedi wrote:
> On 2016/05/04 00:30:04, scheib wrote:
> > I don't remember if / how we can flag documentation for private only API
> > surfaces. It is nice to have it documented - but poor to have external devs
> > believe they can use it.
> 
> We can [nodoc] it but then it goes away from any document generation. The
> advertisement APIs are also similarly restricted, but we haven't nodoc'ed
them.
> We should follow the same pattern for now; I don't see a reason why in M53 we
> wouldn't just open these APIs up for everyone on Chrome OS/Linux.

Thx, to be clear though as the code is now
  "contexts": ["blessed_extension"]
means that only whitelisted apps can use the apis, right?

https://codereview.chromium.org/1915243003/diff/400001/chrome/common/extensio...
chrome/common/extensions/api/bluetooth_low_energy.idl:372: // ID is invalid, the
lastError will be set.
On 2016/05/04 00:42:44, Rahul Chaturvedi wrote:
> On 2016/05/04 00:30:04, scheib wrote:
> > lastError is set, but also the ServiceResultCallback contains error
responses.
> > Do we need both?
> 
> We won't get a service error since we won't even try to register a service is
a
> user gives an invalid service ID. There would be no point. Instead, we use the
> usual mechanism to let the user know that the service ID itself is invalid.

Sorry I'm not following --- there are two error routes now (at least per this
comment and idl): lastError and the ServiceResultCallback. Are there different
scenarios which cause each in turn to be utilized? Why are both needed?

[rkc, 2016-05-04 01:53:29]

https://codereview.chromium.org/1915243003/diff/400001/chrome/common/extensio...
File chrome/common/extensions/api/bluetooth_low_energy.idl (right):

https://codereview.chromium.org/1915243003/diff/400001/chrome/common/extensio...
chrome/common/extensions/api/bluetooth_low_energy.idl:233: // Create a locally
hosted GATT service. This service can be registered
On 2016/05/04 01:26:32, scheib wrote:
> On 2016/05/04 00:42:44, Rahul Chaturvedi wrote:
> > On 2016/05/04 00:30:04, scheib wrote:
> > > I don't remember if / how we can flag documentation for private only API
> > > surfaces. It is nice to have it documented - but poor to have external
devs
> > > believe they can use it.
> > 
> > We can [nodoc] it but then it goes away from any document generation. The
> > advertisement APIs are also similarly restricted, but we haven't nodoc'ed
> them.
> > We should follow the same pattern for now; I don't see a reason why in M53
we
> > wouldn't just open these APIs up for everyone on Chrome OS/Linux.
> 
> Thx, to be clear though as the code is now
>   "contexts": ["blessed_extension"]
> means that only whitelisted apps can use the apis, right?

Actually, I haven't really documented this. The whitelist is not enforced via
the features system - that simply prevents the API from being exposed on
platforms that it isn't supported on.

The whitelist is enforced via the peripheral permission. Not all apps are
allowed to use that permission.

Added documentation to all the GATT functions and events mention the requirement
of the peripheral permission.

https://codereview.chromium.org/1915243003/diff/400001/chrome/common/extensio...
chrome/common/extensions/api/bluetooth_low_energy.idl:372: // ID is invalid, the
lastError will be set.
On 2016/05/04 01:26:32, scheib wrote:
> On 2016/05/04 00:42:44, Rahul Chaturvedi wrote:
> > On 2016/05/04 00:30:04, scheib wrote:
> > > lastError is set, but also the ServiceResultCallback contains error
> responses.
> > > Do we need both?
> > 
> > We won't get a service error since we won't even try to register a service
is
> a
> > user gives an invalid service ID. There would be no point. Instead, we use
the
> > usual mechanism to let the user know that the service ID itself is invalid.
> 
> Sorry I'm not following --- there are two error routes now (at least per this
> comment and idl): lastError and the ServiceResultCallback. Are there different
> scenarios which cause each in turn to be utilized? Why are both needed?

Yes. The lastError error route is used when the call to register service itself
is invalid since it is done with an invalid service ID. We don't consider this
an error of register service, but rather a user error - who is giving us an
invalid service ID.

If the registerService call itself fails, then it is an error with the action of
trying to register a service, in which case, we return the result via callback
with an appropriate error code. That error code will indicate a registration
failure, not the failure of the user to give us a valid service ID to register.

Same for unregisterService.

[scheib, 2016-05-04 02:30:32]

LGTM.

[rkc, 2016-05-04 03:28:50]

The CQ bit was checked by rkc@chromium.org

[rkc, 2016-05-04 03:28:51]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org, rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/1915243003/#ps480001
(title: " ")

[commit-bot: I haz the power, 2016-05-04 03:29:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1915243003/480001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1915243003/480001

[commit-bot: I haz the power, 2016-05-04 04:53:18]

Description was changed from

==========
API Bindings for GATT server functionality.

This CL adds the stub API bindings for adding GATT server functionality
to the chrome.bluetoothLowEnergy API. This only adds stubs with no actual
functional code.

The CL does do some cleanup on the existing code (mostly with getting rid
of IWYU lint issues in several files). It also makes the changes required
to the existing code for the minor changes in the existing IDL code.

R=ortuno@chromium.org, scheib@chromium.org
BUG=606217
==========

to

==========
API Bindings for GATT server functionality.

This CL adds the stub API bindings for adding GATT server functionality
to the chrome.bluetoothLowEnergy API. This only adds stubs with no actual
functional code.

The CL does do some cleanup on the existing code (mostly with getting rid
of IWYU lint issues in several files). It also makes the changes required
to the existing code for the minor changes in the existing IDL code.

R=ortuno@chromium.org, scheib@chromium.org
BUG=606217
==========

[commit-bot: I haz the power, 2016-05-04 04:53:20]

Committed patchset #25 (id:480001)

[commit-bot: I haz the power, 2016-05-04 04:55:10]

Description was changed from

==========
API Bindings for GATT server functionality.

This CL adds the stub API bindings for adding GATT server functionality
to the chrome.bluetoothLowEnergy API. This only adds stubs with no actual
functional code.

The CL does do some cleanup on the existing code (mostly with getting rid
of IWYU lint issues in several files). It also makes the changes required
to the existing code for the minor changes in the existing IDL code.

R=ortuno@chromium.org, scheib@chromium.org
BUG=606217
==========

to

==========
API Bindings for GATT server functionality.

This CL adds the stub API bindings for adding GATT server functionality
to the chrome.bluetoothLowEnergy API. This only adds stubs with no actual
functional code.

The CL does do some cleanup on the existing code (mostly with getting rid
of IWYU lint issues in several files). It also makes the changes required
to the existing code for the minor changes in the existing IDL code.

R=ortuno@chromium.org, scheib@chromium.org
BUG=606217

Committed: https://crrev.com/0ffb07da3432b430adefad9041d4a0f3f4767020
Cr-Commit-Position: refs/heads/master@{#391446}
==========

[commit-bot: I haz the power, 2016-05-04 04:55:12]

Patchset 25 (id:??) landed as
https://crrev.com/0ffb07da3432b430adefad9041d4a0f3f4767020
Cr-Commit-Position: refs/heads/master@{#391446}