Introducing chrome.usb.getDevices/openDevice API

Introducing chrome.usb.getDevices/openDevice API

Currently chrome.usb.findDevices always yields new ids and there is no way to know the consistency among different calls.

chrome.usb.getDevices API will be introduced which yields stable ids for each device and remains unchanged until the device is unplugged.

API Proposal:
https://docs.google.com/a/chromium.org/document/d/16OJQRjW6ZRkIQqA3qIEVFBRY3tFLFnlNXlTGJ5pgPfg/edit

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

[Bei Zhang, 2013-08-21 22:35:35]

Hi Antony,

Please give an owner's review on these files:

  chrome/browser/extensions/extension_function_histogram_value.h
  chrome/common/extensions/api/usb.idl
  chrome/test/data/extensions/api_test/usb/device_handling/test.js
  chrome/test/data/extensions/api_test/usb/reset_device/manifest.json
  chrome/test/data/extensions/api_test/usb/reset_device/test.js

Thanks,
Bei

[asargent_no_longer_on_chrome, 2013-08-22 21:06:13]

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

https://codereview.chromium.org/22914023/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/usb/usb_api.cc:34: namespace OpenDevice =
usb::OpenDevice;
nit: can you sort these alphabetically? It would make them easier to read

https://codereview.chromium.org/22914023/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/usb/usb_api.cc:337:
std::vector<linked_ptr<EndpointDescriptor> >* endpoints) {
nit: It looks like you removed the "static" linkage declaration from these
functions, which is fine since they're all in an anonymous namespace anyway. But
there are still several functions above marked as static. I think it would best
if either:

-All the functions in this part of the file are *outside* of an anonymous
namespace and marked static.

or

-All of them are *inside* an anonymous namespace and none of them are marked as
static. 

I don't care which way we do it, we should just be consistent in the file.

https://codereview.chromium.org/22914023/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/usb/usb_api.cc:615:
SetResult(PopulateDeviceHandle(
If the device handle could live on the FILE thread, could you cut the thread
hops from UI->IO->FILE->IO->UI to just UI->FILE->UI ?

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

https://codereview.chromium.org/22914023/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/usb/usb_api.h:124: // Create ApiResources and
reply.
Does it actually create a UsbDeviceResource or some other type of ApiResource?

Maybe change this to something like:

// Creates a UsbDeviceResource and sends a reply. Should be run on the 
// IO thread.

https://codereview.chromium.org/22914023/diff/1/chrome/common/extensions/api/...
File chrome/common/extensions/api/usb.idl (right):

https://codereview.chromium.org/22914023/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/usb.idl:22: // A dictionary returned by
|getDevices| to identify a connected device.
optional suggestion: you can omit the "A dictionary" part from this comment,
since it's clear the thing just below the comment is a dictionary. E.g.


// Returned by |getDevices| to identify a connected device.

https://codereview.chromium.org/22914023/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/usb.idl:30: // A dictionary returned by
|openDevice| to be used for USB communication.
same suggestion about "a dictionary".

Also, it might be useful to include something in the comment about the expected
lifetime that this data is valid. E.g. will I always get the same |handle| value
back for a given device, so that it's safe for developers to store this value in
persistently such as in indexedDB/chrome.storage.local/etc. ?  Or is it only
valid while the device is open, and after closeDevice or chrome restart it
becomes invalid?

https://codereview.chromium.org/22914023/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/usb.idl:34: long? productId;
Previously calls to findDevices would return a dictionary that always had these
two fields defined, right? If so then I don't think we can make them optional,
because that could break developer code. Can you just make them be 0 in the
situations where you would otherwise leave them undefined?

https://codereview.chromium.org/22914023/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/usb.idl:170: // a series of |getDevices| calls.
do you mean "a series of |openDevice| calls"?

https://codereview.chromium.org/22914023/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/usb.idl:174: FindDevicesCallback callback);
Do we still want to have developers using findDevices in new code, or would we
rather have developers switch over to always just using getDevices/openDevice?

If we don't want them to use it anymore, we can say something in the comment
like "this is deprecated, please use getDevices/openDevice instead", or possibly
even mark it as [nodoc] to hide it in the documentation.

[Bei Zhang, 2013-08-23 22:52:00]

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

https://codereview.chromium.org/22914023/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/usb/usb_api.cc:34: namespace OpenDevice =
usb::OpenDevice;
On 2013/08/22 21:06:13, Antony Sargent wrote:
> nit: can you sort these alphabetically? It would make them easier to read

Done.

https://codereview.chromium.org/22914023/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/usb/usb_api.cc:337:
std::vector<linked_ptr<EndpointDescriptor> >* endpoints) {
On 2013/08/22 21:06:13, Antony Sargent wrote:
> nit: It looks like you removed the "static" linkage declaration from these
> functions, which is fine since they're all in an anonymous namespace anyway.
But
> there are still several functions above marked as static. I think it would
best
> if either:
> 
> -All the functions in this part of the file are *outside* of an anonymous
> namespace and marked static.
> 
> or
> 
> -All of them are *inside* an anonymous namespace and none of them are marked
as
> static. 
> 
> I don't care which way we do it, we should just be consistent in the file.
> 

Done.

https://codereview.chromium.org/22914023/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/usb/usb_api.cc:615:
SetResult(PopulateDeviceHandle(
This is addressed by another CL.

On 2013/08/22 21:06:13, Antony Sargent wrote:
> If the device handle could live on the FILE thread, could you cut the thread
> hops from UI->IO->FILE->IO->UI to just UI->FILE->UI ?
>

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

https://codereview.chromium.org/22914023/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/usb/usb_api.h:124: // Create ApiResources and
reply.
This is not needed since we simplified the threading.


On 2013/08/22 21:06:13, Antony Sargent wrote:
> Does it actually create a UsbDeviceResource or some other type of ApiResource?
> 
> Maybe change this to something like:
> 
> // Creates a UsbDeviceResource and sends a reply. Should be run on the 
> // IO thread.

https://codereview.chromium.org/22914023/diff/1/chrome/common/extensions/api/...
File chrome/common/extensions/api/usb.idl (right):

https://codereview.chromium.org/22914023/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/usb.idl:22: // A dictionary returned by
|getDevices| to identify a connected device.
On 2013/08/22 21:06:13, Antony Sargent wrote:
> optional suggestion: you can omit the "A dictionary" part from this comment,
> since it's clear the thing just below the comment is a dictionary. E.g.
> 
> 
> // Returned by |getDevices| to identify a connected device.
> 
> 

Done.

https://codereview.chromium.org/22914023/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/usb.idl:30: // A dictionary returned by
|openDevice| to be used for USB communication.
On 2013/08/22 21:06:13, Antony Sargent wrote:
> same suggestion about "a dictionary".
> 
> Also, it might be useful to include something in the comment about the
expected
> lifetime that this data is valid. E.g. will I always get the same |handle|
value
> back for a given device, so that it's safe for developers to store this value
in
> persistently such as in indexedDB/chrome.storage.local/etc. ?  Or is it only
> valid while the device is open, and after closeDevice or chrome restart it
> becomes invalid?

Done.

https://codereview.chromium.org/22914023/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/usb.idl:34: long? productId;
Done. Made a comment to explain it.

On 2013/08/22 21:06:13, Antony Sargent wrote:
> Previously calls to findDevices would return a dictionary that always had
these
> two fields defined, right? If so then I don't think we can make them optional,
> because that could break developer code. Can you just make them be 0 in the
> situations where you would otherwise leave them undefined?

https://codereview.chromium.org/22914023/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/usb.idl:170: // a series of |getDevices| calls.
On 2013/08/22 21:06:13, Antony Sargent wrote:
> do you mean "a series of |openDevice| calls"?

Done.

https://codereview.chromium.org/22914023/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/usb.idl:174: FindDevicesCallback callback);
This function might be handy to some developers. Since I have fixed all the
crashing due to the old findDevices, I will just leave it as is.

On 2013/08/22 21:06:13, Antony Sargent wrote:
> Do we still want to have developers using findDevices in new code, or would we
> rather have developers switch over to always just using getDevices/openDevice?
> 
> If we don't want them to use it anymore, we can say something in the comment
> like "this is deprecated, please use getDevices/openDevice instead", or
possibly
> even mark it as [nodoc] to hide it in the documentation.

[asargent_no_longer_on_chrome, 2013-08-23 23:57:00]

lgtm w/ a few final nits

https://codereview.chromium.org/22914023/diff/1/chrome/common/extensions/api/...
File chrome/common/extensions/api/usb.idl (right):

https://codereview.chromium.org/22914023/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/usb.idl:174: FindDevicesCallback callback);
On 2013/08/23 22:52:00, Bei Zhang wrote:
> This function might be handy to some developers. Since I have fixed all the
> crashing due to the old findDevices, I will just leave it as is.
> 
> On 2013/08/22 21:06:13, Antony Sargent wrote:
> > Do we still want to have developers using findDevices in new code, or would
we
> > rather have developers switch over to always just using
getDevices/openDevice?
> > 
> > If we don't want them to use it anymore, we can say something in the comment
> > like "this is deprecated, please use getDevices/openDevice instead", or
> possibly
> > even mark it as [nodoc] to hide it in the documentation. 
> 

Ok

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

https://codereview.chromium.org/22914023/diff/4001/chrome/browser/extensions/...
chrome/browser/extensions/api/usb/usb_api.cc:24: namespace ListInterfaces =
usb::ListInterfaces;
nit: 'ListInterfaces' still isn't sorted in the right place. In emacs, select
the region and do M-x sort-lines. In vi, first exit, then run emacs, then follow
the emacs instructions.

https://codereview.chromium.org/22914023/diff/4001/chrome/common/extensions/a...
File chrome/common/extensions/api/usb.idl (right):

https://codereview.chromium.org/22914023/diff/4001/chrome/common/extensions/a...
chrome/common/extensions/api/usb.idl:38: // work together if the device allows
it. The device handle will be closed
I'm slightly confused by what you mean here when you say "All device handles can
work together if the device allows it". Are you referring to the situation where
a developer opens the same device twice? In that case, would they get an error
on the second attempt to open the device, or would they get back another
DeviceHandle dictionary? If they get another DeviceHandle dictionary, would it
have the same or a  different |handle| value from the first dictionary?

Or are you referring to the situation where two *different apps* both have the
same device open at the same time?

https://codereview.chromium.org/22914023/diff/4001/chrome/common/extensions/a...
chrome/common/extensions/api/usb.idl:45: // When a device handle is closed by
calling |closeDevice| or unplug the
nit: unplug -> unplugging

https://codereview.chromium.org/22914023/diff/4001/chrome/common/extensions/a...
chrome/common/extensions/api/usb.idl:47: // the flying transfers will be
canceled immediately.
nit: flying isn't quite the right word here; maybe "in progress" or "active"
would be better

https://codereview.chromium.org/22914023/diff/4001/chrome/common/extensions/a...
chrome/common/extensions/api/usb.idl:52: long? productId;
You still need to remove the '?' from vendorId and productId here.

https://codereview.chromium.org/22914023/diff/4001/chrome/common/extensions/a...
chrome/common/extensions/api/usb.idl:188: // If a device is failed to be opend,
its handles will not be returned.
nit: opend -> opened

https://codereview.chromium.org/22914023/diff/4001/chrome/common/extensions/a...
chrome/common/extensions/api/usb.idl:191: // a series of |openDevice| calls,
then collect all the successfully opened
nit: "then collect" -> "and returning"

https://codereview.chromium.org/22914023/diff/4001/chrome/common/extensions/a...
chrome/common/extensions/api/usb.idl:215: // handle of the same process, the
call will fail.
This last sentence is a little confusing. Could we just say something like:

"If the interface is already claimed by another app, the call will fail."

?

Or did you also want to convey the fact that the device could be open by some
other non-chrome program running on the OS (or even the OS itself) ?

https://codereview.chromium.org/22914023/diff/4001/chrome/common/extensions/a...
chrome/common/extensions/api/usb.idl:284: // device will appear to be
disconected then reconnected. That case you must
nit: "That case" -> "In that case"

[Bei Zhang, 2013-08-26 07:59:58]

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

https://codereview.chromium.org/22914023/diff/4001/chrome/browser/extensions/...
chrome/browser/extensions/api/usb/usb_api.cc:24: namespace ListInterfaces =
usb::ListInterfaces;
To be supportive to Google products, I used Google Docs Spreadsheet instead.

On 2013/08/23 23:57:01, Antony Sargent wrote:
> nit: 'ListInterfaces' still isn't sorted in the right place. In emacs, select
> the region and do M-x sort-lines. In vi, first exit, then run emacs, then
follow
> the emacs instructions.

https://codereview.chromium.org/22914023/diff/4001/chrome/common/extensions/a...
File chrome/common/extensions/api/usb.idl (right):

https://codereview.chromium.org/22914023/diff/4001/chrome/common/extensions/a...
chrome/common/extensions/api/usb.idl:38: // work together if the device allows
it. The device handle will be closed
On 2013/08/23 23:57:01, Antony Sargent wrote:
> I'm slightly confused by what you mean here when you say "All device handles
can
> work together if the device allows it". Are you referring to the situation
where
> a developer opens the same device twice? In that case, would they get an error
> on the second attempt to open the device, or would they get back another
> DeviceHandle dictionary? If they get another DeviceHandle dictionary, would it
> have the same or a  different |handle| value from the first dictionary?
> 
> Or are you referring to the situation where two *different apps* both have the
> same device open at the same time?

Depending on type of the device, it could be totally legit to open a device
twice. Just like you can open a file twice and read/write at different parts of
it.

The USB Spec defines that a USB device contains at least one interface.
Different types of services can be provided with those interfaces. In order to
communicate with an interface, you need to claim it first. Only one device
handle on the host can claim each interface. Whereas some devices provides only
one interface. That case you still can create multiple handle to the device, but
only one of them at a time can communicate with an interfaces (the others can
send non-interface-targeted requests though).

https://codereview.chromium.org/22914023/diff/4001/chrome/common/extensions/a...
chrome/common/extensions/api/usb.idl:45: // When a device handle is closed by
calling |closeDevice| or unplug the
On 2013/08/23 23:57:01, Antony Sargent wrote:
> nit: unplug -> unplugging

Done.

https://codereview.chromium.org/22914023/diff/4001/chrome/common/extensions/a...
chrome/common/extensions/api/usb.idl:47: // the flying transfers will be
canceled immediately.
On 2013/08/23 23:57:01, Antony Sargent wrote:
> nit: flying isn't quite the right word here; maybe "in progress" or "active"
> would be better

Done.

https://codereview.chromium.org/22914023/diff/4001/chrome/common/extensions/a...
chrome/common/extensions/api/usb.idl:52: long? productId;
On 2013/08/23 23:57:01, Antony Sargent wrote:
> You still need to remove the '?' from vendorId and productId here.

Why is that?

https://codereview.chromium.org/22914023/diff/4001/chrome/common/extensions/a...
chrome/common/extensions/api/usb.idl:188: // If a device is failed to be opend,
its handles will not be returned.
On 2013/08/23 23:57:01, Antony Sargent wrote:
> nit: opend -> opened

Done.

https://codereview.chromium.org/22914023/diff/4001/chrome/common/extensions/a...
chrome/common/extensions/api/usb.idl:191: // a series of |openDevice| calls,
then collect all the successfully opened
On 2013/08/23 23:57:01, Antony Sargent wrote:
> nit: "then collect" -> "and returning"

Done.

https://codereview.chromium.org/22914023/diff/4001/chrome/common/extensions/a...
chrome/common/extensions/api/usb.idl:215: // handle of the same process, the
call will fail.
Yes. Both non-chrome apps and the OS can claim the interface as well.

On 2013/08/23 23:57:01, Antony Sargent wrote:
> This last sentence is a little confusing. Could we just say something like:
> 
> "If the interface is already claimed by another app, the call will fail."
> 
> ?
> 
> Or did you also want to convey the fact that the device could be open by some
> other non-chrome program running on the OS (or even the OS itself) ?
> 
>

https://codereview.chromium.org/22914023/diff/4001/chrome/common/extensions/a...
chrome/common/extensions/api/usb.idl:284: // device will appear to be
disconected then reconnected. That case you must
On 2013/08/23 23:57:01, Antony Sargent wrote:
> nit: "That case" -> "In that case"

Done.

[scheib, 2013-08-26 18:27:56]

https://codereview.chromium.org/22914023/diff/80001/chrome/common/extensions/...
File chrome/common/extensions/api/usb.idl (right):

https://codereview.chromium.org/22914023/diff/80001/chrome/common/extensions/...
chrome/common/extensions/api/usb.idl:25: // but they only serve as checksums.
You can use a directionary with only
dictionary

https://codereview.chromium.org/22914023/diff/80001/chrome/common/extensions/...
chrome/common/extensions/api/usb.idl:43: // directionary with only the |handle|
field to identify the device handle.
dictionary

https://codereview.chromium.org/22914023/diff/80001/chrome/common/extensions/...
chrome/common/extensions/api/usb.idl:158: dictionary EnumerateDevicesOptions2 {
Why are two dictionaries with the same members needed?

https://codereview.chromium.org/22914023/diff/80001/chrome/common/extensions/...
chrome/common/extensions/api/usb.idl:180: // Opens a USB device retured by
|getDevices|.
returned

[Bei Zhang, 2013-08-26 20:21:31]

Thanks for the review!

Antony: Please note that I have a question about 'dictionary Device' there.

https://codereview.chromium.org/22914023/diff/80001/chrome/common/extensions/...
File chrome/common/extensions/api/usb.idl (right):

https://codereview.chromium.org/22914023/diff/80001/chrome/common/extensions/...
chrome/common/extensions/api/usb.idl:25: // but they only serve as checksums.
You can use a directionary with only
On 2013/08/26 18:27:57, scheib wrote:
> dictionary

Done.

https://codereview.chromium.org/22914023/diff/80001/chrome/common/extensions/...
chrome/common/extensions/api/usb.idl:43: // directionary with only the |handle|
field to identify the device handle.
On 2013/08/26 18:27:57, scheib wrote:
> dictionary

Done.

https://codereview.chromium.org/22914023/diff/80001/chrome/common/extensions/...
chrome/common/extensions/api/usb.idl:158: dictionary EnumerateDevicesOptions2 {
On 2013/08/26 18:27:57, scheib wrote:
> Why are two dictionaries with the same members needed?

ಠ_ಠ Who did this? I've no memory of doing it.

https://codereview.chromium.org/22914023/diff/80001/chrome/common/extensions/...
chrome/common/extensions/api/usb.idl:180: // Opens a USB device retured by
|getDevices|.
On 2013/08/26 18:27:57, scheib wrote:
> returned

Done.

[asargent_no_longer_on_chrome, 2013-08-26 22:15:19]

https://codereview.chromium.org/22914023/diff/80001/chrome/common/extensions/...
File chrome/common/extensions/api/usb.idl (right):

https://codereview.chromium.org/22914023/diff/80001/chrome/common/extensions/...
chrome/common/extensions/api/usb.idl:158: dictionary EnumerateDevicesOptions2 {
On 2013/08/26 20:21:31, Bei Zhang wrote:
> On 2013/08/26 18:27:57, scheib wrote:
> > Why are two dictionaries with the same members needed?
> 
> ಠ_ಠ Who did this? I've no memory of doing it.

Do you mean you have no memory of writing this code locally and uploading it to
the codereview?


You can see the history of a local branch by doing:

git reflog show BRANCHNAME

(Another way to find this list is to do 'git cherry -v origin/trunk')

For each of those, you can do 'git show <hash>' to see the diff.

[Bei Zhang, 2013-08-28 17:43:25]

Fixed the IDL.
Please take another look.

[asargent_no_longer_on_chrome, 2013-08-28 20:52:42]

lgtm

https://codereview.chromium.org/22914023/diff/91001/chrome/common/extensions/...
File chrome/common/extensions/api/usb.idl (right):

https://codereview.chromium.org/22914023/diff/91001/chrome/common/extensions/...
chrome/common/extensions/api/usb.idl:26: long device;
For a given device, I assume the |device| value I get from calling getDevices()
be different from the |handle| value I would get if I call findDevices
(otherwise we wouldn't bother having two dictionaries?). So it might be good to
add a comment here (or in the DeviceHandle comments below) that Device.device
and DeviceHandle.handle represent different kinds of identifiers.

[scheib, 2013-08-28 23:23:48]

I only looked at .idl; lgtm though I think DeviceHandle should be renamed.

https://codereview.chromium.org/22914023/diff/91001/chrome/common/extensions/...
File chrome/common/extensions/api/usb.idl (right):

https://codereview.chromium.org/22914023/diff/91001/chrome/common/extensions/...
chrome/common/extensions/api/usb.idl:39: dictionary DeviceHandle {
This is really a connection handle, not a device handle. Similar to our
discussion on the hid api I think you should rename this to e.g.
ConnectionHandle.

https://codereview.chromium.org/22914023/diff/91001/chrome/common/extensions/...
chrome/common/extensions/api/usb.idl:157: // Lists USB devices specified by
vendorId/productId pair.
Will the optional |interfaceId| be used? (same for findDevices)

[Bei Zhang, 2013-08-29 20:26:06]

https://codereview.chromium.org/22914023/diff/91001/chrome/common/extensions/...
File chrome/common/extensions/api/usb.idl (right):

https://codereview.chromium.org/22914023/diff/91001/chrome/common/extensions/...
chrome/common/extensions/api/usb.idl:26: long device;
On 2013/08/28 20:52:42, Antony Sargent wrote:
> For a given device, I assume the |device| value I get from calling
getDevices()
> be different from the |handle| value I would get if I call findDevices
> (otherwise we wouldn't bother having two dictionaries?). So it might be good
to
> add a comment here (or in the DeviceHandle comments below) that Device.device
> and DeviceHandle.handle represent different kinds of identifiers. 
> 
> 
> 

Done.

https://codereview.chromium.org/22914023/diff/91001/chrome/common/extensions/...
chrome/common/extensions/api/usb.idl:39: dictionary DeviceHandle {
On 2013/08/28 23:23:48, scheib wrote:
> This is really a connection handle, not a device handle. Similar to our
> discussion on the hid api I think you should rename this to e.g.
> ConnectionHandle.

"Device handle" is a well-known term in libusb to describe a connection to a USB
device.

https://codereview.chromium.org/22914023/diff/91001/chrome/common/extensions/...
chrome/common/extensions/api/usb.idl:157: // Lists USB devices specified by
vendorId/productId pair.
On 2013/08/28 23:23:48, scheib wrote:
> Will the optional |interfaceId| be used? (same for findDevices)

Done.

[scheib, 2013-08-29 20:47:58]

https://codereview.chromium.org/22914023/diff/105001/chrome/common/extensions...
File chrome/common/extensions/api/usb.idl (right):

https://codereview.chromium.org/22914023/diff/105001/chrome/common/extensions...
chrome/common/extensions/api/usb.idl:40: // The id of the USB device handle for.
It represents different kinds of
This rewording sounds wrong. Delete 'for'?

[Bei Zhang, 2013-08-29 21:02:05]

https://codereview.chromium.org/22914023/diff/105001/chrome/common/extensions...
File chrome/common/extensions/api/usb.idl (right):

https://codereview.chromium.org/22914023/diff/105001/chrome/common/extensions...
chrome/common/extensions/api/usb.idl:40: // The id of the USB device handle for.
It represents different kinds of
On 2013/08/29 20:47:58, scheib wrote:
> This rewording sounds wrong. Delete 'for'?

I gave up describing the difference between the two ids. I believe you would
always need a minimum amount of domain  knowledge to use this API anyway.

[Bei Zhang, 2013-08-31 00:14:51]

Added chrome.usb.requestAccess.

[Bei Zhang, 2013-09-01 07:25:06]

Hi Zelidrag,

Please review. Please also take a look a the API proposal.

Thanks,
Bei

[Bei Zhang, 2013-09-01 07:25:37]

Hi Pavel,

Please review.

Thanks,
Bei

[zel, 2013-09-03 17:01:41]

https://codereview.chromium.org/22914023/diff/140001/chrome/common/extensions...
File chrome/common/extensions/api/usb.idl (right):

https://codereview.chromium.org/22914023/diff/140001/chrome/common/extensions...
chrome/common/extensions/api/usb.idl:29: // ChromeOS only. The id of interface
that your app wants to claim.
Why is this ChromeOS only? Why wouldn't we force targeting only of interfaces
that are unclaimed by the OS on other platforms as well?

https://codereview.chromium.org/22914023/diff/140001/chrome/common/extensions...
chrome/common/extensions/api/usb.idl:159: // Only available on ChromeOS. It has
no effect on other platforms.
same as above

[Bei Zhang, 2013-09-03 21:27:16]

https://codereview.chromium.org/22914023/diff/140001/chrome/common/extensions...
File chrome/common/extensions/api/usb.idl (right):

https://codereview.chromium.org/22914023/diff/140001/chrome/common/extensions...
chrome/common/extensions/api/usb.idl:29: // ChromeOS only. The id of interface
that your app wants to claim.
On 2013/09/03 17:01:41, zel wrote:
> Why is this ChromeOS only? Why wouldn't we force targeting only of interfaces
> that are unclaimed by the OS on other platforms as well?

This field is mistaken placed here. Removed.

https://codereview.chromium.org/22914023/diff/140001/chrome/common/extensions...
chrome/common/extensions/api/usb.idl:159: // Only available on ChromeOS. It has
no effect on other platforms.
On 2013/09/03 17:01:41, zel wrote:
> same as above


Created a enhancement to the docs in this CL:
https://codereview.chromium.org/23549014/

[pfeldman, 2013-09-04 07:16:54]

https://codereview.chromium.org/22914023/diff/158001/chrome/browser/usb/usb_s...
File chrome/browser/usb/usb_service.h (right):

https://codereview.chromium.org/22914023/diff/158001/chrome/browser/usb/usb_s...
chrome/browser/usb/usb_service.h:42: scoped_refptr<UsbDevice>
GetDeviceById(uint32 unique_id);
What is this id? How does user obtain one? Ref-counted objects are typically
using instances for identity. It sounds like the only way to obtain this id is
to ask for it from the device. I.e. the caller has had the device reference. If
that is for client-side binding, then this mapping belongs to the client.

[Bei Zhang, 2013-09-04 17:53:10]

On 2013/09/04 07:16:54, pfeldman wrote:
>
https://codereview.chromium.org/22914023/diff/158001/chrome/browser/usb/usb_s...
> File chrome/browser/usb/usb_service.h (right):
> 
>
https://codereview.chromium.org/22914023/diff/158001/chrome/browser/usb/usb_s...
> chrome/browser/usb/usb_service.h:42: scoped_refptr<UsbDevice>
> GetDeviceById(uint32 unique_id);
> What is this id? How does user obtain one? Ref-counted objects are typically
> using instances for identity. It sounds like the only way to obtain this id is
> to ask for it from the device. I.e. the caller has had the device reference.
If
> that is for client-side binding, then this mapping belongs to the client.

I will address this in another CL.

[pfeldman, 2013-09-04 18:00:16]

Please add TODO on id removal. chrome/browser/usb lgtm

[commit-bot: I haz the power, 2013-09-04 18:52:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ikarienator@chromium.org/22914023/164001

[commit-bot: I haz the power, 2013-09-04 21:28:37]

Retried try job too often on win_rel for step(s) browser_tests,
chrome_frame_net_tests, chrome_frame_tests, chrome_frame_unittests,
content_browsertests, mini_installer_test, nacl_integration,
sync_integration_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-09-04 21:31:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ikarienator@chromium.org/22914023/190001

[commit-bot: I haz the power, 2013-09-05 01:51:29]

Change committed as 221328