bluetooth: Impl uses new WebBluetoothGATTRemoteServer content/browser.

bluetooth: Impl uses new WebBluetoothGATTRemoteServer content/browser.

Initial implementation of connectGATT. Next steps is to actually connect to the device.

Second of a set of 3 patches to implement connectGATT.

  [1] https://codereview.chromium.org/1093633003
  [2] This patch.
  [3] https://codereview.chromium.org/1060593004

BUG=421668, 476735
Committed: https://crrev.com/503cd360ea98d90f5726913ac10344648529c942
Cr-Commit-Position: refs/heads/master@{#325971}

[ortuno, 2015-04-17 02:03:57]

Patchset #1 (id:1) has been deleted

[scheib, 2015-04-19 04:44:19]

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

[scheib, 2015-04-19 04:44:20]

LGTM with simple fixes.

https://codereview.chromium.org/1082273006/diff/20001/content/child/bluetooth...
File content/child/bluetooth/bluetooth_dispatcher.cc (right):

https://codereview.chromium.org/1082273006/diff/20001/content/child/bluetooth...
content/child/bluetooth/bluetooth_dispatcher.cc:20: using
blink::WebBluetoothGATTRemoteServer;
sort these lines. (surprised a script didn't mention it already?)

https://codereview.chromium.org/1082273006/diff/20001/content/child/bluetooth...
File content/child/bluetooth/bluetooth_dispatcher.h (right):

https://codereview.chromium.org/1082273006/diff/20001/content/child/bluetooth...
content/child/bluetooth/bluetooth_dispatcher.h:77: // TODO(ortuno): Create an
callback interface to use in the map.
Drop the TODO comment for now. I'm not certain we'll end up using an interface.
Looking around some I see several examples where simple strong typed IDMap
members are used and duplicated for each call. I also see some that merge the
callbacks, (WebIDBCallbacks, PermissionDispatcher::CallbackInformation) but I'm
not convinced the code complication there is worth it. To be complete, it would
be trivial to static cast the types in an IDMap<void*, but this doesn't seem
like the situation I'd be willing to do that.

https://codereview.chromium.org/1082273006/diff/20001/content/common/bluetoot...
File content/common/bluetooth/bluetooth_messages.h (right):

https://codereview.chromium.org/1082273006/diff/20001/content/common/bluetoot...
content/common/bluetooth/bluetooth_messages.h:121: // Informs the rendered that
the connection request |request_id| succeeded.
"renderer"

[scheib, 2015-04-19 04:45:20]

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

[scheib, 2015-04-19 04:45:21]

tsepez@chromium.org: Please review changes in 
 content/common/bluetooth/bluetooth_messages.h

[Tom Sepez, 2015-04-20 15:54:00]

lgtm

https://codereview.chromium.org/1082273006/diff/20001/content/browser/bluetoo...
File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right):

https://codereview.chromium.org/1082273006/diff/20001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:156: // TODO(ortuno): Add
actual implementation of connectGATT.
Please CC me when you fix the TODO.  I'd like to make a quick check that you're
not trusting these values as sent by a renderer.

https://codereview.chromium.org/1082273006/diff/20001/content/child/bluetooth...
File content/child/bluetooth/bluetooth_dispatcher.cc (right):

https://codereview.chromium.org/1082273006/diff/20001/content/child/bluetooth...
content/child/bluetooth/bluetooth_dispatcher.cc:151:
DCHECK(pending_connect_requests_.Lookup(request_id)) << request_id;
This is in the renderer, so it OK to assume that the more trusted browser
process won't send a bad response, right?

[scheib, 2015-04-20 16:50:13]

https://codereview.chromium.org/1082273006/diff/20001/content/browser/bluetoo...
File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right):

https://codereview.chromium.org/1082273006/diff/20001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:156: // TODO(ortuno): Add
actual implementation of connectGATT.
On 2015/04/20 15:54:00, Tom Sepez wrote:
> Please CC me when you fix the TODO.  I'd like to make a quick check that
you're
> not trusting these values as sent by a renderer.

Thanks Tom, good point:

Thread and request ID are only meaningful to the renderer, the browser must just
pass them back. 

instance ID will need to be managed with care. We plan to have an "allowed
devices map"[1], as per spec, on this BluetoothDispatcherHost with device
instance IDs that have been approved for the renderer. Gio, that's something we
should put together before implementing the actual response for
OnConnectGATTOnUI.

[1] https://webbluetoothcg.github.io/web-bluetooth/#dfn-allowed-devices-map

