Adding USB ID vendor and product lookups.

Adding USB ID vendor and product lookups.

This change adds a tool for generating the lookup table, a wrapper around the
lookup table, and the build infrastructure required to invoke the rule when
compiling.

BUG=127302
TBR=sky


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

[Garret Kelly, 2012-10-30 20:35:47]

This is the initial version, the DOX comments are TODOs.

[bryeung, 2012-10-31 15:01:27]

I'm good with the UsbIds name.

https://codereview.chromium.org/11344039/diff/3001/device/device.gyp
File device/device.gyp (right):

https://codereview.chromium.org/11344039/diff/3001/device/device.gyp#newcode73
device/device.gyp:73: 'type': '<(library)',
should be 'static_library'

Please see:
http://codereview.chromium.org/11276052/

https://codereview.chromium.org/11344039/diff/3001/device/usb/usb_ids.cc
File device/usb/usb_ids.cc (right):

https://codereview.chromium.org/11344039/diff/3001/device/usb/usb_ids.cc#newc...
device/usb/usb_ids.cc:11: using device::UsbProduct;
seems weird to have these when you're in a device/ file.  Can you just add them
below?  Or move the anonymous namespace into the device namespace?

https://codereview.chromium.org/11344039/diff/3001/device/usb/usb_ids.cc#newc...
device/usb/usb_ids.cc:30: namespace device {
blank line after namespace lines

https://codereview.chromium.org/11344039/diff/3001/device/usb/usb_ids.cc#newc...
device/usb/usb_ids.cc:37: 
either remove this blank line or add it below for consistency

https://codereview.chromium.org/11344039/diff/3001/tools/usb_ids/usb_ids.py
File tools/usb_ids/usb_ids.py (right):

https://codereview.chromium.org/11344039/diff/3001/tools/usb_ids/usb_ids.py#n...
tools/usb_ids/usb_ids.py:43: print "oops!"
could this actually happen?  If not, I'd prefer a more aggressive failure.

https://codereview.chromium.org/11344039/diff/3001/tools/usb_ids/usb_ids.py#n...
tools/usb_ids/usb_ids.py:52: for vendor in vendors:
why iterate again?  Couldn't this be built up as you append to vendors?  In
fact, why do we have vendors at all here?

https://codereview.chromium.org/11344039/diff/3001/tools/usb_ids/usb_ids.py#n...
tools/usb_ids/usb_ids.py:92: description="Generates an embeddable USB ID lookup
table.")
"an embeddable" could maybe changed to "a C++" for additional clarity

https://codereview.chromium.org/11344039/diff/3001/tools/usb_ids/usb_ids.py#n...
tools/usb_ids/usb_ids.py:105: namespace device {
blank line after namespace

https://codereview.chromium.org/11344039/diff/3001/tools/usb_ids/usb_ids.py#n...
tools/usb_ids/usb_ids.py:110: }  // namespace device
blank line before end of namespace

[Garret Kelly, 2012-10-31 15:24:26]

PTAL

http://codereview.chromium.org/11344039/diff/3001/device/usb/usb_ids.cc
File device/usb/usb_ids.cc (right):

http://codereview.chromium.org/11344039/diff/3001/device/usb/usb_ids.cc#newco...
device/usb/usb_ids.cc:11: using device::UsbProduct;
> seems weird to have these when you're in a device/ file.  Can you just add
them
> below?  Or move the anonymous namespace into the device namespace?

Done.

http://codereview.chromium.org/11344039/diff/3001/device/usb/usb_ids.cc#newco...
device/usb/usb_ids.cc:30: namespace device {
On 2012/10/31 15:01:27, bryeung wrote:
> blank line after namespace lines

Done.

http://codereview.chromium.org/11344039/diff/3001/device/usb/usb_ids.cc#newco...
device/usb/usb_ids.cc:37: 
On 2012/10/31 15:01:27, bryeung wrote:
> either remove this blank line or add it below for consistency

Done.

http://codereview.chromium.org/11344039/diff/3001/tools/usb_ids/usb_ids.py
File tools/usb_ids/usb_ids.py (right):

http://codereview.chromium.org/11344039/diff/3001/tools/usb_ids/usb_ids.py#ne...
tools/usb_ids/usb_ids.py:43: print "oops!"
> could this actually happen?  If not, I'd prefer a more aggressive failure.

If the usb.ids file is properly formatted, then no. This is really to catch the
case where something strange has snuck in and is messing up the parsing. I've
made this throw a descriptive exception.

http://codereview.chromium.org/11344039/diff/3001/tools/usb_ids/usb_ids.py#ne...
tools/usb_ids/usb_ids.py:52: for vendor in vendors:
> why iterate again?  Couldn't this be built up as you append to vendors?  In
> fact, why do we have vendors at all here?

Good catch. Done.

http://codereview.chromium.org/11344039/diff/3001/tools/usb_ids/usb_ids.py#ne...
tools/usb_ids/usb_ids.py:92: description="Generates an embeddable USB ID lookup
table.")
On 2012/10/31 15:01:27, bryeung wrote:
> "an embeddable" could maybe changed to "a C++" for additional clarity

Done.

http://codereview.chromium.org/11344039/diff/3001/tools/usb_ids/usb_ids.py#ne...
tools/usb_ids/usb_ids.py:105: namespace device {
On 2012/10/31 15:01:27, bryeung wrote:
> blank line after namespace

Done.

http://codereview.chromium.org/11344039/diff/3001/tools/usb_ids/usb_ids.py#ne...
tools/usb_ids/usb_ids.py:110: }  // namespace device
On 2012/10/31 15:01:27, bryeung wrote:
> blank line before end of namespace

Done.

[bryeung, 2012-10-31 15:28:37]

just the two last comments, lgtm

http://codereview.chromium.org/11344039/diff/3001/device/device.gyp
File device/device.gyp (right):

http://codereview.chromium.org/11344039/diff/3001/device/device.gyp#newcode73
device/device.gyp:73: 'type': '<(library)',
On 2012/10/31 15:01:27, bryeung wrote:
> should be 'static_library'
> 
> Please see:
> http://codereview.chromium.org/11276052/

Looks like you missed this fix?

http://codereview.chromium.org/11344039/diff/9001/tools/usb_ids/usb_ids.py
File tools/usb_ids/usb_ids.py (right):

http://codereview.chromium.org/11344039/diff/9001/tools/usb_ids/usb_ids.py#ne...
tools/usb_ids/usb_ids.py:46: except StopIteration:
do you still need this now that you're not using the iter?

[Garret Kelly, 2012-10-31 15:49:53]

Will commit once third_party/usb_ids change is approved.

http://codereview.chromium.org/11344039/diff/3001/device/device.gyp
File device/device.gyp (right):

http://codereview.chromium.org/11344039/diff/3001/device/device.gyp#newcode73
device/device.gyp:73: 'type': '<(library)',
Done.

http://codereview.chromium.org/11344039/diff/9001/tools/usb_ids/usb_ids.py
File tools/usb_ids/usb_ids.py (right):

http://codereview.chromium.org/11344039/diff/9001/tools/usb_ids/usb_ids.py#ne...
tools/usb_ids/usb_ids.py:46: except StopIteration:
On 2012/10/31 15:28:38, bryeung wrote:
> do you still need this now that you're not using the iter?

Done.

[Paweł Hajdan Jr., 2012-10-31 19:03:21]

Drive-by with comments about Linux-distro related bits.

I appreciate that the initial version of the patch contained switches for the
distros.

https://codereview.chromium.org/11344039/diff/15002/device/device.gyp
File device/device.gyp (right):

https://codereview.chromium.org/11344039/diff/15002/device/device.gyp#newcode8
device/device.gyp:8: 'use_system_usbutils%': 0,
nit: This won't be needed with my other comment. :)

https://codereview.chromium.org/11344039/diff/15002/device/device.gyp#newcode92
device/device.gyp:92: 'usb_ids_path': '/var/lib/usbutils/usb.ids',
nit: You can just let distros configure gyp with the right path. Just do
'usb_ids_path%': '<(DEPTH)/third_party/usb_ids/usb.ids' above (note the "%").

[commit-bot: I haz the power, 2012-12-05 20:05:54]

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

[commit-bot: I haz the power, 2012-12-05 20:06:04]

Presubmit check for 11344039-7009 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome

Presubmit checks took 1.0s to calculate.

[commit-bot: I haz the power, 2012-12-05 20:12:31]

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

[commit-bot: I haz the power, 2012-12-05 22:41:20]

Change committed as 171339