Add chrome.usb.listInterfaces API

This adds a way to interrogate the USB interfaces. It will list out all
the possible interfaces for a particular device along with all of its
USB descriptions.

BUG=224132
TEST=Automated test for listInterfaces, manual test for actual libusb interaction

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=202097

[miket_OOO, 2013-04-26 16:32:03]

Kenny, will you please edit this issue description to get rid of the WIP status,
then "Publish+Mail Comments" so that Bei sees it? Thanks!

[adlr, 2013-04-26 17:33:07]

I think a rebase is needed for one of the enums.

Also, looks like a test needs to be updated:

chrome/browser/extensions/api/usb/usb_apitest.cc:48:44: error:
'TransferDirection' does not name a type

[Bei Zhang, 2013-04-26 17:48:06]

Hi Kenny,

I was going to say good if you fix the tests, but unfortunately, due to
crbug/223817, we are going to have a small refactor here. You might want to wait
until then and rearrange this change. Sorry.

Best regards,
Bei

[NOT Mike Tsao (try miket), 2013-04-26 19:00:00]

Let's get this CL landed. It was here first, so it's fair to make the later CL
deal with the merge conflicts. If there is a reason why this CL is going to be
completely inapplicable, that's one thing. But that doesn't seem to be the case.

Land this CL, then whoever fixes 223817 can reconcile the source (including this
CL) with those changes.

[Bei Zhang, 2013-04-26 22:38:00]

Thanks Kenny! This is a very desirable feature.

Please add unit tests and browser tests for the new feature.

https://codereview.chromium.org/12471013/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/usb/usb_api.cc (right):

https://codereview.chromium.org/12471013/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/usb/usb_api.cc:98: default:
I believe this should be:

default:
  NOTREACHED();
  return false;


Do not need NOTREACHED() on dead code.

https://codereview.chromium.org/12471013/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/usb/usb_api.cc:493: config_ = new
UsbConfigDescriptor(platform_config);
Can you put some comment to explain these two lines?

https://codereview.chromium.org/12471013/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/usb/usb_api.h (right):

https://codereview.chromium.org/12471013/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/usb/usb_api.h:47: UsbInterfaceDirection* output);
I think direction is not a property of USB interface. This should be called
either UsbTransferDirection or UsbEndpointDirection.

https://codereview.chromium.org/12471013/diff/1/chrome/browser/usb/usb_device.h
File chrome/browser/usb/usb_device.h (right):

