bluetooth: Move GetCharacteristic(s) over to Mojo

bluetooth: Move GetCharacteristic(s) over to Mojo

Instead of having to different functions to get characteristics we use the same
function with a single_characteristic parameter to indicate if only one
characteristic should be return.

BUG=508771
Committed: https://crrev.com/b0fb6a18c3d6b4689fb848da0767a1e6c78400d5
Cr-Commit-Position: refs/heads/master@{#389966}

[ortuno, 2016-04-15 23:14:27]

Description was changed from

==========
bluetooth: (WIP) Move GetCharacteristic(s) over to Mojo

BUG=
==========

to

==========
bluetooth: Move GetCharacteristic(s) over to Mojo

Instead of having to different functions to get characteristics we use the same
function with a single_characteristic parameter to indicate if only one
characteristic should be return.

BUG=508771
==========

[ortuno, 2016-04-19 23:17:26]

ortuno@chromium.org changed reviewers:
+ jyasskin@chromium.org

[ortuno, 2016-04-19 23:17:26]

jyasskin: PTAL

[Jeffrey Yasskin, 2016-04-22 01:16:38]

https://codereview.chromium.org/1861013005/diff/160001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_dispatcher_host.h (right):

https://codereview.chromium.org/1861013005/diff/160001/content/browser/blueto...
content/browser/bluetooth/bluetooth_dispatcher_host.h:59: CacheQueryResult
QueryCacheForService(const url::Origin& origin,
It might make sense to make BluetoothServiceImpl a friend so you don't need to
gradually rearrange things into the public section during this refactoring.

https://codereview.chromium.org/1861013005/diff/160001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_metrics.h (right):

https://codereview.chromium.org/1861013005/diff/160001/content/browser/blueto...
content/browser/bluetooth/bluetooth_metrics.h:150: // getCharacteristic() &
getCharacteristics() Metrics
nit: I'd spell out "and" since & is a C++ token.

https://codereview.chromium.org/1861013005/diff/160001/content/browser/blueto...
content/browser/bluetooth/bluetooth_metrics.h:167: void
RecordGetCharacteristicsOutcome(bool single_characteristic,
Prefer enum-classes over boolean arguments unless the meaning of 'true' is
really clear at the call site.

You could name this enum to work with getPrimaryServices(),
getIncludedServices(), and getDescriptors() too.

https://codereview.chromium.org/1861013005/diff/160001/content/browser/blueto...
File content/browser/bluetooth/web_bluetooth_service_impl.cc (right):

https://codereview.chromium.org/1861013005/diff/160001/content/browser/blueto...
content/browser/bluetooth/web_bluetooth_service_impl.cc:8: //
descriptor_to_characteristic_] implies a hostile renderer because a renderer
This may not be true after all: if we revoke access to an attribute concurrently
with the renderer making a request about that attribute, then we should just
deny the request and not kill the renderer. No need to fix that while you're
migrating to Mojo, but we should fix it eventually.

https://codereview.chromium.org/1861013005/diff/160001/content/browser/blueto...
content/browser/bluetooth/web_bluetooth_service_impl.cc:231: : nullptr;
The characteristic has a UUID even if the user isn't filtering by UUID. We
should include it unconditionally, right?

https://codereview.chromium.org/1861013005/diff/160001/content/browser/blueto...
content/browser/bluetooth/web_bluetooth_service_impl.cc:485: CacheQueryResult
result = GetBluetoothDispatcherHost()->QueryCacheForService(
Do you need to close the binding_ if this returns BAD_RENDERER?

https://codereview.chromium.org/1861013005/diff/160001/content/browser/blueto...
File content/browser/bluetooth/web_bluetooth_service_impl.h (right):

https://codereview.chromium.org/1861013005/diff/160001/content/browser/blueto...
content/browser/bluetooth/web_bluetooth_service_impl.h:30: struct
CacheQueryResult;
You only need one of the #include or the forward declaration.

https://codereview.chromium.org/1861013005/diff/160001/content/renderer/bluet...
File content/renderer/bluetooth/web_bluetooth_impl.cc (right):

https://codereview.chromium.org/1861013005/diff/160001/content/renderer/bluet...
content/renderer/bluetooth/web_bluetooth_impl.cc:148:
blink::WebVector<blink::WebBluetoothRemoteGATTCharacteristicInit*>
Keep dcheng's TODO here, to make this work with smart pointers.

https://codereview.chromium.org/1861013005/diff/160001/content/renderer/bluet...
content/renderer/bluetooth/web_bluetooth_impl.cc:161:
callbacks->onSuccess(promise_characteristics);
std::move the vector?

https://codereview.chromium.org/1861013005/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTService.cpp
(right):

https://codereview.chromium.org/1861013005/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTService.cpp:35:
class GetCharacteristicsCallback : public
WebBluetoothGetCharacteristicsCallbacks {
Are you sure the extra complexity in this class is worth being able to unify
OnGetCharacteristicsComplete()? It seems like it would be clearer and maybe
shorter to keep separate single- vs multiple-result paths on the renderer side.

https://codereview.chromium.org/1861013005/diff/160001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom
(right):

https://codereview.chromium.org/1861013005/diff/160001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom:93:
bool single_characteristic,
We should make this an enum so the call sites are clearer than 'true' and
'false'.

https://codereview.chromium.org/1861013005/diff/160001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom:94:
string? characteristics_uuid) => (
We should get this type-checked. I'm sorry for not noticing when it went into
bluetooth_messages.h, but that should have been a BluetoothUUID, not just a
string. IIRC, Mojo now has support for validators on particular arguments.

[ortuno, 2016-04-25 15:23:00]

Patchset #10 (id:180001) has been deleted

[ortuno, 2016-04-25 15:29:46]

https://codereview.chromium.org/1861013005/diff/160001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_dispatcher_host.h (right):

https://codereview.chromium.org/1861013005/diff/160001/content/browser/blueto...
content/browser/bluetooth/bluetooth_dispatcher_host.h:59: CacheQueryResult
QueryCacheForService(const url::Origin& origin,
On 2016/04/22 at 01:16:37, Jeffrey Yasskin wrote:
> It might make sense to make BluetoothServiceImpl a friend so you don't need to
gradually rearrange things into the public section during this refactoring.

Done.

https://codereview.chromium.org/1861013005/diff/160001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_metrics.h (right):

https://codereview.chromium.org/1861013005/diff/160001/content/browser/blueto...
content/browser/bluetooth/bluetooth_metrics.h:150: // getCharacteristic() &
getCharacteristics() Metrics
On 2016/04/22 at 01:16:37, Jeffrey Yasskin wrote:
> nit: I'd spell out "and" since & is a C++ token.

Done.

https://codereview.chromium.org/1861013005/diff/160001/content/browser/blueto...
content/browser/bluetooth/bluetooth_metrics.h:167: void
RecordGetCharacteristicsOutcome(bool single_characteristic,
On 2016/04/22 at 01:16:37, Jeffrey Yasskin wrote:
> Prefer enum-classes over boolean arguments unless the meaning of 'true' is
really clear at the call site.
> 
> You could name this enum to work with getPrimaryServices(),
getIncludedServices(), and getDescriptors() too.

Done.

https://codereview.chromium.org/1861013005/diff/160001/content/browser/blueto...
File content/browser/bluetooth/web_bluetooth_service_impl.cc (right):

https://codereview.chromium.org/1861013005/diff/160001/content/browser/blueto...
content/browser/bluetooth/web_bluetooth_service_impl.cc:8: //
descriptor_to_characteristic_] implies a hostile renderer because a renderer
On 2016/04/22 at 01:16:38, Jeffrey Yasskin wrote:
> This may not be true after all: if we revoke access to an attribute
concurrently with the renderer making a request about that attribute, then we
should just deny the request and not kill the renderer. No need to fix that
while you're migrating to Mojo, but we should fix it eventually.

Yup. Opened issue so we don't forget: http://crbug.com/605962

https://codereview.chromium.org/1861013005/diff/160001/content/browser/blueto...
content/browser/bluetooth/web_bluetooth_service_impl.cc:231: : nullptr;
On 2016/04/22 at 01:16:38, Jeffrey Yasskin wrote:
> The characteristic has a UUID even if the user isn't filtering by UUID. We
should include it unconditionally, right?

If the user is filtering by UUID then the renderer knows the UUID already so
there is no need to send it over. This optimization existed in Dispatcher Host
before and I tried to keep it here. It definitely makes the code harder to read
(here and on the renderer) and I don't expect users to be calling
GetCharacteristic a lot so I wouldn't be opposed to removing it.

https://codereview.chromium.org/1861013005/diff/160001/content/browser/blueto...
content/browser/bluetooth/web_bluetooth_service_impl.cc:485: CacheQueryResult
result = GetBluetoothDispatcherHost()->QueryCacheForService(
On 2016/04/22 at 01:16:38, Jeffrey Yasskin wrote:
> Do you need to close the binding_ if this returns BAD_RENDERER?

Done.

https://codereview.chromium.org/1861013005/diff/160001/content/browser/blueto...
File content/browser/bluetooth/web_bluetooth_service_impl.h (right):

https://codereview.chromium.org/1861013005/diff/160001/content/browser/blueto...
content/browser/bluetooth/web_bluetooth_service_impl.h:30: struct
CacheQueryResult;
On 2016/04/22 at 01:16:38, Jeffrey Yasskin wrote:
> You only need one of the #include or the forward declaration.

Done.

https://codereview.chromium.org/1861013005/diff/160001/content/renderer/bluet...
File content/renderer/bluetooth/web_bluetooth_impl.cc (right):

https://codereview.chromium.org/1861013005/diff/160001/content/renderer/bluet...
content/renderer/bluetooth/web_bluetooth_impl.cc:148:
blink::WebVector<blink::WebBluetoothRemoteGATTCharacteristicInit*>
On 2016/04/22 at 01:16:38, Jeffrey Yasskin wrote:
> Keep dcheng's TODO here, to make this work with smart pointers.

Done.

https://codereview.chromium.org/1861013005/diff/160001/content/renderer/bluet...
content/renderer/bluetooth/web_bluetooth_impl.cc:161:
callbacks->onSuccess(promise_characteristics);
On 2016/04/22 at 01:16:38, Jeffrey Yasskin wrote:
> std::move the vector?

This is passed by reference. Is that wrong?

https://codereview.chromium.org/1861013005/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTService.cpp
(right):

https://codereview.chromium.org/1861013005/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTService.cpp:35:
class GetCharacteristicsCallback : public
WebBluetoothGetCharacteristicsCallbacks {
On 2016/04/22 at 01:16:38, Jeffrey Yasskin wrote:
> Are you sure the extra complexity in this class is worth being able to unify
OnGetCharacteristicsComplete()? It seems like it would be clearer and maybe
shorter to keep separate single- vs multiple-result paths on the renderer side.

Thinking this through:

Right now the reason we use this class is so that we can use a single
WebCallbacks type when we call webBluetooth->getCharacteristics(). If we decide
to have separate paths, we could go back to using CallbackPromiseAdapter and get
rid of this class but we would need two functions in WebBluetoothImpl, one for
each WebCallbacks type. Each function will also have to have its own callback
with its own logic.

This seems fine for now but I'm scared we will go back to this in the long run:

We will need to implement a similar class as this for setting the parent of the
object (See ConnectCallback in BluetoothRemoteGATTServer). I think we would want
to implement only one of these classes and that class would look a lot like the
class in this CL.

Another thing to keep in mind is that the content/renderer layer is going away
soon so any decisions we take regarding it won't matter for too long.

So in conclusion I think we will eventually need this class and it already
allows us to remove some repeated code.

https://codereview.chromium.org/1861013005/diff/160001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom
(right):

https://codereview.chromium.org/1861013005/diff/160001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom:93:
bool single_characteristic,
On 2016/04/22 at 01:16:38, Jeffrey Yasskin wrote:
> We should make this an enum so the call sites are clearer than 'true' and
'false'.

Done.

https://codereview.chromium.org/1861013005/diff/160001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom:94:
string? characteristics_uuid) => (
On 2016/04/22 at 01:16:38, Jeffrey Yasskin wrote:
> We should get this type-checked. I'm sorry for not noticing when it went into
bluetooth_messages.h, but that should have been a BluetoothUUID, not just a
string. IIRC, Mojo now has support for validators on particular arguments.

No problem! I already noticed :) Yes, Mojo does have support but it's been
changing a lot lately so I've been advised to stay away from it until it
stabilizes a bit. I was planning on passing strings for now and once the move is
complete and typemaps are more stable I was going to replace all instances with
BluetoothUUID. Do you think that's acceptable? Otherwise I can start now, but
this patch will get slightly bigger.

[ortuno, 2016-04-25 15:30:01]

The CQ bit was checked by ortuno@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-25 15:30:16]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1861013005/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1861013005/220001

[commit-bot: I haz the power, 2016-04-25 15:32:29]

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

[commit-bot: I haz the power, 2016-04-25 15:32:30]

Dry run: Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_gn...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[ortuno, 2016-04-25 15:42:39]

The CQ bit was checked by ortuno@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-25 15:42:48]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1861013005/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1861013005/240001

[commit-bot: I haz the power, 2016-04-25 15:53:36]

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

[commit-bot: I haz the power, 2016-04-25 15:53:37]

Dry run: Try jobs failed on following builders:
  cast_shell_android on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[ortuno, 2016-04-25 16:29:57]

The CQ bit was checked by ortuno@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-25 16:30:15]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1861013005/280001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1861013005/280001

[Jeffrey Yasskin, 2016-04-25 17:26:11]

LGTM, thanks!

https://codereview.chromium.org/1861013005/diff/160001/content/browser/blueto...
File content/browser/bluetooth/web_bluetooth_service_impl.cc (right):

https://codereview.chromium.org/1861013005/diff/160001/content/browser/blueto...
content/browser/bluetooth/web_bluetooth_service_impl.cc:231: : nullptr;
On 2016/04/25 15:29:45, ortuno wrote:
> On 2016/04/22 at 01:16:38, Jeffrey Yasskin wrote:
> > The characteristic has a UUID even if the user isn't filtering by UUID. We
> should include it unconditionally, right?
> 
> If the user is filtering by UUID then the renderer knows the UUID already so
> there is no need to send it over. This optimization existed in Dispatcher Host
> before and I tried to keep it here. It definitely makes the code harder to
read
> (here and on the renderer) and I don't expect users to be calling
> GetCharacteristic a lot so I wouldn't be opposed to removing it.

Yeah, we might DCHECK that they're the same on the renderer side, but I like
having simpler code. Sending an extra 36 bytes isn't likely to be significant,
and we could optimize UUIDs in general if it is.

https://codereview.chromium.org/1861013005/diff/160001/content/renderer/bluet...
File content/renderer/bluetooth/web_bluetooth_impl.cc (right):

https://codereview.chromium.org/1861013005/diff/160001/content/renderer/bluet...
content/renderer/bluetooth/web_bluetooth_impl.cc:161:
callbacks->onSuccess(promise_characteristics);
On 2016/04/25 15:29:45, ortuno wrote:
> On 2016/04/22 at 01:16:38, Jeffrey Yasskin wrote:
> > std::move the vector?
> 
> This is passed by reference. Is that wrong?

Eventually, we should make as much as possible move things all the way through,
but it's probably not a big problem, and may not even be making a copy
underneath. So never mind.

https://codereview.chromium.org/1861013005/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTService.cpp
(right):

https://codereview.chromium.org/1861013005/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTService.cpp:35:
class GetCharacteristicsCallback : public
WebBluetoothGetCharacteristicsCallbacks {
On 2016/04/25 15:29:45, ortuno wrote:
> On 2016/04/22 at 01:16:38, Jeffrey Yasskin wrote:
> > Are you sure the extra complexity in this class is worth being able to unify
> OnGetCharacteristicsComplete()? It seems like it would be clearer and maybe
> shorter to keep separate single- vs multiple-result paths on the renderer
side.
> 
> Thinking this through:
> 
> Right now the reason we use this class is so that we can use a single
> WebCallbacks type when we call webBluetooth->getCharacteristics(). If we
decide
> to have separate paths, we could go back to using CallbackPromiseAdapter and
get
> rid of this class but we would need two functions in WebBluetoothImpl, one for
> each WebCallbacks type. Each function will also have to have its own callback
> with its own logic.
> 
> This seems fine for now but I'm scared we will go back to this in the long
run:
> 
> We will need to implement a similar class as this for setting the parent of
the
> object (See ConnectCallback in BluetoothRemoteGATTServer). I think we would
want
> to implement only one of these classes and that class would look a lot like
the
> class in this CL.
> 
> Another thing to keep in mind is that the content/renderer layer is going away
> soon so any decisions we take regarding it won't matter for too long.
> 
> So in conclusion I think we will eventually need this class and it already
> allows us to remove some repeated code.

Ok, sounds good to keep it. Thanks for explaining the tradeoffs!

https://codereview.chromium.org/1861013005/diff/160001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom
(right):

https://codereview.chromium.org/1861013005/diff/160001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom:94:
string? characteristics_uuid) => (
On 2016/04/25 15:29:45, ortuno wrote:
> On 2016/04/22 at 01:16:38, Jeffrey Yasskin wrote:
> > We should get this type-checked. I'm sorry for not noticing when it went
into
> bluetooth_messages.h, but that should have been a BluetoothUUID, not just a
> string. IIRC, Mojo now has support for validators on particular arguments.
> 
> No problem! I already noticed :) Yes, Mojo does have support but it's been
> changing a lot lately so I've been advised to stay away from it until it
> stabilizes a bit. I was planning on passing strings for now and once the move
is
> complete and typemaps are more stable I was going to replace all instances
with
> BluetoothUUID. Do you think that's acceptable? Otherwise I can start now, but
> this patch will get slightly bigger.

Waiting sounds good.

https://codereview.chromium.org/1861013005/diff/280001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom
(right):

https://codereview.chromium.org/1861013005/diff/280001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom:72:
enum WebBluetoothQueryType {
Maybe s/Type/Quantity/, but it's not a big deal.

[commit-bot: I haz the power, 2016-04-25 17:57:49]

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

[commit-bot: I haz the power, 2016-04-25 17:57:50]

Dry run: This issue passed the CQ dry run.

[ortuno, 2016-04-25 20:37:41]

Thanks!

https://codereview.chromium.org/1861013005/diff/160001/content/browser/blueto...
File content/browser/bluetooth/web_bluetooth_service_impl.cc (right):

https://codereview.chromium.org/1861013005/diff/160001/content/browser/blueto...
content/browser/bluetooth/web_bluetooth_service_impl.cc:231: : nullptr;
On 2016/04/25 at 17:26:10, Jeffrey Yasskin wrote:
> On 2016/04/25 15:29:45, ortuno wrote:
> > On 2016/04/22 at 01:16:38, Jeffrey Yasskin wrote:
> > > The characteristic has a UUID even if the user isn't filtering by UUID. We
> > should include it unconditionally, right?
> > 
> > If the user is filtering by UUID then the renderer knows the UUID already so
> > there is no need to send it over. This optimization existed in Dispatcher
Host
> > before and I tried to keep it here. It definitely makes the code harder to
read
> > (here and on the renderer) and I don't expect users to be calling
> > GetCharacteristic a lot so I wouldn't be opposed to removing it.
> 
> Yeah, we might DCHECK that they're the same on the renderer side, but I like
having simpler code. Sending an extra 36 bytes isn't likely to be significant,
and we could optimize UUIDs in general if it is.

Done.

https://codereview.chromium.org/1861013005/diff/160001/content/renderer/bluet...
File content/renderer/bluetooth/web_bluetooth_impl.cc (right):

https://codereview.chromium.org/1861013005/diff/160001/content/renderer/bluet...
content/renderer/bluetooth/web_bluetooth_impl.cc:161:
callbacks->onSuccess(promise_characteristics);
On 2016/04/25 at 17:26:10, Jeffrey Yasskin wrote:
> On 2016/04/25 15:29:45, ortuno wrote:
> > On 2016/04/22 at 01:16:38, Jeffrey Yasskin wrote:
> > > std::move the vector?
> > 
> > This is passed by reference. Is that wrong?
> 
> Eventually, we should make as much as possible move things all the way
through, but it's probably not a big problem, and may not even be making a copy
underneath. So never mind.

Acknowledge.

https://codereview.chromium.org/1861013005/diff/160001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom
(right):

https://codereview.chromium.org/1861013005/diff/160001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom:94:
string? characteristics_uuid) => (
On 2016/04/25 at 17:26:11, Jeffrey Yasskin wrote:
> On 2016/04/25 15:29:45, ortuno wrote:
> > On 2016/04/22 at 01:16:38, Jeffrey Yasskin wrote:
> > > We should get this type-checked. I'm sorry for not noticing when it went
into
> > bluetooth_messages.h, but that should have been a BluetoothUUID, not just a
> > string. IIRC, Mojo now has support for validators on particular arguments.
> > 
> > No problem! I already noticed :) Yes, Mojo does have support but it's been
> > changing a lot lately so I've been advised to stay away from it until it
> > stabilizes a bit. I was planning on passing strings for now and once the
move is
> > complete and typemaps are more stable I was going to replace all instances
with
> > BluetoothUUID. Do you think that's acceptable? Otherwise I can start now,
but
> > this patch will get slightly bigger.
> 
> Waiting sounds good.

I opened an issue so that we don't forget.

https://codereview.chromium.org/1861013005/diff/280001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom
(right):

https://codereview.chromium.org/1861013005/diff/280001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom:72:
enum WebBluetoothQueryType {
On 2016/04/25 at 17:26:11, Jeffrey Yasskin wrote:
> Maybe s/Type/Quantity/, but it's not a big deal.

Done.

[ortuno, 2016-04-25 20:40:21]

ortuno@chromium.org changed reviewers:
+ palmer@chromium.org, rockot@chromium.org

[ortuno, 2016-04-25 20:40:22]

palmer: PTAL at bluetooth_messages and web_bluetooth.mojom
rockot: PTAL at mojo parts i.e. web_bluetooth_service_impl.h/cc
web_bluetooth_impl.h/cc web_bluetooth.mojom

[palmer, 2016-04-25 21:19:01]

https://codereview.chromium.org/1861013005/diff/300001/content/browser/blueto...
File content/browser/bluetooth/web_bluetooth_service_impl.cc (right):

https://codereview.chromium.org/1861013005/diff/300001/content/browser/blueto...
content/browser/bluetooth/web_bluetooth_service_impl.cc:217: // If value is
already in map, DCHECK it's valid.
Is this purely a code correctness check, or is it something bad that could
happen at runtime in a production build?

https://codereview.chromium.org/1861013005/diff/300001/content/browser/blueto...
File content/browser/bluetooth/web_bluetooth_service_impl.h (right):

https://codereview.chromium.org/1861013005/diff/300001/content/browser/blueto...
content/browser/bluetooth/web_bluetooth_service_impl.h:130: // result.error will
contain the error that should be send to the renderer.
Typo: "...sent to..."

https://codereview.chromium.org/1861013005/diff/300001/content/browser/blueto...
content/browser/bluetooth/web_bluetooth_service_impl.h:149: // Maps to get the
object's parent based on it's instanceID.
Typo: "its".

Since this comment is introducing several things below, have a blank line after
it. (As you did on line 134, above.)

https://codereview.chromium.org/1861013005/diff/300001/content/browser/blueto...
content/browser/bluetooth/web_bluetooth_service_impl.h:151:
std::unordered_map<std::string, std::string> characteristic_to_service_;
Not |characteristic_id_to_service_|, to match
|characteristic_id_to_notify_session_|? (I'm not sure which is right, just
wondering about consistency.)

[ortuno, 2016-04-25 22:43:23]

Thanks!

https://codereview.chromium.org/1861013005/diff/300001/content/browser/blueto...
File content/browser/bluetooth/web_bluetooth_service_impl.cc (right):

https://codereview.chromium.org/1861013005/diff/300001/content/browser/blueto...
content/browser/bluetooth/web_bluetooth_service_impl.cc:217: // If value is
already in map, DCHECK it's valid.
On 2016/04/25 at 21:19:01, palmer wrote:
> Is this purely a code correctness check, or is it something bad that could
happen at runtime in a production build?

It's only for code correctness. In the past, this line has caught bugs in our
tests because we re-use identifiers in tests. In production we will never see
issues because identifiers are guaranteed to be unique.

https://codereview.chromium.org/1861013005/diff/300001/content/browser/blueto...
File content/browser/bluetooth/web_bluetooth_service_impl.h (right):

https://codereview.chromium.org/1861013005/diff/300001/content/browser/blueto...
content/browser/bluetooth/web_bluetooth_service_impl.h:130: // result.error will
contain the error that should be send to the renderer.
On 2016/04/25 at 21:19:01, palmer wrote:
> Typo: "...sent to..."

Done.

https://codereview.chromium.org/1861013005/diff/300001/content/browser/blueto...
content/browser/bluetooth/web_bluetooth_service_impl.h:149: // Maps to get the
object's parent based on it's instanceID.
On 2016/04/25 at 21:19:01, palmer wrote:
> Typo: "its".
> 

Done.

> Since this comment is introducing several things below, have a blank line
after it. (As you did on line 134, above.)

Done.

https://codereview.chromium.org/1861013005/diff/300001/content/browser/blueto...
content/browser/bluetooth/web_bluetooth_service_impl.h:151:
std::unordered_map<std::string, std::string> characteristic_to_service_;
On 2016/04/25 at 21:19:01, palmer wrote:
> Not |characteristic_id_to_service_|, to match
|characteristic_id_to_notify_session_|? (I'm not sure which is right, just
wondering about consistency.)

Changed to characteristic_id_to_service_id_.

[ortuno, 2016-04-25 22:43:28]

The CQ bit was checked by ortuno@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-25 22:43:53]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1861013005/320001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1861013005/320001

[commit-bot: I haz the power, 2016-04-26 00:47:05]

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

[commit-bot: I haz the power, 2016-04-26 00:47:07]

Dry run: This issue passed the CQ dry run.

[Ken Rockot(use gerrit already), 2016-04-26 14:34:17]

lgtm

[palmer, 2016-04-26 23:15:19]

lgtm

[ortuno, 2016-04-26 23:16:32]

The CQ bit was checked by ortuno@chromium.org

[ortuno, 2016-04-26 23:16:32]

The patchset sent to the CQ was uploaded after l-g-t-m from
jyasskin@chromium.org
Link to the patchset: https://codereview.chromium.org/1861013005/#ps320001
(title: "Address palmer's comments")

[commit-bot: I haz the power, 2016-04-26 23:17:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1861013005/320001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1861013005/320001

[commit-bot: I haz the power, 2016-04-27 01:45:39]

Committed patchset #16 (id:320001)

[commit-bot: I haz the power, 2016-04-27 01:46:47]

Description was changed from

==========
bluetooth: Move GetCharacteristic(s) over to Mojo

Instead of having to different functions to get characteristics we use the same
function with a single_characteristic parameter to indicate if only one
characteristic should be return.

BUG=508771
==========

to

==========
bluetooth: Move GetCharacteristic(s) over to Mojo

Instead of having to different functions to get characteristics we use the same
function with a single_characteristic parameter to indicate if only one
characteristic should be return.

BUG=508771

Committed: https://crrev.com/b0fb6a18c3d6b4689fb848da0767a1e6c78400d5
Cr-Commit-Position: refs/heads/master@{#389966}
==========

[commit-bot: I haz the power, 2016-04-27 01:46:48]

Patchset 16 (id:??) landed as
https://crrev.com/b0fb6a18c3d6b4689fb848da0767a1e6c78400d5
Cr-Commit-Position: refs/heads/master@{#389966}