https://codereview.chromium.org/1082273006/diff/20001/content/child/bluetooth...
File content/child/bluetooth/bluetooth_dispatcher.cc (right):

https://codereview.chromium.org/1082273006/diff/20001/content/child/bluetooth...
content/child/bluetooth/bluetooth_dispatcher.cc:151:
DCHECK(pending_connect_requests_.Lookup(request_id)) << request_id;
On 2015/04/20 15:54:00, Tom Sepez wrote:
> This is in the renderer, so it OK to assume that the more trusted browser
> process won't send a bad response, right?

Right. If the browser is functioning correctly it will send a valid response. It
is not a security concern if it doesn't, we'll just crash the renderer on the
call to onSuccess below from null pointer. As it's not expected to be data we
should handle, and not a security concern, it's DCHECK.
https://www.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREAC...

[scheib, 2015-04-20 16:51:21]

https://codereview.chromium.org/1082273006/diff/20001/content/browser/bluetoo...
File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right):

https://codereview.chromium.org/1082273006/diff/20001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:156: // TODO(ortuno): Add
actual implementation of connectGATT.
On 2015/04/20 16:50:12, scheib wrote:
> On 2015/04/20 15:54:00, Tom Sepez wrote:
> > Please CC me when you fix the TODO.  I'd like to make a quick check that
> you're
> > not trusting these values as sent by a renderer.
> 
> Thanks Tom, good point:
> 
> Thread and request ID are only meaningful to the renderer, the browser must
just
> pass them back. 
> 
> instance ID will need to be managed with care. We plan to have an "allowed
> devices map"[1], as per spec, on this BluetoothDispatcherHost with device
> instance IDs that have been approved for the renderer. Gio, that's something
we
> should put together before implementing the actual response for
> OnConnectGATTOnUI.
> 
> [1] https://webbluetoothcg.github.io/web-bluetooth/#dfn-allowed-devices-map

Gio, please expand this TODO comment to clearly call out the need to check the
allowed devices map before landing this patch.

[ortuno, 2015-04-20 19:54:55]

Patchset #2 (id:40001) has been deleted

[ortuno, 2015-04-20 20:09:33]

@scheib: The blink interface uses WebString now. Could you confirm that
bluetooth_dispatcher is the right place to turn it into a std::string?

https://codereview.chromium.org/1082273006/diff/20001/content/browser/bluetoo...
File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right):

https://codereview.chromium.org/1082273006/diff/20001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:156: // TODO(ortuno): Add
actual implementation of connectGATT.
On 2015/04/20 at 16:51:21, scheib wrote:
> On 2015/04/20 16:50:12, scheib wrote:
> > On 2015/04/20 15:54:00, Tom Sepez wrote:
> > > Please CC me when you fix the TODO.  I'd like to make a quick check that
> > you're
> > > not trusting these values as sent by a renderer.

Will do.

> > 
> > Thanks Tom, good point:
> > 
> > Thread and request ID are only meaningful to the renderer, the browser must
just
> > pass them back. 
> > 
> > instance ID will need to be managed with care. We plan to have an "allowed
> > devices map"[1], as per spec, on this BluetoothDispatcherHost with device
> > instance IDs that have been approved for the renderer. Gio, that's something
we
> > should put together before implementing the actual response for
> > OnConnectGATTOnUI.
> > 
> > [1] https://webbluetoothcg.github.io/web-bluetooth/#dfn-allowed-devices-map
> 
> Gio, please expand this TODO comment to clearly call out the need to check the
allowed devices map before landing this patch.

Done.

https://codereview.chromium.org/1082273006/diff/20001/content/child/bluetooth...
File content/child/bluetooth/bluetooth_dispatcher.cc (right):

