Experimental USB API

Experimental USB API

This change adds a new API for low-level access to USB devices. The change
depends on https://chromiumcodereview.appspot.com/10161035/

BUG=none
TEST=none

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=134423

[Garret Kelly, 2012-04-25 21:09:09]

Hi Mike and Antony,
I've split out stuff that needs review by other people into
https://chromiumcodereview.appspot.com/10161035/, so this change won't build on
its own. I know it looks daunting, but a lot of the files are just 1-chunk
changes.

If you're looking for a change that has everything all together and builds it's
here: https://chromiumcodereview.appspot.com/10154008/

Garret

[miket_OOO, 2012-04-25 23:10:37]

Almost looks good, but you seem to be missing the feature configuration that
restricts this API to platform apps. There is a recent example here:
http://codereview.chromium.org/9839105/

https://chromiumcodereview.appspot.com/10224009/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/api/api_resource_event_notifier.h (right):

https://chromiumcodereview.appspot.com/10224009/diff/1/chrome/browser/extensi...
chrome/browser/extensions/api/api_resource_event_notifier.h:70: void
DispatchEvent(const std::string &extension, DictionaryValue* event);
Placement of reference/pointers vs. whitespace (et seq.).

https://chromiumcodereview.appspot.com/10224009/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/api/usb/usb_api.cc (right):

https://chromiumcodereview.appspot.com/10224009/diff/1/chrome/browser/extensi...
chrome/browser/extensions/api/usb/usb_api.cc:31: const int source_id =
ExtractSrcId(2);
This temporary variable is a little confusing because of the const. Can we kill
two birds with one stone and just get rid of the variable?

