bluetooth: Use BluetoothUUID instead of string when sending uuids

bluetooth: Use BluetoothUUID instead of string when sending uuids

This patch introduces a Typemap from mojo::String to unique_ptr<device::BluetoothUUID>.

Mapping to base::Optional allows us to have a nullable type.

BUG=606446
Committed: https://crrev.com/e491483f14621c2bf2587c2ccbf3905b57d5acac
Cr-Commit-Position: refs/heads/master@{#399259}

[ortuno, 2016-05-25 23:17:10]

Description was changed from

==========
bluetooth: Use BluetoothUUID instead of string when sending uuids

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

to

==========
bluetooth: Use BluetoothUUID instead of string when sending uuids

This patch introduces a Typemap from mojo::String to
unique_ptr<device::BluetoothUUID>.

Mapping to unique_ptr allows us to have a nullable type.

BUG=606446
==========

[ortuno, 2016-05-25 23:17:32]

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

[commit-bot: I haz the power, 2016-05-25 23:18:42]

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

[commit-bot: I haz the power, 2016-05-25 23:25:36]

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

[commit-bot: I haz the power, 2016-05-25 23:25:37]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[ortuno, 2016-05-25 23:33:14]

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

[commit-bot: I haz the power, 2016-05-25 23:33:55]

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

[commit-bot: I haz the power, 2016-05-25 23:50:49]

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

[commit-bot: I haz the power, 2016-05-25 23:50:50]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[ortuno, 2016-05-26 00:02:47]

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

[commit-bot: I haz the power, 2016-05-26 00:03:50]

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

[commit-bot: I haz the power, 2016-05-26 00:19:01]

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

[commit-bot: I haz the power, 2016-05-26 00:19:02]

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

[ortuno, 2016-05-26 15:49:08]

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

[commit-bot: I haz the power, 2016-05-26 15:49:27]

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

[commit-bot: I haz the power, 2016-05-26 16:32:57]

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

[commit-bot: I haz the power, 2016-05-26 16:32:57]

Dry run: Try jobs failed on following builders:
  win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)

[ortuno, 2016-05-26 19:56:44]

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

[commit-bot: I haz the power, 2016-05-26 19:57:27]

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

[ortuno, 2016-05-26 20:46:12]

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

[ortuno, 2016-05-26 20:46:13]

rockot: PTAL. No need to do a thorough review of content/ code as I'll add
jyasskin for it.

https://codereview.chromium.org/2015463004/diff/140001/device/bluetooth/mojo/...
File device/bluetooth/mojo/bluetooth_uuid.typemap (right):

https://codereview.chromium.org/2015463004/diff/140001/device/bluetooth/mojo/...
device/bluetooth/mojo/bluetooth_uuid.typemap:11: type_mappings = [
"device.mojom.BluetoothUUID=std::unique_ptr<device::BluetoothUUID>" ]
It would be nice if I could define a different type that is used for nullable
fields e.g.:

type_mappings = [ "device.mojom.BluetoothUUID=device::BluetoothUUID" ]
type_mappings = [
"device.mojom.BluetoothUUID?=std::unique_ptr<device::BluetoothUUID>" ]

[Ken Rockot(use gerrit already), 2016-05-26 21:05:07]

https://codereview.chromium.org/2015463004/diff/140001/device/bluetooth/mojo/...
File device/bluetooth/mojo/BUILD.gn (right):

https://codereview.chromium.org/2015463004/diff/140001/device/bluetooth/mojo/...
device/bluetooth/mojo/BUILD.gn:5:
import("//mojo/public/tools/bindings/mojom.gni")
Please put this stuff in device/bluetooth/public/interfaces - I realize that
pattern is not currently followed by other mojom stuff in top-level directories
like ui or url, but it ultimately should be (imho). I would start treating
anything in //device as if it should be in //components.

https://codereview.chromium.org/2015463004/diff/140001/device/bluetooth/mojo/...
File device/bluetooth/mojo/bluetooth_uuid.mojom (right):

https://codereview.chromium.org/2015463004/diff/140001/device/bluetooth/mojo/...
device/bluetooth/mojo/bluetooth_uuid.mojom:8: string? uuid;
It seems pointless to make this nullable

https://codereview.chromium.org/2015463004/diff/140001/device/bluetooth/mojo/...
File device/bluetooth/mojo/bluetooth_uuid.typemap (right):

https://codereview.chromium.org/2015463004/diff/140001/device/bluetooth/mojo/...
device/bluetooth/mojo/bluetooth_uuid.typemap:11: type_mappings = [
"device.mojom.BluetoothUUID=std::unique_ptr<device::BluetoothUUID>" ]
On 2016/05/26 at 20:46:13, ortuno wrote:
> It would be nice if I could define a different type that is used for nullable
fields e.g.:
> 
> type_mappings = [ "device.mojom.BluetoothUUID=device::BluetoothUUID" ]
> type_mappings = [
"device.mojom.BluetoothUUID?=std::unique_ptr<device::BluetoothUUID>" ]

I don't think we want to force people to do that. I think we should try to
establish a general purpose solution for nullable types that we can apply
regardless of typemapping. This is an ongoing discussion that we can hopefully
resolve soon.

As an alternative interim solution for you, would it maybe be cleaner to support
an "invalid" state for your device::BluetoothUUID type rather than map to a
unique_ptr?

https://codereview.chromium.org/2015463004/diff/140001/device/bluetooth/mojo/...
File device/bluetooth/mojo/bluetooth_uuid_struct_traits.h (right):

https://codereview.chromium.org/2015463004/diff/140001/device/bluetooth/mojo/...
device/bluetooth/mojo/bluetooth_uuid_struct_traits.h:18: return !uuid.get();
nit: !uuid should be sufficient

https://codereview.chromium.org/2015463004/diff/140001/device/bluetooth/mojo/...
device/bluetooth/mojo/bluetooth_uuid_struct_traits.h:22: *output = nullptr;
optional style/opinion nit: perhaps output->reset() would be more readable

https://codereview.chromium.org/2015463004/diff/140001/device/bluetooth/mojo/...
device/bluetooth/mojo/bluetooth_uuid_struct_traits.h:27: DCHECK(uuid.get());
nit: DCHECK(uuid) should be sufficient

[commit-bot: I haz the power, 2016-05-26 22:42:41]

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

[commit-bot: I haz the power, 2016-05-26 22:42:42]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[ortuno, 2016-05-27 03:46:53]

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

[commit-bot: I haz the power, 2016-05-27 03:47:25]

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