https://codereview.chromium.org/12471013/diff/1/chrome/browser/usb/usb_device...
chrome/browser/usb/usb_device.h:67: virtual void
ListInterfaces(PlatformUsbConfigDescriptor* config,
This is actually GetConfigDescriptor.

https://codereview.chromium.org/12471013/diff/1/chrome/browser/usb/usb_device...
chrome/browser/usb/usb_device.h:146: UsbEndpointType interface_type,
Isn't this endpoint_type?

https://codereview.chromium.org/12471013/diff/1/chrome/browser/usb/usb_interf...
File chrome/browser/usb/usb_interface.h (right):

https://codereview.chromium.org/12471013/diff/1/chrome/browser/usb/usb_interf...
chrome/browser/usb/usb_interface.h:23: enum UsbEndpointType {
Why not UsbTransferType? This is actually mapping to libusb_transfer_type.

I understand that this can be retrieve from the endpoint attribute, but it is
also used in libusb_transfer.


Also, probably not a big deal, we may consider making these values consistent
with the libusb ones?

https://codereview.chromium.org/12471013/diff/1/chrome/browser/usb/usb_interf...
chrome/browser/usb/usb_interface.h:30: enum UsbInterfaceDirection {
Fix the naming as mentioned elsewhere.

https://codereview.chromium.org/12471013/diff/1/chrome/browser/usb/usb_interf...
chrome/browser/usb/usb_interface.h:54: virtual ~UsbEndpointDescriptor();
Classes that are ref-counted should have destructors that are declared protected
or private.

Put the destructor to the private section and add "friend class
base::RefCounted<UsbEndpointDescriptor>;". Please fix all of them.

https://codereview.chromium.org/12471013/diff/1/chrome/browser/usb/usb_interf...
chrome/browser/usb/usb_interface.h:76: size_t numEndpoints() const;
GetNumEndpoints

https://codereview.chromium.org/12471013/diff/1/chrome/browser/usb/usb_interf...
chrome/browser/usb/usb_interface.h:78: getEndpoint(size_t index) const;
Should be GetEndpointAt;
Fix all of them.

[Kenny Root (Google), 2013-05-08 01:15:08]

https://codereview.chromium.org/12471013/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/usb/usb_api.cc (right):

https://codereview.chromium.org/12471013/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/usb/usb_api.cc:98: default:
On 2013/04/26 22:38:01, Bei Zhang wrote:
> I believe this should be:
> 
> default:
>   NOTREACHED();
>   return false;
> 
> 
> Do not need NOTREACHED() on dead code.

Done.

https://codereview.chromium.org/12471013/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/usb/usb_api.cc:493: config_ = new
UsbConfigDescriptor(platform_config);
On 2013/04/26 22:38:01, Bei Zhang wrote:
> Can you put some comment to explain these two lines?

Oh, I see your point. This doesn't actually work. Rewrote this part to make more
sense. See my comment below as well.

https://codereview.chromium.org/12471013/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/usb/usb_api.cc:505: for (size_t i = 0,
numInterfaces = config_->numInterfaces();
I might be able to move this all into AsyncWorkStart. Is that preferable?

https://codereview.chromium.org/12471013/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/usb/usb_api.h (right):

https://codereview.chromium.org/12471013/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/usb/usb_api.h:47: UsbInterfaceDirection* output);
On 2013/04/26 22:38:01, Bei Zhang wrote:
> I think direction is not a property of USB interface. This should be called
> either UsbTransferDirection or UsbEndpointDirection.

Done.

https://codereview.chromium.org/12471013/diff/1/chrome/browser/usb/usb_device.h
File chrome/browser/usb/usb_device.h (right):

https://codereview.chromium.org/12471013/diff/1/chrome/browser/usb/usb_device...
chrome/browser/usb/usb_device.h:67: virtual void
ListInterfaces(PlatformUsbConfigDescriptor* config,
On 2013/04/26 22:38:01, Bei Zhang wrote:
> This is actually GetConfigDescriptor.

You mean change the name?

https://codereview.chromium.org/12471013/diff/1/chrome/browser/usb/usb_device...
chrome/browser/usb/usb_device.h:146: UsbEndpointType interface_type,
On 2013/04/26 22:38:01, Bei Zhang wrote:
> Isn't this endpoint_type?

Done.

https://codereview.chromium.org/12471013/diff/1/chrome/browser/usb/usb_interf...
File chrome/browser/usb/usb_interface.h (right):

https://codereview.chromium.org/12471013/diff/1/chrome/browser/usb/usb_interf...
chrome/browser/usb/usb_interface.h:23: enum UsbEndpointType {
On 2013/04/26 22:38:01, Bei Zhang wrote:
> Why not UsbTransferType? This is actually mapping to libusb_transfer_type.
> 
> I understand that this can be retrieve from the endpoint attribute, but it is
> also used in libusb_transfer.
> 
> 
> Also, probably not a big deal, we may consider making these values consistent
> with the libusb ones?

Done.

https://codereview.chromium.org/12471013/diff/1/chrome/browser/usb/usb_interf...
chrome/browser/usb/usb_interface.h:30: enum UsbInterfaceDirection {
On 2013/04/26 22:38:01, Bei Zhang wrote:
> Fix the naming as mentioned elsewhere.

Done.

https://codereview.chromium.org/12471013/diff/1/chrome/browser/usb/usb_interf...
chrome/browser/usb/usb_interface.h:54: virtual ~UsbEndpointDescriptor();
On 2013/04/26 22:38:01, Bei Zhang wrote:
> Classes that are ref-counted should have destructors that are declared
protected
> or private.
> 
> Put the destructor to the private section and add "friend class
> base::RefCounted<UsbEndpointDescriptor>;". Please fix all of them.

Done.

https://codereview.chromium.org/12471013/diff/1/chrome/browser/usb/usb_interf...
chrome/browser/usb/usb_interface.h:76: size_t numEndpoints() const;
On 2013/04/26 22:38:01, Bei Zhang wrote:
> GetNumEndpoints

Done.

https://codereview.chromium.org/12471013/diff/1/chrome/browser/usb/usb_interf...
chrome/browser/usb/usb_interface.h:78: getEndpoint(size_t index) const;
On 2013/04/26 22:38:01, Bei Zhang wrote:
> Should be GetEndpointAt;
> Fix all of them.

Done.

[Kenny Root (Google), 2013-05-08 02:29:34]

https://codereview.chromium.org/12471013/diff/13001/chrome/browser/extensions...
File chrome/browser/extensions/api/usb/usb_api.cc (right):

https://codereview.chromium.org/12471013/diff/13001/chrome/browser/extensions...
chrome/browser/extensions/api/usb/usb_api.cc:167: return false;
Looks like I did that and accidentally reverted it.. will fix it in the next
patchset

https://codereview.chromium.org/12471013/diff/13001/chrome/browser/usb/usb_in...
File chrome/browser/usb/usb_interface.cc (right):

https://codereview.chromium.org/12471013/diff/13001/chrome/browser/usb/usb_in...
chrome/browser/usb/usb_interface.cc:147:
UsbConfigDescriptor::~UsbConfigDescriptor() {}
This needs libusb_free_config_descriptor, but it seems like a layering problem
if it's freed here.

[Bei Zhang, 2013-05-14 20:55:47]

lgtm except for the RemoveUsbDeviceResource issue in usb_api.cc.

Please fix that and rebase.

https://codereview.chromium.org/12471013/diff/13001/chrome/browser/usb/usb_in...
File chrome/browser/usb/usb_interface.cc (right):

https://codereview.chromium.org/12471013/diff/13001/chrome/browser/usb/usb_in...
chrome/browser/usb/usb_interface.cc:147:
UsbConfigDescriptor::~UsbConfigDescriptor() {}
I think this is the right place.

On 2013/05/08 02:29:34, Kenny Root (Google) wrote:
> This needs libusb_free_config_descriptor, but it seems like a layering problem
> if it's freed here.

https://codereview.chromium.org/12471013/diff/24001/chrome/browser/extensions...
File chrome/browser/extensions/api/usb/usb_api.cc (right):

https://codereview.chromium.org/12471013/diff/24001/chrome/browser/extensions...
chrome/browser/extensions/api/usb/usb_api.cc:438: UsbService* const service =
UsbServiceFactory::GetInstance()->GetForProfile(
This is why we get fatal error in profile_keyed_base_factory.cc. We are creating
the service on IO thread.

No worry though. This will be changed after my refactor.

https://codereview.chromium.org/12471013/diff/24001/chrome/browser/extensions...
chrome/browser/extensions/api/usb/usb_api.cc:487:
RemoveUsbDeviceResource(parameters_->device.handle);
Please remove this.

If removing this causes crash, don't worry, we are working on that.

[Kenny Root (Google), 2013-05-15 04:43:13]

Patch with response coming.

https://codereview.chromium.org/12471013/diff/24001/chrome/browser/extensions...
File chrome/browser/extensions/api/usb/usb_api.cc (right):

https://codereview.chromium.org/12471013/diff/24001/chrome/browser/extensions...
chrome/browser/extensions/api/usb/usb_api.cc:438: UsbService* const service =
UsbServiceFactory::GetInstance()->GetForProfile(
On 2013/05/14 20:55:47, Bei Zhang wrote:
> This is why we get fatal error in profile_keyed_base_factory.cc. We are
creating
> the service on IO thread.
> 
> No worry though. This will be changed after my refactor.

Oh, that makes sense. I was combing all over my changed lines for the bug.

https://codereview.chromium.org/12471013/diff/24001/chrome/browser/extensions...
chrome/browser/extensions/api/usb/usb_api.cc:487:
RemoveUsbDeviceResource(parameters_->device.handle);
On 2013/05/14 20:55:47, Bei Zhang wrote:
> Please remove this.
> 
> If removing this causes crash, don't worry, we are working on that.

Done.

[Kenny Root (Google), 2013-05-15 06:57:57]

Just sent PS6 to the trybot and it failed, but I can't reproduce the issue. Is
it a makefile dependency problem?

[Bei Zhang, 2013-05-16 17:46:42]

Please rebase to master before commit.

Thanks for the effort!

https://codereview.chromium.org/12471013/diff/46001/chrome/browser/extensions...
File chrome/browser/extensions/api/usb/usb_manual_apitest.cc (right):

https://codereview.chromium.org/12471013/diff/46001/chrome/browser/extensions...
chrome/browser/extensions/api/usb/usb_manual_apitest.cc:22:
PermissionsRequestFunction::SetIgnoreUserGestureForTests(true);
I think you need to change this to extensions::PermissionsRequestFunction to
compile. The ninja target name is 'browser_tests'. I'm not quite sure if make
has the same target name.

[Kenny Root (Google), 2013-05-17 00:03:35]

Hi, Garrett,

Could you review this change? It's ready for submission and you're still in the
OWNERS file.

[Garret Kelly, 2013-05-22 15:17:50]

OWNERS LGTM.

Garret

[commit-bot: I haz the power, 2013-05-23 01:01:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kroot@google.com/12471013/64001

[commit-bot: I haz the power, 2013-05-23 01:22:22]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[Kenny Root (Google), 2013-05-23 16:55:45]

Antony, could you review this? I need extensions OWNERS approval.

[asargent_no_longer_on_chrome, 2013-05-23 18:32:36]

LGTM

[commit-bot: I haz the power, 2013-05-23 19:20:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kroot@google.com/12471013/64001

[commit-bot: I haz the power, 2013-05-23 19:20:13]

Failed to apply patch for
chrome/browser/extensions/extension_function_histogram_value.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/extensions/extension_function_histogram_value.h
  Hunk #1 FAILED at 529.
  1 out of 1 hunk FAILED -- saving rejects to file
chrome/browser/extensions/extension_function_histogram_value.h.rej

Patch:       chrome/browser/extensions/extension_function_histogram_value.h
Index: chrome/browser/extensions/extension_function_histogram_value.h
diff --git a/chrome/browser/extensions/extension_function_histogram_value.h
b/chrome/browser/extensions/extension_function_histogram_value.h
index
8bc1a745926823241543df2a6cc5f63b80f6cd33..7a2260666b74b165444874298dce3094a23e765c
100644
--- a/chrome/browser/extensions/extension_function_histogram_value.h
+++ b/chrome/browser/extensions/extension_function_histogram_value.h
@@ -529,6 +529,7 @@ enum HistogramValue {
   RUNTIME_GETPLATFORMINFO,
   WEBVIEW_INSERTCSS,
   METRICSPRIVATE_GETISCRASHRECORDINGENABLED,
+  USB_LISTINTERFACES,
   ENUM_BOUNDARY // Last entry: Add new entries above.
 };

[commit-bot: I haz the power, 2013-05-23 19:20:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kroot@google.com/12471013/64001

[commit-bot: I haz the power, 2013-05-23 19:20:56]

Failed to apply patch for
chrome/browser/extensions/extension_function_histogram_value.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/extensions/extension_function_histogram_value.h
  Hunk #1 FAILED at 529.
  1 out of 1 hunk FAILED -- saving rejects to file
chrome/browser/extensions/extension_function_histogram_value.h.rej

Patch:       chrome/browser/extensions/extension_function_histogram_value.h
Index: chrome/browser/extensions/extension_function_histogram_value.h
diff --git a/chrome/browser/extensions/extension_function_histogram_value.h
b/chrome/browser/extensions/extension_function_histogram_value.h
index
8bc1a745926823241543df2a6cc5f63b80f6cd33..7a2260666b74b165444874298dce3094a23e765c
100644
--- a/chrome/browser/extensions/extension_function_histogram_value.h
+++ b/chrome/browser/extensions/extension_function_histogram_value.h
@@ -529,6 +529,7 @@ enum HistogramValue {
   RUNTIME_GETPLATFORMINFO,
   WEBVIEW_INSERTCSS,
   METRICSPRIVATE_GETISCRASHRECORDINGENABLED,
+  USB_LISTINTERFACES,
   ENUM_BOUNDARY // Last entry: Add new entries above.
 };

[commit-bot: I haz the power, 2013-05-23 19:57:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kroot@google.com/12471013/77002

[commit-bot: I haz the power, 2013-05-23 23:23:29]

Retried try job too often on win_rel for step(s) content_browsertests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-05-24 01:07:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kroot@google.com/12471013/77002

[commit-bot: I haz the power, 2013-05-24 01:07:34]

Failed to apply patch for
chrome/browser/extensions/extension_function_histogram_value.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/extensions/extension_function_histogram_value.h
  Hunk #1 FAILED at 530.
  1 out of 1 hunk FAILED -- saving rejects to file
chrome/browser/extensions/extension_function_histogram_value.h.rej

Patch:       chrome/browser/extensions/extension_function_histogram_value.h
Index: chrome/browser/extensions/extension_function_histogram_value.h
diff --git a/chrome/browser/extensions/extension_function_histogram_value.h
b/chrome/browser/extensions/extension_function_histogram_value.h
index
44b709f0766fa633a1ec5c4c1a8675a6d7b9d982..4cff0f0709ce87066a450ce88174df51060e36ef
100644
--- a/chrome/browser/extensions/extension_function_histogram_value.h
+++ b/chrome/browser/extensions/extension_function_histogram_value.h
@@ -530,6 +530,7 @@ enum HistogramValue {
   WEBVIEW_INSERTCSS,
   METRICSPRIVATE_GETISCRASHRECORDINGENABLED,
   IDENTITYPRIVATE_GETRESOURCES,
+  USB_LISTINTERFACES,
   ENUM_BOUNDARY // Last entry: Add new entries above.
 };

[commit-bot: I haz the power, 2013-05-24 04:21:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kroot@google.com/12471013/111001

[commit-bot: I haz the power, 2013-05-24 04:21:36]

Failed to apply patch for
chrome/browser/extensions/extension_function_histogram_value.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/extensions/extension_function_histogram_value.h
  Hunk #1 FAILED at 531.
  1 out of 1 hunk FAILED -- saving rejects to file
chrome/browser/extensions/extension_function_histogram_value.h.rej

Patch:       chrome/browser/extensions/extension_function_histogram_value.h
Index: chrome/browser/extensions/extension_function_histogram_value.h
diff --git a/chrome/browser/extensions/extension_function_histogram_value.h
b/chrome/browser/extensions/extension_function_histogram_value.h
index
b1c7f055e1962623ae1af6baffee84d14e2db344..fa916d649b7e1bfdc697eeb23c08d0fce83c02e0
100644
--- a/chrome/browser/extensions/extension_function_histogram_value.h
+++ b/chrome/browser/extensions/extension_function_histogram_value.h
@@ -531,6 +531,7 @@ enum HistogramValue {
   METRICSPRIVATE_GETISCRASHRECORDINGENABLED,
   IDENTITYPRIVATE_GETRESOURCES,
   NOTIFICATIONS_GET_ALL,
+  USB_LISTINTERFACES,
   ENUM_BOUNDARY // Last entry: Add new entries above.
 };

[commit-bot: I haz the power, 2013-05-24 04:39:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kroot@google.com/12471013/116001

[commit-bot: I haz the power, 2013-05-24 10:45:38]

Retried try job too often on win_rel for step(s) base_unittests,
cacheinvalidation_unittests, cc_unittests, check_deps, chrome_frame_net_tests,
chrome_frame_tests, chrome_frame_unittests, chromedriver2_unittests,
components_unittests, content_browsertests, content_unittests, crypto_unittests,
gpu_unittests, installer_util_unittests, ipc_tests, jingle_unittests,
media_unittests, mini_installer_test, nacl_integration, net_unittests,
ppapi_unittests, printing_unittests, remoting_unittests, sql_unittests,
sync_integration_tests, sync_unit_tests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-05-24 14:04:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kroot@google.com/12471013/116001

[commit-bot: I haz the power, 2013-05-24 16:08:21]

Change committed as 202097