https://chromiumcodereview.appspot.com/10224009/diff/1/chrome/browser/extensi...
chrome/browser/extensions/api/usb/usb_api.cc:41: UsbDevice *const device =
service->FindDevice(parameters_->vendor_id,
Reminder on */& whitespace.

https://chromiumcodereview.appspot.com/10224009/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/api/usb/usb_device_resource.h (right):

https://chromiumcodereview.appspot.com/10224009/diff/1/chrome/browser/extensi...
chrome/browser/extensions/api/usb/usb_device_resource.h:37: // methods with
completion callbacks that call OnTransferComplete on the vent
event

https://chromiumcodereview.appspot.com/10224009/diff/1/chrome/common/extensio...
File chrome/common/extensions/api/experimental.usb.idl (right):

https://chromiumcodereview.appspot.com/10224009/diff/1/chrome/common/extensio...
chrome/common/extensions/api/experimental.usb.idl:24: DOMString direction;
A little vertical whitespace would be helpful for readability.

https://chromiumcodereview.appspot.com/10224009/diff/1/chrome/common/extensio...
chrome/common/extensions/api/experimental.usb.idl:58: // trasnsfer, including
returned data.
spelling

https://chromiumcodereview.appspot.com/10224009/diff/1/chrome/common/extensio...
chrome/common/extensions/api/experimental.usb.idl:63: // A value of 1 indicates
that the transfer was a success. Other values
This feels nonstandard. Could zero indicate success instead, so that a boolean
test can distinguish success from failure? Push back if I'm misunderstanding
your model.

https://chromiumcodereview.appspot.com/10224009/diff/1/chrome/common/extensio...
chrome/common/extensions/api/experimental.usb.idl:64: // indicate faliure.
spelling

https://chromiumcodereview.appspot.com/10224009/diff/1/chrome/common/extensio...
chrome/common/extensions/api/experimental.usb.idl:82: // Invoked by the
extension API whenever an event occurs the device(s)
occurs for? with?

[Garret Kelly, 2012-04-26 20:13:44]

Done! PTAL.

https://chromiumcodereview.appspot.com/10224009/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/api/api_resource_event_notifier.h (right):

https://chromiumcodereview.appspot.com/10224009/diff/1/chrome/browser/extensi...
chrome/browser/extensions/api/api_resource_event_notifier.h:70: void
DispatchEvent(const std::string &extension, DictionaryValue* event);
On 2012/04/25 23:10:37, miket wrote:
> Placement of reference/pointers vs. whitespace (et seq.).

Done.

https://chromiumcodereview.appspot.com/10224009/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/api/usb/usb_api.cc (right):

https://chromiumcodereview.appspot.com/10224009/diff/1/chrome/browser/extensi...
chrome/browser/extensions/api/usb/usb_api.cc:31: const int source_id =
ExtractSrcId(2);
On 2012/04/25 23:10:37, miket wrote:
> This temporary variable is a little confusing because of the const. Can we
kill
> two birds with one stone and just get rid of the variable?

Done.

https://chromiumcodereview.appspot.com/10224009/diff/1/chrome/browser/extensi...
chrome/browser/extensions/api/usb/usb_api.cc:41: UsbDevice *const device =
service->FindDevice(parameters_->vendor_id,
On 2012/04/25 23:10:37, miket wrote:
> Reminder on */& whitespace.

Done.

https://chromiumcodereview.appspot.com/10224009/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/api/usb/usb_device_resource.h (right):

https://chromiumcodereview.appspot.com/10224009/diff/1/chrome/browser/extensi...
chrome/browser/extensions/api/usb/usb_device_resource.h:37: // methods with
completion callbacks that call OnTransferComplete on the vent
On 2012/04/25 23:10:37, miket wrote:
> event

Done.

https://chromiumcodereview.appspot.com/10224009/diff/1/chrome/common/extensio...
File chrome/common/extensions/api/experimental.usb.idl (right):

https://chromiumcodereview.appspot.com/10224009/diff/1/chrome/common/extensio...
chrome/common/extensions/api/experimental.usb.idl:24: DOMString direction;
On 2012/04/25 23:10:37, miket wrote:
> A little vertical whitespace would be helpful for readability.

Done.

https://chromiumcodereview.appspot.com/10224009/diff/1/chrome/common/extensio...
chrome/common/extensions/api/experimental.usb.idl:58: // trasnsfer, including
returned data.
On 2012/04/25 23:10:37, miket wrote:
> spelling

Done.

https://chromiumcodereview.appspot.com/10224009/diff/1/chrome/common/extensio...
chrome/common/extensions/api/experimental.usb.idl:63: // A value of 1 indicates
that the transfer was a success. Other values
On 2012/04/25 23:10:37, miket wrote:
> This feels nonstandard. Could zero indicate success instead, so that a boolean
> test can distinguish success from failure? Push back if I'm misunderstanding
> your model.

Done.

https://chromiumcodereview.appspot.com/10224009/diff/1/chrome/common/extensio...
chrome/common/extensions/api/experimental.usb.idl:64: // indicate faliure.
On 2012/04/25 23:10:37, miket wrote:
> spelling

Done.

https://chromiumcodereview.appspot.com/10224009/diff/1/chrome/common/extensio...
chrome/common/extensions/api/experimental.usb.idl:82: // Invoked by the
extension API whenever an event occurs the device(s)
On 2012/04/25 23:10:37, miket wrote:
> occurs for? with?

Done.

[miket_OOO, 2012-04-26 20:24:05]

Yarr, that be a fine change. LGTM.

[Garret Kelly, 2012-04-26 20:31:21]

Thank ye fer' yer' review, cap'n! Will cq+ as soon the usb_service/device change
lands.

[asargent_no_longer_on_chrome, 2012-04-26 20:32:41]

I have a few style nits - will try and complete review soon...

On Thu, Apr 26, 2012 at 1:31 PM, <gdk@chromium.org> wrote:

> Thank ye fer' yer' review, cap'n! Will cq+ as soon the usb_service/device
> change
> lands.
>
>
https://chromiumcodereview.**appspot.com/10224009/<https://chromiumcodereview...
>

[asargent_no_longer_on_chrome, 2012-04-26 21:21:45]

lgtm with some nits to address before checking in.

Also, can you file a follow-up bug to add some tests? We'll want to block the
launch bug for the moving the USB API out of experimental on it.

https://chromiumcodereview.appspot.com/10224009/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/api/api_resource_event_notifier.cc (right):

https://chromiumcodereview.appspot.com/10224009/diff/1/chrome/browser/extensi...
chrome/browser/extensions/api/api_resource_event_notifier.cc:24: 
nit: I have a slight preference to not have this extra blank line here. If you
feel strongly about it no big deal.

https://chromiumcodereview.appspot.com/10224009/diff/1/chrome/browser/extensi...
chrome/browser/extensions/api/api_resource_event_notifier.cc:29: const char
kEventTypeTransferComplete[] = "transferComplete";
how about moving this up 1 line to be directly underneath
kEventTypeWriteComplete?

https://chromiumcodereview.appspot.com/10224009/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/api/api_resource_event_notifier.h (right):

https://chromiumcodereview.appspot.com/10224009/diff/1/chrome/browser/extensi...
chrome/browser/extensions/api/api_resource_event_notifier.h:70: void
DispatchEvent(const std::string &extension, DictionaryValue* event);
nit: instead of 'extension', let's call this parameter something like
'event_name'. (When I see a std::string variable named extension, I wonder if
it's an extension id or something). Same thing for the DispatchEventOnUIThread
and SendEventWithResultCode functions.

https://chromiumcodereview.appspot.com/10224009/diff/1/chrome/common/extensio...
File chrome/common/extensions/api/experimental.usb.idl (right):

https://chromiumcodereview.appspot.com/10224009/diff/1/chrome/common/extensio...
chrome/common/extensions/api/experimental.usb.idl:56: // When a USB event occurs
the event handler specified in findDevice will
Can you clarify here that it's the onEvent property of the DeviceOptions
parameter to findDevice? (I was initially confused that it might be the
FindDeviceCallback)

https://chromiumcodereview.appspot.com/10224009/diff/1/chrome/common/extensio...
chrome/common/extensions/api/experimental.usb.idl:94: // the Device object.
comment nit: The FindDeviceCallback should fire on either success or failure -
it's just that the device is null in the failure case.

https://chromiumcodereview.appspot.com/10224009/diff/1/chrome/common/extensio...
chrome/common/extensions/api/experimental.usb.idl:122: // Performs a control
transfer on the specified device.
nit: Should this say "Performs an interrupt transfer ..." ?

https://chromiumcodereview.appspot.com/10224009/diff/6001/chrome/browser/exte...
File chrome/browser/extensions/api/usb/usb_device_resource.h (right):

https://chromiumcodereview.appspot.com/10224009/diff/6001/chrome/browser/exte...
chrome/browser/extensions/api/usb/usb_device_resource.h:55: base::Lock lock_;
Danger Will Robinson! We almost never want to do explicit locking. I will
comment more in the .cc file where you're using this....

...ok, looks like you don't actually use it, so you can just remove.

We usually prefer to just do operations to a piece of state on a given thread
(UI,FILE, etc.), possibly using PostTask to transfer ownership over to another
thread to do some work and then having a control flow there that eventually
PostTask's back to the original thread. The most common pattern is to start on
the UI thread, post a task to do some work on the FILE thread, and then post a
task back to the UI thread.

https://chromiumcodereview.appspot.com/10224009/diff/6001/chrome/browser/exte...
chrome/browser/extensions/api/usb/usb_device_resource.h:58:
DISALLOW_EVIL_CONSTRUCTORS(UsbDeviceResource);
nit: I think the synonym DISALLOW_COPY_AND_ASSIGN is the preferred macro these
days

https://chromiumcodereview.appspot.com/10224009/diff/6001/chrome/renderer/ext...
File chrome/renderer/extensions/experimental.usb_custom_bindings.cc (right):

https://chromiumcodereview.appspot.com/10224009/diff/6001/chrome/renderer/ext...
chrome/renderer/extensions/experimental.usb_custom_bindings.cc:14: return
v8::Integer::New(next_event_id++);
I notice this is essentially the same as the socket code's id generation. FYI,
I've filed crbug.com/25237 to add some common infrastructure so this isn't
needed.

https://chromiumcodereview.appspot.com/10224009/diff/6001/chrome/renderer/res...
File chrome/renderer/resources/extensions/experimental.usb_custom_bindings.js
(right):

https://chromiumcodereview.appspot.com/10224009/diff/6001/chrome/renderer/res...
chrome/renderer/resources/extensions/experimental.usb_custom_bindings.js:7: //
bindings out of a per-extension mechanism into a generalized mechanism.
Oh, I see you've got the same idea. =)

[Garret Kelly, 2012-04-27 00:32:34]

asargent: Addressed all review points. PTAL.

http://codereview.chromium.org/10224009/diff/1/chrome/browser/extensions/api/...
File chrome/browser/extensions/api/api_resource_event_notifier.cc (right):

http://codereview.chromium.org/10224009/diff/1/chrome/browser/extensions/api/...
chrome/browser/extensions/api/api_resource_event_notifier.cc:29: const char
kEventTypeTransferComplete[] = "transferComplete";
> how about moving this up 1 line to be directly underneath
> kEventTypeWriteComplete?

I had separated the groups based on the extension they were intended for.

http://codereview.chromium.org/10224009/diff/1/chrome/browser/extensions/api/...
File chrome/browser/extensions/api/api_resource_event_notifier.h (right):

http://codereview.chromium.org/10224009/diff/1/chrome/browser/extensions/api/...
chrome/browser/extensions/api/api_resource_event_notifier.h:70: void
DispatchEvent(const std::string &extension, DictionaryValue* event);
On 2012/04/26 21:21:45, Antony Sargent wrote:
> nit: instead of 'extension', let's call this parameter something like
> 'event_name'. (When I see a std::string variable named extension, I wonder if
> it's an extension id or something). Same thing for the DispatchEventOnUIThread
> and SendEventWithResultCode functions.

What about event_handler? The string describes the js object that will receive
and handle the message, so that seems fitting.

http://codereview.chromium.org/10224009/diff/1/chrome/common/extensions/api/e...
File chrome/common/extensions/api/experimental.usb.idl (right):

http://codereview.chromium.org/10224009/diff/1/chrome/common/extensions/api/e...
chrome/common/extensions/api/experimental.usb.idl:56: // When a USB event occurs
the event handler specified in findDevice will
On 2012/04/26 21:21:45, Antony Sargent wrote:
> Can you clarify here that it's the onEvent property of the DeviceOptions
> parameter to findDevice? (I was initially confused that it might be the
> FindDeviceCallback)
> 

Done.

http://codereview.chromium.org/10224009/diff/1/chrome/common/extensions/api/e...
chrome/common/extensions/api/experimental.usb.idl:94: // the Device object.
On 2012/04/26 21:21:45, Antony Sargent wrote:
> comment nit: The FindDeviceCallback should fire on either success or failure -
> it's just that the device is null in the failure case.

Quite right. It actually does, and this comment is incorrect. Fixed!

http://codereview.chromium.org/10224009/diff/1/chrome/common/extensions/api/e...
chrome/common/extensions/api/experimental.usb.idl:122: // Performs a control
transfer on the specified device.
On 2012/04/26 21:21:45, Antony Sargent wrote:
> nit: Should this say "Performs an interrupt transfer ..." ?

D'oh! Done.

http://codereview.chromium.org/10224009/diff/6001/chrome/browser/extensions/a...
File chrome/browser/extensions/api/usb/usb_device_resource.h (right):

http://codereview.chromium.org/10224009/diff/6001/chrome/browser/extensions/a...
chrome/browser/extensions/api/usb/usb_device_resource.h:55: base::Lock lock_;
On 2012/04/26 21:21:45, Antony Sargent wrote:
> Danger Will Robinson! We almost never want to do explicit locking. I will
> comment more in the .cc file where you're using this....
> 
> ...ok, looks like you don't actually use it, so you can just remove.
> 
> We usually prefer to just do operations to a piece of state on a given thread
> (UI,FILE, etc.), possibly using PostTask to transfer ownership over to another
> thread to do some work and then having a control flow there that eventually
> PostTask's back to the original thread. The most common pattern is to start on
> the UI thread, post a task to do some work on the FILE thread, and then post a
> task back to the UI thread.

Hmm, then perhaps I need to do a bit more thinking about the design of the
UsbService going forward. It has its own thread that drives event handling
(thanks to Windows, otherwise this would all live as a set of watched fds in the
IO thread), so finishing a transfer will be performed there. Since I'm not doing
in-flight transfer tracking in this CL I'll just remove it for now and work on
it later.

http://codereview.chromium.org/10224009/diff/6001/chrome/browser/extensions/a...
chrome/browser/extensions/api/usb/usb_device_resource.h:58:
DISALLOW_EVIL_CONSTRUCTORS(UsbDeviceResource);
On 2012/04/26 21:21:45, Antony Sargent wrote:
> nit: I think the synonym DISALLOW_COPY_AND_ASSIGN is the preferred macro these
> days

Oh yeah, I'd forgotten about that. Done.

[asargent_no_longer_on_chrome, 2012-04-27 03:39:17]

LGTM. Feel free to ping me if you need some guidance on how to handle the
threading and mutual exclusion issues.

[commit-bot: I haz the power, 2012-04-27 16:57:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gdk@chromium.org/10224009/21001

[commit-bot: I haz the power, 2012-04-27 17:18:04]

Try job failure for 10224009-21001 (retry) on linux_chromeos for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2012-04-27 17:50:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gdk@chromium.org/10224009/34002

[commit-bot: I haz the power, 2012-04-27 19:12:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gdk@chromium.org/10224009/41002

[commit-bot: I haz the power, 2012-04-27 20:08:12]

Try job failure for 10224009-41002 (retry) on mac_rel for step "unit_tests".
It's a second try, previously, steps "unit_tests, check_deps" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-04-27 20:21:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gdk@chromium.org/10224009/36005

[commit-bot: I haz the power, 2012-04-27 22:50:09]

Can't apply patch for file
chrome/browser/extensions/api/api_resource_event_notifier.cc.
While running patch -p1 --forward --force;
patching file chrome/browser/extensions/api/api_resource_event_notifier.cc
Hunk #2 succeeded at 50 with fuzz 1 (offset -1 lines).
Hunk #3 FAILED at 74.
Hunk #4 FAILED at 89.
Hunk #5 succeeded at 114 (offset 6 lines).
Hunk #6 FAILED at 137.
3 out of 6 hunks FAILED -- saving rejects to file
chrome/browser/extensions/api/api_resource_event_notifier.cc.rej

[commit-bot: I haz the power, 2012-04-27 23:52:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gdk@chromium.org/10224009/33005

[commit-bot: I haz the power, 2012-04-28 02:13:12]

Change committed as 134423

[miket_OOO, 2012-04-28 04:25:41]

On 2012/04/28 02:13:12, I haz the power (commit-bot) wrote:
> Change committed as 134423

THREE CHEERS FOR THE CQ!

[Evan Stade, 2012-06-12 18:09:48]

http://codereview.chromium.org/10224009/diff/33005/chrome/common/extensions/e...
File chrome/common/extensions/extension_permission_set.cc (right):

http://codereview.chromium.org/10224009/diff/33005/chrome/common/extensions/e...
chrome/common/extensions/extension_permission_set.cc:266:
ExtensionPermissionMessage::kNone, kFlagNone);
why is this ExtensionPermissionMessage::kNone?

[Garret Kelly, 2012-06-12 18:13:01]

> why is this ExtensionPermissionMessage::kNone?

The USB and Bluetooth permission model is still under development.
http://code.google.com/p/chromium/issues/detail?id=127302