Adding UsbService and UsbDevice constructs.

Adding UsbService and UsbDevice constructs

UsbService and UsbDevice allow for platform-independent access to the system's
USB subsystem.

BUG=none
TEST=none


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

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

[sail, 2012-04-25 21:06:34]

https://chromiumcodereview.appspot.com/10161035/diff/1/chrome/browser/profile...
File chrome/browser/profiles/profile.h (right):

https://chromiumcodereview.appspot.com/10161035/diff/1/chrome/browser/profile...
chrome/browser/profiles/profile.h:399: #if !defined(OS_ANDROID)
it's a bad idea to have #ifdefs in code. The general approach is to define this
on all platform and implement a stub for android.

If this is not practical then another approach would be to move this ifdef to
only the profile_impl.cc file. This ways callers can do something like this:
  service = profile()->GetUsbService();
  // Service may not exists on some platforms.
  if (!service)
    return;

[Elliot Glaysher, 2012-04-25 21:10:19]

https://chromiumcodereview.appspot.com/10161035/diff/1/chrome/browser/profile...
File chrome/browser/profiles/profile_impl.h (right):

https://chromiumcodereview.appspot.com/10161035/diff/1/chrome/browser/profile...
chrome/browser/profiles/profile_impl.h:228: scoped_ptr<UsbService> usb_service_;
NACK.

This exactly the sort of thing we're trying to avoid.
https://sites.google.com/a/chromium.org/dev/developers/design-documents/profi...

https://chromiumcodereview.appspot.com/10161035/diff/1/chrome/browser/usb/usb...
File chrome/browser/usb/usb_service.h (right):

