chrome.bluetoothLowEnergy: Implement getService and getServices.

chrome.bluetoothLowEnergy: Implement getService and getServices.

This patch implements the getService and getServices functions of the
bluetoothLowEnergy API. The key changes introduced are:

* BluetoothLowEnergyEventRouter, which tracks Observer events from the
device/bluetooth API, maintains a mapping from instance IDs to the necessary
metadata to look up a GATT object from the adapter, and handles basic logic
such as conversion between device::BluetoothGatt* and
extensions::api::bluetooth_low_energy::* objects and sending of API events.

* Implementation of getService and getServices functions.

* In browser unit tests for the above functions.

BUG=265663
TEST=browser_tests --gtest_filter=BluetoothLowEnergyApiTest.*, device_unittests

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

[armansito, 2014-04-24 02:24:15]

miket@: PTAL at c/b/e/browser_context_keyed_service_factories.cc
rpaquay@: overall API logic in c/b/e/a/bluetooth_low_energy/*, tests etc.
keybuk@: everything

[keybuk, 2014-04-24 15:05:20]

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

https://codereview.chromium.org/256413003/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/bluetooth_low_energy/bluetooth_low_energy_event_router.cc:41:
apibt::BluetoothDeviceToApiDevice(*service->GetDevice(), device);
I'm thinking now that this is overkill, we should just put the address here, and
let the user deal with calling chrome.bluetooth.getDevice on that address

https://codereview.chromium.org/256413003/diff/1/device/bluetooth/test/mock_b...
File device/bluetooth/test/mock_bluetooth_gatt_service.h (right):

https://codereview.chromium.org/256413003/diff/1/device/bluetooth/test/mock_b...
device/bluetooth/test/mock_bluetooth_gatt_service.h:24: const BluetoothUUID
uuid,
& ?

[rpaquay, 2014-04-24 20:38:45]

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

https://codereview.chromium.org/256413003/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/bluetooth_low_energy/bluetooth_low_energy_api.cc:89:
weak_ptr_factory_.GetWeakPtr()))) {
Does this work? I would think you should pass "this" (as ExtensionFunction are
ref-counted). AFAIK, the extension function instance with be released right
after RunImpl if you don't use "this" here (see
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...).

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

https://codereview.chromium.org/256413003/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/bluetooth_low_energy/bluetooth_low_energy_event_router.cc:143:
base::ListValue* service_list = new base::ListValue();
You should not have to build the list manually, you should be able to use
apibtle::GetServices::Results::Create() (e.g.
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...)

[armansito, 2014-04-24 20:51:22]

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

https://codereview.chromium.org/256413003/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/bluetooth_low_energy/bluetooth_low_energy_event_router.cc:41:
apibt::BluetoothDeviceToApiDevice(*service->GetDevice(), device);
On 2014/04/24 15:05:20, keybuk wrote:
> I'm thinking now that this is overkill, we should just put the address here,
and
> let the user deal with calling chrome.bluetooth.getDevice on that address

Done.

https://codereview.chromium.org/256413003/diff/1/device/bluetooth/test/mock_b...
File device/bluetooth/test/mock_bluetooth_gatt_service.h (right):

https://codereview.chromium.org/256413003/diff/1/device/bluetooth/test/mock_b...
device/bluetooth/test/mock_bluetooth_gatt_service.h:24: const BluetoothUUID
uuid,
On 2014/04/24 15:05:20, keybuk wrote:
> & ?

oops, done.

[armansito, 2014-04-24 22:02:00]

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

https://codereview.chromium.org/256413003/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/bluetooth_low_energy/bluetooth_low_energy_api.cc:89:
weak_ptr_factory_.GetWeakPtr()))) {
On 2014/04/24 20:38:45, rpaquay wrote:
> Does this work? I would think you should pass "this" (as ExtensionFunction are
> ref-counted). AFAIK, the extension function instance with be released right
> after RunImpl if you don't use "this" here (see
>
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...).

I'm confused, if you actually pass "this" here and the object gets deleted
before the callback gets called, wouldn't that be an invalid access?

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

https://codereview.chromium.org/256413003/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/bluetooth_low_energy/bluetooth_low_energy_event_router.cc:143:
base::ListValue* service_list = new base::ListValue();
On 2014/04/24 20:38:45, rpaquay wrote:
> You should not have to build the list manually, you should be able to use
> apibtle::GetServices::Results::Create() (e.g.
>
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...)

Done.

[rpaquay, 2014-04-24 22:12:46]

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

https://codereview.chromium.org/256413003/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/bluetooth_low_energy/bluetooth_low_energy_api.cc:89:
weak_ptr_factory_.GetWeakPtr()))) {
On 2014/04/24 22:02:00, armansito wrote:
> On 2014/04/24 20:38:45, rpaquay wrote:
> > Does this work? I would think you should pass "this" (as ExtensionFunction
are
> > ref-counted). AFAIK, the extension function instance with be released right
> > after RunImpl if you don't use "this" here (see
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...).
> 
> I'm confused, if you actually pass "this" here and the object gets deleted
> before the callback gets called, wouldn't that be an invalid access?

AFAIK:

1) If you pass "this", Bind calls "this->AddRef()" to keep the object alive
until after the callback is invoked.
2) If you pass "weak_ptr_factory" in this case, the extension function will be
almost immediately deleted because the "Dispatch()" function has a
"scoped_refptr" to it.

Basically, the way I understand the current code is that, in case
InitilizeAdapterAndInvokeCallback" is truly asynchronous, the callback will
actually never be called because the extension function will be deleted right
away.

[armansito, 2014-04-24 22:38:33]

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

https://codereview.chromium.org/256413003/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/bluetooth_low_energy/bluetooth_low_energy_api.cc:89:
weak_ptr_factory_.GetWeakPtr()))) {
On 2014/04/24 22:12:46, rpaquay wrote:
> On 2014/04/24 22:02:00, armansito wrote:
> > On 2014/04/24 20:38:45, rpaquay wrote:
> > > Does this work? I would think you should pass "this" (as ExtensionFunction
> are
> > > ref-counted). AFAIK, the extension function instance with be released
right
> > > after RunImpl if you don't use "this" here (see
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...).
> > 
> > I'm confused, if you actually pass "this" here and the object gets deleted
> > before the callback gets called, wouldn't that be an invalid access?
> 
> AFAIK:
> 
> 1) If you pass "this", Bind calls "this->AddRef()" to keep the object alive
> until after the callback is invoked.
> 2) If you pass "weak_ptr_factory" in this case, the extension function will be
> almost immediately deleted because the "Dispatch()" function has a
> "scoped_refptr" to it.
> 
> Basically, the way I understand the current code is that, in case
> InitilizeAdapterAndInvokeCallback" is truly asynchronous, the callback will
> actually never be called because the extension function will be deleted right
> away.

Ah got it, I didn't realize that ExtensionFunction was refcounted, thanks for
catching that. Changed the code to pass 'this'.

[rpaquay, 2014-04-24 23:00:03]

https://codereview.chromium.org/256413003/diff/40001/chrome/browser/extension...
File
chrome/browser/extensions/api/bluetooth_low_energy/bluetooth_low_energy_api.h
(right):

https://codereview.chromium.org/256413003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/bluetooth_low_energy/bluetooth_low_energy_api.h:75:
base::WeakPtrFactory<BluetoothLowEnergyExtensionFunction> weak_ptr_factory_;
Is this field still needed?

[armansito, 2014-04-24 23:02:31]

https://codereview.chromium.org/256413003/diff/40001/chrome/browser/extension...
File
chrome/browser/extensions/api/bluetooth_low_energy/bluetooth_low_energy_api.h
(right):

https://codereview.chromium.org/256413003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/bluetooth_low_energy/bluetooth_low_energy_api.h:75:
base::WeakPtrFactory<BluetoothLowEnergyExtensionFunction> weak_ptr_factory_;
On 2014/04/24 23:00:04, rpaquay wrote:
> Is this field still needed?

It's removed in the latest patch set.

[rpaquay, 2014-04-24 23:07:26]

lgtm

[keybuk, 2014-04-25 00:30:54]

lgtm

[miket_OOO, 2014-04-25 18:25:16]

lgtm

[armansito, 2014-04-25 18:25:42]

The CQ bit was checked by armansito@chromium.org

[commit-bot: I haz the power, 2014-04-25 22:12:26]

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

[commit-bot: I haz the power, 2014-04-26 00:52:11]

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

[commit-bot: I haz the power, 2014-04-26 00:52:11]

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

[armansito, 2014-04-26 21:34:15]

The CQ bit was checked by armansito@chromium.org

[commit-bot: I haz the power, 2014-04-26 21:34:49]

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

[commit-bot: I haz the power, 2014-04-26 21:35:03]

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

[commit-bot: I haz the power, 2014-04-26 21:35:04]

Failed to apply patch for chrome/chrome_browser_extensions.gypi:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/chrome_browser_extensions.gypi
  Hunk #1 FAILED at 163.
  1 out of 1 hunk FAILED -- saving rejects to file
chrome/chrome_browser_extensions.gypi.rej

Patch:       chrome/chrome_browser_extensions.gypi
Index: chrome/chrome_browser_extensions.gypi
diff --git a/chrome/chrome_browser_extensions.gypi
b/chrome/chrome_browser_extensions.gypi
index
f23578cf3634a7bb65e19c554deb6e702f787924..42853d27389b9fc3bfa1eb9c03ba5b2c120105ea
100644
--- a/chrome/chrome_browser_extensions.gypi
+++ b/chrome/chrome_browser_extensions.gypi
@@ -163,6 +163,8 @@
        
'browser/extensions/api/bluetooth/bluetooth_socket_event_dispatcher.cc',
        
'browser/extensions/api/bluetooth_low_energy/bluetooth_low_energy_api.cc',
        
'browser/extensions/api/bluetooth_low_energy/bluetooth_low_energy_api.h',
+       
'browser/extensions/api/bluetooth_low_energy/bluetooth_low_energy_event_router.cc',
+       
'browser/extensions/api/bluetooth_low_energy/bluetooth_low_energy_event_router.h',
        
'browser/extensions/api/bookmark_manager_private/bookmark_manager_private_api.cc',
        
'browser/extensions/api/bookmark_manager_private/bookmark_manager_private_api.h',
         'browser/extensions/api/bookmarks/bookmark_api_constants.cc',

[armansito, 2014-04-26 22:15:53]

The CQ bit was checked by armansito@chromium.org

[commit-bot: I haz the power, 2014-04-26 22:18:16]

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

[commit-bot: I haz the power, 2014-04-26 22:55:29]

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

[commit-bot: I haz the power, 2014-04-26 22:55:29]

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

[armansito, 2014-04-26 23:02:59]

The CQ bit was checked by armansito@chromium.org

[commit-bot: I haz the power, 2014-04-26 23:03:52]

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

[commit-bot: I haz the power, 2014-04-28 08:06:15]

Change committed as 266484