HID backend

HID backend

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

[Bei Zhang, 2014-01-21 12:43:59]

Hi Ken,

Please review.

PS: If you think some files are just too horrible to review, please let me know
and I will refine it.

Thanks,
Bei

[Ken Rockot(use gerrit already), 2014-01-21 23:03:39]

Looking pretty good, comments inline

https://codereview.chromium.org/143883005/diff/1/device/hid/hid_connection.cc
File device/hid/hid_connection.cc (right):

https://codereview.chromium.org/143883005/diff/1/device/hid/hid_connection.cc...
device/hid/hid_connection.cc:16: }
nit: vertical whitespace

https://codereview.chromium.org/143883005/diff/1/device/hid/hid_connection.h
File device/hid/hid_connection.h (right):

https://codereview.chromium.org/143883005/diff/1/device/hid/hid_connection.h#...
device/hid/hid_connection.h:20: HidIOCallback;
nit: Maybe move into HidConnection as HidConnection::IOCallback?

https://codereview.chromium.org/143883005/diff/1/device/hid/hid_connection_win.h
File device/hid/hid_connection_win.h (right):

https://codereview.chromium.org/143883005/diff/1/device/hid/hid_connection_wi...
device/hid/hid_connection_win.h:20: public:
base::MessageLoopForIO::IOHandler essentially provides the same abstraction as
your AsyncTransfer.

See c/b/e/api/serial_io_handler_win.{h,cc} for an example usage

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

https://codereview.chromium.org/143883005/diff/1/device/hid/hid_service.h#new...
device/hid/hid_service.h:57: virtual void DeviceAdd(HidDeviceInfo info);
nit: I think verb-noun naming would be preferable: AddDevice and RemoveDevice
rather than DeviceAdd DeviceRemove.

Same goes for the PlatformDeviceAdd and PlatformDeviceRemove methods in platform
impls.

https://codereview.chromium.org/143883005/diff/1/device/hid/hid_service_linux.cc
File device/hid/hid_service_linux.cc (right):

https://codereview.chromium.org/143883005/diff/1/device/hid/hid_service_linux...
device/hid/hid_service_linux.cc:28: const char* kUdevName = "udev";
nit: Preference in Chrome code seems to be array form (i.e. const char kFoo[] =
...). I'm not sure if there is a good reason to prefer this, but there are
differences.

https://codereview.chromium.org/143883005/diff/1/device/hid/hid_service_linux...
device/hid/hid_service_linux.cc:209: 
TODO, at least? Or do you intend to implement in this CL?

https://codereview.chromium.org/143883005/diff/1/device/hid/hid_service_linux.h
File device/hid/hid_service_linux.h (right):

https://codereview.chromium.org/143883005/diff/1/device/hid/hid_service_linux...
device/hid/hid_service_linux.h:22: template<typename T, void func(T*)>
Can this struct and all the typedefs instead be private to HidServiceLinux?

https://codereview.chromium.org/143883005/diff/1/device/hid/hid_service_linux...
device/hid/hid_service_linux.h:23: struct GeneralDeleter {
naming nit: How about just Deleter? It's already general :)

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

https://codereview.chromium.org/143883005/diff/1/device/hid/hid_utils_mac.h#n...
device/hid/hid_utils_mac.h:14: 
Can you move these into a namespace or make the naming more specific to HID?
i.e. device::hid::GetIntProperty or device::GetHIDIntProperty?

[Ken Rockot(use gerrit already), 2014-01-21 23:08:18]

Forgot one comment: What about threading restrictions on HIDConnection? Should
it guarantee that Read/Write/etc are called from the FILE thread as well?

[Bei Zhang, 2014-01-24 12:48:47]

https://codereview.chromium.org/143883005/diff/1/device/hid/hid_connection.cc
File device/hid/hid_connection.cc (right):

https://codereview.chromium.org/143883005/diff/1/device/hid/hid_connection.cc...
device/hid/hid_connection.cc:16: }
On 2014/01/21 23:03:39, Ken Rockot wrote:
> nit: vertical whitespace

