chrome.bluetoothLowEnergy: Introduce connect and disconnect methods.

chrome.bluetoothLowEnergy: Introduce connect and disconnect methods.

This CL introduces the connect and disconnect methods to the bluetoothLowEnergy
API, which allow an app to initiate and hold on to the connection to a low
energy device. The API calls internally manage the allocation of an API
resource corresponding to a per app, per device device::BluetoothGattConnection
object.

BUG=381305
TEST=browser_tests --gtest_filter=BluetoothLowEnergyApiTest.*

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

[armansito, 2014-06-19 17:12:15]

kalman@: bluetooth_low_energy.idl OWNERS
isherman@: extension_function_histogram_value.h OWNERS
keybuk@: General review.

[keybuk, 2014-06-19 17:23:49]

https://codereview.chromium.org/349433002/diff/1/chrome/browser/extensions/ap...
File
chrome/browser/extensions/api/bluetooth_low_energy/bluetooth_low_energy_connection.cc
(right):

https://codereview.chromium.org/349433002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/bluetooth_low_energy/bluetooth_low_energy_connection.cc:36:
}
I leave it up to kalman to decide whether having a persistent api resource is a
good thing or not

https://codereview.chromium.org/349433002/diff/1/chrome/common/extensions/api...
File chrome/common/extensions/api/bluetooth_low_energy.idl (right):

https://codereview.chromium.org/349433002/diff/1/chrome/common/extensions/api...
chrome/common/extensions/api/bluetooth_low_energy.idl:126: static void
disconnect(DOMString deviceAddress, ResultCallback callback);
Make the disconnect callback optional for consistency with the socket API

[not at google - send to devlin, 2014-06-19 17:27:09]

IDL lgtm as a rubberstamp sort of thing. +rpaquay for extensions.

https://codereview.chromium.org/349433002/diff/1/chrome/browser/extensions/ap...
File
chrome/browser/extensions/api/bluetooth_low_energy/bluetooth_low_energy_connection.cc
(right):

https://codereview.chromium.org/349433002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/bluetooth_low_energy/bluetooth_low_energy_connection.cc:36:
}
On 2014/06/19 17:23:49, keybuk wrote:
> I leave it up to kalman to decide whether having a persistent api resource is
a
> good thing or not

I'm quite sure we do want this, it's important for event pages. But double-check
with Renaud to make sure I understand it correctly. I've never dealt with this
code before.

[Ilya Sherman, 2014-06-19 17:49:45]

Please run the script to update //tools/metrics/histograms/histograms.xml.

[armansito, 2014-06-19 18:02:03]

On 2014/06/19 17:49:45, Ilya Sherman wrote:
> Please run the script to update //tools/metrics/histograms/histograms.xml.

done.

[rpaquay, 2014-06-19 19:07:27]

https://codereview.chromium.org/349433002/diff/1/chrome/browser/extensions/ap...
File
chrome/browser/extensions/api/bluetooth_low_energy/bluetooth_low_energy_connection.cc
(right):

https://codereview.chromium.org/349433002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/bluetooth_low_energy/bluetooth_low_energy_connection.cc:36:
}
On 2014/06/19 17:27:09, kalman wrote:
> On 2014/06/19 17:23:49, keybuk wrote:
> > I leave it up to kalman to decide whether having a persistent api resource
is
> a
> > good thing or not
> 
> I'm quite sure we do want this, it's important for event pages. But
double-check
> with Renaud to make sure I understand it correctly. I've never dealt with this
> code before.

Hmm... I don't quite know the use cases for this API, but what I can say is
that:

By making the connection persistent, we are saying that "disconnect" will be
called *only* when either
1) The app calls "disconnect" explicitly, or
2) The Chrome browser process shutdowns.

In particular, this means "disconnect" will *not* be called when an app is
unloaded due to idle time (that includes the common scenario of an app closed
through the user closing the last opened window of the app).

If this is the behavior we want, I would think we need to document it in the
"connect" method, as this is clearly different than the default behavior of
connect/disconnect for sockets, for example.

Another option would be to expose to "persistent" flag through the API, if we
think both use-cases are needed.

https://codereview.chromium.org/349433002/diff/20001/chrome/browser/extension...
File
chrome/browser/extensions/api/bluetooth_low_energy/bluetooth_low_energy_event_router.h
(right):

