Implement the advertising functions for the BLE API.

Implement the advertising functions for the BLE API.

This CL implements the RegisterAdvertisement and UnregisterAdvertisement
functions for the Chrome API.

BUG=466375
Committed: https://crrev.com/58666c84553ecc058af5e6d178d6d8d3f47e99c1
Cr-Commit-Position: refs/heads/master@{#330182}

[armansito, 2015-05-08 22:46:12]

armansito@chromium.org changed reviewers:
+ armansito@chromium.org

[armansito, 2015-05-08 22:46:13]

https://codereview.chromium.org/1132173002/diff/1/extensions/browser/api/blue...
File extensions/browser/api/bluetooth_low_energy/bluetooth_api_advertisement.cc
(right):

https://codereview.chromium.org/1132173002/diff/1/extensions/browser/api/blue...
extensions/browser/api/bluetooth_low_energy/bluetooth_api_advertisement.cc:1: //
Copyright 2014 The Chromium Authors. All rights reserved.
s/2014/2015/

https://codereview.chromium.org/1132173002/diff/1/extensions/browser/api/blue...
File extensions/browser/api/bluetooth_low_energy/bluetooth_api_advertisement.h
(right):

https://codereview.chromium.org/1132173002/diff/1/extensions/browser/api/blue...
extensions/browser/api/bluetooth_low_energy/bluetooth_api_advertisement.h:1: //
Copyright 2014 The Chromium Authors. All rights reserved.
nit: s/2014/2015/

https://codereview.chromium.org/1132173002/diff/1/extensions/browser/api/blue...
extensions/browser/api/bluetooth_low_energy/bluetooth_api_advertisement.h:17: }
nit: "}  // namespace device"

https://codereview.chromium.org/1132173002/diff/1/extensions/browser/api/blue...
extensions/browser/api/bluetooth_low_energy/bluetooth_api_advertisement.h:25: //
BluetoothAdvertisementXxx class. All methods must be called on the
Probably no need to mention "platform differences", since device/bluetooth
already handles that for you.

https://codereview.chromium.org/1132173002/diff/1/extensions/browser/api/blue...
extensions/browser/api/bluetooth_low_energy/bluetooth_api_advertisement.h:38: //
being called on the UI thread.
Again, since there is no platform magic happening at the extensions level, this
comment might be confusing. It should be enough to say "BluetoothAdvertisement
methods require being called on the UI thread."

https://codereview.chromium.org/1132173002/diff/1/extensions/browser/api/blue...
File extensions/browser/api/bluetooth_low_energy/bluetooth_low_energy_api.cc
(right):

https://codereview.chromium.org/1132173002/diff/1/extensions/browser/api/blue...
extensions/browser/api/bluetooth_low_energy/bluetooth_low_energy_api.cc:134: }
You'll need unit tests for these functions. It's OK to cover these using browser
tests.

https://codereview.chromium.org/1132173002/diff/1/extensions/browser/api/blue...
File extensions/browser/api/bluetooth_low_energy/bluetooth_low_energy_api.h
(left):

https://codereview.chromium.org/1132173002/diff/1/extensions/browser/api/blue...
extensions/browser/api/bluetooth_low_energy/bluetooth_low_energy_api.h:333: :
public BluetoothLowEnergyExtensionFunction {
I'm trying to understand why this base class is necessary. I don't see you
initialize advertisements_manager_ anywhere and I don't see the AddAdvertisement
and RemoveAdvertisement functions get called anywhere either. Wouldn't it be
better to do this the way BluetoothLowEnergyConnectFunction is implemented?
Basically the BluetoothLowEnergyEventRouter class deals with checking whether an
API resource exists or not and creates the correct entry as needed. I don't see
where you're doing that management in this CL, though maybe I missed something?

[armansito, 2015-05-08 23:08:19]

https://codereview.chromium.org/1132173002/diff/1/extensions/browser/api/blue...
File extensions/browser/api/bluetooth_low_energy/bluetooth_low_energy_api.cc
(right):

https://codereview.chromium.org/1132173002/diff/1/extensions/browser/api/blue...
extensions/browser/api/bluetooth_low_energy/bluetooth_low_energy_api.cc:839:
BluetoothApiAdvertisement* advertisement) {
DCHECK(advertisements_manager);

https://codereview.chromium.org/1132173002/diff/1/extensions/browser/api/blue...
extensions/browser/api/bluetooth_low_energy/bluetooth_low_energy_api.cc:845: int
advertisement_id) {
DCHECK(advertisements_manager);

https://codereview.chromium.org/1132173002/diff/1/extensions/browser/api/blue...
extensions/browser/api/bluetooth_low_energy/bluetooth_low_energy_api.cc:850: int
advertisement_id) {
DCHECK(advertisements_manager);

https://codereview.chromium.org/1132173002/diff/1/extensions/browser/api/blue...
extensions/browser/api/bluetooth_low_energy/bluetooth_low_energy_api.cc:904: if
(params->advertisement.manufacturer_data)
Use curly braces for these two if statement. Even though it's a single-statement
body, it's split across multiple lines so it's difficult to parse (and I
remember there being a Chrome style rule for that somewhere...)

https://codereview.chromium.org/1132173002/diff/1/extensions/browser/api/blue...
extensions/browser/api/bluetooth_low_energy/bluetooth_low_energy_api.cc:908: if
(params->advertisement.service_data)
ditto

[rkc, 2015-05-11 21:59:35]

https://codereview.chromium.org/1132173002/diff/1/extensions/browser/api/blue...
File extensions/browser/api/bluetooth_low_energy/bluetooth_api_advertisement.cc
(right):

https://codereview.chromium.org/1132173002/diff/1/extensions/browser/api/blue...
extensions/browser/api/bluetooth_low_energy/bluetooth_api_advertisement.cc:1: //
Copyright 2014 The Chromium Authors. All rights reserved.
On 2015/05/08 22:46:13, armansito wrote:
> s/2014/2015/

Done.

https://codereview.chromium.org/1132173002/diff/1/extensions/browser/api/blue...
File extensions/browser/api/bluetooth_low_energy/bluetooth_api_advertisement.h
(right):

https://codereview.chromium.org/1132173002/diff/1/extensions/browser/api/blue...
extensions/browser/api/bluetooth_low_energy/bluetooth_api_advertisement.h:1: //
Copyright 2014 The Chromium Authors. All rights reserved.
On 2015/05/08 22:46:13, armansito wrote:
> nit: s/2014/2015/

Done.

https://codereview.chromium.org/1132173002/diff/1/extensions/browser/api/blue...
extensions/browser/api/bluetooth_low_energy/bluetooth_api_advertisement.h:17: }
On 2015/05/08 22:46:13, armansito wrote:
> nit: "}  // namespace device"