Done.

https://codereview.chromium.org/143883005/diff/1/device/hid/hid_connection.h
File device/hid/hid_connection.h (right):

https://codereview.chromium.org/143883005/diff/1/device/hid/hid_connection.h#...
device/hid/hid_connection.h:20: HidIOCallback;
On 2014/01/21 23:03:39, Ken Rockot wrote:
> nit: Maybe move into HidConnection as HidConnection::IOCallback?

Done.

https://codereview.chromium.org/143883005/diff/1/device/hid/hid_connection_win.h
File device/hid/hid_connection_win.h (right):

https://codereview.chromium.org/143883005/diff/1/device/hid/hid_connection_wi...
device/hid/hid_connection_win.h:20: public:
On 2014/01/21 23:03:39, Ken Rockot wrote:
> base::MessageLoopForIO::IOHandler essentially provides the same abstraction as
> your AsyncTransfer.
> 
> See c/b/e/api/serial_io_handler_win.{h,cc} for an example usage

As I tested, it only works on IO thread, were IO is not allowed...
The current approach works on any thread. I moved it into the HidServiceWin
class.

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

https://codereview.chromium.org/143883005/diff/1/device/hid/hid_service.h#new...
device/hid/hid_service.h:57: virtual void DeviceAdd(HidDeviceInfo info);
Right...

This name was inherited from the good old
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/ga...

Done.

On 2014/01/21 23:03:39, Ken Rockot wrote:
> nit: I think verb-noun naming would be preferable: AddDevice and RemoveDevice
> rather than DeviceAdd DeviceRemove.
> 
> Same goes for the PlatformDeviceAdd and PlatformDeviceRemove methods in
platform
> impls.

https://codereview.chromium.org/143883005/diff/1/device/hid/hid_service_linux.cc
File device/hid/hid_service_linux.cc (right):

https://codereview.chromium.org/143883005/diff/1/device/hid/hid_service_linux...
device/hid/hid_service_linux.cc:28: const char* kUdevName = "udev";
Yeah right... I think it's because this way you can get the size of the string
pretty easily. Nice catch.

Done.

On 2014/01/21 23:03:39, Ken Rockot wrote:
> nit: Preference in Chrome code seems to be array form (i.e. const char kFoo[]
=
> ...). I'm not sure if there is a good reason to prefer this, but there are
> differences.

https://codereview.chromium.org/143883005/diff/1/device/hid/hid_service_linux...
device/hid/hid_service_linux.cc:209: 
On 2014/01/21 23:03:39, Ken Rockot wrote:
> TODO, at least? Or do you intend to implement in this CL?

Done.

https://codereview.chromium.org/143883005/diff/1/device/hid/hid_service_linux.h
File device/hid/hid_service_linux.h (right):

https://codereview.chromium.org/143883005/diff/1/device/hid/hid_service_linux...
device/hid/hid_service_linux.h:22: template<typename T, void func(T*)>
I believe the whole file is pretty private. Since they are all typedefs and
template, no symbols are exported.

On 2014/01/21 23:03:39, Ken Rockot wrote:
> Can this struct and all the typedefs instead be private to HidServiceLinux?

