Move string descriptor getters from UsbDeviceHandle to UsbDevice.

Move string descriptor getters from UsbDeviceHandle to UsbDevice.

The common string descriptors: iManufacturer, iProduct and iSerialNumber
should be accessible without opening the device. On Linux these can be
read out of sysfs without having access to the usbfs device node. This
is critical on Chrome OS because otherwise the permission broker needs
to be asked for permission to access the device. On other platforms we
fall back to opening the device temporarily. This will stop being the
case on OS X when libusb is no longer used and on Windows this will be
part of the initial enumeration process.

BUG=
TBR=dgozman@chromium.org

Committed: https://crrev.com/43c5c906400e24aa59382d47a8654c35c2cb9cc6
Cr-Commit-Position: refs/heads/master@{#296802}

[Reilly Grant (use Gerrit), 2014-09-25 03:55:13]

reillyg@chromium.org changed reviewers:
+ rpaquay@chromium.org

[scheib, 2014-09-25 17:36:03]

scheib@chromium.org changed reviewers:
+ scheib@chromium.org

[scheib, 2014-09-25 17:36:04]

https://codereview.chromium.org/601073002/diff/1/device/usb/usb_device.h
File device/usb/usb_device.h (right):

https://codereview.chromium.org/601073002/diff/1/device/usb/usb_device.h#newc...
device/usb/usb_device.h:58: // Gets the manufacturer string from the device.
"from the device" implies getting data from an actual device, possibly implying
more work than is actually happening. Also, return value isn't specified.
Consider "Gets the manufacturer string of the device, or returns false." Ditto
below.

https://codereview.chromium.org/601073002/diff/1/device/usb/usb_device_handle.h
File device/usb/usb_device_handle.h (right):

https://codereview.chromium.org/601073002/diff/1/device/usb/usb_device_handle...
device/usb/usb_device_handle.h:59: virtual bool GetStringDescriptor(uint8
string_id, base::string16* string) = 0;
Why not an enum for string_id? Oh (after research), because we're exposing the
libusb string id concept. Should we do that on the public interface? For this
refactor, consider if you'll take that on.

Then, add function comment, please, and if still using a uint8 please document
what it is.

https://codereview.chromium.org/601073002/diff/1/device/usb/usb_device_handle...
File device/usb/usb_device_handle_impl.cc (right):

https://codereview.chromium.org/601073002/diff/1/device/usb/usb_device_handle...
device/usb/usb_device_handle_impl.cc:412: bool
UsbDeviceHandleImpl::GetStringDescriptor(uint8 string_id,
Move to match declaration order.

https://codereview.chromium.org/601073002/diff/1/device/usb/usb_device_impl.cc
File device/usb/usb_device_impl.cc (right):

https://codereview.chromium.org/601073002/diff/1/device/usb/usb_device_impl.c...
device/usb/usb_device_impl.cc:326: if (manufacturer_.empty()) {
Consider caching into manufacturer_ on all platforms. On OS_LINUX fail if not
cached.

[Reilly Grant (use Gerrit), 2014-09-25 17:58:45]

https://codereview.chromium.org/601073002/diff/1/device/usb/usb_device.h
File device/usb/usb_device.h (right):

https://codereview.chromium.org/601073002/diff/1/device/usb/usb_device.h#newc...
device/usb/usb_device.h:58: // Gets the manufacturer string from the device.
On 2014/09/25 17:36:04, scheib wrote:
> "from the device" implies getting data from an actual device, possibly
implying
> more work than is actually happening. Also, return value isn't specified.
> Consider "Gets the manufacturer string of the device, or returns false." Ditto
> below.

Done.

https://codereview.chromium.org/601073002/diff/1/device/usb/usb_device_handle.h
File device/usb/usb_device_handle.h (right):

https://codereview.chromium.org/601073002/diff/1/device/usb/usb_device_handle...
device/usb/usb_device_handle.h:59: virtual bool GetStringDescriptor(uint8
string_id, base::string16* string) = 0;
On 2014/09/25 17:36:04, scheib wrote:
> Why not an enum for string_id? Oh (after research), because we're exposing the
> libusb string id concept. Should we do that on the public interface? For this
> refactor, consider if you'll take that on.
> 
> Then, add function comment, please, and if still using a uint8 please document
> what it is.

It'll be gone completely from this interface when I refactor this code. All of
this patch is a bit of a hack to get around libusb annoyances.

https://codereview.chromium.org/601073002/diff/1/device/usb/usb_device_handle...
File device/usb/usb_device_handle_impl.cc (right):

https://codereview.chromium.org/601073002/diff/1/device/usb/usb_device_handle...
device/usb/usb_device_handle_impl.cc:412: bool
UsbDeviceHandleImpl::GetStringDescriptor(uint8 string_id,
On 2014/09/25 17:36:04, scheib wrote:
> Move to match declaration order.

This file is all out of order. It's actually the function above that's in the
wrong place. I'm rewriting this whole thing soon.

https://codereview.chromium.org/601073002/diff/1/device/usb/usb_device_impl.cc
File device/usb/usb_device_impl.cc (right):

https://codereview.chromium.org/601073002/diff/1/device/usb/usb_device_impl.c...
device/usb/usb_device_impl.cc:326: if (manufacturer_.empty()) {
On 2014/09/25 17:36:04, scheib wrote:
> Consider caching into manufacturer_ on all platforms. On OS_LINUX fail if not
> cached.

GetStringDescriptor caches for the other platforms.

[scheib, 2014-09-25 19:47:05]

lgtm, but please pick one of the options below:

https://codereview.chromium.org/601073002/diff/1/device/usb/usb_device_handle...
File device/usb/usb_device_handle_impl.cc (right):

https://codereview.chromium.org/601073002/diff/1/device/usb/usb_device_handle...
device/usb/usb_device_handle_impl.cc:412: bool
UsbDeviceHandleImpl::GetStringDescriptor(uint8 string_id,
On 2014/09/25 17:58:44, reillyg wrote:
> On 2014/09/25 17:36:04, scheib wrote:
> > Move to match declaration order.
> 
> This file is all out of order. It's actually the function above that's in the
> wrong place. I'm rewriting this whole thing soon.

Fix at least the methods that you move the declarations of, otherwise once any
file is 'out of order' it will never approach order. Or have a one off patch to
fix the file, or promise pinky swear that you'll reorder the file right after
this patch and CC/R me on it. ;)

[Reilly Grant (use Gerrit), 2014-09-25 19:55:15]

https://codereview.chromium.org/601073002/diff/1/device/usb/usb_device_handle...
File device/usb/usb_device_handle_impl.cc (right):

https://codereview.chromium.org/601073002/diff/1/device/usb/usb_device_handle...
device/usb/usb_device_handle_impl.cc:412: bool
UsbDeviceHandleImpl::GetStringDescriptor(uint8 string_id,
On 2014/09/25 19:47:05, scheib wrote:
> On 2014/09/25 17:58:44, reillyg wrote:
> > On 2014/09/25 17:36:04, scheib wrote:
> > > Move to match declaration order.
> > 
> > This file is all out of order. It's actually the function above that's in
the
> > wrong place. I'm rewriting this whole thing soon.
> 
> Fix at least the methods that you move the declarations of, otherwise once any
> file is 'out of order' it will never approach order. Or have a one off patch
to
> fix the file, or promise pinky swear that you'll reorder the file right after
> this patch and CC/R me on it. ;)

Done. I should stop being lazy.

[Reilly Grant (use Gerrit), 2014-09-25 20:47:10]

The CQ bit was checked by reillyg@chromium.org

[commit-bot: I haz the power, 2014-09-25 20:48:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/601073002/40001

[commit-bot: I haz the power, 2014-09-25 22:02:55]

Committed patchset #3 (id:40001) as 201f948f2d0f48ee7d427611ad286320b6f4f211

[commit-bot: I haz the power, 2014-09-25 22:03:40]

Patchset 3 (id:??) landed as
https://crrev.com/43c5c906400e24aa59382d47a8654c35c2cb9cc6
Cr-Commit-Position: refs/heads/master@{#296802}