Done.

https://codereview.chromium.org/1132173002/diff/1/extensions/browser/api/blue...
extensions/browser/api/bluetooth_low_energy/bluetooth_api_advertisement.h:25: //
BluetoothAdvertisementXxx class. All methods must be called on the
On 2015/05/08 22:46:13, armansito wrote:
> Probably no need to mention "platform differences", since device/bluetooth
> already handles that for you.

Done.

https://codereview.chromium.org/1132173002/diff/1/extensions/browser/api/blue...
extensions/browser/api/bluetooth_low_energy/bluetooth_api_advertisement.h:38: //
being called on the UI thread.
On 2015/05/08 22:46:13, armansito wrote:
> Again, since there is no platform magic happening at the extensions level,
this
> comment might be confusing. It should be enough to say "BluetoothAdvertisement
> methods require being called on the UI thread."

Done.

https://codereview.chromium.org/1132173002/diff/1/extensions/browser/api/blue...
File extensions/browser/api/bluetooth_low_energy/bluetooth_low_energy_api.cc
(right):

https://codereview.chromium.org/1132173002/diff/1/extensions/browser/api/blue...
extensions/browser/api/bluetooth_low_energy/bluetooth_low_energy_api.cc:134: }
On 2015/05/08 22:46:13, armansito wrote:
> You'll need unit tests for these functions. It's OK to cover these using
browser
> tests.

Both these functions are getting exercised by the apitest that I added.