https://codereview.chromium.org/143883005/diff/1/device/hid/hid_service_linux...
device/hid/hid_service_linux.h:23: struct GeneralDeleter {
On 2014/01/21 23:03:39, Ken Rockot wrote:
> naming nit: How about just Deleter? It's already general :)

Wonderful!!! Now I can fit everything in 80chs.

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

https://codereview.chromium.org/143883005/diff/1/device/hid/hid_utils_mac.h#n...
device/hid/hid_utils_mac.h:14: 
On 2014/01/21 23:03:39, Ken Rockot wrote:
> Can you move these into a namespace or make the naming more specific to HID?
> i.e. device::hid::GetIntProperty or device::GetHIDIntProperty?

Done.

[Ken Rockot(use gerrit already), 2014-01-28 23:30:50]

I reviewed this for Bei and everything lgtm now. Need owner.

[miket_OOO, 2014-01-29 20:15:31]

On 2014/01/28 23:30:50, Ken Rockot wrote:
> I reviewed this for Bei and everything lgtm now. Need owner.

OWNER LGTM see nits

[commit-bot: I haz the power, 2014-01-29 20:48:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ikarienator@chromium.org/143883005/550001

[commit-bot: I haz the power, 2014-01-29 21:16:13]

Failed to trigger a try job on win_rel
HTTP Error 400: Bad Request

[commit-bot: I haz the power, 2014-01-29 21:16:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ikarienator@chromium.org/143883005/570001

[commit-bot: I haz the power, 2014-01-29 22:50:32]

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

[commit-bot: I haz the power, 2014-01-29 23:09:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ikarienator@chromium.org/143883005/590001

[commit-bot: I haz the power, 2014-01-30 04:38:29]

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

[commit-bot: I haz the power, 2014-01-30 06:00:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ikarienator@chromium.org/143883005/610001

[commit-bot: I haz the power, 2014-01-30 07:47:55]

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

[commit-bot: I haz the power, 2014-01-30 08:33:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ikarienator@chromium.org/143883005/610001

[commit-bot: I haz the power, 2014-01-30 10:41:09]

Change committed as 247926

[miket_OOO, 2014-02-03 18:42:18]

Sorry, pressed wrong button. Please review these suggestions.

https://codereview.chromium.org/143883005/diff/450001/device/hid/hid.gyp
File device/hid/hid.gyp (right):

https://codereview.chromium.org/143883005/diff/450001/device/hid/hid.gyp#newc...
device/hid/hid.gyp:1: # Copyright 2013 The Chromium Authors. All rights
reserved.
2014 new file

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

https://codereview.chromium.org/143883005/diff/450001/device/hid/hid_connecti...
device/hid/hid_connection.h:42: HidConnection(HidDeviceInfo device_info);
explicit?

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

https://codereview.chromium.org/143883005/diff/450001/device/hid/hid_connecti...
device/hid/hid_connection_mac.cc:118: /* Not using numbered Reports.
prefer //

https://codereview.chromium.org/143883005/diff/450001/device/hid/hid_connecti...
File device/hid/hid_connection_win.cc (right):

https://codereview.chromium.org/143883005/diff/450001/device/hid/hid_connecti...
device/hid/hid_connection_win.cc:78: DWORD bytes_transfered;
I think the preferred spelling has a double-r, not a huge deal

https://codereview.chromium.org/143883005/diff/450001/device/hid/hid_connecti...
device/hid/hid_connection_win.cc:133: if (size + !device_info_.has_report_id <
report_size) {
that is weird logic (boolean cast to int)

https://codereview.chromium.org/143883005/diff/450001/device/hid/hid_connecti...
File device/hid/hid_connection_win.h (right):

https://codereview.chromium.org/143883005/diff/450001/device/hid/hid_connecti...
device/hid/hid_connection_win.h:63: HidConnectionWin(HidDeviceInfo device_info);
explicit?

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

https://codereview.chromium.org/143883005/diff/450001/device/hid/hid_utils_ma...
device/hid/hid_utils_mac.h:18: CFStringRef key,
indent

[Jorge Lucangeli Obes, 2014-02-05 18:06:57]

On 2014/02/03 18:42:18, miket wrote:
> Sorry, pressed wrong button. Please review these suggestions.
> 
> https://codereview.chromium.org/143883005/diff/450001/device/hid/hid.gyp
> File device/hid/hid.gyp (right):
> 
>
https://codereview.chromium.org/143883005/diff/450001/device/hid/hid.gyp#newc...
> device/hid/hid.gyp:1: # Copyright 2013 The Chromium Authors. All rights
> reserved.
> 2014 new file
> 
>
https://codereview.chromium.org/143883005/diff/450001/device/hid/hid_connecti...
> File device/hid/hid_connection.h (right):
> 
>
https://codereview.chromium.org/143883005/diff/450001/device/hid/hid_connecti...
> device/hid/hid_connection.h:42: HidConnection(HidDeviceInfo device_info);
> explicit?
> 
>
https://codereview.chromium.org/143883005/diff/450001/device/hid/hid_connecti...
> File device/hid/hid_connection_mac.cc (right):
> 
>
https://codereview.chromium.org/143883005/diff/450001/device/hid/hid_connecti...
> device/hid/hid_connection_mac.cc:118: /* Not using numbered Reports.
> prefer //
> 
>
https://codereview.chromium.org/143883005/diff/450001/device/hid/hid_connecti...
> File device/hid/hid_connection_win.cc (right):
> 
>
https://codereview.chromium.org/143883005/diff/450001/device/hid/hid_connecti...
> device/hid/hid_connection_win.cc:78: DWORD bytes_transfered;
> I think the preferred spelling has a double-r, not a huge deal
> 
>
https://codereview.chromium.org/143883005/diff/450001/device/hid/hid_connecti...
> device/hid/hid_connection_win.cc:133: if (size + !device_info_.has_report_id <
> report_size) {
> that is weird logic (boolean cast to int)
> 
>
https://codereview.chromium.org/143883005/diff/450001/device/hid/hid_connecti...
> File device/hid/hid_connection_win.h (right):
> 
>
https://codereview.chromium.org/143883005/diff/450001/device/hid/hid_connecti...
> device/hid/hid_connection_win.h:63: HidConnectionWin(HidDeviceInfo
device_info);
> explicit?
> 
>
https://codereview.chromium.org/143883005/diff/450001/device/hid/hid_utils_mac.h
> File device/hid/hid_utils_mac.h (right):
> 
>
https://codereview.chromium.org/143883005/diff/450001/device/hid/hid_utils_ma...
> device/hid/hid_utils_mac.h:18: CFStringRef key,
> indent

There were significant concerns with certain implementation details on
https://codereview.chromium.org/25666010/, so instead of addressing that you
went ahead and landed a CL excluding the reviewers that expressed those
concerns?

(Specifically, the use of a dedicated thread on the Mac implementation.)

[Mark Mentovai, 2014-02-12 19:35:58]

I was asked to do a post-commit review of the Mac design and implementation.
Here are my preliminary thoughts after only looking at hid_service_mac.cc. My
major question is “what’s this extra thread about?” but I have many more
questions and comments inline, and I’m sure I’ll have more once I do a closer
read-through and look at the other files.

Note that I don’t think that it’s actually a good idea to do this post-commit
review on the same CL as the original codereview issue. It’s just that I don’t
have a better place to put these comments until a new codereview issue is opened
up. You start a new codereview issue as you begin to look at these questions,
and we should move our discussion to there.

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

https://codereview.chromium.org/143883005/diff/650001/device/hid/hid_service_...
device/hid/hid_service_mac.cc:7: #include <map>
Unused.

https://codereview.chromium.org/143883005/diff/650001/device/hid/hid_service_...
device/hid/hid_service_mac.cc:8: #include <set>
Unused.

https://codereview.chromium.org/143883005/diff/650001/device/hid/hid_service_...
device/hid/hid_service_mac.cc:24: #include <CoreFoundation/CoreFoundation.h>
C system headers before C++ system headers. This is in the style guide.

https://codereview.chromium.org/143883005/diff/650001/device/hid/hid_service_...
device/hid/hid_service_mac.cc:32: base::ThreadRestrictions::AssertIOAllowed();
Which thread exactly are you expecting this to run on?

https://codereview.chromium.org/143883005/diff/650001/device/hid/hid_service_...
device/hid/hid_service_mac.cc:34:
hid_manager_ref_.reset(IOHIDManagerCreate(kCFAllocatorDefault,
It’s normal to write NULL, which has the same meaning as kCFAllocatorDefault.

https://codereview.chromium.org/143883005/diff/650001/device/hid/hid_service_...
device/hid/hid_service_mac.cc:35: kIOHIDOptionsTypeNone));
This is an option value for IOHIDManagerOpen, but you are calling
IOHIDManagerCreate. You want kIOHIDManagerOptionNone.

https://codereview.chromium.org/143883005/diff/650001/device/hid/hid_service_...
device/hid/hid_service_mac.cc:37: CFGetTypeID(hid_manager_ref_) !=
IOHIDManagerGetTypeID()) {
Does this ever actually happen, or is it just paranoia?

https://codereview.chromium.org/143883005/diff/650001/device/hid/hid_service_...
device/hid/hid_service_mac.cc:41: CFRetain(hid_manager_ref_);
IOHIDManagerCreate gave you a reference already, why do you need a second one?

https://codereview.chromium.org/143883005/diff/650001/device/hid/hid_service_...
device/hid/hid_service_mac.cc:47: &HidServiceMac::AddDeviceCallback,
HidServiceMac:: is implied. Same on line 51.

https://codereview.chromium.org/143883005/diff/650001/device/hid/hid_service_...
device/hid/hid_service_mac.cc:54: // Blocking operation to enumerate all the
pre-existing devices.
What about Enumerate is blocking?

https://codereview.chromium.org/143883005/diff/650001/device/hid/hid_service_...
device/hid/hid_service_mac.cc:63: // run potentially blocking routines on it.
What might block?

https://codereview.chromium.org/143883005/diff/650001/device/hid/hid_service_...
device/hid/hid_service_mac.cc:109: HidServiceMac*
HidServiceMac::InstanceFromContext(void* context) {
private + static + no need to access member data = no need to be a (static)
class member function at all. It can just be a simple anonymous-namespaced
helper function in this file. Fight interface clutter!

https://codereview.chromium.org/143883005/diff/650001/device/hid/hid_service_...
device/hid/hid_service_mac.cc:116: IOHIDDeviceRef ref) {
Everything in the world of CF is a reference. You’ve used the name “ref”
everywhere. It’s about the least descriptive name possible. This one, along with
many others, should be “hid_device”. You should go through your code and make
sure you don’t have anything named “ref” (or variants like “raw_ref”). In cases
where you have names like “hid_manager_ref_”, the _ref is extraneous noise and
should be removed.

https://codereview.chromium.org/143883005/diff/650001/device/hid/hid_service_...
device/hid/hid_service_mac.cc:143: IOHIDDeviceRef
HidServiceMac::FindDevice(std::string id) {
If you were just using the location ID as an internal identifier to correlate
device removals to additions, then it would probably be fine. However, seeing it
used as a part of the API makes it seem like other code might be hanging on to
these “platform device IDs” and using them for other purposes, such as trying to
accurately identify a specific device. It could be desirable for there to be
persistence in a single device’s “platform device ID” across removals and
additions. In that case, the location ID might not be the best choice.

I haven’t read through this entire change or the expected behavior of consumers
yet, so I don’t know if this is actually a problem, but it’s something that we
can think about and discuss. In that case, there may be a better way to assign
“platform device IDs” based on other properties of the device—if none of the
other, better properties are available, you can always fall back to the
relatively unsophisticated location ID.

https://codereview.chromium.org/143883005/diff/650001/device/hid/hid_service_...
device/hid/hid_service_mac.cc:143: IOHIDDeviceRef
HidServiceMac::FindDevice(std::string id) {
More thoughts on location ID:

Enumerating devices in this manner on my laptop shows four “Apple Internal
Keyboard/Trackpad” devices at the same location ID. If I plug in an external
keyboard, I get two more “Apple Keyboard” devices at their own location ID.

Do all of these entries need to be tracked individually?

What if some of the entries at a single common location, but not all of them,
wind up getting removed?

https://codereview.chromium.org/143883005/diff/650001/device/hid/hid_service_...
device/hid/hid_service_mac.cc:146: CFIndex count = CFSetGetCount(devices);
Hmm…this is kind of cumbersome.

https://codereview.chromium.org/143883005/diff/650001/device/hid/hid_service_...
device/hid/hid_service_mac.cc:150: for (CFIndex i = 0; i < count; i++) {
Usually use preincrement. Line 170 also.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Preinc...

https://codereview.chromium.org/143883005/diff/650001/device/hid/hid_service_...
device/hid/hid_service_mac.cc:153: &int_property)) {
Weird indentation.

https://codereview.chromium.org/143883005/diff/650001/device/hid/hid_service_...
device/hid/hid_service_mac.cc:154: if (id == base::HexEncode(&int_property,
sizeof(int_property))) {
What’s with the weird stringification if these are always just ints on this
platform? device_id (and platform_device_id) should probably not be of type
std::string all the time. The platform should decide what type to use, and Mac
should be using a smarter type, such as uint32_t.

https://codereview.chromium.org/143883005/diff/650001/device/hid/hid_service_...
device/hid/hid_service_mac.cc:166: CFIndex count = CFSetGetCount(devices);
This is cumbersome just like line 146, and now you’re doing it a second time.
Code duplication looks like a good opportunity for refactoring.

https://codereview.chromium.org/143883005/diff/650001/device/hid/hid_service_...
device/hid/hid_service_mac.cc:171: // Takes ownership. Will be released inside
PlatformDeviceAdd.
The function is actually named PlatformAddDevice.

https://codereview.chromium.org/143883005/diff/650001/device/hid/hid_service_...
device/hid/hid_service_mac.cc:182: // Auto-release.
“Autorelease” has a special meaning in Mac programming. This is not autorelease.
Remove the comment, because it should be obvious what the next line does
(especially after you rename the variables).

https://codereview.chromium.org/143883005/diff/650001/device/hid/hid_service_...
device/hid/hid_service_mac.cc:193: if (GetHidIntProperty(ref,
CFSTR(kIOHIDVendorIDKey), &int_property)) {
All of this “if this succeeded then use its value” testing is getting kind of
cumbersome too. Since none of these calls seem to care about the failure case,
you don’t actually care about testing success or failure. You need versions of
GetHidIntProperty and GetHidStringProperty that simply return their value, or a
default value (0 or an empty string) if the “get” fails.

https://codereview.chromium.org/143883005/diff/650001/device/hid/hid_service_...
device/hid/hid_service_mac.cc:214: &int_property)) {
Weird indentation

https://codereview.chromium.org/143883005/diff/650001/device/hid/hid_service_...
device/hid/hid_service_mac.cc:229: // Auto-release.
Nope.

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

https://codereview.chromium.org/143883005/diff/650001/device/hid/hid_service_...
device/hid/hid_service_mac.h:56: IOHIDDeviceRef FindDevice(std::string id);
The name “id” will cause trouble if this is #included into an Objective-C++
(.mm) file. Choose a different name. device_id is good, since that’s what you
used for Connect. Or you may want to use platform_device_id, since that’s what
the superclass uses in Connect. (Unless there’s a good reason to give an
argument a different name in a base and derived class declaration, give them the
same name. Gratuitous changes are confusing.)

[Ken Rockot(use gerrit already), 2014-02-13 00:04:54]

Thank you for the review, Mark. Leaving replies inline on this CL, but the new
code is over at https://codereview.chromium.org/161823002/

All the small stuff has been addressed, and I've tentatively axed the extra
thread. The device identification question remains open for now.

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

https://codereview.chromium.org/143883005/diff/650001/device/hid/hid_service_...
device/hid/hid_service_mac.cc:7: #include <map>
On 2014/02/12 19:35:59, Mark Mentovai wrote:
> Unused.

Done.

https://codereview.chromium.org/143883005/diff/650001/device/hid/hid_service_...
device/hid/hid_service_mac.cc:8: #include <set>
On 2014/02/12 19:35:59, Mark Mentovai wrote:
> Unused.

Done.

https://codereview.chromium.org/143883005/diff/650001/device/hid/hid_service_...
device/hid/hid_service_mac.cc:24: #include <CoreFoundation/CoreFoundation.h>
On 2014/02/12 19:35:59, Mark Mentovai wrote:
> C system headers before C++ system headers. This is in the style guide.

Done.

https://codereview.chromium.org/143883005/diff/650001/device/hid/hid_service_...
device/hid/hid_service_mac.cc:32: base::ThreadRestrictions::AssertIOAllowed();
On 2014/02/12 19:35:59, Mark Mentovai wrote:
> Which thread exactly are you expecting this to run on?

FILE thread

https://codereview.chromium.org/143883005/diff/650001/device/hid/hid_service_...
device/hid/hid_service_mac.cc:34:
hid_manager_ref_.reset(IOHIDManagerCreate(kCFAllocatorDefault,
On 2014/02/12 19:35:59, Mark Mentovai wrote:
> It’s normal to write NULL, which has the same meaning as kCFAllocatorDefault.

Done.

https://codereview.chromium.org/143883005/diff/650001/device/hid/hid_service_...
device/hid/hid_service_mac.cc:35: kIOHIDOptionsTypeNone));
On 2014/02/12 19:35:59, Mark Mentovai wrote:
> This is an option value for IOHIDManagerOpen, but you are calling
> IOHIDManagerCreate. You want kIOHIDManagerOptionNone.

Done.

https://codereview.chromium.org/143883005/diff/650001/device/hid/hid_service_...
device/hid/hid_service_mac.cc:37: CFGetTypeID(hid_manager_ref_) !=
IOHIDManagerGetTypeID()) {
On 2014/02/12 19:35:59, Mark Mentovai wrote:
> Does this ever actually happen, or is it just paranoia?

As far as I can tell, paranoia.

https://codereview.chromium.org/143883005/diff/650001/device/hid/hid_service_...
device/hid/hid_service_mac.cc:41: CFRetain(hid_manager_ref_);
On 2014/02/12 19:35:59, Mark Mentovai wrote:
> IOHIDManagerCreate gave you a reference already, why do you need a second one?

Don't. Done.

https://codereview.chromium.org/143883005/diff/650001/device/hid/hid_service_...
device/hid/hid_service_mac.cc:47: &HidServiceMac::AddDeviceCallback,
On 2014/02/12 19:35:59, Mark Mentovai wrote:
> HidServiceMac:: is implied. Same on line 51.

Done.

https://codereview.chromium.org/143883005/diff/650001/device/hid/hid_service_...
device/hid/hid_service_mac.cc:54: // Blocking operation to enumerate all the
pre-existing devices.
On 2014/02/12 19:35:59, Mark Mentovai wrote:
> What about Enumerate is blocking?

I think in this case the original concern (which seems questionable to me) was
that IOHIDManagerCopyDevices may perform potentially blocking I/O operations.

https://codereview.chromium.org/143883005/diff/650001/device/hid/hid_service_...
device/hid/hid_service_mac.cc:63: // run potentially blocking routines on it.
On 2014/02/12 19:35:59, Mark Mentovai wrote:
> What might block?

In this case I can only imagine the concern was that IOHIDDeviceGetProperty
could block for some reason. Is this not the case?

If not, and if IOHIDManagerCopyDevices is also safe, I'm actually a little
confounded as to why the UI thread wouldn't be suitable for all of this work.

https://codereview.chromium.org/143883005/diff/650001/device/hid/hid_service_...
device/hid/hid_service_mac.cc:109: HidServiceMac*
HidServiceMac::InstanceFromContext(void* context) {
On 2014/02/12 19:35:59, Mark Mentovai wrote:
> private + static + no need to access member data = no need to be a (static)
> class member function at all. It can just be a simple anonymous-namespaced
> helper function in this file. Fight interface clutter!

Done.

https://codereview.chromium.org/143883005/diff/650001/device/hid/hid_service_...
device/hid/hid_service_mac.cc:116: IOHIDDeviceRef ref) {
On 2014/02/12 19:35:59, Mark Mentovai wrote:
> Everything in the world of CF is a reference. You’ve used the name “ref”
> everywhere. It’s about the least descriptive name possible. This one, along
with
> many others, should be “hid_device”. You should go through your code and make
> sure you don’t have anything named “ref” (or variants like “raw_ref”). In
cases
> where you have names like&nbsp;“hid_manager_ref_”, the _ref is extraneous
noise and
> should be removed.

Done.

https://codereview.chromium.org/143883005/diff/650001/device/hid/hid_service_...
device/hid/hid_service_mac.cc:143: IOHIDDeviceRef
HidServiceMac::FindDevice(std::string id) {
On 2014/02/12 19:35:59, Mark Mentovai wrote:
> More thoughts on location ID:
> 
> Enumerating devices in this manner on my laptop shows four “Apple Internal
> Keyboard/Trackpad” devices at the same location ID. If I plug in an external
> keyboard, I get two more “Apple Keyboard” devices at their own location ID.
> 
> Do all of these entries need to be tracked individually?
> 
> What if some of the entries at a single common location, but not all of them,
> wind up getting removed?

Leaving this question open for now.

https://codereview.chromium.org/143883005/diff/650001/device/hid/hid_service_...
device/hid/hid_service_mac.cc:150: for (CFIndex i = 0; i < count; i++) {
On 2014/02/12 19:35:59, Mark Mentovai wrote:
> Usually use preincrement. Line 170 also.
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Preinc...

Done.

https://codereview.chromium.org/143883005/diff/650001/device/hid/hid_service_...
device/hid/hid_service_mac.cc:153: &int_property)) {
On 2014/02/12 19:35:59, Mark Mentovai wrote:
> Weird indentation.

Ew.

https://codereview.chromium.org/143883005/diff/650001/device/hid/hid_service_...
device/hid/hid_service_mac.cc:166: CFIndex count = CFSetGetCount(devices);
On 2014/02/12 19:35:59, Mark Mentovai wrote:
> This is cumbersome just like line 146, and now you’re doing it a second time.
> Code duplication looks like a good opportunity for refactoring.

Done.

https://codereview.chromium.org/143883005/diff/650001/device/hid/hid_service_...
device/hid/hid_service_mac.cc:171: // Takes ownership. Will be released inside
PlatformDeviceAdd.
On 2014/02/12 19:35:59, Mark Mentovai wrote:
> The function is actually named PlatformAddDevice.

Done.

https://codereview.chromium.org/143883005/diff/650001/device/hid/hid_service_...
device/hid/hid_service_mac.cc:182: // Auto-release.
On 2014/02/12 19:35:59, Mark Mentovai wrote:
> “Autorelease”&nbsp;has a special meaning in Mac programming. This is not
autorelease.
> Remove the comment, because it should be obvious what the next line does
> (especially after you rename the variables).

Done.

https://codereview.chromium.org/143883005/diff/650001/device/hid/hid_service_...
device/hid/hid_service_mac.cc:193: if (GetHidIntProperty(ref,
CFSTR(kIOHIDVendorIDKey), &int_property)) {
On 2014/02/12 19:35:59, Mark Mentovai wrote:
> All of this “if this succeeded then use its value” testing is getting kind of
> cumbersome too. Since none of these calls seem to care about the failure case,
> you don’t actually care about testing success or failure. You need versions of
> GetHidIntProperty and GetHidStringProperty that simply return their value, or
a
> default value (0 or an empty string) if the “get” fails.

Done.

https://codereview.chromium.org/143883005/diff/650001/device/hid/hid_service_...
device/hid/hid_service_mac.cc:214: &int_property)) {
On 2014/02/12 19:35:59, Mark Mentovai wrote:
> Weird indentation

Done.

https://codereview.chromium.org/143883005/diff/650001/device/hid/hid_service_...
device/hid/hid_service_mac.cc:214: &int_property)) {
On 2014/02/12 19:35:59, Mark Mentovai wrote:
> Weird indentation

Done.

https://codereview.chromium.org/143883005/diff/650001/device/hid/hid_service_...
device/hid/hid_service_mac.cc:229: // Auto-release.
On 2014/02/12 19:35:59, Mark Mentovai wrote:
> Nope.

Done.

https://codereview.chromium.org/143883005/diff/650001/device/hid/hid_service_...
device/hid/hid_service_mac.cc:229: // Auto-release.
On 2014/02/12 19:35:59, Mark Mentovai wrote:
> Nope.

Done.