bluetooth: Use BluetoothUUID instead of string when sending uuids

content/renderer/bluetooth/bluetooth_type_converters.cc

Index: content/renderer/bluetooth/bluetooth_type_converters.cc
diff --git a/content/renderer/bluetooth/bluetooth_type_converters.cc b/content/renderer/bluetooth/bluetooth_type_converters.cc
index 863e2ef859d6eb10ca430e5e8cc3580b40a83317..c7bf1a7948979c4a696ddd358ccc2879c247d3ac 100644
--- a/content/renderer/bluetooth/bluetooth_type_converters.cc
+++ b/content/renderer/bluetooth/bluetooth_type_converters.cc
@@ -16,8 +16,10 @@ blink::mojom::WebBluetoothScanFilterPtr TypeConverter<
                                                 web_filter) {
   blink::mojom::WebBluetoothScanFilterPtr filter =
       blink::mojom::WebBluetoothScanFilter::New();
+
   if (!web_filter.services.isEmpty())
-    filter->services = Array<String>::From(web_filter.services);
+    filter->services =
+        Array<base::Optional<device::BluetoothUUID>>::From(web_filter.services);
 
   if (web_filter.hasName)
     filter->name = String::From(web_filter.name);
@@ -27,6 +29,7 @@ blink::mojom::WebBluetoothScanFilterPtr TypeConverter<
   return filter;
 }
 
+// static
 blink::mojom::WebBluetoothRequestDeviceOptionsPtr
 TypeConverter<blink::mojom::WebBluetoothRequestDeviceOptionsPtr,
               blink::WebRequestDeviceOptions>::
@@ -37,8 +40,21 @@ TypeConverter<blink::mojom::WebBluetoothRequestDeviceOptionsPtr,
   options->filters = mojo::Array<blink::mojom::WebBluetoothScanFilterPtr>::From(
       web_options.filters);
   options->optional_services =
-      mojo::Array<mojo::String>::From(web_options.optionalServices);
+      mojo::Array<base::Optional<device::BluetoothUUID>>::From(
+          web_options.optionalServices);
   return options;
 }
 
+// static
+base::Optional<device::BluetoothUUID>

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

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]

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]

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.
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
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]

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.
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.

+TypeConverter<base::Optional<device::BluetoothUUID>, blink::WebString>::Convert(
+    const blink::WebString& web_string) {
+  base::Optional<device::BluetoothUUID> uuid =
+      device::BluetoothUUID(web_string.utf8());
+
+  DCHECK(uuid->IsValid());
+
+  return uuid;
+}
+
 }  // namespace mojo