https://codereview.chromium.org/1132173002/diff/1/extensions/browser/api/blue...
extensions/browser/api/bluetooth_low_energy/bluetooth_low_energy_api.cc:839:
BluetoothApiAdvertisement* advertisement) {
On 2015/05/08 23:08:19, armansito wrote:
> DCHECK(advertisements_manager);

Done.

https://codereview.chromium.org/1132173002/diff/1/extensions/browser/api/blue...
extensions/browser/api/bluetooth_low_energy/bluetooth_low_energy_api.cc:845: int
advertisement_id) {
On 2015/05/08 23:08:19, armansito wrote:
> DCHECK(advertisements_manager);

Done.

https://codereview.chromium.org/1132173002/diff/1/extensions/browser/api/blue...
extensions/browser/api/bluetooth_low_energy/bluetooth_low_energy_api.cc:850: int
advertisement_id) {
On 2015/05/08 23:08:19, armansito wrote:
> DCHECK(advertisements_manager);

Done.

https://codereview.chromium.org/1132173002/diff/1/extensions/browser/api/blue...
extensions/browser/api/bluetooth_low_energy/bluetooth_low_energy_api.cc:904: if
(params->advertisement.manufacturer_data)
On 2015/05/08 23:08:19, armansito wrote:
> Use curly braces for these two if statement. Even though it's a
single-statement
> body, it's split across multiple lines so it's difficult to parse (and I
> remember there being a Chrome style rule for that somewhere...)

Done.

https://codereview.chromium.org/1132173002/diff/1/extensions/browser/api/blue...
extensions/browser/api/bluetooth_low_energy/bluetooth_low_energy_api.cc:908: if
(params->advertisement.service_data)
On 2015/05/08 23:08:19, armansito wrote:
> ditto

Done.

https://codereview.chromium.org/1132173002/diff/1/extensions/browser/api/blue...
File extensions/browser/api/bluetooth_low_energy/bluetooth_low_energy_api.h
(left):

https://codereview.chromium.org/1132173002/diff/1/extensions/browser/api/blue...
extensions/browser/api/bluetooth_low_energy/bluetooth_low_energy_api.h:333: :
public BluetoothLowEnergyExtensionFunction {
On 2015/05/08 22:46:13, armansito wrote:
> I'm trying to understand why this base class is necessary. I don't see you
> initialize advertisements_manager_ anywhere and I don't see the
AddAdvertisement
> and RemoveAdvertisement functions get called anywhere either. Wouldn't it be
> better to do this the way BluetoothLowEnergyConnectFunction is implemented?
> Basically the BluetoothLowEnergyEventRouter class deals with checking whether
an
> API resource exists or not and creates the correct entry as needed. I don't
see
> where you're doing that management in this CL, though maybe I missed
something?

Explained offline.

[armansito, 2015-05-13 00:58:32]

lgtm

[rkc, 2015-05-13 02:42:58]

The CQ bit was checked by rkc@chromium.org

[commit-bot: I haz the power, 2015-05-13 02:43:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1132173002/20001

[commit-bot: I haz the power, 2015-05-13 02:50:16]

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

[commit-bot: I haz the power, 2015-05-13 02:50:17]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[rkc, 2015-05-13 16:56:30]

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

[rkc, 2015-05-13 16:56:31]

scheib@ PTAL for owners review for,

extensions/common/api/_behavior_features.json
extensions/extensions.gypi

Thanks!

[scheib, 2015-05-13 17:08:40]

https://codereview.chromium.org/1132173002/diff/40001/extensions/common/api/_...
File extensions/common/api/_behavior_features.json (right):

https://codereview.chromium.org/1132173002/diff/40001/extensions/common/api/_...
extensions/common/api/_behavior_features.json:28:
"9E287A8257E58EFB13E89C86A4B75A3AC4B058D8"   // BLE test extension.
From top of file:
Google employees: please update http://go/chrome-api-whitelist to map
// hashes back to ids.
We still need an issue that holds the extension ID for reverse mapping.

[rkc, 2015-05-13 17:28:13]

https://codereview.chromium.org/1132173002/diff/40001/extensions/common/api/_...
File extensions/common/api/_behavior_features.json (right):

https://codereview.chromium.org/1132173002/diff/40001/extensions/common/api/_...
extensions/common/api/_behavior_features.json:28:
"9E287A8257E58EFB13E89C86A4B75A3AC4B058D8"   // BLE test extension.
On 2015/05/13 17:08:40, scheib wrote:
> From top of file:
> Google employees: please update http://go/chrome-api-whitelist to map
> // hashes back to ids.
> We still need an issue that holds the extension ID for reverse mapping.

Done.

[scheib, 2015-05-13 17:35:08]

extensions/common/api/_behavior_features.json
extensions/extensions.gypi
LGTM

[rkc, 2015-05-13 17:36:17]

The CQ bit was checked by rkc@chromium.org

[rkc, 2015-05-13 17:36:17]

The patchset sent to the CQ was uploaded after l-g-t-m from
armansito@chromium.org
Link to the patchset: https://codereview.chromium.org/1132173002/#ps40002
(title: " ")

[commit-bot: I haz the power, 2015-05-13 17:36:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1132173002/40002

[commit-bot: I haz the power, 2015-05-13 19:13:08]

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

[commit-bot: I haz the power, 2015-05-13 19:13:09]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[rkc, 2015-05-13 22:34:49]

The CQ bit was checked by rkc@chromium.org

[rkc, 2015-05-13 22:34:50]

The patchset sent to the CQ was uploaded after l-g-t-m from scheib@chromium.org,
armansito@chromium.org
Link to the patchset: https://codereview.chromium.org/1132173002/#ps70001
(title: " ")

[commit-bot: I haz the power, 2015-05-13 22:35:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1132173002/70001

[commit-bot: I haz the power, 2015-05-14 00:05:08]

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

[commit-bot: I haz the power, 2015-05-14 00:05:10]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[rkc, 2015-05-15 18:24:45]

The CQ bit was checked by rkc@chromium.org

[rkc, 2015-05-15 18:24:46]

The patchset sent to the CQ was uploaded after l-g-t-m from scheib@chromium.org,
armansito@chromium.org
Link to the patchset: https://codereview.chromium.org/1132173002/#ps90001
(title: " ")

[commit-bot: I haz the power, 2015-05-15 18:24:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1132173002/90001

[commit-bot: I haz the power, 2015-05-15 20:16:45]

Committed patchset #6 (id:90001)

[325583992843-0clp48nf3nc7pk3ll9lp3k65k4ra53j4, 2015-05-16 05:29:47]

Patchset 6 (id:??) landed as
https://crrev.com/58666c84553ecc058af5e6d178d6d8d3f47e99c1
Cr-Commit-Position: refs/heads/master@{#330182}

[commit-bot: I haz the power, 2015-05-18 11:26:48]

Patchset 6 (id:??) landed as
https://crrev.com/58666c84553ecc058af5e6d178d6d8d3f47e99c1
Cr-Commit-Position: refs/heads/master@{#330182}