Use Mojo enum types in the C++ USB interface

Use Mojo enum types in the C++ USB interface

This change removes a number of type converters and will make it easier
to port existing code over to the Mojo USB service in the future.

BUG=699790
Review-Url: https://codereview.chromium.org/2821813002
Cr-Commit-Position: refs/heads/master@{#465513}
Committed: https://chromium.googlesource.com/chromium/src/+/70e5417cbbad78ff3e5ce8dc3d21517c6f4eee69

[Reilly Grant (use Gerrit), 2017-04-15 02:58:41]

reillyg@chromium.org changed reviewers:
+ dgozman@chromium.org, mbarbella@chromium.org, mcasas@chromium.org

[Reilly Grant (use Gerrit), 2017-04-15 02:58:42]

dgozman@, please review *android*
mbarbella@, please review IPC changes
mcasas@, please do a general review

[Reilly Grant (use Gerrit), 2017-04-15 02:58:46]

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

[commit-bot: I haz the power, 2017-04-15 02:58:54]

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, 2017-04-15 03:03:22]

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

[commit-bot: I haz the power, 2017-04-15 03:03:23]

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

[Reilly Grant (use Gerrit), 2017-04-15 03:18:34]

Fix circular dependency

[Reilly Grant (use Gerrit), 2017-04-15 03:18:37]

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

[commit-bot: I haz the power, 2017-04-15 03:18:52]

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, 2017-04-15 04:22:51]

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

[commit-bot: I haz the power, 2017-04-15 04:22:53]

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

[dgozman, 2017-04-17 17:03:15]

devtools lgtm

[Martin Barbella, 2017-04-17 17:08:23]

mojom lgtm

[mcasas, 2017-04-17 17:15:16]

lgtm with some random suggestions/nits.

https://codereview.chromium.org/2821813002/diff/20001/chrome/browser/devtools...
File chrome/browser/devtools/device/usb/android_usb_browsertest.cc (right):

https://codereview.chromium.org/2821813002/diff/20001/chrome/browser/devtools...
chrome/browser/devtools/device/usb/android_usb_browsertest.cc:224: }
nit: no need for {} for one-line bodies.

https://codereview.chromium.org/2821813002/diff/20001/chrome/browser/devtools...
chrome/browser/devtools/device/usb/android_usb_browsertest.cc:226:
device::UsbTransferStatus status = broken_
micro-nit: const

https://codereview.chromium.org/2821813002/diff/20001/device/usb/public/inter...
File device/usb/public/interfaces/device.mojom (right):