[commit-bot: I haz the power, 2016-05-27 05:34:53]

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

[commit-bot: I haz the power, 2016-05-27 05:34:55]

Dry run: This issue passed the CQ dry run.

[ortuno, 2016-05-27 14:38:36]

https://codereview.chromium.org/2015463004/diff/140001/device/bluetooth/mojo/...
File device/bluetooth/mojo/BUILD.gn (right):

https://codereview.chromium.org/2015463004/diff/140001/device/bluetooth/mojo/...
device/bluetooth/mojo/BUILD.gn:5:
import("//mojo/public/tools/bindings/mojom.gni")
On 2016/05/26 at 21:05:07, Ken Rockot wrote:
> Please put this stuff in device/bluetooth/public/interfaces - I realize that
pattern is not currently followed by other mojom stuff in top-level directories
like ui or url, but it ultimately should be (imho). I would start treating
anything in //device as if it should be in //components.

Done.

https://codereview.chromium.org/2015463004/diff/140001/device/bluetooth/mojo/...
File device/bluetooth/mojo/bluetooth_uuid.mojom (right):

https://codereview.chromium.org/2015463004/diff/140001/device/bluetooth/mojo/...
device/bluetooth/mojo/bluetooth_uuid.mojom:8: string? uuid;
On 2016/05/26 at 21:05:07, Ken Rockot wrote:
> It seems pointless to make this nullable

Done

https://codereview.chromium.org/2015463004/diff/140001/device/bluetooth/mojo/...
File device/bluetooth/mojo/bluetooth_uuid.typemap (right):

https://codereview.chromium.org/2015463004/diff/140001/device/bluetooth/mojo/...
device/bluetooth/mojo/bluetooth_uuid.typemap:11: type_mappings = [
"device.mojom.BluetoothUUID=std::unique_ptr<device::BluetoothUUID>" ]
On 2016/05/26 at 21:05:07, Ken Rockot wrote:
> On 2016/05/26 at 20:46:13, ortuno wrote:
> > It would be nice if I could define a different type that is used for
nullable fields e.g.:
> > 
> > type_mappings = [ "device.mojom.BluetoothUUID=device::BluetoothUUID" ]
> > type_mappings = [
"device.mojom.BluetoothUUID?=std::unique_ptr<device::BluetoothUUID>" ]
> 
> I don't think we want to force people to do that. I think we should try to
establish a general purpose solution for nullable types that we can apply
regardless of typemapping. This is an ongoing discussion that we can hopefully
resolve soon.
> 

Yes. That would be even nicer.

> As an alternative interim solution for you, would it maybe be cleaner to
support an "invalid" state for your device::BluetoothUUID type rather than map
to a unique_ptr?

BluetoothUUID already has an "invalid" state but we use that for validation. See
bluetooth_uuid_struct_traits.h

https://codereview.chromium.org/2015463004/diff/140001/device/bluetooth/mojo/...
File device/bluetooth/mojo/bluetooth_uuid_struct_traits.h (right):

https://codereview.chromium.org/2015463004/diff/140001/device/bluetooth/mojo/...
device/bluetooth/mojo/bluetooth_uuid_struct_traits.h:18: return !uuid.get();
On 2016/05/26 at 21:05:07, Ken Rockot wrote:
> nit: !uuid should be sufficient

Done.

https://codereview.chromium.org/2015463004/diff/140001/device/bluetooth/mojo/...
device/bluetooth/mojo/bluetooth_uuid_struct_traits.h:22: *output = nullptr;
On 2016/05/26 at 21:05:07, Ken Rockot wrote:
> optional style/opinion nit: perhaps output->reset() would be more readable

Done.

https://codereview.chromium.org/2015463004/diff/140001/device/bluetooth/mojo/...
device/bluetooth/mojo/bluetooth_uuid_struct_traits.h:27: DCHECK(uuid.get());
On 2016/05/26 at 21:05:07, Ken Rockot wrote:
> nit: DCHECK(uuid) should be sufficient

Done.

[Ken Rockot(use gerrit already), 2016-05-27 14:57:42]

lgtm

https://codereview.chromium.org/2015463004/diff/140001/device/bluetooth/mojo/...
File device/bluetooth/mojo/bluetooth_uuid.typemap (right):

https://codereview.chromium.org/2015463004/diff/140001/device/bluetooth/mojo/...
device/bluetooth/mojo/bluetooth_uuid.typemap:11: type_mappings = [
"device.mojom.BluetoothUUID=std::unique_ptr<device::BluetoothUUID>" ]
On 2016/05/27 at 14:38:35, ortuno wrote:
> On 2016/05/26 at 21:05:07, Ken Rockot wrote:
> > On 2016/05/26 at 20:46:13, ortuno wrote:
> > > It would be nice if I could define a different type that is used for
nullable fields e.g.:
> > > 
> > > type_mappings = [ "device.mojom.BluetoothUUID=device::BluetoothUUID" ]
> > > type_mappings = [
"device.mojom.BluetoothUUID?=std::unique_ptr<device::BluetoothUUID>" ]
> > 
> > I don't think we want to force people to do that. I think we should try to
establish a general purpose solution for nullable types that we can apply
regardless of typemapping. This is an ongoing discussion that we can hopefully
resolve soon.
> > 
> 
> Yes. That would be even nicer.
> 
> > As an alternative interim solution for you, would it maybe be cleaner to
support an "invalid" state for your device::BluetoothUUID type rather than map
to a unique_ptr?
> 
> BluetoothUUID already has an "invalid" state but we use that for validation.
See bluetooth_uuid_struct_traits.h

Doh, right.

[ortuno, 2016-05-27 16:01:38]

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

[ortuno, 2016-05-27 16:01:39]

jyasskin: PTAL

[Jeffrey Yasskin, 2016-05-28 04:38:06]

https://codereview.chromium.org/2015463004/diff/180001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_allowed_devices_map.cc (right):

