Clean up HID backend and API.

Clean up HID backend and API.

The Mac backend no longer creates its own thread and instead simply enforces single-threaded usage on any thread which supports I/O. Ref management has also been sanitized.

The Linux backend now implements feature report support.

The backend interface no longer expects implicit report IDs in buffers passed to Write or SetFeatureReport. Instead these interfaces (as well as GetFeatureReport) take explicit report ID arguments.

The API interface has been updated to reflect the improved report ID treatment.

Finally, the API also now exposes opaque device identifiers on enumeration, rather than exposing raw system paths or other information that could be potentially sensitive.

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

[Mark Mentovai, 2014-02-13 14:36:08]

“Old chunk mismatch.” I know, Rietveld kicks out 500s like it’s going out of
style. Can you try another upload? It looks like patch set 1 uploaded OK, but
you’ve made some additional changes since then.

[Ken Rockot(use gerrit already), 2014-02-13 18:06:07]

On 2014/02/13 14:36:08, Mark Mentovai wrote:
> “Old chunk mismatch.” I know, Rietveld kicks out 500s like it’s going out of
> style. Can you try another upload? It looks like patch set 1 uploaded OK, but
> you’ve made some additional changes since then.

Yep, looks like the CL is readable again.

[Mark Mentovai, 2014-02-13 21:19:33]

Convenience link to original change where the first batch of comments went:
https://codereview.chromium.org/143883005

[Mark Mentovai, 2014-02-13 22:37:50]

I’m pretty upset that I just spent an hour carefully reviewing this, and you
didn’t even put in the time to compile it. I stopped when I realized that you
hadn’t.

Don’t do this again. You need to build your changes on all platforms (feeding
them to the try bots is fine), and you need to test them by running your code on
platforms where you’ve made substantial enough changes that it’s warranted.
You’ve absolutely crossed that threshold on the Mac with the changes you’ve made
here.

I won’t waste my time reviewing uncompiled or untested code.

https://codereview.chromium.org/161823002/diff/380001/device/hid/hid_service_...
File device/hid/hid_service_mac.cc (right):

https://codereview.chromium.org/161823002/diff/380001/device/hid/hid_service_...
device/hid/hid_service_mac.cc:31: typedef std::vector<IOHIDDeviceRef>
HidDeviceList;
#include <vector> to use this.

Or you could use a std::set (and <set>) to implicitly keep the same promises
that the CFSet used to populate this did.

https://codereview.chromium.org/161823002/diff/380001/device/hid/hid_service_...
device/hid/hid_service_mac.cc:47: CFSetApplyFunction(devices,
HidEnumerationBackInserter, device_list);
Great! This is so much better than allocating space and copying the pointers out
of the CFSet just to iterate over them. Thanks.