https://codereview.chromium.org/2821813002/diff/20001/device/usb/public/inter...
device/usb/public/interfaces/device.mojom:30: enum UsbControlTransferRecipient {
nit: This seems pretty similar to the Spec's "USBRecipient" enum,
perhaps we could add a link such as, e.g. [1] ? (Similarly
for USBRequestType <-> Spec's UsbControlTransferType).

[1] https://wicg.github.io/webusb/#enumdef-usbrecipient

https://codereview.chromium.org/2821813002/diff/20001/device/usb/usb_device_h...
File device/usb/usb_device_handle_impl.cc (right):

https://codereview.chromium.org/2821813002/diff/20001/device/usb/usb_device_h...
device/usb/usb_device_handle_impl.cc:105: default:
Nit: no need for default: since we cover all 
libusb_transfer_status enum members.

https://codereview.chromium.org/2821813002/diff/20001/device/usb/usb_device_h...
File device/usb/usb_device_handle_win.cc (right):

https://codereview.chromium.org/2821813002/diff/20001/device/usb/usb_device_h...
device/usb/usb_device_handle_win.cc:180: if (hub_handle_.IsValid()) {
nit: this method would be clear with early-returns, i.e.:

  if (!hub_handle_.IsValid()) {
    task_runner_->PostTask(... TRANSFER_ERROR...);
    return;
  }
  if ( !INBOUND || !STANDARD || !DEVICE ...) {
    task_runner_->PostTask(... TRANSFER_ERROR...);
    return;  
  }

( Or perhaps merging all these conditions)  
Maybe not in this CL, up to you.

[Reilly Grant (use Gerrit), 2017-04-17 18:05:36]

Address mcasas@ comments

[Reilly Grant (use Gerrit), 2017-04-17 18:05:54]

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

[commit-bot: I haz the power, 2017-04-17 18:06:25]

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, 2017-04-17 18:31:34]

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

[commit-bot: I haz the power, 2017-04-17 18:31:35]

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

[Reilly Grant (use Gerrit), 2017-04-17 19:17:13]

Android compiler doesn't trust exhaustive switch statements

[Reilly Grant (use Gerrit), 2017-04-17 19:20:49]

https://codereview.chromium.org/2821813002/diff/20001/chrome/browser/devtools...
File chrome/browser/devtools/device/usb/android_usb_browsertest.cc (right):

https://codereview.chromium.org/2821813002/diff/20001/chrome/browser/devtools...
chrome/browser/devtools/device/usb/android_usb_browsertest.cc:224: }
On 2017/04/17 17:15:15, mcasas wrote:
> nit: no need for {} for one-line bodies.

Since I'm not touching this line in this patch I'd rather not do unrelated
cleanups given this is already a large change.

https://codereview.chromium.org/2821813002/diff/20001/chrome/browser/devtools...
chrome/browser/devtools/device/usb/android_usb_browsertest.cc:226:
device::UsbTransferStatus status = broken_
On 2017/04/17 17:15:15, mcasas wrote:
> micro-nit: const

I don't like marking local variables const unless there is a compelling reason
to. I don't believe it provides any safety or optimization benefit here.

https://codereview.chromium.org/2821813002/diff/20001/device/usb/public/inter...
File device/usb/public/interfaces/device.mojom (right):

https://codereview.chromium.org/2821813002/diff/20001/device/usb/public/inter...
device/usb/public/interfaces/device.mojom:30: enum UsbControlTransferRecipient {
On 2017/04/17 17:15:15, mcasas wrote:
> nit: This seems pretty similar to the Spec's "USBRecipient" enum,
> perhaps we could add a link such as, e.g. [1] ? (Similarly
> for USBRequestType <-> Spec's UsbControlTransferType).
> 
> [1] https://wicg.github.io/webusb/#enumdef-usbrecipient

Done.

https://codereview.chromium.org/2821813002/diff/20001/device/usb/usb_device_h...
File device/usb/usb_device_handle_impl.cc (right):

https://codereview.chromium.org/2821813002/diff/20001/device/usb/usb_device_h...
device/usb/usb_device_handle_impl.cc:105: default:
On 2017/04/17 17:15:15, mcasas wrote:
> Nit: no need for default: since we cover all 
> libusb_transfer_status enum members.

Done.

https://codereview.chromium.org/2821813002/diff/20001/device/usb/usb_device_h...
File device/usb/usb_device_handle_win.cc (right):

https://codereview.chromium.org/2821813002/diff/20001/device/usb/usb_device_h...
device/usb/usb_device_handle_win.cc:180: if (hub_handle_.IsValid()) {
On 2017/04/17 17:15:15, mcasas wrote:
> nit: this method would be clear with early-returns, i.e.:
> 
>   if (!hub_handle_.IsValid()) {
>     task_runner_->PostTask(... TRANSFER_ERROR...);
>     return;
>   }
>   if ( !INBOUND || !STANDARD || !DEVICE ...) {
>     task_runner_->PostTask(... TRANSFER_ERROR...);
>     return;  
>   }
> 
> ( Or perhaps merging all these conditions)  
> Maybe not in this CL, up to you.

I have a follow-up CL that adds more logic to this function and I will consider
these changes there.

[Reilly Grant (use Gerrit), 2017-04-17 21:07:59]

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

[commit-bot: I haz the power, 2017-04-17 21:08:30]

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

[Reilly Grant (use Gerrit), 2017-04-17 21:23:08]

MSVC++ doesn't trust them either

[Reilly Grant (use Gerrit), 2017-04-17 21:23:13]

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

[commit-bot: I haz the power, 2017-04-17 21:23:45]

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, 2017-04-17 22:28:23]

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

[commit-bot: I haz the power, 2017-04-17 22:28:25]

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

[Reilly Grant (use Gerrit), 2017-04-17 23:34:27]

Fix up //device/usb dependencies in //extensions/browser/api

[Reilly Grant (use Gerrit), 2017-04-17 23:34:33]

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

[commit-bot: I haz the power, 2017-04-17 23: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, 2017-04-18 01:56:29]

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

[commit-bot: I haz the power, 2017-04-18 01:56:30]

Dry run: This issue passed the CQ dry run.

[Reilly Grant (use Gerrit), 2017-04-19 05:02:04]

The CQ bit was checked by reillyg@chromium.org

[Reilly Grant (use Gerrit), 2017-04-19 05:02:04]

The patchset sent to the CQ was uploaded after l-g-t-m from
mbarbella@chromium.org, mcasas@chromium.org, dgozman@chromium.org
Link to the patchset: https://codereview.chromium.org/2821813002/#ps120001
(title: "Fix up //device/usb dependencies in //extensions/browser/api")

[commit-bot: I haz the power, 2017-04-19 05:02:22]

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, 2017-04-19 06:44:30]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1492578124557990,
"parent_rev": "1bae530acabe60fccb84e27d2a3f1c34a692be57", "commit_rev":
"70e5417cbbad78ff3e5ce8dc3d21517c6f4eee69"}

[commit-bot: I haz the power, 2017-04-19 06:44:40]

Description was changed from

==========
Use Mojo enum types in the C++ USB interface

This change removes a number of type converters and will make it easier
to port existing code over to the Mojo USB service in the future.

BUG=699790
==========

to

==========
Use Mojo enum types in the C++ USB interface

This change removes a number of type converters and will make it easier
to port existing code over to the Mojo USB service in the future.

BUG=699790

Review-Url: https://codereview.chromium.org/2821813002
Cr-Commit-Position: refs/heads/master@{#465513}
Committed:
https://chromium.googlesource.com/chromium/src/+/70e5417cbbad78ff3e5ce8dc3d21...
==========

[commit-bot: I haz the power, 2017-04-19 06:44:42]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/70e5417cbbad78ff3e5ce8dc3d21...