https://codereview.chromium.org/2015463004/diff/180001/content/browser/blueto...
content/browser/bluetooth/bluetooth_allowed_devices_map.cc:164: for (const
std::unique_ptr<BluetoothUUID>& uuid : filter->services) {
I'm definitely sad that there's no type-based way to say that these UUIDs will
never be null. We can wait until Ken's general solution though.

https://codereview.chromium.org/2015463004/diff/180001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_allowed_devices_map_unittest.cc
(right):

https://codereview.chromium.org/2015463004/diff/180001/content/browser/blueto...
content/browser/bluetooth/bluetooth_allowed_devices_map_unittest.cc:26: const
BluetoothUUID kGlucoseUUID(kGlucoseUUIDString);
Didn't we just get rid of these? ;-)

https://codereview.chromium.org/2015463004/diff/180001/content/browser/blueto...
content/browser/bluetooth/bluetooth_allowed_devices_map_unittest.cc:40:
std::unique_ptr<BluetoothUUID> GetGlucoseUUID() {
Maybe "GetGlucoseUUIDPtr()" to be clear how this is different from kGlucoseUUID.

https://codereview.chromium.org/2015463004/diff/180001/content/browser/blueto...
content/browser/bluetooth/bluetooth_allowed_devices_map_unittest.cc:41: return
base::WrapUnique(new BluetoothUUID(kGlucoseUUID));
We have base::MakeUnique<BluetoothUUID>(kGlucoseUUID) now. Try to switch to that
everywhere.

https://codereview.chromium.org/2015463004/diff/180001/content/browser/blueto...
content/browser/bluetooth/bluetooth_allowed_devices_map_unittest.cc:213:
scanFilter1Clone->services.push_back(GetGlucoseUUID());
The fact that we can't clone messages with unique_ptrs is also sad. Using
base::Optional would fix this, and I think WTF::Optional's definition means
it'll be fine to use that in blink too.

https://codereview.chromium.org/2015463004/diff/180001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_blacklist_unittest.cc (right):

https://codereview.chromium.org/2015463004/diff/180001/content/browser/blueto...
content/browser/bluetooth/bluetooth_blacklist_unittest.cc:20: bool
OptionalServicesEqual(
Write this to return an AssertionResult, and then you can give a good error
message for mismatches:
https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuid...

https://codereview.chromium.org/2015463004/diff/180001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_metrics.cc (right):

https://codereview.chromium.org/2015463004/diff/180001/content/browser/blueto...
content/browser/bluetooth/bluetooth_metrics.cc:31: // TODO(520284): Other than
verifying that uuid exists, this logic
If 'uuid' becomes a base::Optional, s/exists/contains a value/. Otherwise, "is
non-null".

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

https://codereview.chromium.org/2015463004/diff/180001/content/browser/blueto...
content/browser/bluetooth/web_bluetooth_service_impl.cc:428: if
(characteristics_uuid.get() &&
unique_ptr and base::Optional can be directly used in an && expression without
an explicit .get().

https://codereview.chromium.org/2015463004/diff/180001/content/renderer/bluet...
File content/renderer/bluetooth/bluetooth_type_converters.cc (right):

https://codereview.chromium.org/2015463004/diff/180001/content/renderer/bluet...
content/renderer/bluetooth/bluetooth_type_converters.cc:52: return
base::WrapUnique(new device::BluetoothUUID(web_string.utf8()));
Shouldn't the TypeConverter kill the connection if the UUID was malformed?

https://codereview.chromium.org/2015463004/diff/180001/device/bluetooth/publi...
File device/bluetooth/public/interfaces/bluetooth_uuid.typemap (right):

https://codereview.chromium.org/2015463004/diff/180001/device/bluetooth/publi...
device/bluetooth/public/interfaces/bluetooth_uuid.typemap:11: type_mappings = [
"device.mojom.BluetoothUUID=std::unique_ptr<device::BluetoothUUID>" ]
See if this can be base::Optional instead of unique_ptr: it matches nullable
semantics better.

[ortuno, 2016-05-31 17:06:27]

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

[commit-bot: I haz the power, 2016-05-31 17:06:48]

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

[ortuno, 2016-05-31 17:30:47]

https://codereview.chromium.org/2015463004/diff/180001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_allowed_devices_map.cc (right):

https://codereview.chromium.org/2015463004/diff/180001/content/browser/blueto...
content/browser/bluetooth/bluetooth_allowed_devices_map.cc:164: for (const
std::unique_ptr<BluetoothUUID>& uuid : filter->services) {
On 2016/05/28 at 04:38:06, Jeffrey Yasskin wrote:
> I'm definitely sad that there's no type-based way to say that these UUIDs will
never be null. We can wait until Ken's general solution though.

I'm OK with waiting.

https://codereview.chromium.org/2015463004/diff/180001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_allowed_devices_map_unittest.cc
(right):

https://codereview.chromium.org/2015463004/diff/180001/content/browser/blueto...
content/browser/bluetooth/bluetooth_allowed_devices_map_unittest.cc:26: const
BluetoothUUID kGlucoseUUID(kGlucoseUUIDString);
On 2016/05/28 at 04:38:06, Jeffrey Yasskin wrote:
> Didn't we just get rid of these? ;-)

We did! But turns out we needed them hehe

https://codereview.chromium.org/2015463004/diff/180001/content/browser/blueto...
content/browser/bluetooth/bluetooth_allowed_devices_map_unittest.cc:40:
std::unique_ptr<BluetoothUUID> GetGlucoseUUID() {
On 2016/05/28 at 04:38:06, Jeffrey Yasskin wrote:
> Maybe "GetGlucoseUUIDPtr()" to be clear how this is different from
kGlucoseUUID.

Since these now return an Optional, I changed them to ...UUIDOpt.

https://codereview.chromium.org/2015463004/diff/180001/content/browser/blueto...
content/browser/bluetooth/bluetooth_allowed_devices_map_unittest.cc:41: return
base::WrapUnique(new BluetoothUUID(kGlucoseUUID));
On 2016/05/28 at 04:38:06, Jeffrey Yasskin wrote:
> We have base::MakeUnique<BluetoothUUID>(kGlucoseUUID) now. Try to switch to
that everywhere.

No longer needed.

https://codereview.chromium.org/2015463004/diff/180001/content/browser/blueto...
content/browser/bluetooth/bluetooth_allowed_devices_map_unittest.cc:213:
scanFilter1Clone->services.push_back(GetGlucoseUUID());
On 2016/05/28 at 04:38:06, Jeffrey Yasskin wrote:
> The fact that we can't clone messages with unique_ptrs is also sad. Using
base::Optional would fix this, and I think WTF::Optional's definition means
it'll be fine to use that in blink too.

Done. FWIW once we Onion-Soup Web Bluetooth, blink will not pass a Optional or a
BluetoothUUID it will just pass a WTF::String.

https://codereview.chromium.org/2015463004/diff/180001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_blacklist_unittest.cc (right):

https://codereview.chromium.org/2015463004/diff/180001/content/browser/blueto...
content/browser/bluetooth/bluetooth_blacklist_unittest.cc:20: bool
OptionalServicesEqual(
On 2016/05/28 at 04:38:06, Jeffrey Yasskin wrote:
> Write this to return an AssertionResult, and then you can give a good error
message for mismatches:
https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuid...

Now that we are using Optional we can just use the Equals method of mojo::Array.

https://codereview.chromium.org/2015463004/diff/180001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_metrics.cc (right):

https://codereview.chromium.org/2015463004/diff/180001/content/browser/blueto...
content/browser/bluetooth/bluetooth_metrics.cc:31: // TODO(520284): Other than
verifying that uuid exists, this logic
On 2016/05/28 at 04:38:06, Jeffrey Yasskin wrote:
> If 'uuid' becomes a base::Optional, s/exists/contains a value/. Otherwise, "is
non-null".

Done.

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

https://codereview.chromium.org/2015463004/diff/180001/content/browser/blueto...
content/browser/bluetooth/web_bluetooth_service_impl.cc:428: if
(characteristics_uuid.get() &&
On 2016/05/28 at 04:38:06, Jeffrey Yasskin wrote:
> unique_ptr and base::Optional can be directly used in an && expression without
an explicit .get().

I think that .get() is more explicit, but I've been told to use remove the
.get() a bunch of times. So I'll try to do that from now on.

https://codereview.chromium.org/2015463004/diff/180001/content/renderer/bluet...
File content/renderer/bluetooth/bluetooth_type_converters.cc (right):

https://codereview.chromium.org/2015463004/diff/180001/content/renderer/bluet...
content/renderer/bluetooth/bluetooth_type_converters.cc:52: return
base::WrapUnique(new device::BluetoothUUID(web_string.utf8()));
On 2016/05/28 at 04:38:06, Jeffrey Yasskin wrote:
> Shouldn't the TypeConverter kill the connection if the UUID was malformed?

The type converter doesn't know anything about the mojo pipe. We could (D)CHECK
that the new UUID object is valid but it seems a bit redundant since we do
validation when receiving the message anyway.

https://codereview.chromium.org/2015463004/diff/180001/device/bluetooth/publi...
File device/bluetooth/public/interfaces/bluetooth_uuid.typemap (right):

https://codereview.chromium.org/2015463004/diff/180001/device/bluetooth/publi...
device/bluetooth/public/interfaces/bluetooth_uuid.typemap:11: type_mappings = [
"device.mojom.BluetoothUUID=std::unique_ptr<device::BluetoothUUID>" ]
On 2016/05/28 at 04:38:06, Jeffrey Yasskin wrote:
> See if this can be base::Optional instead of unique_ptr: it matches nullable
semantics better.

Done.

[commit-bot: I haz the power, 2016-05-31 17:47:51]

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

[commit-bot: I haz the power, 2016-05-31 17:47:52]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Jeffrey Yasskin, 2016-05-31 21:52:58]

LGTM

https://codereview.chromium.org/2015463004/diff/180001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_blacklist_unittest.cc (right):

https://codereview.chromium.org/2015463004/diff/180001/content/browser/blueto...
content/browser/bluetooth/bluetooth_blacklist_unittest.cc:20: bool
OptionalServicesEqual(
On 2016/05/31 17:30:47, ortuno wrote:
> On 2016/05/28 at 04:38:06, Jeffrey Yasskin wrote:
> > Write this to return an AssertionResult, and then you can give a good error
> message for mismatches:
>
https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuid...
> 
> Now that we are using Optional we can just use the Equals method of
mojo::Array.

SGTM

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

https://codereview.chromium.org/2015463004/diff/180001/content/browser/blueto...
content/browser/bluetooth/web_bluetooth_service_impl.cc:428: if
(characteristics_uuid.get() &&
On 2016/05/31 17:30:47, ortuno wrote:
> On 2016/05/28 at 04:38:06, Jeffrey Yasskin wrote:
> > unique_ptr and base::Optional can be directly used in an && expression
without
> an explicit .get().
> 
> I think that .get() is more explicit, but I've been told to use remove the
> .get() a bunch of times. So I'll try to do that from now on.

I'm totally happy with things like "!= nullptr" or "!= base::nullopt", which
express the type of the thing getting compared, but .get() doesn't state the
type so it seems less useful.

https://codereview.chromium.org/2015463004/diff/180001/content/renderer/bluet...
File content/renderer/bluetooth/bluetooth_type_converters.cc (right):

https://codereview.chromium.org/2015463004/diff/180001/content/renderer/bluet...
content/renderer/bluetooth/bluetooth_type_converters.cc:52: return
base::WrapUnique(new device::BluetoothUUID(web_string.utf8()));
On 2016/05/31 17:30:47, ortuno wrote:
> On 2016/05/28 at 04:38:06, Jeffrey Yasskin wrote:
> > Shouldn't the TypeConverter kill the connection if the UUID was malformed?
> 
> The type converter doesn't know anything about the mojo pipe. We could
(D)CHECK
> that the new UUID object is valid but it seems a bit redundant since we do
> validation when receiving the message anyway.

And I guess since this is the renderer side anyway, it makes sense to trust the
source.

Let's DCHECK redundantly, so that if we miss any validation we at least get
told.

https://codereview.chromium.org/2015463004/diff/220001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_allowed_devices_map_unittest.cc
(right):

https://codereview.chromium.org/2015463004/diff/220001/content/browser/blueto...
content/browser/bluetooth/bluetooth_allowed_devices_map_unittest.cc:44: return
base::make_optional(BluetoothUUID(kHeartRateUUID));
You can drop the BluetoothUUID() copy constructor calls now. Because
Optional<T>(const T&) is implicit, you can even drop the make_optional() calls,
but I'm happy keeping those.

Because GetHeartRateUUIDOpt() is just replacing
base::make_optional(kHeartRateUUID), would it make sense to use that call
directly at the call sites instead of having these wrapper functions? You can
even 'using' away the 'base::'. (That's not necessarily the right thing to do,
but it's worth considering.)

https://codereview.chromium.org/2015463004/diff/220001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_uuid.h (right):

https://codereview.chromium.org/2015463004/diff/220001/device/bluetooth/bluet...
device/bluetooth/bluetooth_uuid.h:103: struct hash<device::BluetoothUUID> {
Nice, thanks.

[ortuno, 2016-06-01 15:37:58]

I'll try to land this now to avoid new functions using strings rather than
uuids.

https://codereview.chromium.org/2015463004/diff/180001/content/renderer/bluet...
File content/renderer/bluetooth/bluetooth_type_converters.cc (right):

https://codereview.chromium.org/2015463004/diff/180001/content/renderer/bluet...
content/renderer/bluetooth/bluetooth_type_converters.cc:52: return
base::WrapUnique(new device::BluetoothUUID(web_string.utf8()));
On 2016/05/31 at 21:52:57, Jeffrey Yasskin wrote:
> On 2016/05/31 17:30:47, ortuno wrote:
> > On 2016/05/28 at 04:38:06, Jeffrey Yasskin wrote:
> > > Shouldn't the TypeConverter kill the connection if the UUID was malformed?
> > 
> > The type converter doesn't know anything about the mojo pipe. We could
(D)CHECK
> > that the new UUID object is valid but it seems a bit redundant since we do
> > validation when receiving the message anyway.
> 
> And I guess since this is the renderer side anyway, it makes sense to trust
the source.
> 
> Let's DCHECK redundantly, so that if we miss any validation we at least get
told.

Done.

https://codereview.chromium.org/2015463004/diff/220001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_allowed_devices_map_unittest.cc
(right):

https://codereview.chromium.org/2015463004/diff/220001/content/browser/blueto...
content/browser/bluetooth/bluetooth_allowed_devices_map_unittest.cc:44: return
base::make_optional(BluetoothUUID(kHeartRateUUID));
On 2016/05/31 at 21:52:57, Jeffrey Yasskin wrote:
> You can drop the BluetoothUUID() copy constructor calls now. Because
Optional<T>(const T&) is implicit, you can even drop the make_optional() calls,
but I'm happy keeping those.
> 
> Because GetHeartRateUUIDOpt() is just replacing
base::make_optional(kHeartRateUUID), would it make sense to use that call
directly at the call sites instead of having these wrapper functions? You can
even 'using' away the 'base::'. (That's not necessarily the right thing to do,
but it's worth considering.)

Done. I also removed base::make_optional since the only reason we use an
optional type there, is that mojo doesn't yet support two different types for
nullable and non-nullable.

[ortuno, 2016-06-01 15:43:38]

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

[ortuno, 2016-06-01 15:43:40]

tsepez: PTAL at third_party/WebKit/public/platform/modules/web_bluetooth.mojom
and device/bluetooth/public/interfaces/

[ortuno, 2016-06-01 15:48:08]

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

[commit-bot: I haz the power, 2016-06-01 15:48:41]

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

[yzshen1, 2016-06-01 16:12:58]

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

[yzshen1, 2016-06-01 16:13:00]

(No need to block on this comment, just a FYI)

https://codereview.chromium.org/2015463004/diff/260001/device/bluetooth/publi...
File device/bluetooth/public/interfaces/bluetooth_uuid_struct_traits.h (right):

https://codereview.chromium.org/2015463004/diff/260001/device/bluetooth/publi...
device/bluetooth/public/interfaces/bluetooth_uuid_struct_traits.h:26: static
base::StringPiece uuid(
nit: you could simply use static const std::string& uuid(...) { return
uuid->canonical_value(); }

(Actually you can use any type that has a StringTraits defined.)

Similarly, you can directly read a std::string from ReadUuid on line 36.

[Tom Sepez, 2016-06-01 17:52:52]

mojom LGTM

[ortuno, 2016-06-01 18:28:12]

Thanks!

https://codereview.chromium.org/2015463004/diff/260001/device/bluetooth/publi...
File device/bluetooth/public/interfaces/bluetooth_uuid_struct_traits.h (right):

https://codereview.chromium.org/2015463004/diff/260001/device/bluetooth/publi...
device/bluetooth/public/interfaces/bluetooth_uuid_struct_traits.h:26: static
base::StringPiece uuid(
On 2016/06/01 at 16:12:59, yzshen1 wrote:
> nit: you could simply use static const std::string& uuid(...) { return
uuid->canonical_value(); }
> 
> (Actually you can use any type that has a StringTraits defined.)
> 
> Similarly, you can directly read a std::string from ReadUuid on line 36.

Nice. Done.

[ortuno, 2016-06-01 18:29:04]

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

[ortuno, 2016-06-01 18:29:06]

esprehn: PTAL at third_party/WebKit/public/blink.gyp and
third_party/WebKit/public/BUILD.gn

[ortuno, 2016-06-01 18:29:11]

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

[commit-bot: I haz the power, 2016-06-01 18:29:29]

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

[ortuno, 2016-06-02 17:14:39]

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

[ortuno, 2016-06-02 17:14:40]

japhet: esprehn is OOO. PTAL at third_party/WebKit/public/blink.gyp and
third_party/WebKit/public/BUILD.gn

[ortuno, 2016-06-08 00:18:10]

esprehn or japhet: ping?

[Tom Sepez, 2016-06-08 16:49:44]

lgtm

[esprehn, 2016-06-08 20:34:44]

esprehn@chromium.org changed reviewers:
+ jbroman@chromium.org

[esprehn, 2016-06-08 20:34:45]

jbroman@ can you take this one?

[jbroman, 2016-06-08 21:08:08]

Sure, I can take a look.

The blink changes seem reasonable to me, though I have a few brief comments
about the rest.

(I don't have public/OWNERS, though, so you'll still need a stamp from esprehn
or equivalent.)

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

https://codereview.chromium.org/2015463004/diff/300001/content/browser/blueto...
content/browser/bluetooth/bluetooth_blacklist_unittest.cc:379:
mojo::Array<base::Optional<BluetoothUUID>>();
nit: Not added in this CL, but you could just default-construct here (and
elsewhere in this file):

mojo::Array<base::optional<BluetoothUUID>> expected;

https://codereview.chromium.org/2015463004/diff/300001/content/renderer/bluet...
File content/renderer/bluetooth/bluetooth_type_converters.h (right):

https://codereview.chromium.org/2015463004/diff/300001/content/renderer/bluet...
content/renderer/bluetooth/bluetooth_type_converters.h:35: template <>
Is this type converter used? It looks like web_bluetooth_impl.cc explicitly
makes a base::Optional<device::BluetoothUUID>.

https://codereview.chromium.org/2015463004/diff/300001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_uuid.h (right):

https://codereview.chromium.org/2015463004/diff/300001/device/bluetooth/bluet...
device/bluetooth/bluetooth_uuid.h:103: struct hash<device::BluetoothUUID> {
the style guide forbids specializing std::hash:

https://chromium-cpp.appspot.com/
https://google.github.io/styleguide/cppguide.html#std_hash

The recommended workaround at present is to instead write a custom hasher, and
pass that explicitly to std::unordered_* (verbose though it may be).

https://codereview.chromium.org/2015463004/diff/300001/device/bluetooth/publi...
File device/bluetooth/public/interfaces/bluetooth_uuid.typemap (right):

https://codereview.chromium.org/2015463004/diff/300001/device/bluetooth/publi...
device/bluetooth/public/interfaces/bluetooth_uuid.typemap:7:
"//device/bluetooth/bluetooth_uuid.h",
nit: sort these paths?

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

https://codereview.chromium.org/2015463004/diff/300001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom:1: //
Copyright 2016 The Chromium Authors. All rights reserved.
I assume the plan is ultimately for Blink to directly invoke this service?
(Currently a lot of this stuff seems to be proxied via content/renderer/ which
actually does the IPC.)

[ortuno, 2016-06-09 16:15:59]

Thanks!

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

https://codereview.chromium.org/2015463004/diff/300001/content/browser/blueto...
content/browser/bluetooth/bluetooth_blacklist_unittest.cc:379:
mojo::Array<base::Optional<BluetoothUUID>>();
On 2016/06/08 at 21:08:07, jbroman wrote:
> nit: Not added in this CL, but you could just default-construct here (and
elsewhere in this file):
> 
> mojo::Array<base::optional<BluetoothUUID>> expected;

Done.

https://codereview.chromium.org/2015463004/diff/300001/content/renderer/bluet...
File content/renderer/bluetooth/bluetooth_type_converters.h (right):

https://codereview.chromium.org/2015463004/diff/300001/content/renderer/bluet...
content/renderer/bluetooth/bluetooth_type_converters.h:35: template <>
On 2016/06/08 at 21:08:07, jbroman wrote:
> Is this type converter used? It looks like web_bluetooth_impl.cc explicitly
makes a base::Optional<device::BluetoothUUID>.

Yup, it's used in the previous type converter.

https://codereview.chromium.org/2015463004/diff/300001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_uuid.h (right):

https://codereview.chromium.org/2015463004/diff/300001/device/bluetooth/bluet...
device/bluetooth/bluetooth_uuid.h:103: struct hash<device::BluetoothUUID> {
On 2016/06/08 at 21:08:07, jbroman wrote:
> the style guide forbids specializing std::hash:
> 
> https://chromium-cpp.appspot.com/
> https://google.github.io/styleguide/cppguide.html#std_hash
> 
> The recommended workaround at present is to instead write a custom hasher, and
pass that explicitly to std::unordered_* (verbose though it may be).

Done. Huh, good to know, though, I wish there was an exception for very simple
hash functions like this one.

https://codereview.chromium.org/2015463004/diff/300001/device/bluetooth/publi...
File device/bluetooth/public/interfaces/bluetooth_uuid.typemap (right):

https://codereview.chromium.org/2015463004/diff/300001/device/bluetooth/publi...
device/bluetooth/public/interfaces/bluetooth_uuid.typemap:7:
"//device/bluetooth/bluetooth_uuid.h",
On 2016/06/08 at 21:08:07, jbroman wrote:
> nit: sort these paths?

Done.

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

https://codereview.chromium.org/2015463004/diff/300001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom:1: //
Copyright 2016 The Chromium Authors. All rights reserved.
On 2016/06/08 at 21:08:08, jbroman wrote:
> I assume the plan is ultimately for Blink to directly invoke this service?
(Currently a lot of this stuff seems to be proxied via content/renderer/ which
actually does the IPC.)

Yup. There is so much code that we decided to do it in two phases:

1. Use mojo for IPC. (Almost done. Some clean up, like this one, remains.)
2. Onion Soup

[dcheng, 2016-06-09 16:20:38]

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

[dcheng, 2016-06-09 16:20:40]

https://codereview.chromium.org/2015463004/diff/300001/content/renderer/bluet...
File content/renderer/bluetooth/bluetooth_type_converters.cc (right):

https://codereview.chromium.org/2015463004/diff/300001/content/renderer/bluet...
content/renderer/bluetooth/bluetooth_type_converters.cc:49:
base::Optional<device::BluetoothUUID>
We should avoid adding new type converters. Can you use a type map [1] here
instead?

[1] http://www.chromium.org/developers/design-documents/mojo/type-mapping

[ortuno, 2016-06-09 16:29:23]

https://codereview.chromium.org/2015463004/diff/300001/content/renderer/bluet...
File content/renderer/bluetooth/bluetooth_type_converters.cc (right):

https://codereview.chromium.org/2015463004/diff/300001/content/renderer/bluet...
content/renderer/bluetooth/bluetooth_type_converters.cc:49:
base::Optional<device::BluetoothUUID>
On 2016/06/09 at 16:20:40, dcheng wrote:
> We should avoid adding new type converters. Can you use a type map [1] here
instead?
> 
> [1] http://www.chromium.org/developers/design-documents/mojo/type-mapping

OOO: Why should we avoid new type converters?

This patch actually introduces a type map, but we are calling the functions from
renderer/ which doesn't use the blink variant mojo functions so we have to
convert from Web types to the type the functions expect.

Also, this is just a helper function so that we can call
mojo::Array<...>::From() and it will go away once we onion-soup Web Bluetooth.

[dcheng, 2016-06-09 18:08:48]

https://codereview.chromium.org/2015463004/diff/300001/content/renderer/bluet...
File content/renderer/bluetooth/bluetooth_type_converters.cc (right):

https://codereview.chromium.org/2015463004/diff/300001/content/renderer/bluet...
content/renderer/bluetooth/bluetooth_type_converters.cc:49:
base::Optional<device::BluetoothUUID>
On 2016/06/09 16:29:23, ortuno wrote:
> On 2016/06/09 at 16:20:40, dcheng wrote:
> > We should avoid adding new type converters. Can you use a type map [1] here
> instead?
> > 
> > [1] http://www.chromium.org/developers/design-documents/mojo/type-mapping
> 
> OOO: Why should we avoid new type converters?

Several reasons:
- Type converters are redundant now that we have StructTraits.
- Type converters can't signal failure. Type converters that receive malformed
input have to try to construct a reasonable default.
- Type converters are going to be removed down the road.

> 
> This patch actually introduces a type map, but we are calling the functions
from
> renderer/ which doesn't use the blink variant mojo functions so we have to
> convert from Web types to the type the functions expect.
> 

You can use one typemap for Blink variants and another for content. For example,
for URL:
https://cs.chromium.org/search/?q=file:typemap+URL&sq=package:chromium&type=cs

> Also, this is just a helper function so that we can call
> mojo::Array<...>::From() and it will go away once we onion-soup Web Bluetooth.

Line 55 assumes that the other end is always going to send a valid uuid. We
can't make that assumption in this code: we need to signal failure somehow.

[ortuno, 2016-06-09 19:54:50]

https://codereview.chromium.org/2015463004/diff/300001/content/renderer/bluet...
File content/renderer/bluetooth/bluetooth_type_converters.cc (right):

https://codereview.chromium.org/2015463004/diff/300001/content/renderer/bluet...
content/renderer/bluetooth/bluetooth_type_converters.cc:49:
base::Optional<device::BluetoothUUID>
On 2016/06/09 at 18:08:48, dcheng wrote:
> On 2016/06/09 16:29:23, ortuno wrote:
> > On 2016/06/09 at 16:20:40, dcheng wrote:
> > > We should avoid adding new type converters. Can you use a type map [1]
here
> > instead?
> > > 
> > > [1] http://www.chromium.org/developers/design-documents/mojo/type-mapping
> > 
> > OOO: Why should we avoid new type converters?
> 
> Several reasons:
> - Type converters are redundant now that we have StructTraits.
> - Type converters can't signal failure. Type converters that receive malformed
input have to try to construct a reasonable default.
> - Type converters are going to be removed down the road.
> 
> > 
> > This patch actually introduces a type map, but we are calling the functions
from
> > renderer/ which doesn't use the blink variant mojo functions so we have to
> > convert from Web types to the type the functions expect.
> > 
> 
> You can use one typemap for Blink variants and another for content. For
example, for URL:
https://cs.chromium.org/search/?q=file:typemap+URL&sq=package:chromium&type=cs
> 

Maybe I'm missing something but that doesn't apply here. We are in
content/renderer so we have to use the content/ variant. That's why we need to
convert from a WebString to an Optional<BluetoothUUID> which is what the content
variant mojo functions use.

> > Also, this is just a helper function so that we can call
> > mojo::Array<...>::From() and it will go away once we onion-soup Web
Bluetooth.
> 
> Line 55 assumes that the other end is always going to send a valid uuid. We
can't make that assumption in this code: we need to signal failure somehow.

I think that's a fair assumption. We are in the renderer process and it is
another class in the renderer that ends up calling this function, so we should
be able to trust it. We do have failure signal, that's the point of this patch
:) If we send an invalid UUID, Mojo will close the pipe when validating the
message. See: device/bluetooth/public/interfaces/bluetooth_uuid_struct_traits.h

Just to be clear, we are not using these type converters to convert any types
that we get from mojo callbacks into blink types. It's actually the opposite. We
are converting blink types into content types that we can send over mojo. I
realize that that's what typemaps are for but again we are not in blink so we
can't use the blink variant. We have to manually convert from a Web type to a
content type. So we have two options: we can take advantage of the Type
Converter interface like this patch does or we can write our own helper
functions, with for loops and everything. I'm OK with doing the latter but it
seems the result is the same either way.

[ortuno, 2016-06-09 20:22:32]

Description was changed from

==========
bluetooth: Use BluetoothUUID instead of string when sending uuids

This patch introduces a Typemap from mojo::String to
unique_ptr<device::BluetoothUUID>.

Mapping to unique_ptr allows us to have a nullable type.

BUG=606446
==========

to

==========
bluetooth: Use BluetoothUUID instead of string when sending uuids

This patch introduces a Typemap from mojo::String to
unique_ptr<device::BluetoothUUID>.

Mapping to base::Optional allows us to have a nullable type.

BUG=606446
==========

[dcheng, 2016-06-09 21:07:49]

On 2016/06/09 19:54:50, ortuno wrote:
>
https://codereview.chromium.org/2015463004/diff/300001/content/renderer/bluet...
> File content/renderer/bluetooth/bluetooth_type_converters.cc (right):
> 
>
https://codereview.chromium.org/2015463004/diff/300001/content/renderer/bluet...
> content/renderer/bluetooth/bluetooth_type_converters.cc:49:
> base::Optional<device::BluetoothUUID>
> On 2016/06/09 at 18:08:48, dcheng wrote:
> > On 2016/06/09 16:29:23, ortuno wrote:
> > > On 2016/06/09 at 16:20:40, dcheng wrote:
> > > > We should avoid adding new type converters. Can you use a type map [1]
> here
> > > instead?
> > > > 
> > > > [1]
http://www.chromium.org/developers/design-documents/mojo/type-mapping
> > > 
> > > OOO: Why should we avoid new type converters?
> > 
> > Several reasons:
> > - Type converters are redundant now that we have StructTraits.
> > - Type converters can't signal failure. Type converters that receive
malformed
> input have to try to construct a reasonable default.
> > - Type converters are going to be removed down the road.
> > 
> > > 
> > > This patch actually introduces a type map, but we are calling the
functions
> from
> > > renderer/ which doesn't use the blink variant mojo functions so we have to
> > > convert from Web types to the type the functions expect.
> > > 
> > 
> > You can use one typemap for Blink variants and another for content. For
> example, for URL:
> https://cs.chromium.org/search/?q=file:typemap+URL&sq=package:chromium&type=cs
> > 
> 
> Maybe I'm missing something but that doesn't apply here. We are in
> content/renderer so we have to use the content/ variant. That's why we need to
> convert from a WebString to an Optional<BluetoothUUID> which is what the
content
> variant mojo functions use.
> 
> > > Also, this is just a helper function so that we can call
> > > mojo::Array<...>::From() and it will go away once we onion-soup Web
> Bluetooth.
> > 
> > Line 55 assumes that the other end is always going to send a valid uuid. We
> can't make that assumption in this code: we need to signal failure somehow.
> 
> I think that's a fair assumption. We are in the renderer process and it is
> another class in the renderer that ends up calling this function, so we should
> be able to trust it. We do have failure signal, that's the point of this patch
> :) If we send an invalid UUID, Mojo will close the pipe when validating the
> message. See:
device/bluetooth/public/interfaces/bluetooth_uuid_struct_traits.h
> 
> Just to be clear, we are not using these type converters to convert any types
> that we get from mojo callbacks into blink types. It's actually the opposite.
We
> are converting blink types into content types that we can send over mojo. I
> realize that that's what typemaps are for but again we are not in blink so we
> can't use the blink variant. We have to manually convert from a Web type to a
> content type. So we have two options: we can take advantage of the Type
> Converter interface like this patch does or we can write our own helper
> functions, with for loops and everything. I'm OK with doing the latter but it
> seems the result is the same either way.

Hmmm...
Given the limited scope of this type converter (and the fact that it's scoped to
renderer-only code), I guess it's OK. Thanks for explaining.

[ortuno, 2016-06-10 14:41:05]

jbroman: ping?

[jbroman, 2016-06-10 14:47:48]

lgtm

[ortuno, 2016-06-10 14:49:27]

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

[commit-bot: I haz the power, 2016-06-10 14:49:41]

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

[ortuno, 2016-06-10 14:50:59]

jbroman: Thanks!
esprehn: I still need owners review for third_party/WebKit/public/

[commit-bot: I haz the power, 2016-06-10 14:57:35]

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

[commit-bot: I haz the power, 2016-06-10 14:57:37]

Dry run: Try jobs failed on following builders:
  ios-device on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[esprehn, 2016-06-10 15:19:56]

lgtm

[ortuno, 2016-06-10 15:24:06]

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

[commit-bot: I haz the power, 2016-06-10 15:24:23]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2015463004/340001

[ortuno, 2016-06-10 18:08:40]

The CQ bit was unchecked by ortuno@chromium.org

[ortuno, 2016-06-10 18:08:52]

The CQ bit was checked by ortuno@chromium.org

[ortuno, 2016-06-10 18:08:52]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
jyasskin@chromium.org, tsepez@chromium.org, esprehn@chromium.org,
jbroman@chromium.org
Link to the patchset: https://codereview.chromium.org/2015463004/#ps340001
(title: "Only bulid mojo targets on non-iOS")

[commit-bot: I haz the power, 2016-06-10 18:09:34]

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

[commit-bot: I haz the power, 2016-06-10 18:16:53]

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

[commit-bot: I haz the power, 2016-06-10 18:16:55]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[ortuno, 2016-06-10 18:19:38]

The CQ bit was checked by ortuno@chromium.org

[ortuno, 2016-06-10 18:19:39]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
esprehn@chromium.org, jbroman@chromium.org, tsepez@chromium.org,
jyasskin@chromium.org
Link to the patchset: https://codereview.chromium.org/2015463004/#ps360001
(title: "Fix merge conflict")

[commit-bot: I haz the power, 2016-06-10 18:20:26]

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

[commit-bot: I haz the power, 2016-06-10 20:15:21]

Committed patchset #19 (id:360001)

[commit-bot: I haz the power, 2016-06-10 20:15:31]

CQ bit was unchecked

[commit-bot: I haz the power, 2016-06-10 20:16:27]

Description was changed from

==========
bluetooth: Use BluetoothUUID instead of string when sending uuids

This patch introduces a Typemap from mojo::String to
unique_ptr<device::BluetoothUUID>.

Mapping to base::Optional allows us to have a nullable type.

BUG=606446
==========

to

==========
bluetooth: Use BluetoothUUID instead of string when sending uuids

This patch introduces a Typemap from mojo::String to
unique_ptr<device::BluetoothUUID>.

Mapping to base::Optional allows us to have a nullable type.

BUG=606446

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

[commit-bot: I haz the power, 2016-06-10 20:16:29]

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

[krasin, 2016-06-11 23:11:00]

FYI: this CL has broken 'CFI Linux' buildbot:
https://build.chromium.org/p/chromium.fyi/builders/CFI%20Linux/builds/5690

To reproduce it locally (requires a Linux-x86_64 machine with at least 64 GB
RAM):

$ gn gen out/gn-cfi '--args=is_cfi=true is_debug=false is_component_build=false
symbol_level=1 dcheck_always_on=true' --check
$ build/download_gold_plugin.py
$ ninja -C out/gn-cfi content_unittests
$ ./out/gn-cfi/content_unittests
--gtest_filter=BluetoothBlacklistTest.RemoveExcludedUuids_Matching

In order to get more diagnostics, use_cfi_diag=true might be used. See
https://www.chromium.org/developers/testing/control-flow-integrity for more
details.

[Jeffrey Yasskin, 2016-06-12 00:06:18]

Ivan, please have the bot include line numbers in its stack traces. Without
those, it takes much more work to diagnose errors.

On Sat, Jun 11, 2016 at 4:11 PM, <krasin@google.com> wrote:

> FYI: this CL has broken 'CFI Linux' buildbot:
> https://build.chromium.org/p/chromium.fyi/builders/CFI%20Linux/builds/5690
>
> To reproduce it locally (requires a Linux-x86_64 machine with at least 64
> GB
> RAM):
>
> $ gn gen out/gn-cfi '--args=is_cfi=true is_debug=false
> is_component_build=false
> symbol_level=1 dcheck_always_on=true' --check
> $ build/download_gold_plugin.py
> $ ninja -C out/gn-cfi content_unittests
> $ ./out/gn-cfi/content_unittests
> --gtest_filter=BluetoothBlacklistTest.RemoveExcludedUuids_Matching
>
> In order to get more diagnostics, use_cfi_diag=true might be used. See
> https://www.chromium.org/developers/testing/control-flow-integrity for
> more
> details.
>
>
> https://codereview.chromium.org/2015463004/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[Jeffrey Yasskin, 2016-06-12 00:06:18]

Ivan, please have the bot include line numbers in its stack traces. Without
those, it takes much more work to diagnose errors.

On Sat, Jun 11, 2016 at 4:11 PM, <krasin@google.com> wrote:

> FYI: this CL has broken 'CFI Linux' buildbot:
> https://build.chromium.org/p/chromium.fyi/builders/CFI%20Linux/builds/5690
>
> To reproduce it locally (requires a Linux-x86_64 machine with at least 64
> GB
> RAM):
>
> $ gn gen out/gn-cfi '--args=is_cfi=true is_debug=false
> is_component_build=false
> symbol_level=1 dcheck_always_on=true' --check
> $ build/download_gold_plugin.py
> $ ninja -C out/gn-cfi content_unittests
> $ ./out/gn-cfi/content_unittests
> --gtest_filter=BluetoothBlacklistTest.RemoveExcludedUuids_Matching
>
> In order to get more diagnostics, use_cfi_diag=true might be used. See
> https://www.chromium.org/developers/testing/control-flow-integrity for
> more
> details.
>
>
> https://codereview.chromium.org/2015463004/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[krasin, 2016-06-12 00:16:12]

I agree. Filed https://crbug.com/619350 for ensuring we have line numbers on
those bots.

I will also file a bug for this failure and will include a proper stack trace
into it.

[krasin, 2016-06-12 00:22:21]

Filed the bug to track the current issue with CFI + Bluetooth:
https://crbug.com/619351