Expose device RSSI and Tx power via the chrome.bluetooth API.

Expose device RSSI and Tx power via the chrome.bluetooth API.

This CL only implements the fetching of these values on Mac.

BUG=365966
TEST=browser_tests
R=kalman@chromium.org, keybuk@chromium.org

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

[not at google - send to devlin, 2014-04-23 01:34:13]

json lgtm

https://codereview.chromium.org/246603008/diff/1/chrome/common/extensions/api...
File chrome/common/extensions/api/bluetooth_private.json (right):

https://codereview.chromium.org/246603008/diff/1/chrome/common/extensions/api...
chrome/common/extensions/api/bluetooth_private.json:41: "parameters": [
odd having parameters at the start. same for other functions.

[Ilya Sherman, 2014-04-23 01:35:59]

https://codereview.chromium.org/246603008/diff/1/chrome/common/extensions/api...
File chrome/common/extensions/api/bluetooth_private.json (right):

https://codereview.chromium.org/246603008/diff/1/chrome/common/extensions/api...
chrome/common/extensions/api/bluetooth_private.json:41: "parameters": [
On 2014/04/23 01:34:13, kalman wrote:
> odd having parameters at the start. same for other functions.

I thought so too, but was matching the local style of this file.  Would you
prefer me to change it as part of this CL, or in a separate CL?

[not at google - send to devlin, 2014-04-23 01:37:21]

On 2014/04/23 01:35:59, Ilya Sherman wrote:
>
https://codereview.chromium.org/246603008/diff/1/chrome/common/extensions/api...
> File chrome/common/extensions/api/bluetooth_private.json (right):
> 
>
https://codereview.chromium.org/246603008/diff/1/chrome/common/extensions/api...
> chrome/common/extensions/api/bluetooth_private.json:41: "parameters": [
> On 2014/04/23 01:34:13, kalman wrote:
> > odd having parameters at the start. same for other functions.
> 
> I thought so too, but was matching the local style of this file.  Would you
> prefer me to change it as part of this CL, or in a separate CL?

if you don't mind changing it now that'd be great.

[keybuk, 2014-04-23 08:59:19]

https://codereview.chromium.org/246603008/diff/1/chrome/common/extensions/api...
File chrome/common/extensions/api/bluetooth_private.json (right):

https://codereview.chromium.org/246603008/diff/1/chrome/common/extensions/api...
chrome/common/extensions/api/bluetooth_private.json:59: "name": "getDeviceRSSI",
If the RSSI is simply a property of the device, there seems to be no reason for
this to be added to the private API as a method with a callback - it could be
added to the chrome.bluetooth Device object as a property

Even if the RSSI is a callback to controller method for the adapter, it doesn't
seem out of scope to add this to the public API

https://codereview.chromium.org/246603008/diff/1/device/bluetooth/bluetooth_d...
File device/bluetooth/bluetooth_device.h (right):

https://codereview.chromium.org/246603008/diff/1/device/bluetooth/bluetooth_d...
device/bluetooth/bluetooth_device.h:233: virtual int GetRSSI() const = 0;
Is this the RSSI of inquiry/AD packets - or is this the RSSI of the connection
to the device?

If the latter, then this will require a round-trip to the controller so should
be a method with a callback

[Ilya Sherman, 2014-04-24 06:45:16]

Thanks for the review!

https://codereview.chromium.org/246603008/diff/1/chrome/common/extensions/api...
File chrome/common/extensions/api/bluetooth_private.json (right):

https://codereview.chromium.org/246603008/diff/1/chrome/common/extensions/api...
chrome/common/extensions/api/bluetooth_private.json:59: "name": "getDeviceRSSI",
On 2014/04/23 08:59:19, keybuk wrote:
> If the RSSI is simply a property of the device, there seems to be no reason
for
> this to be added to the private API as a method with a callback - it could be
> added to the chrome.bluetooth Device object as a property
> 
> Even if the RSSI is a callback to controller method for the adapter, it
doesn't
> seem out of scope to add this to the public API

The intent is to be able to poll for the current RSSI of a connected device. 
I'm happy to add this to the public API if you think that's a more appropriate
location for it.

We were also planning to expose the transmit (Tx) power of the adapter.  Does it
seem reasonable to add that to the public API as well?

https://codereview.chromium.org/246603008/diff/1/device/bluetooth/bluetooth_d...
File device/bluetooth/bluetooth_device.h (right):

https://codereview.chromium.org/246603008/diff/1/device/bluetooth/bluetooth_d...
device/bluetooth/bluetooth_device.h:233: virtual int GetRSSI() const = 0;
On 2014/04/23 08:59:19, keybuk wrote:
> Is this the RSSI of inquiry/AD packets - or is this the RSSI of the connection
> to the device?
> 
> If the latter, then this will require a round-trip to the controller so should
> be a method with a callback

It's the latter.  Do you want me to include a pair of callbacks -- one for
success and one for failure?  FWIW, on Mac, the call is synchronous.  Maybe it
makes sense not to add callbacks until we implement this on a platform that
needs them?  Of course, if you prefer that I add callbacks now, I'm happy to do
that as well.

[keybuk, 2014-04-24 11:25:51]

https://codereview.chromium.org/246603008/diff/1/chrome/common/extensions/api...
File chrome/common/extensions/api/bluetooth_private.json (right):

https://codereview.chromium.org/246603008/diff/1/chrome/common/extensions/api...
chrome/common/extensions/api/bluetooth_private.json:59: "name": "getDeviceRSSI",
I think both of these things can go in the public API

Especially for LE, writing proximity apps is something everyone should do


If the host subsystem is doing the polling, and maintaining the values, then
this can just be a property of bluetooth.Device

https://codereview.chromium.org/246603008/diff/1/device/bluetooth/bluetooth_d...
File device/bluetooth/bluetooth_device.h (right):

https://codereview.chromium.org/246603008/diff/1/device/bluetooth/bluetooth_d...
device/bluetooth/bluetooth_device.h:233: virtual int GetRSSI() const = 0;
If synchronous then there should definitely be a callback, you cannae block the
UI thread cap'n

[Ilya Sherman, 2014-04-26 03:26:59]

keybuk@, PTAL.  I've now implemented the RSSI and Tx power as properties on the
Device class.

[keybuk, 2014-04-27 09:08:44]

https://codereview.chromium.org/246603008/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/bluetooth/bluetooth_api_utils.cc (right):

https://codereview.chromium.org/246603008/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/bluetooth/bluetooth_api_utils.cc:116: int rssi =
device.GetRSSI();
RSSI can also be provided when not connected - e.g. RSSI of received Inquiry
results and AD

We should provide those too, and they can go in the same property

https://codereview.chromium.org/246603008/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/bluetooth/bluetooth_api_utils.cc:124:
out->maximum_host_transmit_power.reset(new int(maximum_transmit_power));
These on the other hand only make sense when connected

(Inquiry / Advertising TX Power can go in an Adapter property)

[Ilya Sherman, 2014-04-29 00:51:17]

https://codereview.chromium.org/246603008/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/bluetooth/bluetooth_api_utils.cc (right):

https://codereview.chromium.org/246603008/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/bluetooth/bluetooth_api_utils.cc:116: int rssi =
device.GetRSSI();
On 2014/04/27 09:08:44, keybuk wrote:
> RSSI can also be provided when not connected - e.g. RSSI of received Inquiry
> results and AD
> 
> We should provide those too, and they can go in the same property

I extended the IDL documentation to specify this, and moved the GetRSSI() call
out of the if stmt.  However, I don't plan to implement fetching the RSSI from
received Inquiry results as part of this CL, because I don't know how to read
that data on Mac.  I instead added a NOTIMPLEMENTED() for that case, as a
reminder to come back to this at a later time.  Does that sound reasonable?

https://codereview.chromium.org/246603008/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/bluetooth/bluetooth_api_utils.cc:124:
out->maximum_host_transmit_power.reset(new int(maximum_transmit_power));
On 2014/04/27 09:08:44, keybuk wrote:
> These on the other hand only make sense when connected
> 
> (Inquiry / Advertising TX Power can go in an Adapter property)

Ack.

[keybuk, 2014-04-29 15:59:01]

lgtm

[Ilya Sherman, 2014-04-29 20:00:14]

Ben, could you please take another look?  This API moved from bluetoothPrivate
to bluetooth, so looks different than when you approved the CL.

Nico, mind doing a quick pass for Obj-C style?

[not at google - send to devlin, 2014-04-29 20:01:58]

lgtm

[Nico, 2014-04-29 22:11:32]

bluetooth_device_mac lgtm

[Ilya Sherman, 2014-04-29 22:22:12]

The CQ bit was checked by isherman@chromium.org

[commit-bot: I haz the power, 2014-04-29 22:24:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/isherman@chromium.org/246603008/80001

[commit-bot: I haz the power, 2014-04-30 00:50:05]

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

[commit-bot: I haz the power, 2014-04-30 00:50:06]

Try jobs failed on following builders:
  mac_chromium_rel on tryserver.chromium

[Ilya Sherman, 2014-04-30 22:21:47]

The CQ bit was checked by isherman@chromium.org

[commit-bot: I haz the power, 2014-04-30 22:23:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/isherman@chromium.org/246603008/100001

[commit-bot: I haz the power, 2014-04-30 23:33:42]

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

[commit-bot: I haz the power, 2014-04-30 23:33:42]

Try jobs failed on following builders:
  win_chromium_rel on tryserver.chromium

[Ilya Sherman, 2014-04-30 23:34:27]

The CQ bit was checked by isherman@chromium.org

[commit-bot: I haz the power, 2014-04-30 23:35:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/isherman@chromium.org/246603008/100001

[commit-bot: I haz the power, 2014-05-01 00:49:16]

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

[commit-bot: I haz the power, 2014-05-01 00:49:16]

Try jobs failed on following builders:
  mac_chromium_rel on tryserver.chromium

[Ilya Sherman, 2014-05-01 00:51:03]

The CQ bit was checked by isherman@chromium.org

[commit-bot: I haz the power, 2014-05-01 00:51:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/isherman@chromium.org/246603008/100001

[commit-bot: I haz the power, 2014-05-01 06:13:33]

Change committed as 267458