https://codereview.chromium.org/349433002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/bluetooth_low_energy/bluetooth_low_energy_event_router.h:83:
const ErrorCallback& error_callback);
Why are using both a "Status" return value and an "error_callback" with a
Status? I'd suggest having error_callback only, since this is called on the UI
thread.

https://codereview.chromium.org/349433002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/bluetooth_low_energy/bluetooth_low_energy_event_router.h:91:
const base::Closure& callback);
Same comment as above, I'd suggest having "error_callback" instead of "status"
return value.

[Ilya Sherman, 2014-06-19 20:59:38]

Histograms LGTM

[armansito, 2014-06-19 21:58:35]

https://codereview.chromium.org/349433002/diff/1/chrome/browser/extensions/ap...
File
chrome/browser/extensions/api/bluetooth_low_energy/bluetooth_low_energy_connection.cc
(right):

https://codereview.chromium.org/349433002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/bluetooth_low_energy/bluetooth_low_energy_connection.cc:36:
}
On 2014/06/19 19:07:26, rpaquay wrote:
> On 2014/06/19 17:27:09, kalman wrote:
> > On 2014/06/19 17:23:49, keybuk wrote:
> > > I leave it up to kalman to decide whether having a persistent api resource
> is
> > a
> > > good thing or not
> > 
> > I'm quite sure we do want this, it's important for event pages. But
> double-check
> > with Renaud to make sure I understand it correctly. I've never dealt with
this
> > code before.
> 
> Hmm... I don't quite know the use cases for this API, but what I can say is
> that:
> 
> By making the connection persistent, we are saying that "disconnect" will be
> called *only* when either
> 1) The app calls "disconnect" explicitly, or
> 2) The Chrome browser process shutdowns.
> 
> In particular, this means "disconnect" will *not* be called when an app is
> unloaded due to idle time (that includes the common scenario of an app closed
> through the user closing the last opened window of the app).
> 
> If this is the behavior we want, I would think we need to document it in the
> "connect" method, as this is clearly different than the default behavior of
> connect/disconnect for sockets, for example.
> 
> Another option would be to expose to "persistent" flag through the API, if we
> think both use-cases are needed.

After offline discussion with rpaquay@, I added an optional ConnectProperties
dictionary argument to connect with "persistent" field. If the field is not
provided, then the default value will be false.

https://codereview.chromium.org/349433002/diff/1/chrome/common/extensions/api...
File chrome/common/extensions/api/bluetooth_low_energy.idl (right):

https://codereview.chromium.org/349433002/diff/1/chrome/common/extensions/api...
chrome/common/extensions/api/bluetooth_low_energy.idl:126: static void
disconnect(DOMString deviceAddress, ResultCallback callback);
On 2014/06/19 17:23:49, keybuk wrote:
> Make the disconnect callback optional for consistency with the socket API

Done.

https://codereview.chromium.org/349433002/diff/20001/chrome/browser/extension...
File
chrome/browser/extensions/api/bluetooth_low_energy/bluetooth_low_energy_event_router.h
(right):

https://codereview.chromium.org/349433002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/bluetooth_low_energy/bluetooth_low_energy_event_router.h:83:
const ErrorCallback& error_callback);
On 2014/06/19 19:07:27, rpaquay wrote:
> Why are using both a "Status" return value and an "error_callback" with a
> Status? I'd suggest having error_callback only, since this is called on the UI
> thread.

Done.

https://codereview.chromium.org/349433002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/bluetooth_low_energy/bluetooth_low_energy_event_router.h:91:
const base::Closure& callback);
On 2014/06/19 19:07:26, rpaquay wrote:
> Same comment as above, I'd suggest having "error_callback" instead of "status"
> return value.

Done.

[rpaquay, 2014-06-19 22:09:49]

idl + persistent usage lgtm

[keybuk, 2014-06-20 00:06:15]

lgtm

[armansito, 2014-06-20 04:49:55]

The CQ bit was checked by armansito@chromium.org

[commit-bot: I haz the power, 2014-06-20 04:54:21]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/armansito@chromium.org/349433002/60001

[commit-bot: I haz the power, 2014-06-20 10:42:13]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)

[commit-bot: I haz the power, 2014-06-20 11:03:30]

Change committed as 278669