https://chromiumcodereview.appspot.com/10161035/diff/1/chrome/browser/usb/usb...
chrome/browser/usb/usb_service.h:23: class UsbService {
Is it safe if there are multiple UsbServices? When you make something attached
to the Profile, you assert that you can have one per open profile.

[Garret Kelly, 2012-04-25 21:15:17]

https://chromiumcodereview.appspot.com/10161035/diff/1/chrome/browser/profile...
File chrome/browser/profiles/profile_impl.h (right):

https://chromiumcodereview.appspot.com/10161035/diff/1/chrome/browser/profile...
chrome/browser/profiles/profile_impl.h:228: scoped_ptr<UsbService> usb_service_;
Oops! My mistake. Thank you for the pointer. Fixing now.

[miket_OOO, 2012-04-25 21:24:36]

LGTM, mostly nit comments. But I agree with Elliot's point about Profile as a
dumping ground. Yoz on extensions did at least some of this refactoring so if
you're looking for a better home for your stuff, he might be good to talk to.

https://chromiumcodereview.appspot.com/10161035/diff/1/chrome/browser/profile...
File chrome/browser/profiles/profile.h (right):

https://chromiumcodereview.appspot.com/10161035/diff/1/chrome/browser/profile...
chrome/browser/profiles/profile.h:400: // Returns or creates and returns an
instance of the UsbService.
This is a minor point, but the "or creates and returns" feels like an
implementation detail that the caller shouldn't care about. Isn't "Returns..."
enough?

https://chromiumcodereview.appspot.com/10161035/diff/1/chrome/browser/profile...
File chrome/browser/profiles/profile_impl.cc (right):

https://chromiumcodereview.appspot.com/10161035/diff/1/chrome/browser/profile...
chrome/browser/profiles/profile_impl.cc:103: #endif  // !defined(OS_ANDROID)
This include approximately duplicates the one in the .h.

https://chromiumcodereview.appspot.com/10161035/diff/1/chrome/browser/profile...
chrome/browser/profiles/profile_impl.cc:538: usb_service_.reset(NULL);
Might as well put the reset inside the block. A few dozen nanoseconds faster on
Android.

https://chromiumcodereview.appspot.com/10161035/diff/1/chrome/browser/profile...
File chrome/browser/profiles/profile_impl.h (right):

https://chromiumcodereview.appspot.com/10161035/diff/1/chrome/browser/profile...
chrome/browser/profiles/profile_impl.h:228: scoped_ptr<UsbService> usb_service_;
Shouldn't this (and the include) be guarded with the OS_ANDROID constant, too?
Or are you OK with the pointer being there but not accessible? (I'm OK either
way, just want to make sure it's not a mistake.)

https://chromiumcodereview.appspot.com/10161035/diff/1/chrome/browser/usb/usb...
File chrome/browser/usb/usb_device.cc (right):

https://chromiumcodereview.appspot.com/10161035/diff/1/chrome/browser/usb/usb...
chrome/browser/usb/usb_device.cc:137: reinterpret_cast<unsigned char
*>(buffer->data()), length,
uint8 is a more Chrome-idiomatic type than unsigned char.

https://chromiumcodereview.appspot.com/10161035/diff/1/chrome/browser/usb/usb...
File chrome/browser/usb/usb_device.h (right):

https://chromiumcodereview.appspot.com/10161035/diff/1/chrome/browser/usb/usb...
chrome/browser/usb/usb_device.h:19: typedef libusb_transfer
*PlatformUsbTransferHandle;
style: whitespace should be after *.

https://chromiumcodereview.appspot.com/10161035/diff/1/chrome/browser/usb/usb...
chrome/browser/usb/usb_device.h:36: // Usually you will not want to directly
create a UsbDevice, favouring to let
"favouring": I'll let this spelling slide in light of your provenance.

https://chromiumcodereview.appspot.com/10161035/diff/1/chrome/browser/usb/usb...
chrome/browser/usb/usb_device.h:52: net::IOBuffer *buffer,
Pointer placement (et seq.)

https://chromiumcodereview.appspot.com/10161035/diff/1/chrome/browser/usb/usb...
chrome/browser/usb/usb_device.h:96: UsbService *const service_;
It's nice to leave a comment explaining why a naked pointer can't be protected
with a RAII device.

https://chromiumcodereview.appspot.com/10161035/diff/1/chrome/browser/usb/usb...
File chrome/browser/usb/usb_service.h (right):

https://chromiumcodereview.appspot.com/10161035/diff/1/chrome/browser/usb/usb...
chrome/browser/usb/usb_service.h:35: UsbDevice *FindDevice(const uint16
vendor_id, const uint16 product_id);
Why const a primitive parameter? (not entirely a rhetorical question; I believe
it's a no-op, but happy to learn something.)

[Garret Kelly, 2012-04-26 02:05:15]

> chrome/browser/usb/usb_service.h:23: class UsbService {
> Is it safe if there are multiple UsbServices? When you make something attached
> to the Profile, you assert that you can have one per open profile.

Yep, the code is written with this specifically in mind. Each new UsbService
acquires its own platform context in order to isolate operations in one profile
from another. It would be possible to probe certain parts of other USB
operations in another profile if they shared the same UsbService.

[Garret Kelly, 2012-04-26 02:21:42]

https://chromiumcodereview.appspot.com/10161035/diff/1/chrome/browser/usb/usb...
File chrome/browser/usb/usb_device.cc (right):

https://chromiumcodereview.appspot.com/10161035/diff/1/chrome/browser/usb/usb...
chrome/browser/usb/usb_device.cc:137: reinterpret_cast<unsigned char
*>(buffer->data()), length,
On 2012/04/25 21:24:36, miket wrote:
> uint8 is a more Chrome-idiomatic type than unsigned char.

Done. I'd done it just to match the signatures of the transfer functions.

https://chromiumcodereview.appspot.com/10161035/diff/1/chrome/browser/usb/usb...
File chrome/browser/usb/usb_device.h (right):

https://chromiumcodereview.appspot.com/10161035/diff/1/chrome/browser/usb/usb...
chrome/browser/usb/usb_device.h:19: typedef libusb_transfer
*PlatformUsbTransferHandle;
On 2012/04/25 21:24:36, miket wrote:
> style: whitespace should be after *.

Done.

https://chromiumcodereview.appspot.com/10161035/diff/1/chrome/browser/usb/usb...
chrome/browser/usb/usb_device.h:36: // Usually you will not want to directly
create a UsbDevice, favouring to let
> "favouring": I'll let this spelling slide in light of your provenance.

gdk@determinist:~/sandbox/chrome/src$ git grep colour | wc -l
122
gdk@determinist:~/sandbox/chrome/src$ git grep color | wc -l
17708

Changed to en_US locale. :)

https://chromiumcodereview.appspot.com/10161035/diff/1/chrome/browser/usb/usb...
chrome/browser/usb/usb_device.h:52: net::IOBuffer *buffer,
On 2012/04/25 21:24:36, miket wrote:
> Pointer placement (et seq.)

Done.

https://chromiumcodereview.appspot.com/10161035/diff/1/chrome/browser/usb/usb...
chrome/browser/usb/usb_device.h:96: UsbService *const service_;
On 2012/04/25 21:24:36, miket wrote:
> It's nice to leave a comment explaining why a naked pointer can't be protected
> with a RAII device.

Done.

https://chromiumcodereview.appspot.com/10161035/diff/1/chrome/browser/usb/usb...
File chrome/browser/usb/usb_service.h (right):

https://chromiumcodereview.appspot.com/10161035/diff/1/chrome/browser/usb/usb...
chrome/browser/usb/usb_service.h:35: UsbDevice *FindDevice(const uint16
vendor_id, const uint16 product_id);
> Why const a primitive parameter? (not entirely a rhetorical question; I
believe
> it's a no-op, but happy to learn something.)

My justification's a little weak. :)

I'm in the habit of const'ing everything in sight, especially in the case of
function parameters. If a function is called to deal with a specific set of
inputs I find it easier to think about the inputs as being fixed for the
duration of the call.

[Garret Kelly, 2012-04-26 02:24:16]

I've updated this CL per the review comments, would you kind reviewerfolk mind
PTAL?

[sail, 2012-04-26 04:35:43]

https://chromiumcodereview.appspot.com/10161035/diff/16001/chrome/chrome_brow...
File chrome/chrome_browser.gypi (right):

https://chromiumcodereview.appspot.com/10161035/diff/16001/chrome/chrome_brow...
chrome/chrome_browser.gypi:4509: 'browser/usb/usb_device.h',
can you just add browser/usb to the "sources/" list?

[Garret Kelly, 2012-04-26 15:10:16]

https://chromiumcodereview.appspot.com/10161035/diff/16001/chrome/chrome_brow...
File chrome/chrome_browser.gypi (right):

https://chromiumcodereview.appspot.com/10161035/diff/16001/chrome/chrome_brow...
chrome/chrome_browser.gypi:4509: 'browser/usb/usb_device.h',
On 2012/04/26 04:35:44, sail wrote:
> can you just add browser/usb to the "sources/" list?

Done.

[miket_OOO, 2012-04-26 16:33:46]

> > Why const a primitive parameter? (not entirely a rhetorical question; I
> believe
> > it's a no-op, but happy to learn something.)
> 
> My justification's a little weak. :)
> 
> I'm in the habit of const'ing everything in sight, especially in the case of
> function parameters. If a function is called to deal with a specific set of
> inputs I find it easier to think about the inputs as being fixed for the
> duration of the call.

I hear you; it's the same appeal as final in Java. The issue I've run into at
Google, or perhaps working on any large team, is that this kind of usage is more
distracting than helpful to the team at large UNLESS it's pervasive. (To be
clear, I'm talking about const as a static-analysis aid in a local block of
code, rather than const a contract in an API to foreign callers.) If the usage
is not pervasive, then it causes other people reading the code to wonder why a
certain section suddenly has consts while the rest didn't.

Otherwise LGTM.

[Garret Kelly, 2012-04-26 17:51:32]

sail, erg: Would you mind giving this CL a final look?

[Garret Kelly, 2012-04-26 17:51:59]

On 2012/04/26 17:51:32, Garret Kelly wrote:
> sail, erg: Would you mind giving this CL a final look?

(I ask because I need an OWNERS LGTM from one of you :D)

[sail, 2012-04-26 17:57:42]

On 2012/04/26 17:51:59, Garret Kelly wrote:
> On 2012/04/26 17:51:32, Garret Kelly wrote:
> > sail, erg: Would you mind giving this CL a final look?
> 
> (I ask because I need an OWNERS LGTM from one of you :D)

I'll let erg take over since the only relevant aspect is the use of Profile
keyed service.

[commit-bot: I haz the power, 2012-04-27 00:47:32]

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

[commit-bot: I haz the power, 2012-04-27 02:34:10]

Change committed as 134216

[commit-bot: I haz the power, 2012-04-27 14:36:47]

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

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

Change committed as 134278

[Marius, 2013-01-31 21:32:06]

https://codereview.chromium.org/10161035/diff/37001/chrome/browser/usb/usb_de...
File chrome/browser/usb/usb_device.cc (right):

https://codereview.chromium.org/10161035/diff/37001/chrome/browser/usb/usb_de...
chrome/browser/usb/usb_device.cc:64: static void HandleTransferCompletion(struct
libusb_transfer* transfer) {
static void LIBUSB_CALL HandleTransferCompletion(..) {
to match expected calling convention on Windows libusb.