https://codereview.chromium.org/161823002/diff/380001/device/hid/hid_service_...
device/hid/hid_service_mac.cc:54: if (!hid_manager_ || CFGetTypeID(hid_manager_)
!= IOHIDManagerGetTypeID()) {
Since the type-check portion of this is pretty cheesey and you agree that it’s
probably just paranoia, it’s better as a DCHECK than an always-enabled test with
a graceful failure mode. You can leave the !hid_manager_ test alone with an
early return, though.

https://codereview.chromium.org/161823002/diff/380001/device/hid/hid_service_...
device/hid/hid_service_mac.cc:65: initialized_ = true;
This is unused.

Well, OK, not exactly. It’s unused in this class, but I see that it’s a
protected member of the base class. That’s not a good design. If anyone actually
calls the base’s initialized(), then it should be turned into a pure virtual
Initialized() (capital letter I), and implementations should either “return
true” (looks like that’s what this one should do) or do whatever else is
appropriate. But if there are no callers, or if in each implementation it would
be logically impossible to return anything other than “true” after the
constructor runs, then initialized() (regardless of how capital the I is) is
pointless and it should be removed altogether.

https://codereview.chromium.org/161823002/diff/380001/device/hid/hid_service_...
device/hid/hid_service_mac.cc:68: HidServiceMac::~HidServiceMac() {
StopWatchingDevices(); }
This may be dangerous if you hit the early return in the constructor. You might
want to check whether hid_manager_ is non-NULL.

(I wouldn’t resurrect initialized_ just for this sake, since now that the
threading nastiness is gone, bool(hid_manager_) has effectively the same meaning
that initialized_ was intended to carry.

https://codereview.chromium.org/161823002/diff/380001/device/hid/hid_service_...
device/hid/hid_service_mac.cc:77: hid_manager_, CFRunLoopGetMain(),
kCFRunLoopDefaultMode);
Now that this is expected to run on the main thread, you should add some
thread-safety DCHECKs so that any publicly-callable function will traverse one.
Any private function that’s expected to run on a specific thread (like
Add/RemoveDeviceCallback) should also execute this type of DCHECK.

I don’t care if you do this by comparing CFRunLoopGetMain() to
CFRunLoopGetCurrent() when you want to check for the main thread, or use
something more Chromey like BrowserThread::CurrentlyOn(BrowserThread::UI). I
expect you’ll need to use the latter to check that you’re on the FILE thread, if
you’re actually allowed to depend on content in here—see below.

https://codereview.chromium.org/161823002/diff/380001/device/hid/hid_service_...
device/hid/hid_service_mac.cc:92: BrowserThread::PostTask(BrowserThread::FILE,
To use this, you need to #include "content/public/browser/browser_thread.h"

I am surprised that you would have BrowserThread incidentally defined in this
file with just the #includes here. I also don’t know if device/hid is even
allowed to depend on content. It doesn’t declare this dependency in
device/hid/hid.gyp, although if it’s supposed to use content and is in fact
allowed to, it would need to.

Do you know how this is supposed to work in the grand scheme of layered
components?

https://codereview.chromium.org/161823002/diff/380001/device/hid/hid_service_...
device/hid/hid_service_mac.cc:117: int32_t int_property = 0;
No need to initialize.

https://codereview.chromium.org/161823002/diff/380001/device/hid/hid_service_...
device/hid/hid_service_mac.cc:118: if (TryGetHidIntProperty(*iter,
Since you’ve used *iter a couple of times in this function (and a couple of
times in Enumerate below), you might want to stick it in a local “IOHIDDeviceRef
device” or “IOHIDDeviceRef hid_device” variable—that’d also help the code be a
tiny bit more self-documenting. TryGetHidIntProperty(hid_device, …) makes a
little more sense than TryGetHidIntProperty(*iter, …). Same in Enumerate below.

https://codereview.chromium.org/161823002/diff/380001/device/hid/hid_service_...
device/hid/hid_service_mac.cc:121: if (id == base::HexEncode(&int_property,
sizeof(int_property))) {
Stopping here. You obviously didn’t even compile this.

https://codereview.chromium.org/161823002/diff/380001/device/hid/hid_service_...
device/hid/hid_service_mac.cc:143: HidDeviceInfo device_info;
Don’t declare stuff ’til you use it.

https://codereview.chromium.org/161823002/diff/380001/device/hid/hid_service_...
device/hid/hid_service_mac.cc:144: base::ScopedCFTypeRef<IOHIDDeviceRef>
device(hid_device);
The same discussion here applies to PlatformRemoveDevice.

This assumes that this function owns hid_device. When this function is called by
Enumerate, that’s reasonable, because Enumerate has added a reference by calling
CFRetain, and it’s expecting PlatformAddDevice to assume ownership of that
reference. But what about when it’s called by AddDeviceCallback?

It’s a little unorthodox for a callback function to be given a reference that
it’s expected to assume ownership of. It’s not unheard of, it’s just weird (and
in my experience, it’s more likely to happen in IOKit than in other parts of the
system). The header doesn’t say anything as far as I can see.

Let’s scrutinize this one a bit. Do you know for a fact that device-matching and
device-removal callbacks are expected to assume ownership of their
IOHIDDeviceRef argument? Is there any sign that this code is over-releasing now?
Would it leak if this scoper were removed? (A standalone stress test of this
design, maybe with a debugging malloc, could help answer that.)

Luckily for us, the source code is available. I’m looking at 10.9.0
IOKitUser-907.1.13/hid.subproj/IOHIDManager.c and it appears to me that there is
no extra reference. Do you agree? If so, get rid of this scoper and the one in
PlatformRemoveDevice, and get rid of the CFRetain in Enumerate.

https://codereview.chromium.org/161823002/diff/380001/device/hid/hid_service_...
device/hid/hid_service_mac.cc:147: int32_t location_id = 0;
No need to initialize.

https://codereview.chromium.org/161823002/diff/380001/device/hid/hid_service_...
device/hid/hid_service_mac.cc:148: if (!TryGetHidIntProperty(device,
CFSTR(kIOHIDLocationIDKey), &location_id))
Just a reminder that “is the location ID the right thing to use?” is something
that we need to revisit. How is this handled on other platforms? Do they also
show a device multiple times under the same ID?

https://codereview.chromium.org/161823002/diff/380001/device/hid/hid_service_...
device/hid/hid_service_mac.cc:172: std::string device_id;
Don’t declare until use. You save a line of code that way, too.

https://codereview.chromium.org/161823002/diff/380001/device/hid/hid_service_...
device/hid/hid_service_mac.cc:173: int32_t int_property = 0;
No need to initialize.

https://codereview.chromium.org/161823002/diff/380001/device/hid/hid_utils_ma...
File device/hid/hid_utils_mac.cc (right):

https://codereview.chromium.org/161823002/diff/380001/device/hid/hid_utils_ma...
device/hid/hid_utils_mac.cc:8: #include "base/logging.h"
Unused, remove.

https://codereview.chromium.org/161823002/diff/380001/device/hid/hid_utils_ma...
device/hid/hid_utils_mac.cc:12: #if defined(OS_MACOSX)
This is kinda silly, this file is only compiled on Mac.

https://codereview.chromium.org/161823002/diff/380001/device/hid/hid_utils_ma...
device/hid/hid_utils_mac.cc:13: #include <CoreFoundation/CoreFoundation.h>
Without the #ifdef, you can move these up after hid_utils_mac.h.

https://codereview.chromium.org/161823002/diff/380001/device/hid/hid_utils_ma...
device/hid/hid_utils_mac.cc:35: CFNumberRef ref =
base::mac::CFCast<CFNumberRef>(
You might think this is crazy-paranoid (and we can talk about it if you do), but
you might want to add something that makes sure that the number actually fits
into an int32_t. There’s CFNumberGetType, but it’s clunky to use. Unfortunately,
there’s no really good way to ask a CFNumber (or an NSNumber) “hey, will your
value fit into this data type without truncation or sign-flipping?”

https://codereview.chromium.org/161823002/diff/380001/device/hid/hid_utils_mac.h
File device/hid/hid_utils_mac.h (right):

https://codereview.chromium.org/161823002/diff/380001/device/hid/hid_utils_ma...
device/hid/hid_utils_mac.h:8: #include "base/callback.h"
Both of these #includes are unused.

https://codereview.chromium.org/161823002/diff/380001/device/hid/hid_utils_ma...
device/hid/hid_utils_mac.h:11: #include <IOKit/hid/IOHIDManager.h>
You also need <CoreFoundation/CoreFoundation.h>, <string>, and something that
guarantees you get int32_t, either "base/basictypes.h" or <stdint.h>.

Once you put those #includes in this file, you won’t need to also list them in
the .cc.

[Ken Rockot(use gerrit already), 2014-02-13 22:40:30]

On 2014/02/13 22:37:50, Mark Mentovai wrote:
> I’m pretty upset that I just spent an hour carefully reviewing this, and you
> didn’t even put in the time to compile it. I stopped when I realized that you
> hadn’t.
> 
> Don’t do this again. You need to build your changes on all platforms (feeding
> them to the try bots is fine), and you need to test them by running your code
on
> platforms where you’ve made substantial enough changes that it’s warranted.
> You’ve absolutely crossed that threshold on the Mac with the changes you’ve
made
> here.
> 
> I won’t waste my time reviewing uncompiled or untested code.

I'm sorry, I should have communicated that this was still a work in progress. I
was hoping to at least resolve questions about the removal of the extra thread
and device identification, and make sure that this was moving in the right
direction.

[Mark Mentovai, 2014-02-13 22:45:38]

It is moving in the right direction. Next time, please be very explicit about
the level of polish. I would have been happy to give it a quick 5-minute
once-over in the state that it’s in now.

[Ken Rockot(use gerrit already), 2014-02-18 19:43:09]

Ok. Progress. This is really ready for review now.

One thing I noticed is that the kIOHIDManagerOptionNone value (and indeed the
entire kIOHIDManagerOptions enum) appears to be undefined on some try bots. I
assume this is because they're using different versions of the SDK and IOKit's
HID API changes often.

I've resolved to using 0 for now in lieu of OptionNone, which as far as I can
tell should always be equivalent.

https://codereview.chromium.org/161823002/diff/380001/device/hid/hid_service_...
File device/hid/hid_service_mac.cc (right):

https://codereview.chromium.org/161823002/diff/380001/device/hid/hid_service_...
device/hid/hid_service_mac.cc:31: typedef std::vector<IOHIDDeviceRef>
HidDeviceList;
On 2014/02/13 22:37:51, Mark Mentovai wrote:
> #include <vector> to use this.
> 
> Or you could use a std::set (and <set>) to implicitly keep the same promises
> that the CFSet used to populate this did.

Done. I left it as a vector since the usage is isolated and consists only of
traversal, and building a vector should be slightly cheaper than building a set.
I don't feel strongly about it either way though.

https://codereview.chromium.org/161823002/diff/380001/device/hid/hid_service_...
device/hid/hid_service_mac.cc:54: if (!hid_manager_ || CFGetTypeID(hid_manager_)
!= IOHIDManagerGetTypeID()) {
On 2014/02/13 22:37:51, Mark Mentovai wrote:
> Since the type-check portion of this is pretty cheesey and you agree that it’s
> probably just paranoia, it’s better as a DCHECK than an always-enabled test
with
> a graceful failure mode. You can leave the !hid_manager_ test alone with an
> early return, though.

Done.

https://codereview.chromium.org/161823002/diff/380001/device/hid/hid_service_...
device/hid/hid_service_mac.cc:65: initialized_ = true;
On 2014/02/13 22:37:51, Mark Mentovai wrote:
> This is unused.
> 
> Well, OK, not exactly. It’s unused in this class, but I see that it’s a
> protected member of the base class. That’s not a good design. If anyone
actually
> calls the base’s initialized(), then it should be turned into a pure virtual
> Initialized() (capital letter I), and implementations should either “return
> true” (looks like that’s what this one should do) or do whatever else is
> appropriate. But if there are no callers, or if in each implementation it
would
> be logically impossible to return anything other than “true” after the
> constructor runs, then initialized() (regardless of how capital the I is) is
> pointless and it should be removed altogether.

Yes I absolutely hate this. Removed altogether. The removal only changes the
behavior of the Windows impl slightly and I don't think preserving the old
behavior is worth separating construction and initialization.

https://codereview.chromium.org/161823002/diff/380001/device/hid/hid_service_...
device/hid/hid_service_mac.cc:68: HidServiceMac::~HidServiceMac() {
StopWatchingDevices(); }
On 2014/02/13 22:37:51, Mark Mentovai wrote:
> This may be dangerous if you hit the early return in the constructor. You
might
> want to check whether hid_manager_ is non-NULL.
> 
> (I wouldn’t resurrect initialized_ just for this sake, since now that the
> threading nastiness is gone, bool(hid_manager_) has effectively the same
meaning
> that initialized_ was intended to carry.

Done.

https://codereview.chromium.org/161823002/diff/380001/device/hid/hid_service_...
device/hid/hid_service_mac.cc:77: hid_manager_, CFRunLoopGetMain(),
kCFRunLoopDefaultMode);
On 2014/02/13 22:37:51, Mark Mentovai wrote:
> Now that this is expected to run on the main thread, you should add some
> thread-safety DCHECKs so that any publicly-callable function will traverse
one.
> Any private function that’s expected to run on a specific thread (like
> Add/RemoveDeviceCallback) should also execute this type of DCHECK.
> 
> I don’t care if you do this by comparing CFRunLoopGetMain() to
> CFRunLoopGetCurrent() when you want to check for the main thread, or use
> something more Chromey like BrowserThread::CurrentlyOn(BrowserThread::UI). I
> expect you’ll need to use the latter to check that you’re on the FILE thread,
if
> you’re actually allowed to depend on content in here—see below.


Thinking about it some more, I'd rather avoid the content dependency even if
it's not strictly necessary to do so.

The base HidService upon construction verifies that the current thread allows
I/O. It also owns a ThreadChecker which most methods can use to ensure they're
running on the original (ctor's) thread. These checks should fulfill our only
real requirements: that the public interface is always used on a single thread
that supports I/O.

The device match/removal callbacks also now DCHECK that
CFRunLoopGetCurrent()==CFRunLoopGetMain().

https://codereview.chromium.org/161823002/diff/380001/device/hid/hid_service_...
device/hid/hid_service_mac.cc:92: BrowserThread::PostTask(BrowserThread::FILE,
On 2014/02/13 22:37:51, Mark Mentovai wrote:
> To use this, you need to #include "content/public/browser/browser_thread.h"
> 
> I am surprised that you would have BrowserThread incidentally defined in this
> file with just the #includes here. I also don’t know if device/hid is even
> allowed to depend on content. It doesn’t declare this dependency in
> device/hid/hid.gyp, although if it’s supposed to use content and is in fact
> allowed to, it would need to.
> 
> Do you know how this is supposed to work in the grand scheme of layered
> components?

I decided to retain a handle to the current MessageLoop in the ctor. Instead of
posting to a specific Chrome thread, these callbacks will post to that original
MessageLoop.

https://codereview.chromium.org/161823002/diff/380001/device/hid/hid_service_...
device/hid/hid_service_mac.cc:117: int32_t int_property = 0;
On 2014/02/13 22:37:51, Mark Mentovai wrote:
> No need to initialize.

Done.

https://codereview.chromium.org/161823002/diff/380001/device/hid/hid_service_...
device/hid/hid_service_mac.cc:118: if (TryGetHidIntProperty(*iter,
On 2014/02/13 22:37:51, Mark Mentovai wrote:
> Since you’ve used *iter a couple of times in this function (and a couple of
> times in Enumerate below), you might want to stick it in a local
“IOHIDDeviceRef
> device” or “IOHIDDeviceRef hid_device” variable—that’d also help the code be a
> tiny bit more self-documenting. TryGetHidIntProperty(hid_device, …) makes a
> little more sense than TryGetHidIntProperty(*iter, …). Same in Enumerate
below.

Done.

https://codereview.chromium.org/161823002/diff/380001/device/hid/hid_service_...
device/hid/hid_service_mac.cc:143: HidDeviceInfo device_info;
On 2014/02/13 22:37:51, Mark Mentovai wrote:
> Don’t declare stuff ’til you use it.

Done.

https://codereview.chromium.org/161823002/diff/380001/device/hid/hid_service_...
device/hid/hid_service_mac.cc:144: base::ScopedCFTypeRef<IOHIDDeviceRef>
device(hid_device);
On 2014/02/13 22:37:51, Mark Mentovai wrote:
> The same discussion here applies to PlatformRemoveDevice.
> 
> This assumes that this function owns hid_device. When this function is called
by
> Enumerate, that’s reasonable, because Enumerate has added a reference by
calling
> CFRetain, and it’s expecting PlatformAddDevice to assume ownership of that
> reference. But what about when it’s called by AddDeviceCallback?
> 
> It’s a little unorthodox for a callback function to be given a reference that
> it’s expected to assume ownership of. It’s not unheard of, it’s just weird
(and
> in my experience, it’s more likely to happen in IOKit than in other parts of
the
> system). The header doesn’t say anything as far as I can see.
> 
> Let’s scrutinize this one a bit. Do you know for a fact that device-matching
and
> device-removal callbacks are expected to assume ownership of their
> IOHIDDeviceRef argument? Is there any sign that this code is over-releasing
now?
> Would it leak if this scoper were removed? (A standalone stress test of this
> design, maybe with a debugging malloc, could help answer that.)
> 
> Luckily for us, the source code is available. I’m looking at 10.9.0
> IOKitUser-907.1.13/hid.subproj/IOHIDManager.c and it appears to me that there
is
> no extra reference. Do you agree? If so, get rid of this scoper and the one in
> PlatformRemoveDevice, and get rid of the CFRetain in Enumerate.

This appears to be correct, the manager does not retain a reference. Removed
scopers and CFRetain in Enumerate.

https://codereview.chromium.org/161823002/diff/380001/device/hid/hid_service_...
device/hid/hid_service_mac.cc:147: int32_t location_id = 0;
On 2014/02/13 22:37:51, Mark Mentovai wrote:
> No need to initialize.

Done.

https://codereview.chromium.org/161823002/diff/380001/device/hid/hid_service_...
device/hid/hid_service_mac.cc:148: if (!TryGetHidIntProperty(device,
CFSTR(kIOHIDLocationIDKey), &location_id))
On 2014/02/13 22:37:51, Mark Mentovai wrote:
> Just a reminder that “is the location ID the right thing to use?”&nbsp;is
something
> that we need to revisit. How is this handled on other platforms? Do they also
> show a device multiple times under the same ID?

I've looked into this and I'm pretty confident this is the right thing to do
now. I don't know what you mean by showing a device "multiple times under the
same ID." The goal here is to have a system-wide unique identifier for a
specific physically connected device, and that is what we get with this location
ID.

On Windows we use the device path, which is a string that looks something like
"\\?\hid#vid=1234&pid=1234#1234abcde{some-guid}", on Linux we have a long /sys
path which reflects the bus topology, and here we have the "location ID."

In all three cases, the key is ultimately derived by the OS from the physical
bus location of the device. I'm not aware of any alternative device property
that could be used for unique device identification on OS X.

https://codereview.chromium.org/161823002/diff/380001/device/hid/hid_service_...
device/hid/hid_service_mac.cc:172: std::string device_id;
On 2014/02/13 22:37:51, Mark Mentovai wrote:
> Don’t declare until use. You save a line of code that way, too.

Done.

https://codereview.chromium.org/161823002/diff/380001/device/hid/hid_service_...
device/hid/hid_service_mac.cc:173: int32_t int_property = 0;
On 2014/02/13 22:37:51, Mark Mentovai wrote:
> No need to initialize.

Done.

https://codereview.chromium.org/161823002/diff/380001/device/hid/hid_utils_ma...
File device/hid/hid_utils_mac.cc (right):

https://codereview.chromium.org/161823002/diff/380001/device/hid/hid_utils_ma...
device/hid/hid_utils_mac.cc:8: #include "base/logging.h"
On 2014/02/13 22:37:51, Mark Mentovai wrote:
> Unused, remove.

Done.

https://codereview.chromium.org/161823002/diff/380001/device/hid/hid_utils_ma...
device/hid/hid_utils_mac.cc:13: #include <CoreFoundation/CoreFoundation.h>
On 2014/02/13 22:37:51, Mark Mentovai wrote:
> Without the #ifdef, you can move these up after hid_utils_mac.h.

Done.

https://codereview.chromium.org/161823002/diff/380001/device/hid/hid_utils_ma...
device/hid/hid_utils_mac.cc:35: CFNumberRef ref =
base::mac::CFCast<CFNumberRef>(
On 2014/02/13 22:37:51, Mark Mentovai wrote:
> You might think this is crazy-paranoid (and we can talk about it if you do),
but
> you might want to add something that makes sure that the number actually fits
> into an int32_t. There’s CFNumberGetType, but it’s clunky to use.
Unfortunately,
> there’s no really good way to ask a CFNumber (or an NSNumber) “hey, will your
> value fit into this data type without truncation or sign-flipping?”

I would be happy to add this, but it does look like it would be cumbersome. I'm
imagining that the result of CFNumberGetType would have to be tested against the
set kCFNumber{SInt8, SInt16, SIn32, Float32, Char, [additional compiler
dependent types]}Type.

https://codereview.chromium.org/161823002/diff/380001/device/hid/hid_utils_mac.h
File device/hid/hid_utils_mac.h (right):

https://codereview.chromium.org/161823002/diff/380001/device/hid/hid_utils_ma...
device/hid/hid_utils_mac.h:8: #include "base/callback.h"
On 2014/02/13 22:37:51, Mark Mentovai wrote:
> Both of these #includes are unused.

Done.

https://codereview.chromium.org/161823002/diff/380001/device/hid/hid_utils_ma...
device/hid/hid_utils_mac.h:11: #include <IOKit/hid/IOHIDManager.h>
On 2014/02/13 22:37:51, Mark Mentovai wrote:
> You also need <CoreFoundation/CoreFoundation.h>, <string>, and something that
> guarantees you get int32_t, either "base/basictypes.h" or <stdint.h>.
> 
> Once you put those #includes in this file, you won’t need to also list them in
> the .cc.

Done.

[Mark Mentovai, 2014-02-19 22:48:15]

Using 0 instead of kIOHIDManagerOptionNone  is fine.

Here’s what I meant about location IDs:

When I do IOHIDManagerCopyDevices on a new IOHIDManagerRef on my laptop, I get
back a set of 5:

Product = Apple Internal Keyboard / Trackpad
LocationID = 0x1d182000
Product = Apple Internal Keyboard / Trackpad
LocationID = 0x1d182000
Product = Apple Internal Keyboard / Trackpad
LocationID = 0x1d182000
Product = Apple Mikey HID Driver
LocationID not found
Product = Apple Internal Keyboard / Trackpad
LocationID = 0x1d182000

Four of these entries are at the same location ID. You might consider them all
the same “device,” but it looks like one is for the keyboard, there are two
different ones for the trackpad, and I guess one is for the parent node of the
rest. If we’re identifying all of them by the same identifier, the location ID,
is it going to be a problem? When some caller says “Connect”, we’ll make a
connection to one of these four IOHIDDeviceRefs, based solely on which one got
saved. Might this mask functionality of composite devices?

https://codereview.chromium.org/161823002/diff/720001/device/hid/hid_connecti...
File device/hid/hid_connection_mac.cc (right):

https://codereview.chromium.org/161823002/diff/720001/device/hid/hid_connecti...
device/hid/hid_connection_mac.cc:63: // A non-zero report ID requires an extra
byte.
What’s special about 0 that you don’t need to put it into the buffer? Isn’t
there an ambiguity problem—whatever ultimately consumes the data won’t know
whether the first byte is the report_id or report_bytes.

https://codereview.chromium.org/161823002/diff/720001/device/hid/hid_connecti...
device/hid/hid_connection_mac.cc:67: buffer->data()[0] = report_id;
report_id is uint32_t. This will truncate it. Is data loss acceptable?

If we’re not using numbered reports at all, maybe you should get rid of all of
the non-0 stuff and simply DCHECK that the report_id is 0.

https://codereview.chromium.org/161823002/diff/720001/device/hid/hid_connecti...
device/hid/hid_connection_mac.cc:120: uint32_t report_id =
static_cast<uint32_t>(buffer->data()[0]);
Similar questions about interpretation ambiguity and data type width apply to
the write case.

What informed this design? Other platforms? Something else? This logic smells
fishy.

https://codereview.chromium.org/161823002/diff/720001/device/hid/hid_connecti...
File device/hid/hid_connection_mac.h (right):

https://codereview.chromium.org/161823002/diff/720001/device/hid/hid_connecti...
device/hid/hid_connection_mac.h:10: #include <queue>
Chrome always puts a blank line between the C system headers and the C++ system
headers. Same in hid_device_info.h, hid_service_linux.cc, and maybe elsewhere.

https://codereview.chromium.org/161823002/diff/720001/device/hid/hid_connecti...
device/hid/hid_connection_mac.h:28: scoped_refptr<net::IOBuffer> buffer;
net::IOBuffer + size = net::IOBufferWithSize?

Same on line 36.

https://codereview.chromium.org/161823002/diff/720001/device/hid/hid_connecti...
device/hid/hid_connection_mac.h:71: CFIndex reportLength);
report_length. Same on line 75 and in the .cc file. (I notice you already fixed
report_id. Thanks.)

https://codereview.chromium.org/161823002/diff/720001/device/hid/hid_connecti...
device/hid/hid_connection_mac.h:88: scoped_ptr_malloc<uint8_t> inbound_buffer_;
#include "base/memory/scoped_ptr.h" to be able to use this.

https://codereview.chromium.org/161823002/diff/720001/device/hid/hid_service.h
File device/hid/hid_service.h (right):

https://codereview.chromium.org/161823002/diff/720001/device/hid/hid_service....
device/hid/hid_service.h:33: bool GetInfo(const std::string& device_id,
HidDeviceInfo* info) const;
Good call turning these into const refs.

https://codereview.chromium.org/161823002/diff/720001/device/hid/hid_service....
device/hid/hid_service.h:54: DeviceMap devices_;
Hmm, protected data. That’s not a good sign. Can this be private?

Using a protected data member as an interface between a superclass and its
subclasses gets hairy and out-of-control very quickly. Especially in cases like
this, where there isn’t even any documentation about what the base class does
with this and what it expects subclasses to do with it.

Most communication between a base class and its subclasses should be through
method calls, not shared data.

https://codereview.chromium.org/161823002/diff/720001/device/hid/hid_service_...
File device/hid/hid_service_mac.cc (right):

https://codereview.chromium.org/161823002/diff/720001/device/hid/hid_service_...
device/hid/hid_service_mac.cc:44: STLClearObject(device_list);
Since the only two callers of this function supply an empty list, you could also
just DCHECK(device_list.empty()) and have there be zero impact on release
builds.

https://codereview.chromium.org/161823002/diff/720001/device/hid/hid_service_...
device/hid/hid_service_mac.cc:71: HidServiceMac::~HidServiceMac() {
StopWatchingDevices(); }
Inline is fine for {}, but avoid writing an entire function body like this,
because it makes it look like there isn’t any real code in it.

[Ken Rockot(use gerrit already), 2014-02-21 02:15:35]

Thanks. You're absolutely right about composite devices. I see that location HID
is totally ambiguous in their presence. I've abandoned using it altogether.
Instead, device IDs on all platforms are now unique integers generated by
HidService.

Each platform impl of HidService maintains its own internal bi-directional
mapping between an ID and some platform-specific resource (unique path strings
on Linux and Windows, retained IOHIDDeviceRef on OS X). For the record, the
unique path strings on Linux and Windows really are unique even between
individual interfaces of a composite device (yay).

I decided to do the bi-di mapping because Connect wants to resolve IDs to these
resources, but system callbacks (namely on-device-unplugged) need to map from
these resources back to device IDs. I'm not terribly happy about having to house
this new HidPlatformResourceBidiMap<T> here, but it doesn't look like there are
any other bimaps in all of Chrome? An alternative I considered was adding
support for platform data to DeviceInfo and having the resource-to-ID lookups
just search the whole device map. That brings with it its own kind of ugliness
though.

In any case, please take a look.

https://codereview.chromium.org/161823002/diff/720001/device/hid/hid_connecti...
File device/hid/hid_connection_mac.cc (right):

https://codereview.chromium.org/161823002/diff/720001/device/hid/hid_connecti...
device/hid/hid_connection_mac.cc:63: // A non-zero report ID requires an extra
byte.
When working with a specific raw device, the developer must already know whether
or not the device supports Report IDs. If it does, the first byte of every input
report is a non-zero Report ID. If it doesn't, the first byte of every input
report is something else (but it is never a zero Report ID).

On other OSes, this is a non-issue; the contents of an input report are passed
along into user space as-is for consumers to interpret. In the case of OS X,
what happens is this:

1. If a device doesn't support Report IDs, report_id is 0 here and the input
report contents (report_bytes) are verbatim what the device sent.

2. If a device DOES support Report IDs, the first byte of the input report is
graciously stripped off by the system and passed in through the report_id
argument.

Injecting the Report ID (if present) back into the buffer provides behavior that
is consistent with other platforms and hopefully consistent with the
expectations of any developer working with raw HID devices.

https://codereview.chromium.org/161823002/diff/720001/device/hid/hid_connecti...
device/hid/hid_connection_mac.cc:67: buffer->data()[0] = report_id;
On 2014/02/19 22:48:15, Mark Mentovai wrote:
> report_id is uint32_t. This will truncate it. Is data loss acceptable?
> 
> If we’re not using numbered reports at all, maybe you should get rid of all of
> the non-0 stuff and simply DCHECK that the report_id is 0.

The protocol only uses a single byte for a Report ID, so this is just an oddity
of IOKit's API. Truncation is fine.

https://codereview.chromium.org/161823002/diff/720001/device/hid/hid_connecti...
device/hid/hid_connection_mac.cc:120: uint32_t report_id =
static_cast<uint32_t>(buffer->data()[0]);
On 2014/02/19 22:48:15, Mark Mentovai wrote:
> Similar questions about interpretation ambiguity and data type width apply to
> the write case.
> 
> What informed this design? Other platforms? Something else? This logic smells
> fishy.

There does not appear to be a great way of determining whether or not a device
supports Report IDs, which is unfortunate given that IOKit always requires
report ID to be given separately from buffer contents (which ultimately must
result in the driver injecting that ID as the first byte in the buffer anyway).

The working solution we went with for this problem was to require users of
HidConnection to always include a Report ID in their output reports, using zero
when sending to a device which does not use Report IDs.

This is what the Windows HID API expects as well - it always expects the first
byte of an output report to be a report ID, and if that ID is zero, it gets
stripped off before the report data is sent to the device.

I considered mimicking IOKit's API instead of Windows' - that is, make Write
take an explicit Report ID argument (which will be injected if non-zero), and
make Read's callback get an explicit Report ID argument which is automatically
stripped from an input report. Unfortunately this introduces an ambiguity that
can't be resolved by the Mac impl: we don't know if we should pluck a Report ID
off a report coming from the device.

So we instead leave it up to the developer to properly interpret the raw data
from the device, and require the developer to unambiguously prefix all output
reports with a Report-ID-or-zero byte.

https://codereview.chromium.org/161823002/diff/720001/device/hid/hid_connecti...
File device/hid/hid_connection_mac.h (right):

https://codereview.chromium.org/161823002/diff/720001/device/hid/hid_connecti...
device/hid/hid_connection_mac.h:10: #include <queue>
On 2014/02/19 22:48:15, Mark Mentovai wrote:
> Chrome always puts a blank line between the C system headers and the C++
system
> headers. Same in hid_device_info.h, hid_service_linux.cc, and maybe elsewhere.

Done.

https://codereview.chromium.org/161823002/diff/720001/device/hid/hid_connecti...
device/hid/hid_connection_mac.h:28: scoped_refptr<net::IOBuffer> buffer;
On 2014/02/19 22:48:15, Mark Mentovai wrote:
> net::IOBuffer + size = net::IOBufferWithSize?
> 
> Same on line 36.

Done all over the place.

https://codereview.chromium.org/161823002/diff/720001/device/hid/hid_connecti...
device/hid/hid_connection_mac.h:71: CFIndex reportLength);
On 2014/02/19 22:48:15, Mark Mentovai wrote:
> report_length. Same on line 75 and in the .cc file. (I notice you already
fixed
> report_id. Thanks.)

Done.

https://codereview.chromium.org/161823002/diff/720001/device/hid/hid_connecti...
device/hid/hid_connection_mac.h:88: scoped_ptr_malloc<uint8_t> inbound_buffer_;
On 2014/02/19 22:48:15, Mark Mentovai wrote:
> #include "base/memory/scoped_ptr.h" to be able to use this.

Done.

https://codereview.chromium.org/161823002/diff/720001/device/hid/hid_service.h
File device/hid/hid_service.h (right):

https://codereview.chromium.org/161823002/diff/720001/device/hid/hid_service....
device/hid/hid_service.h:54: DeviceMap devices_;
On 2014/02/19 22:48:15, Mark Mentovai wrote:
> Hmm, protected data. That’s not a good sign. Can this be private?
> 
> Using a protected data member as an interface between a superclass and its
> subclasses gets hairy and out-of-control very quickly. Especially in cases
like
> this, where there isn’t even any documentation about what the base class does
> with this and what it expects subclasses to do with it.
> 
> Most communication between a base class and its subclasses should be through
> method calls, not shared data.

Agreed. Moved to private.

https://codereview.chromium.org/161823002/diff/720001/device/hid/hid_service_...
File device/hid/hid_service_mac.cc (right):

https://codereview.chromium.org/161823002/diff/720001/device/hid/hid_service_...
device/hid/hid_service_mac.cc:44: STLClearObject(device_list);
On 2014/02/19 22:48:15, Mark Mentovai wrote:
> Since the only two callers of this function supply an empty list, you could
also
> just DCHECK(device_list.empty()) and have there be zero impact on release
> builds.

Done.

https://codereview.chromium.org/161823002/diff/720001/device/hid/hid_service_...
device/hid/hid_service_mac.cc:71: HidServiceMac::~HidServiceMac() {
StopWatchingDevices(); }
On 2014/02/19 22:48:15, Mark Mentovai wrote:
> Inline is fine for {}, but avoid writing an entire function body like this,
> because it makes it look like there isn’t any real code in it.

Done.

[Mark Mentovai, 2014-02-21 17:48:18]

OK, this is definitely shaping up. It’s way better than the state we found it
in. There might be room for even more simplification. What do you think about my
suggestion for handling device IDs?

I’d also like to take a closer look at the Windows and Linux changes, maybe I’ll
get to that after lunch.

https://codereview.chromium.org/161823002/diff/880001/device/hid/hid_connecti...
File device/hid/hid_connection.cc (right):

https://codereview.chromium.org/161823002/diff/880001/device/hid/hid_connecti...
device/hid/hid_connection.cc:23: return device_info_;
It’s OK to write this simple accessor inline in the header.

https://codereview.chromium.org/161823002/diff/880001/device/hid/hid_connecti...
File device/hid/hid_connection.h (right):

https://codereview.chromium.org/161823002/diff/880001/device/hid/hid_connecti...
device/hid/hid_connection.h:38: base::ThreadChecker thread_checker_;
It feels a little bit dirty that this is protected. If the base class and the
subclasses all needed a thread checker, then I’d say it’s OK, since it wouldn’t
really be an interface between the base and subclasses, it would just be a handy
way to share the thread checker instead of having to keep more than one. Since
the base doesn’t use thread_checker_ at all, though, then I’ll suggest making
this a member of each of the subclasses instead.

https://codereview.chromium.org/161823002/diff/880001/device/hid/hid_connecti...
File device/hid/hid_connection_mac.cc (right):

https://codereview.chromium.org/161823002/diff/880001/device/hid/hid_connecti...
device/hid/hid_connection_mac.cc:110: ++data_to_send;
Weird indentation in this block.

https://codereview.chromium.org/161823002/diff/880001/device/hid/hid_device_i...
File device/hid/hid_device_info.h (right):

https://codereview.chromium.org/161823002/diff/880001/device/hid/hid_device_i...
device/hid/hid_device_info.h:27: int device_id;
HidDeviceId, right?

https://codereview.chromium.org/161823002/diff/880001/device/hid/hid_service.h
File device/hid/hid_service.h (right):

https://codereview.chromium.org/161823002/diff/880001/device/hid/hid_service....
device/hid/hid_service.h:56: base::ThreadChecker thread_checker_;
Here’s a case where the thread checker is shared by the base class and its
subclasses, and I think we can relax the rule about protected member variables
for this one because the thread checker isn’t conceptually data.

https://codereview.chromium.org/161823002/diff/880001/device/hid/hid_service_...
File device/hid/hid_service_mac.cc (right):

https://codereview.chromium.org/161823002/diff/880001/device/hid/hid_service_...
device/hid/hid_service_mac.cc:72: HidServiceMac::~HidServiceMac() {
StopWatchingDevices(); }
I mentioned this before at
https://codereview.chromium.org/161823002/diff/720001/device/hid/hid_service_...

> Inline is fine for {}, but avoid writing an entire function body like this,
> because it makes it look like there isn’t any real code in it.

https://codereview.chromium.org/161823002/diff/880001/device/hid/hid_service_...
device/hid/hid_service_mac.cc:168: CFRelease(hid_device);
Since you retained the device before calling AddDevice, you should keep it
balanced by releasing it after RemoveDevice, in case the base class’ dealings
with this object wind up being dependent on its lifetime.

https://codereview.chromium.org/161823002/diff/880001/device/hid/hid_service_...
File device/hid/hid_service_mac.h (right):

https://codereview.chromium.org/161823002/diff/880001/device/hid/hid_service_...
device/hid/hid_service_mac.h:59: // A 1:1 mapping between device IDs and
IOHIDDeviceRefs.
Why? Why go to the trouble of inventing our own handle scheme that we need to
deal with via a map, when all we really care about is (on Mac) the
IOHIDDeviceRef?

You can define HidDeviceId as

#if defined(OS_MACOSX)
typedef IOHIDDeviceRef HidDeviceId;
#else
typedef std::string HidDeviceId;
#endif

and, where possible, pass ’em around as HidDeviceId& to avoid string copies.

[Ken Rockot(use gerrit already), 2014-02-22 01:17:04]

I like the device ID suggestion. I've moved the integer handle mapping up to the
higher level JS API, which is really where it belongs. We definitely want to
mask the underlying identifiers there.

I also did another sanity check on device ownership in the Mac impl and added
comments to clarify ownership acquisitions and assumptions on device refs.

https://codereview.chromium.org/161823002/diff/880001/device/hid/hid_connecti...
File device/hid/hid_connection.cc (right):

https://codereview.chromium.org/161823002/diff/880001/device/hid/hid_connecti...
device/hid/hid_connection.cc:23: return device_info_;
On 2014/02/21 17:48:18, Mark Mentovai wrote:
> It’s OK to write this simple accessor inline in the header.

Done.

https://codereview.chromium.org/161823002/diff/880001/device/hid/hid_connecti...
File device/hid/hid_connection.h (right):

https://codereview.chromium.org/161823002/diff/880001/device/hid/hid_connecti...
device/hid/hid_connection.h:38: base::ThreadChecker thread_checker_;
On 2014/02/21 17:48:18, Mark Mentovai wrote:
> It feels a little bit dirty that this is protected. If the base class and the
> subclasses all needed a thread checker, then I’d say it’s OK, since it
wouldn’t
> really be an interface between the base and subclasses, it would just be a
handy
> way to share the thread checker instead of having to keep more than one. Since
> the base doesn’t use thread_checker_ at all, though, then I’ll suggest making
> this a member of each of the subclasses instead.

Done. One per subclass now.

https://codereview.chromium.org/161823002/diff/880001/device/hid/hid_connecti...
File device/hid/hid_connection_mac.cc (right):

https://codereview.chromium.org/161823002/diff/880001/device/hid/hid_connecti...
device/hid/hid_connection_mac.cc:110: ++data_to_send;
On 2014/02/21 17:48:18, Mark Mentovai wrote:
> Weird indentation in this block.

Done.

https://codereview.chromium.org/161823002/diff/880001/device/hid/hid_device_i...
File device/hid/hid_device_info.h (right):

https://codereview.chromium.org/161823002/diff/880001/device/hid/hid_device_i...
device/hid/hid_device_info.h:27: int device_id;
On 2014/02/21 17:48:18, Mark Mentovai wrote:
> HidDeviceId, right?

Done.

https://codereview.chromium.org/161823002/diff/880001/device/hid/hid_service_...
File device/hid/hid_service_mac.cc (right):

https://codereview.chromium.org/161823002/diff/880001/device/hid/hid_service_...
device/hid/hid_service_mac.cc:72: HidServiceMac::~HidServiceMac() {
StopWatchingDevices(); }
On 2014/02/21 17:48:18, Mark Mentovai wrote:
> I mentioned this before at
>
https://codereview.chromium.org/161823002/diff/720001/device/hid/hid_service_...
> 
> > Inline is fine for {}, but avoid writing an entire function body like this,
> > because it makes it look like there isn’t any real code in it.

I have considered writing the code like this instead, to avoid clang_format
trampling:

Foo::~Foo() {  // http://crbug.com/345750
  Body
}

That may be going too far though.

For now I'm going to let clang_format do its thing, and once the patch is
otherwise ready, I'll be happy to unfold these one-liners before hitting the CQ.

https://codereview.chromium.org/161823002/diff/880001/device/hid/hid_service_...
device/hid/hid_service_mac.cc:168: CFRelease(hid_device);
On 2014/02/21 17:48:18, Mark Mentovai wrote:
> Since you retained the device before calling AddDevice, you should keep it
> balanced by releasing it after RemoveDevice, in case the base class’ dealings
> with this object wind up being dependent on its lifetime.

Done.

https://codereview.chromium.org/161823002/diff/880001/device/hid/hid_service_...
File device/hid/hid_service_mac.h (right):

https://codereview.chromium.org/161823002/diff/880001/device/hid/hid_service_...
device/hid/hid_service_mac.h:59: // A 1:1 mapping between device IDs and
IOHIDDeviceRefs.
On 2014/02/21 17:48:18, Mark Mentovai wrote:
> Why? Why go to the trouble of inventing our own handle scheme that we need to
> deal with via a map, when all we really care about is (on Mac) the
> IOHIDDeviceRef?
> 
> You can define HidDeviceId as
> 
> #if defined(OS_MACOSX)
> typedef IOHIDDeviceRef HidDeviceId;
> #else
> typedef std::string HidDeviceId;
> #endif
> 
> and, where possible, pass ’em around as HidDeviceId& to avoid string copies.

Much simpler. I think the attachment to a consistent underlying type for the
device_id was really due to crossover from our primary intended use of this
interface (exposing a hid JS API to chrome apps). That's really a concern of the
higher-level API though, and it makes no sense to invent new handles at this
level. Thanks.

[Mark Mentovai, 2014-02-24 20:23:35]

I’m being called away to a meeting, but here’s what I’ve got so far.

https://codereview.chromium.org/161823002/diff/880001/device/hid/hid_service_...
File device/hid/hid_service_mac.cc (right):

https://codereview.chromium.org/161823002/diff/880001/device/hid/hid_service_...
device/hid/hid_service_mac.cc:72: HidServiceMac::~HidServiceMac() {
StopWatchingDevices(); }
Ken Rockot wrote:
> That may be going too far though.
> 
> For now I'm going to let clang_format do its thing, and once the patch is
> otherwise ready, I'll be happy to unfold these one-liners before hitting the
CQ.

I agree. Don’t make the code intentionally nasty to avoid its being made nasty
in some other way.

https://codereview.chromium.org/161823002/diff/1210001/chrome/browser/extensi...
File chrome/browser/extensions/api/hid/hid_device_manager.cc (right):

https://codereview.chromium.org/161823002/diff/1210001/chrome/browser/extensi...
chrome/browser/extensions/api/hid/hid_device_manager.cc:14: static
base::LazyInstance<ProfileKeyedAPIFactory<HidDeviceManager> > g_factory =
This is only used by GetFactoryInstance, you can move it into that function.

https://codereview.chromium.org/161823002/diff/1210001/chrome/browser/extensi...
chrome/browser/extensions/api/hid/hid_device_manager.cc:88: int new_id = 0;
No need to initialize.

https://codereview.chromium.org/161823002/diff/1210001/chrome/browser/extensi...
chrome/browser/extensions/api/hid/hid_device_manager.cc:92: new_id =
next_resource_id_++;
Extreme paranoia, but before you hit this line, maybe DCHECK that
next_resource_id_ isn’t std::numeric_limits<int>::min().

https://codereview.chromium.org/161823002/diff/1210001/chrome/browser/extensi...
chrome/browser/extensions/api/hid/hid_device_manager.cc:97: device_ids_ =
new_device_ids;
This is a copy followed by an immediate discard of the source. How about a .swap
instead? Same on the next line.

https://codereview.chromium.org/161823002/diff/1210001/chrome/browser/extensi...
File chrome/browser/extensions/api/hid/hid_device_manager.h (right):

https://codereview.chromium.org/161823002/diff/1210001/chrome/browser/extensi...
chrome/browser/extensions/api/hid/hid_device_manager.h:11: #include <vector>
Unused in the .h. Move to the .cc.

[Ken Rockot(use gerrit already), 2014-02-25 21:30:00]

Cool. Addressed the comments so far.

https://codereview.chromium.org/161823002/diff/1210001/chrome/browser/extensi...
File chrome/browser/extensions/api/hid/hid_device_manager.cc (right):

https://codereview.chromium.org/161823002/diff/1210001/chrome/browser/extensi...
chrome/browser/extensions/api/hid/hid_device_manager.cc:14: static
base::LazyInstance<ProfileKeyedAPIFactory<HidDeviceManager> > g_factory =
On 2014/02/24 20:23:36, Mark Mentovai wrote:
> This is only used by GetFactoryInstance, you can move it into that function.

Done.

https://codereview.chromium.org/161823002/diff/1210001/chrome/browser/extensi...
chrome/browser/extensions/api/hid/hid_device_manager.cc:92: new_id =
next_resource_id_++;
On 2014/02/24 20:23:36, Mark Mentovai wrote:
> Extreme paranoia, but before you hit this line, maybe DCHECK that
> next_resource_id_ isn’t std::numeric_limits<int>::min().

Done.

https://codereview.chromium.org/161823002/diff/1210001/chrome/browser/extensi...
chrome/browser/extensions/api/hid/hid_device_manager.cc:97: device_ids_ =
new_device_ids;
On 2014/02/24 20:23:36, Mark Mentovai wrote:
> This is a copy followed by an immediate discard of the source. How about a
.swap
> instead? Same on the next line.

Done.

https://codereview.chromium.org/161823002/diff/1210001/chrome/browser/extensi...
File chrome/browser/extensions/api/hid/hid_device_manager.h (right):

https://codereview.chromium.org/161823002/diff/1210001/chrome/browser/extensi...
chrome/browser/extensions/api/hid/hid_device_manager.h:11: #include <vector>
On 2014/02/24 20:23:36, Mark Mentovai wrote:
> Unused in the .h. Move to the .cc.

Done.

[Mark Mentovai, 2014-02-26 17:41:43]

I don’t think there’s anything major here other than the one-definition rule
violation, but that’s an easy fix, so LGTM.

https://codereview.chromium.org/161823002/diff/1260001/device/hid/hid_connect...
File device/hid/hid_connection_linux.cc (right):

https://codereview.chromium.org/161823002/diff/1260001/device/hid/hid_connect...
device/hid/hid_connection_linux.cc:32: // Copies a buffer into a new one a
report ID byte inserted at the front.
This comment is missing a word?

https://codereview.chromium.org/161823002/diff/1260001/device/hid/hid_connect...
device/hid/hid_connection_linux.cc:144: write(device_file_.GetPlatformFile(),
buffer->data(), buffer->size());
Do you need to handle a short write or EINTR? Maybe not. I thought about this
for the read on line 97 too but it seems even less necessary there.

https://codereview.chromium.org/161823002/diff/1260001/device/hid/hid_connect...
device/hid/hid_connection_linux.cc:232: strlen(parent_path)) == 0 &&
Compute strlen(parent_path) outside this loop.

But: is this supposed to find paths that are children of one another on the
filesystem? If so, you should make sure that parent_path ends in a slash. If
it’s not guaranteed to, you should build a new string that does end in a slash
for the purposes of this comparison. If the path isn’t supposed to be a child
from a filesystem perspective, ignore this part of this comment.

https://codereview.chromium.org/161823002/diff/1260001/device/hid/hid_connect...
File device/hid/hid_connection_mac.cc (right):

https://codereview.chromium.org/161823002/diff/1260001/device/hid/hid_connect...
device/hid/hid_connection_mac.cc:22: CFRetain(device_.get());
I get that this is done because device_ is a ScopedCFTypeRef that this
HidConnectionMac would not otherwise own. But seeing a CFRetain unbalanced by a
CFRelease is scary. You can use the two-argument form of the ScopedCFTypeRef
constructor in the initializer list above, with base::scoped_policy::RETAIN, to
retain device_ the instant the scoper gets it, and to make a better assertion
about how the memory model for device_ is expected to behave.

https://codereview.chromium.org/161823002/diff/1260001/device/hid/hid_connect...
device/hid/hid_connection_mac.cc:89: HidConnectionMac::~HidConnectionMac() {
Usually the destructor would immediately follow the constructor.

https://codereview.chromium.org/161823002/diff/1260001/device/hid/hid_device_...
File device/hid/hid_device_info.h (right):

https://codereview.chromium.org/161823002/diff/1260001/device/hid/hid_device_...
device/hid/hid_device_info.h:30: const HidDeviceId kInvalidHidDeviceId;
Mark this extern and provide a definition in the .cc file.

[Ken Rockot(use gerrit already), 2014-02-26 22:36:39]

Thank you for the review, Mark.

+scheib@, can you please take a look as c/b/e owner?

https://codereview.chromium.org/161823002/diff/1260001/device/hid/hid_connect...
File device/hid/hid_connection_linux.cc (right):

https://codereview.chromium.org/161823002/diff/1260001/device/hid/hid_connect...
device/hid/hid_connection_linux.cc:32: // Copies a buffer into a new one a
report ID byte inserted at the front.
On 2014/02/26 17:41:44, Mark Mentovai wrote:
> This comment is missing a word?

Done.

https://codereview.chromium.org/161823002/diff/1260001/device/hid/hid_connect...
device/hid/hid_connection_linux.cc:144: write(device_file_.GetPlatformFile(),
buffer->data(), buffer->size());
On 2014/02/26 17:41:44, Mark Mentovai wrote:
> Do you need to handle a short write or EINTR? Maybe not. I thought about this
> for the read on line 97 too but it seems even less necessary there.

I lean against treating short reads or writes as error conditions; the consumer
can decide. I don't see any harm in handling EINTR though, so that's done.

https://codereview.chromium.org/161823002/diff/1260001/device/hid/hid_connect...
device/hid/hid_connection_linux.cc:232: strlen(parent_path)) == 0 &&
On 2014/02/26 17:41:44, Mark Mentovai wrote:
> Compute strlen(parent_path) outside this loop.
> 
> But: is this supposed to find paths that are children of one another on the
> filesystem? If so, you should make sure that parent_path ends in a slash. If
> it’s not guaranteed to, you should build a new string that does end in a slash
> for the purposes of this comparison. If the path isn’t supposed to be a child
> from a filesystem perspective, ignore this part of this comment.

Done.

https://codereview.chromium.org/161823002/diff/1260001/device/hid/hid_connect...
File device/hid/hid_connection_mac.cc (right):

https://codereview.chromium.org/161823002/diff/1260001/device/hid/hid_connect...
device/hid/hid_connection_mac.cc:22: CFRetain(device_.get());
On 2014/02/26 17:41:44, Mark Mentovai wrote:
> I get that this is done because device_ is a ScopedCFTypeRef that this
> HidConnectionMac would not otherwise own. But seeing a CFRetain unbalanced by
a
> CFRelease is scary. You can use the two-argument form of the ScopedCFTypeRef
> constructor in the initializer list above, with base::scoped_policy::RETAIN,
to
> retain device_ the instant the scoper gets it, and to make a better assertion
> about how the memory model for device_ is expected to behave.

Done.

https://codereview.chromium.org/161823002/diff/1260001/device/hid/hid_connect...
device/hid/hid_connection_mac.cc:89: HidConnectionMac::~HidConnectionMac() {
On 2014/02/26 17:41:44, Mark Mentovai wrote:
> Usually the destructor would immediately follow the constructor.

There's a conflict between this convention and the convention of retaining
consistent ordering between interface and implementation. Assuming ctor/dtor
pairing is higher priority and alternating public/private parts in the header is
undesirable, I've moved this.

https://codereview.chromium.org/161823002/diff/1260001/device/hid/hid_device_...
File device/hid/hid_device_info.h (right):

https://codereview.chromium.org/161823002/diff/1260001/device/hid/hid_device_...
device/hid/hid_device_info.h:30: const HidDeviceId kInvalidHidDeviceId;
On 2014/02/26 17:41:44, Mark Mentovai wrote:
> Mark this extern and provide a definition in the .cc file.

Done.

[Mark Mentovai, 2014-02-26 22:41:03]

https://codereview.chromium.org/161823002/diff/1300001/device/hid/hid_device_...
File device/hid/hid_device_info.cc (right):

https://codereview.chromium.org/161823002/diff/1300001/device/hid/hid_device_...
device/hid/hid_device_info.cc:10: const HidDeviceId kInvalidHidDeviceId;
Whoops, I just realized that this is a static constructor. I think we need to
make kInvalidHidDeviceId a const char* const, and rely on char*’s convertability
to std::string.

[Ken Rockot(use gerrit already), 2014-02-26 22:51:01]

https://codereview.chromium.org/161823002/diff/1300001/device/hid/hid_device_...
File device/hid/hid_device_info.cc (right):

https://codereview.chromium.org/161823002/diff/1300001/device/hid/hid_device_...
device/hid/hid_device_info.cc:10: const HidDeviceId kInvalidHidDeviceId;
On 2014/02/26 22:41:04, Mark Mentovai wrote:
> Whoops, I just realized that this is a static constructor. I think we need to
> make kInvalidHidDeviceId a const char* const, and rely on char*’s
convertability
> to std::string.

Doh. Right. Done.

[Mark Mentovai, 2014-02-26 22:57:39]

LGTM

[scheib, 2014-02-26 22:58:09]

lgtm

[Ken Rockot(use gerrit already), 2014-02-26 23:15:07]

The CQ bit was checked by rockot@chromium.org

[commit-bot: I haz the power, 2014-02-26 23:20:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rockot@chromium.org/161823002/1340001

[commit-bot: I haz the power, 2014-02-26 23:46:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-26 23:46:34]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[Ken Rockot(use gerrit already), 2014-02-26 23:53:25]

The CQ bit was checked by rockot@chromium.org

[commit-bot: I haz the power, 2014-02-26 23:56:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rockot@chromium.org/161823002/1360001

[commit-bot: I haz the power, 2014-02-27 05:24:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-27 05:24:22]

Try jobs failed on following builders: mac_chromium_rel

[Ken Rockot(use gerrit already), 2014-02-27 07:50:43]

The CQ bit was checked by rockot@chromium.org

[commit-bot: I haz the power, 2014-02-27 07:51:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rockot@chromium.org/161823002/1350034

[commit-bot: I haz the power, 2014-02-27 09:32:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-27 09:32:35]

Retried try job too often on win_rel for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[Ken Rockot(use gerrit already), 2014-02-27 16:08:59]

The CQ bit was checked by rockot@chromium.org

[commit-bot: I haz the power, 2014-02-27 16:10:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rockot@chromium.org/161823002/1350034

[commit-bot: I haz the power, 2014-02-27 17:29:13]

Change committed as 253853