Use new wrapper types for web_bluetooth.mojom

Use new wrapper types for web_bluetooth.mojom

This CL modified code to use new wrapper types for web_bluetooth.mojom.
Also changed all instances of mojo::Array, mojo::String to std::vector,
std::string since mojo::Array, mojo::String have been deprecated.

BUG=597053
Committed: https://crrev.com/1ef7dd4b548d9ccd5e573bf053043ab79070ae08
Cr-Commit-Position: refs/heads/master@{#434892}

[juncai, 2016-11-16 02:38:17]

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

[commit-bot: I haz the power, 2016-11-16 02:39:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-16 02:56:29]

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

[commit-bot: I haz the power, 2016-11-16 02:56:30]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[juncai, 2016-11-16 18:32:20]

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

[commit-bot: I haz the power, 2016-11-16 18:33:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-16 18:46:24]

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

[commit-bot: I haz the power, 2016-11-16 18:46:25]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[juncai, 2016-11-16 23:47:09]

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

[commit-bot: I haz the power, 2016-11-16 23:48:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-17 00:43:30]

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

[commit-bot: I haz the power, 2016-11-17 00:43:31]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[juncai, 2016-11-17 22:58:09]

Description was changed from

==========
Use new wrapper types for web bluetooth mojom

This CL uses new wrapper types for web bluetooth mojom.

BUG=597053
==========

to

==========
Use new wrapper types for web_bluetooth.mojom

This CL modified code to use new wrapper types for web_bluetooth.mojom. 
Also changed all instances of mojo::Array, mojo::String to std::vector,
std::string since mojo::Array, mojo::String have been deprecated

BUG=597053
==========

[juncai, 2016-11-17 23:07:44]

Description was changed from

==========
Use new wrapper types for web_bluetooth.mojom

This CL modified code to use new wrapper types for web_bluetooth.mojom. 
Also changed all instances of mojo::Array, mojo::String to std::vector,
std::string since mojo::Array, mojo::String have been deprecated

BUG=597053
==========

to

==========
Use new wrapper types for web_bluetooth.mojom

This CL modified code to use new wrapper types for web_bluetooth.mojom. 
Also changed all instances of mojo::Array, mojo::String to std::vector,
std::string since mojo::Array, mojo::String have been deprecated

BUG=597053
==========

[juncai, 2016-11-17 23:07:57]

Description was changed from

==========
Use new wrapper types for web_bluetooth.mojom

This CL modified code to use new wrapper types for web_bluetooth.mojom. 
Also changed all instances of mojo::Array, mojo::String to std::vector,
std::string since mojo::Array, mojo::String have been deprecated

BUG=597053
==========

to

==========
Use new wrapper types for web_bluetooth.mojom

This CL modified code to use new wrapper types for web_bluetooth.mojom. 
Also changed all instances of mojo::Array, mojo::String to std::vector,
std::string since mojo::Array, mojo::String have been deprecated.

BUG=597053
==========

[juncai, 2016-11-18 22:41:54]

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

[commit-bot: I haz the power, 2016-11-18 22:43:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-18 23:15:48]

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

[commit-bot: I haz the power, 2016-11-18 23:15:48]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[juncai, 2016-11-18 23:28:37]

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

[commit-bot: I haz the power, 2016-11-18 23:29:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[juncai, 2016-11-19 00:01:04]

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

[commit-bot: I haz the power, 2016-11-19 00:02:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[juncai, 2016-11-19 01:16:30]

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

[juncai, 2016-11-19 01:16:31]

Please take a look.

[commit-bot: I haz the power, 2016-11-19 02:05:55]

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

[commit-bot: I haz the power, 2016-11-19 02:05:56]

Dry run: This issue passed the CQ dry run.

[ortuno, 2016-11-21 04:42:32]

Thanks for doing this! Looks pretty good, just a couple of small things.

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

https://codereview.chromium.org/2506813003/diff/100001/content/browser/blueto...
content/browser/bluetooth/bluetooth_allowed_devices_map.cc:179: if
(filter->services) {
All this nesting makes the code less readable. I would change it to:

for (const auto& filter : options->filters) {
  if (!filter->services) {
    continue;
  }
  for (const BluetoothUUID& uuid : filter->services.value()) {
    unionOfServices->insert(uuid);
  }
}

Also is there any test in bluetooth_allowed_devices_map_unittest.cc that tests
this?

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

https://codereview.chromium.org/2506813003/diff/100001/content/browser/blueto...
content/browser/bluetooth/bluetooth_allowed_devices_map_unittest.cc:215:
scan_filter1->services = std::vector<BluetoothUUID>();
nit: move this to where services is modified.

https://codereview.chromium.org/2506813003/diff/100001/content/browser/blueto...
content/browser/bluetooth/bluetooth_allowed_devices_map_unittest.cc:298:
scan_filter1->services = std::vector<BluetoothUUID>();
nit: Group this with the line that modifies services. Same for other instances
in the file.

https://codereview.chromium.org/2506813003/diff/100001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_blacklist.cc (right):

https://codereview.chromium.org/2506813003/diff/100001/content/browser/blueto...
content/browser/bluetooth/bluetooth_blacklist.cc:91: if (filter->services) {
Similarly here:

if (!filter->services) {
  continue;
}
...

And make sure there is a test that includes a filter with no services.

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

https://codereview.chromium.org/2506813003/diff/100001/content/browser/blueto...
content/browser/bluetooth/bluetooth_blacklist_unittest.cc:16: BluetoothUUID
Canonicalize(const std::string& str) {
I think we can get rid of this now. Canonicalize is no less typing than
BluetoothUUID

https://codereview.chromium.org/2506813003/diff/100001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_device_chooser_controller.cc (right):

https://codereview.chromium.org/2506813003/diff/100001/content/browser/blueto...
content/browser/bluetooth/bluetooth_device_chooser_controller.cc:155: if
(filter->services) {
if (!filter->services) {
  continue;
}
...

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

https://codereview.chromium.org/2506813003/diff/100001/content/browser/blueto...
content/browser/bluetooth/bluetooth_metrics.cc:70: if (filter->services) {
if (!filter->services) {
  continue;
}
...

https://codereview.chromium.org/2506813003/diff/100001/content/browser/blueto...
content/browser/bluetooth/bluetooth_metrics.cc:104: if (filter->services) {
if (!filter->services) {
  continue;
}
...

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

https://codereview.chromium.org/2506813003/diff/100001/content/browser/blueto...
content/browser/bluetooth/web_bluetooth_service_impl.cc:401: const
base::Optional<device::BluetoothUUID>& services_uuid,
No need for 'device::'

https://codereview.chromium.org/2506813003/diff/100001/content/browser/blueto...
content/browser/bluetooth/web_bluetooth_service_impl.cc:458: const
base::Optional<device::BluetoothUUID>& characteristics_uuid,
No need for "device::"

https://codereview.chromium.org/2506813003/diff/100001/content/browser/blueto...
content/browser/bluetooth/web_bluetooth_service_impl.cc:719: const
base::Optional<device::BluetoothUUID>& services_uuid,
No need for "device::".

https://codereview.chromium.org/2506813003/diff/100001/content/browser/blueto...
content/browser/bluetooth/web_bluetooth_service_impl.cc:797: if
(device->GetName())
GetName is optional so just:

device_ptr->name = device->GetName();

https://codereview.chromium.org/2506813003/diff/100001/content/common/bluetoo...
File content/common/bluetooth/web_bluetooth_device_id.h (left):

https://codereview.chromium.org/2506813003/diff/100001/content/common/bluetoo...
content/common/bluetooth/web_bluetooth_device_id.h:46: // This is required by
gtest to print a readable output on test failures.
Why move this? Without this tests won't be able to print the device id.

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

https://codereview.chromium.org/2506813003/diff/100001/content/renderer/bluet...
content/renderer/bluetooth/bluetooth_type_converters.cc:22: for (const auto
service : web_filter.services) {
Make it a reference.

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

https://codereview.chromium.org/2506813003/diff/100001/content/renderer/bluet...
content/renderer/bluetooth/web_bluetooth_impl.cc:230: if (services) {
Just DCHECK. If result == SUCCESS then we are guaranteed that there are
services.

https://codereview.chromium.org/2506813003/diff/100001/content/renderer/bluet...
content/renderer/bluetooth/web_bluetooth_impl.cc:258: if (characteristics) {
Same here. Just DCHECK.

https://codereview.chromium.org/2506813003/diff/100001/content/renderer/bluet...
content/renderer/bluetooth/web_bluetooth_impl.cc:283: callbacks->onSuccess(value
? value.value() : std::vector<uint8_t>());
And here as well. Just DCHECK. Also does it make sense to move the vector?

https://codereview.chromium.org/2506813003/diff/100001/device/bluetooth/publi...
File device/bluetooth/public/interfaces/uuid.typemap (right):

https://codereview.chromium.org/2506813003/diff/100001/device/bluetooth/publi...
device/bluetooth/public/interfaces/uuid.typemap:12: type_mappings = [
"bluetooth.mojom.UUID=device::BluetoothUUID" ]
Do we still need //base now that we don't need optional?

[juncai, 2016-11-21 20:40:54]

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

[commit-bot: I haz the power, 2016-11-21 20:41:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[juncai, 2016-11-21 21:27:05]

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

https://codereview.chromium.org/2506813003/diff/100001/content/browser/blueto...
content/browser/bluetooth/bluetooth_allowed_devices_map.cc:179: if
(filter->services) {
On 2016/11/21 04:42:31, ortuno wrote:
> All this nesting makes the code less readable. I would change it to:
> 
> for (const auto& filter : options->filters) {
>   if (!filter->services) {
>     continue;
>   }
>   for (const BluetoothUUID& uuid : filter->services.value()) {
>     unionOfServices->insert(uuid);
>   }
> }
> 
> Also is there any test in bluetooth_allowed_devices_map_unittest.cc that tests
> this?

Added test for this.

Done.

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

https://codereview.chromium.org/2506813003/diff/100001/content/browser/blueto...
content/browser/bluetooth/bluetooth_allowed_devices_map_unittest.cc:215:
scan_filter1->services = std::vector<BluetoothUUID>();
On 2016/11/21 04:42:31, ortuno wrote:
> nit: move this to where services is modified.

Done.

https://codereview.chromium.org/2506813003/diff/100001/content/browser/blueto...
content/browser/bluetooth/bluetooth_allowed_devices_map_unittest.cc:298:
scan_filter1->services = std::vector<BluetoothUUID>();
On 2016/11/21 04:42:31, ortuno wrote:
> nit: Group this with the line that modifies services. Same for other instances
> in the file.

Done.

https://codereview.chromium.org/2506813003/diff/100001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_blacklist.cc (right):

https://codereview.chromium.org/2506813003/diff/100001/content/browser/blueto...
content/browser/bluetooth/bluetooth_blacklist.cc:91: if (filter->services) {
On 2016/11/21 04:42:31, ortuno wrote:
> Similarly here:
> 
> if (!filter->services) {
>   continue;
> }
> ...
> 
> And make sure there is a test that includes a filter with no services.

Test added for this.

Done.

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

https://codereview.chromium.org/2506813003/diff/100001/content/browser/blueto...
content/browser/bluetooth/bluetooth_blacklist_unittest.cc:16: BluetoothUUID
Canonicalize(const std::string& str) {
On 2016/11/21 04:42:31, ortuno wrote:
> I think we can get rid of this now. Canonicalize is no less typing than
> BluetoothUUID

Done.

https://codereview.chromium.org/2506813003/diff/100001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_device_chooser_controller.cc (right):

https://codereview.chromium.org/2506813003/diff/100001/content/browser/blueto...
content/browser/bluetooth/bluetooth_device_chooser_controller.cc:155: if
(filter->services) {
On 2016/11/21 04:42:31, ortuno wrote:
> if (!filter->services) {
>   continue;
> }
> ...

Done.

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

https://codereview.chromium.org/2506813003/diff/100001/content/browser/blueto...
content/browser/bluetooth/bluetooth_metrics.cc:70: if (filter->services) {
On 2016/11/21 04:42:31, ortuno wrote:
> if (!filter->services) {
>   continue;
> }
> ...

Done.

https://codereview.chromium.org/2506813003/diff/100001/content/browser/blueto...
content/browser/bluetooth/bluetooth_metrics.cc:104: if (filter->services) {
On 2016/11/21 04:42:31, ortuno wrote:
> if (!filter->services) {
>   continue;
> }
> ...

Done.

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

https://codereview.chromium.org/2506813003/diff/100001/content/browser/blueto...
content/browser/bluetooth/web_bluetooth_service_impl.cc:401: const
base::Optional<device::BluetoothUUID>& services_uuid,
On 2016/11/21 04:42:31, ortuno wrote:
> No need for 'device::'

Done.

https://codereview.chromium.org/2506813003/diff/100001/content/browser/blueto...
content/browser/bluetooth/web_bluetooth_service_impl.cc:458: const
base::Optional<device::BluetoothUUID>& characteristics_uuid,
On 2016/11/21 04:42:31, ortuno wrote:
> No need for "device::"

Done.

https://codereview.chromium.org/2506813003/diff/100001/content/browser/blueto...
content/browser/bluetooth/web_bluetooth_service_impl.cc:719: const
base::Optional<device::BluetoothUUID>& services_uuid,
On 2016/11/21 04:42:31, ortuno wrote:
> No need for "device::".

Done.

https://codereview.chromium.org/2506813003/diff/100001/content/browser/blueto...
content/browser/bluetooth/web_bluetooth_service_impl.cc:797: if
(device->GetName())
On 2016/11/21 04:42:31, ortuno wrote:
> GetName is optional so just:
> 
> device_ptr->name = device->GetName();

Done.

https://codereview.chromium.org/2506813003/diff/100001/content/common/bluetoo...
File content/common/bluetooth/web_bluetooth_device_id.h (left):

https://codereview.chromium.org/2506813003/diff/100001/content/common/bluetoo...
content/common/bluetooth/web_bluetooth_device_id.h:46: // This is required by
gtest to print a readable output on test failures.
On 2016/11/21 04:42:31, ortuno wrote:
> Why move this? Without this tests won't be able to print the device id.

It seems that this operator<< is only used by:
https://cs.chromium.org/chromium/src/content/browser/bluetooth/bluetooth_allo...

And if not moved to bluetooth_allowed_devices_map.cc, the following will
generate compile error:
https://cs.chromium.org/chromium/src/content/renderer/presentation/presentati...
https://cs.chromium.org/chromium/src/content/renderer/presentation/presentati...

So I moved it to bluetooth_allowed_devices_map.cc.

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

https://codereview.chromium.org/2506813003/diff/100001/content/renderer/bluet...
content/renderer/bluetooth/bluetooth_type_converters.cc:22: for (const auto
service : web_filter.services) {
On 2016/11/21 04:42:31, ortuno wrote:
> Make it a reference.

Done.

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

https://codereview.chromium.org/2506813003/diff/100001/content/renderer/bluet...
content/renderer/bluetooth/web_bluetooth_impl.cc:230: if (services) {
On 2016/11/21 04:42:31, ortuno wrote:
> Just DCHECK. If result == SUCCESS then we are guaranteed that there are
> services.

Done.

https://codereview.chromium.org/2506813003/diff/100001/content/renderer/bluet...
content/renderer/bluetooth/web_bluetooth_impl.cc:258: if (characteristics) {
On 2016/11/21 04:42:31, ortuno wrote:
> Same here. Just DCHECK.

Done.

https://codereview.chromium.org/2506813003/diff/100001/content/renderer/bluet...
content/renderer/bluetooth/web_bluetooth_impl.cc:283: callbacks->onSuccess(value
? value.value() : std::vector<uint8_t>());
On 2016/11/21 04:42:31, ortuno wrote:
> And here as well. Just DCHECK. Also does it make sense to move the vector?

Done.

https://codereview.chromium.org/2506813003/diff/100001/device/bluetooth/publi...
File device/bluetooth/public/interfaces/uuid.typemap (right):

https://codereview.chromium.org/2506813003/diff/100001/device/bluetooth/publi...
device/bluetooth/public/interfaces/uuid.typemap:12: type_mappings = [
"bluetooth.mojom.UUID=device::BluetoothUUID" ]
On 2016/11/21 04:42:32, ortuno wrote:
> Do we still need //base now that we don't need optional?

Done.

[commit-bot: I haz the power, 2016-11-21 22:41:20]

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

[commit-bot: I haz the power, 2016-11-21 22:41:22]

Dry run: This issue passed the CQ dry run.

[ortuno, 2016-11-22 02:32:21]

lgtm bar the stream operator. If we can't figure out why it's causing problems
then I would prefer if we just added static_cast<GURL>() to
presentation_dispatcher.cc instead of removing our stream operator.

https://codereview.chromium.org/2506813003/diff/120001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_blacklist.cc (right):

https://codereview.chromium.org/2506813003/diff/120001/content/browser/blueto...
content/browser/bluetooth/bluetooth_blacklist.cc:94: if (IsExcluded(service))
nit: In general, I would just keep the style (in this case brackets) if I don't
own the file since owners might prefer brackets. Most of Bluetooth code has
brackets so I would just leave them.

https://codereview.chromium.org/2506813003/diff/120001/content/browser/blueto...
File content/browser/bluetooth/web_bluetooth_service_impl.cc (left):

https://codereview.chromium.org/2506813003/diff/120001/content/browser/blueto...
content/browser/bluetooth/web_bluetooth_service_impl.cc:145: if
(characteristic->GetUUID() == characteristic_uuid) {
nit: Please leave these. Owners (me) prefer to have brackets :)

[juncai, 2016-11-23 02:54:38]

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

[commit-bot: I haz the power, 2016-11-23 02:55:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-23 02:57:57]

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

[commit-bot: I haz the power, 2016-11-23 02:57:59]

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

[juncai, 2016-11-23 02:59:38]

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

[commit-bot: I haz the power, 2016-11-23 03:00:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-23 03:03:00]

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

[commit-bot: I haz the power, 2016-11-23 03:03:01]

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

[juncai, 2016-11-23 20:11:44]

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

[commit-bot: I haz the power, 2016-11-23 20:12:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-23 20:15:09]

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

[commit-bot: I haz the power, 2016-11-23 20:15:11]

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

[juncai, 2016-11-23 20:45:29]

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

[commit-bot: I haz the power, 2016-11-23 20:46:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[juncai, 2016-11-23 20:54:22]

juncai@chromium.org changed reviewers:
+ avi@chromium.org, dcheng@chromium.org, tedchoc@chromium.org

[juncai, 2016-11-23 20:54:23]

dcheng@chromium.org: Please review changes in 
//third_party/WebKit/public/BUILD.gn

tedchoc@chromium.org: Please review changes in 
//chrome/android/BUILD.gn
//content/public/android/BUILD.gn

avi@chromium.org: Please review changes in 
//content/common/BUILD.gn

https://codereview.chromium.org/2506813003/diff/120001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_blacklist.cc (right):

https://codereview.chromium.org/2506813003/diff/120001/content/browser/blueto...
content/browser/bluetooth/bluetooth_blacklist.cc:94: if (IsExcluded(service))
On 2016/11/22 02:32:20, ortuno wrote:
> nit: In general, I would just keep the style (in this case brackets) if I
don't
> own the file since owners might prefer brackets. Most of Bluetooth code has
> brackets so I would just leave them.

Done.

https://codereview.chromium.org/2506813003/diff/120001/content/browser/blueto...
File content/browser/bluetooth/web_bluetooth_service_impl.cc (left):

https://codereview.chromium.org/2506813003/diff/120001/content/browser/blueto...
content/browser/bluetooth/web_bluetooth_service_impl.cc:145: if
(characteristic->GetUUID() == characteristic_uuid) {
On 2016/11/22 02:32:21, ortuno wrote:
> nit: Please leave these. Owners (me) prefer to have brackets :)

Done.

[Ted C, 2016-11-23 21:10:11]

On 2016/11/23 20:54:23, juncai wrote:
> mailto:dcheng@chromium.org: Please review changes in 
> //third_party/WebKit/public/BUILD.gn
> 
> mailto:tedchoc@chromium.org: Please review changes in 
> //chrome/android/BUILD.gn
> //content/public/android/BUILD.gn
> 
> mailto:avi@chromium.org: Please review changes in 
> //content/common/BUILD.gn
> 
>
https://codereview.chromium.org/2506813003/diff/120001/content/browser/blueto...
> File content/browser/bluetooth/bluetooth_blacklist.cc (right):
> 
>
https://codereview.chromium.org/2506813003/diff/120001/content/browser/blueto...
> content/browser/bluetooth/bluetooth_blacklist.cc:94: if (IsExcluded(service))
> On 2016/11/22 02:32:20, ortuno wrote:
> > nit: In general, I would just keep the style (in this case brackets) if I
> don't
> > own the file since owners might prefer brackets. Most of Bluetooth code has
> > brackets so I would just leave them.
> 
> Done.
> 
>
https://codereview.chromium.org/2506813003/diff/120001/content/browser/blueto...
> File content/browser/bluetooth/web_bluetooth_service_impl.cc (left):
> 
>
https://codereview.chromium.org/2506813003/diff/120001/content/browser/blueto...
> content/browser/bluetooth/web_bluetooth_service_impl.cc:145: if
> (characteristic->GetUUID() == characteristic_uuid) {
> On 2016/11/22 02:32:21, ortuno wrote:
> > nit: Please leave these. Owners (me) prefer to have brackets :)
> 
> Done.

android BUILD.gn - lgtm

[Avi (use Gerrit), 2016-11-23 21:11:04]

content/common/build.gn lgtm

[commit-bot: I haz the power, 2016-11-24 00:26:26]

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

[commit-bot: I haz the power, 2016-11-24 00:26:27]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[dcheng, 2016-11-24 04:56:49]

mojo lgtm with some optional nits

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

https://codereview.chromium.org/2506813003/diff/200001/content/browser/blueto...
content/browser/bluetooth/web_bluetooth_service_impl.cc:311:
std::vector<uint8_t> value) {
Nit: I'm not sure what the desired semantics here are, but as written,
base::Callback will store a copy of |value|, which will then be copied again to
be passed into this function.

It seems like we could avoid copying at all until we're writing bytes to the
wire if this was a const std::vector<uint8_t>&

https://codereview.chromium.org/2506813003/diff/200001/content/common/bluetoo...
File content/common/bluetooth/web_bluetooth_device_id.cc (right):

https://codereview.chromium.org/2506813003/diff/200001/content/common/bluetoo...
content/common/bluetooth/web_bluetooth_device_id.cc:85: const
content::WebBluetoothDeviceId& device_id) {
Strictly speaking, shouldn't this be in namespace content to ensure ADL applies?

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

https://codereview.chromium.org/2506813003/diff/200001/content/renderer/bluet...
content/renderer/bluetooth/bluetooth_type_converters.cc:21: filter->services =
std::vector<device::BluetoothUUID>();
Nit: consider writing this as filter->services.emplace() if you don't want to
explicitly spell out the type (this applies to other places). I don't have a
strong opinion about this either way though.

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

https://codereview.chromium.org/2506813003/diff/200001/content/renderer/bluet...
content/renderer/bluetooth/web_bluetooth_impl.cc:190: device->name ?
blink::WebString::fromUTF8(device->name.value().c_str())
Nit: no c_str() here

https://codereview.chromium.org/2506813003/diff/200001/content/renderer/bluet...
content/renderer/bluetooth/web_bluetooth_impl.cc:276:
callbacks->onSuccess(std::move(value.value()));
Nit: this std::move() has no effect, as |value| is const.

If it's important to move rather than copy, this parameter will have to be
wrapped in a struct.

[juncai, 2016-11-28 19:25:50]

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

[commit-bot: I haz the power, 2016-11-28 19:26:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-28 22:18:45]

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

[commit-bot: I haz the power, 2016-11-28 22:18:46]

Dry run: This issue passed the CQ dry run.

[juncai, 2016-11-28 22:25:21]

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

https://codereview.chromium.org/2506813003/diff/200001/content/browser/blueto...
content/browser/bluetooth/web_bluetooth_service_impl.cc:311:
std::vector<uint8_t> value) {
On 2016/11/24 04:56:49, dcheng wrote:
> Nit: I'm not sure what the desired semantics here are, but as written,
> base::Callback will store a copy of |value|, which will then be copied again
to
> be passed into this function.
> 
> It seems like we could avoid copying at all until we're writing bytes to the
> wire if this was a const std::vector<uint8_t>&

Done.

https://codereview.chromium.org/2506813003/diff/200001/content/common/bluetoo...
File content/common/bluetooth/web_bluetooth_device_id.cc (right):

https://codereview.chromium.org/2506813003/diff/200001/content/common/bluetoo...
content/common/bluetooth/web_bluetooth_device_id.cc:85: const
content::WebBluetoothDeviceId& device_id) {
On 2016/11/24 04:56:49, dcheng wrote:
> Strictly speaking, shouldn't this be in namespace content to ensure ADL
applies?

If it is in the namespace content as before, the following will generate compile
error since it can't find the operator<< overload for WebURL:
https://cs.chromium.org/chromium/src/content/renderer/presentation/presentati...
https://cs.chromium.org/chromium/src/content/renderer/presentation/presentati...

I guess since the above code is in the namespace content, it finds the
operator<< overload for WebBluetoothDeviceId, not WebURL, so it generates
compile error. By moving this operator overloading out of the namespace content,
it will search the global namespace.

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

https://codereview.chromium.org/2506813003/diff/200001/content/renderer/bluet...
content/renderer/bluetooth/bluetooth_type_converters.cc:21: filter->services =
std::vector<device::BluetoothUUID>();
On 2016/11/24 04:56:49, dcheng wrote:
> Nit: consider writing this as filter->services.emplace() if you don't want to
> explicitly spell out the type (this applies to other places). I don't have a
> strong opinion about this either way though.

Done.

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

https://codereview.chromium.org/2506813003/diff/200001/content/renderer/bluet...
content/renderer/bluetooth/web_bluetooth_impl.cc:190: device->name ?
blink::WebString::fromUTF8(device->name.value().c_str())
On 2016/11/24 04:56:49, dcheng wrote:
> Nit: no c_str() here

Done.

https://codereview.chromium.org/2506813003/diff/200001/content/renderer/bluet...
content/renderer/bluetooth/web_bluetooth_impl.cc:276:
callbacks->onSuccess(std::move(value.value()));
On 2016/11/24 04:56:49, dcheng wrote:
> Nit: this std::move() has no effect, as |value| is const.
> 
> If it's important to move rather than copy, this parameter will have to be
> wrapped in a struct.

Done.

[dcheng, 2016-11-28 22:29:33]

https://codereview.chromium.org/2506813003/diff/200001/content/common/bluetoo...
File content/common/bluetooth/web_bluetooth_device_id.cc (right):

https://codereview.chromium.org/2506813003/diff/200001/content/common/bluetoo...
content/common/bluetooth/web_bluetooth_device_id.cc:85: const
content::WebBluetoothDeviceId& device_id) {
On 2016/11/28 22:25:21, juncai wrote:
> On 2016/11/24 04:56:49, dcheng wrote:
> > Strictly speaking, shouldn't this be in namespace content to ensure ADL
> applies?
> 
> If it is in the namespace content as before, the following will generate
compile
> error since it can't find the operator<< overload for WebURL:
>
https://cs.chromium.org/chromium/src/content/renderer/presentation/presentati...
>
https://cs.chromium.org/chromium/src/content/renderer/presentation/presentati...
> 
> I guess since the above code is in the namespace content, it finds the
> operator<< overload for WebBluetoothDeviceId, not WebURL, so it generates
> compile error. By moving this operator overloading out of the namespace
content,
> it will search the global namespace.

I don't think I understand what this has to do with operator<< for GURL (there
is no operator<< for WebURL).

[juncai, 2016-11-29 01:33:29]

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

[commit-bot: I haz the power, 2016-11-29 01:35:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-29 04:15:01]

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

[commit-bot: I haz the power, 2016-11-29 04:15:02]

Dry run: This issue passed the CQ dry run.

[juncai, 2016-11-29 04:23:28]

https://codereview.chromium.org/2506813003/diff/200001/content/common/bluetoo...
File content/common/bluetooth/web_bluetooth_device_id.cc (right):

https://codereview.chromium.org/2506813003/diff/200001/content/common/bluetoo...
content/common/bluetooth/web_bluetooth_device_id.cc:85: const
content::WebBluetoothDeviceId& device_id) {
On 2016/11/28 22:29:33, dcheng wrote:
> On 2016/11/28 22:25:21, juncai wrote:
> > On 2016/11/24 04:56:49, dcheng wrote:
> > > Strictly speaking, shouldn't this be in namespace content to ensure ADL
> > applies?
> > 
> > If it is in the namespace content as before, the following will generate
> compile
> > error since it can't find the operator<< overload for WebURL:
> >
>
https://cs.chromium.org/chromium/src/content/renderer/presentation/presentati...
> >
>
https://cs.chromium.org/chromium/src/content/renderer/presentation/presentati...
> > 
> > I guess since the above code is in the namespace content, it finds the
> > operator<< overload for WebBluetoothDeviceId, not WebURL, so it generates
> > compile error. By moving this operator overloading out of the namespace
> content,
> > it will search the global namespace.
> 
> I don't think I understand what this has to do with operator<< for GURL (there
> is no operator<< for WebURL).

Done.

[juncai, 2016-11-29 04:23:39]

The CQ bit was checked by juncai@chromium.org

[juncai, 2016-11-29 04:23:39]

The patchset sent to the CQ was uploaded after l-g-t-m from ortuno@chromium.org,
tedchoc@chromium.org, avi@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2506813003/#ps240001
(title: "address more comments")

[commit-bot: I haz the power, 2016-11-29 04:23:50]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-29 04:28:25]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1480393419365060,
"parent_rev": "f86888066cfc251fe14269f748afc648d9f7d02f", "commit_rev":
"37b56d4165464d18a5e678a0dba974d3c432c471"}

[commit-bot: I haz the power, 2016-11-29 04:28:49]

Description was changed from

==========
Use new wrapper types for web_bluetooth.mojom

This CL modified code to use new wrapper types for web_bluetooth.mojom. 
Also changed all instances of mojo::Array, mojo::String to std::vector,
std::string since mojo::Array, mojo::String have been deprecated.

BUG=597053
==========

to

==========
Use new wrapper types for web_bluetooth.mojom

This CL modified code to use new wrapper types for web_bluetooth.mojom. 
Also changed all instances of mojo::Array, mojo::String to std::vector,
std::string since mojo::Array, mojo::String have been deprecated.

BUG=597053
==========

[commit-bot: I haz the power, 2016-11-29 04:28:50]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2016-11-29 04:31:58]

Description was changed from

==========
Use new wrapper types for web_bluetooth.mojom

This CL modified code to use new wrapper types for web_bluetooth.mojom. 
Also changed all instances of mojo::Array, mojo::String to std::vector,
std::string since mojo::Array, mojo::String have been deprecated.

BUG=597053
==========

to

==========
Use new wrapper types for web_bluetooth.mojom

This CL modified code to use new wrapper types for web_bluetooth.mojom.
Also changed all instances of mojo::Array, mojo::String to std::vector,
std::string since mojo::Array, mojo::String have been deprecated.

BUG=597053

Committed: https://crrev.com/1ef7dd4b548d9ccd5e573bf053043ab79070ae08
Cr-Commit-Position: refs/heads/master@{#434892}
==========

[commit-bot: I haz the power, 2016-11-29 04:31:59]

Patchset 13 (id:??) landed as
https://crrev.com/1ef7dd4b548d9ccd5e573bf053043ab79070ae08
Cr-Commit-Position: refs/heads/master@{#434892}