https://codereview.chromium.org/1082273006/diff/20001/content/child/bluetooth...
content/child/bluetooth/bluetooth_dispatcher.cc:20: using
blink::WebBluetoothGATTRemoteServer;
On 2015/04/19 at 04:44:19, scheib wrote:
> sort these lines. (surprised a script didn't mention it already?)

Done.

https://codereview.chromium.org/1082273006/diff/20001/content/child/bluetooth...
File content/child/bluetooth/bluetooth_dispatcher.h (right):

https://codereview.chromium.org/1082273006/diff/20001/content/child/bluetooth...
content/child/bluetooth/bluetooth_dispatcher.h:77: // TODO(ortuno): Create an
callback interface to use in the map.
On 2015/04/19 at 04:44:19, scheib wrote:
> Drop the TODO comment for now. I'm not certain we'll end up using an
interface. Looking around some I see several examples where simple strong typed
IDMap members are used and duplicated for each call. I also see some that merge
the callbacks, (WebIDBCallbacks, PermissionDispatcher::CallbackInformation) but
I'm not convinced the code complication there is worth it. To be complete, it
would be trivial to static cast the types in an IDMap<void*, but this doesn't
seem like the situation I'd be willing to do that.

Done.

[scheib, 2015-04-20 20:24:17]

LGTM

On 2015/04/20 20:09:33, ortuno wrote:
> @scheib: The blink interface uses WebString now. Could you confirm that
> bluetooth_dispatcher is the right place to turn it into a std::string?

Yes, as once sent to the content/common data structures the blink interfaces
should no longer be used.

> 
>
https://codereview.chromium.org/1082273006/diff/20001/content/browser/bluetoo...
> File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right):
> 
>
https://codereview.chromium.org/1082273006/diff/20001/content/browser/bluetoo...
> content/browser/bluetooth/bluetooth_dispatcher_host.cc:156: // TODO(ortuno):
Add
> actual implementation of connectGATT.
> On 2015/04/20 at 16:51:21, scheib wrote:
> > On 2015/04/20 16:50:12, scheib wrote:
> > > On 2015/04/20 15:54:00, Tom Sepez wrote:
> > > > Please CC me when you fix the TODO.  I'd like to make a quick check that
> > > you're
> > > > not trusting these values as sent by a renderer.
> 
> Will do.
> 
> > > 
> > > Thanks Tom, good point:
> > > 
> > > Thread and request ID are only meaningful to the renderer, the browser
must
> just
> > > pass them back. 
> > > 
> > > instance ID will need to be managed with care. We plan to have an "allowed
> > > devices map"[1], as per spec, on this BluetoothDispatcherHost with device
> > > instance IDs that have been approved for the renderer. Gio, that's
something
> we
> > > should put together before implementing the actual response for
> > > OnConnectGATTOnUI.
> > > 
> > > [1]
https://webbluetoothcg.github.io/web-bluetooth/#dfn-allowed-devices-map
> > 
> > Gio, please expand this TODO comment to clearly call out the need to check
the
> allowed devices map before landing this patch.
> 
> Done.
> 
>
https://codereview.chromium.org/1082273006/diff/20001/content/child/bluetooth...
> File content/child/bluetooth/bluetooth_dispatcher.cc (right):
> 
>
https://codereview.chromium.org/1082273006/diff/20001/content/child/bluetooth...
> content/child/bluetooth/bluetooth_dispatcher.cc:20: using
> blink::WebBluetoothGATTRemoteServer;
> On 2015/04/19 at 04:44:19, scheib wrote:
> > sort these lines. (surprised a script didn't mention it already?)
> 
> Done.
> 
>
https://codereview.chromium.org/1082273006/diff/20001/content/child/bluetooth...
> File content/child/bluetooth/bluetooth_dispatcher.h (right):
> 
>
https://codereview.chromium.org/1082273006/diff/20001/content/child/bluetooth...
> content/child/bluetooth/bluetooth_dispatcher.h:77: // TODO(ortuno): Create an
> callback interface to use in the map.
> On 2015/04/19 at 04:44:19, scheib wrote:
> > Drop the TODO comment for now. I'm not certain we'll end up using an
> interface. Looking around some I see several examples where simple strong
typed
> IDMap members are used and duplicated for each call. I also see some that
merge
> the callbacks, (WebIDBCallbacks, PermissionDispatcher::CallbackInformation)
but
> I'm not convinced the code complication there is worth it. To be complete, it
> would be trivial to static cast the types in an IDMap<void*, but this doesn't
> seem like the situation I'd be willing to do that.
> 
> Done.

[ortuno, 2015-04-21 01:37:49]

The CQ bit was checked by ortuno@chromium.org

[ortuno, 2015-04-21 01:37:50]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org
Link to the patchset: https://codereview.chromium.org/1082273006/#ps80001
(title: "Fixes based on comments")

[commit-bot: I haz the power, 2015-04-21 01:38:02]

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

[commit-bot: I haz the power, 2015-04-21 02:50:28]

Committed patchset #3 (id:80001)

[commit-bot: I haz the power, 2015-04-21 02:51:37]

Patchset 3 (id:??) landed as
https://crrev.com/503cd360ea98d90f5726913ac10344648529c942
Cr-Commit-Position: refs/heads/master@{#325971}