HID backend

device/hid/hid_service_mac.cc

+// Copyright (c) 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "device/hid/hid_service_mac.h"
+
+#include <map>

[Mark Mentovai, 2014/02/12 19:35:59]

Unused.

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

Done.

+#include <set>

[Mark Mentovai, 2014/02/12 19:35:59]

Unused.

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

Done.

+#include <string>
+
+#include "base/bind.h"
+#include "base/callback.h"
+#include "base/lazy_instance.h"
+#include "base/logging.h"
+#include "base/mac/foundation_util.h"
+#include "base/memory/scoped_vector.h"
+#include "base/strings/string_number_conversions.h"
+#include "base/threading/thread_restrictions.h"
+#include "device/hid/hid_connection.h"
+#include "device/hid/hid_connection_mac.h"
+#include "device/hid/hid_utils_mac.h"
+#include "net/base/io_buffer.h"
+
+#include <CoreFoundation/CoreFoundation.h>

[Mark Mentovai, 2014/02/12 19:35:59]

C system headers before C++ system headers. This is in the style guide.

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

Done.

+#include <IOKit/hid/IOHIDManager.h>
+
+namespace device {
+
+class HidServiceMac;
+
+HidServiceMac::HidServiceMac() : enumeration_runloop_init_(true, false) {
+  base::ThreadRestrictions::AssertIOAllowed();

[Mark Mentovai, 2014/02/12 19:35:59]

Which thread exactly are you expecting this to run on?

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

FILE thread

+
+  hid_manager_ref_.reset(IOHIDManagerCreate(kCFAllocatorDefault,

[Mark Mentovai, 2014/02/12 19:35:59]

It’s normal to write NULL, which has the same meaning as kCFAllocatorDefault.

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

Done.

+                                            kIOHIDOptionsTypeNone));

[Mark Mentovai, 2014/02/12 19:35:59]

This is an option value for IOHIDManagerOpen, but you are calling
IOHIDManagerCreate. You want kIOHIDManagerOptionNone.

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

Done.

+  if (!hid_manager_ref_ ||
+      CFGetTypeID(hid_manager_ref_) != IOHIDManagerGetTypeID()) {

[Mark Mentovai, 2014/02/12 19:35:59]

Does this ever actually happen, or is it just paranoia?

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

As far as I can tell, paranoia.

+    LOG(ERROR) << "Failed to initialize HidManager";
+    return;
+  }
+  CFRetain(hid_manager_ref_);

[Mark Mentovai, 2014/02/12 19:35:59]

IOHIDManagerCreate gave you a reference already, why do you need a second one?

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

Don't. Done.

+
+  // Register for plug/unplug notifications.
+  IOHIDManagerSetDeviceMatching(hid_manager_ref_.get(), NULL);
+  IOHIDManagerRegisterDeviceMatchingCallback(
+      hid_manager_ref_.get(),
+      &HidServiceMac::AddDeviceCallback,

[Mark Mentovai, 2014/02/12 19:35:59]

HidServiceMac:: is implied. Same on line 51.

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

Done.

+      this);
+  IOHIDManagerRegisterDeviceRemovalCallback(
+      hid_manager_ref_.get(),
+      &HidServiceMac::RemoveDeviceCallback,
+      this);
+
+  // Blocking operation to enumerate all the pre-existing devices.

[Mark Mentovai, 2014/02/12 19:35:59]

What about Enumerate is blocking?

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

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

+  Enumerate();
+
+  // Save the owner message loop.
+  message_loop_ = base::MessageLoopProxy::current();
+
+  // Start a thread to monitor HID device changes.
+  // The reason to create a new thread is that by default the only thread in the
+  // browser process having a CFRunLoop is the UI thread. We do not want to
+  // run potentially blocking routines on it.

[Mark Mentovai, 2014/02/12 19:35:59]

What might block?

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

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.

+  enumeration_runloop_thread_.reset(
+      new base::Thread("HidService Device Enumeration Thread"));
+
+  base::Thread::Options options;
+  options.message_loop_type = base::MessageLoop::TYPE_UI;
+  enumeration_runloop_thread_->StartWithOptions(options);
+  enumeration_runloop_thread_->message_loop()->PostTask(
+      FROM_HERE,
+      base::Bind(&HidServiceMac::ScheduleRunLoop, base::Unretained(this)));
+
+  enumeration_runloop_init_.Wait();
+  initialized_ = true;
+}
+
+HidServiceMac::~HidServiceMac() {
+  enumeration_runloop_thread_->message_loop()->PostTask(
+        FROM_HERE,
+        base::Bind(&HidServiceMac::UnscheduleRunLoop, base::Unretained(this)));
+
+  enumeration_runloop_thread_->Stop();
+}
+
+void HidServiceMac::ScheduleRunLoop() {
+  enumeration_runloop_ = CFRunLoopGetCurrent();
+
+  IOHIDManagerScheduleWithRunLoop(
+      hid_manager_ref_,
+      enumeration_runloop_,
+      kCFRunLoopDefaultMode);
+
+  IOHIDManagerOpen(hid_manager_ref_, kIOHIDOptionsTypeNone);
+
+  enumeration_runloop_init_.Signal();
+}
+
+void HidServiceMac::UnscheduleRunLoop() {
+  IOHIDManagerUnscheduleFromRunLoop(
+        hid_manager_ref_,
+        enumeration_runloop_,
+        kCFRunLoopDefaultMode);
+
+  IOHIDManagerClose(hid_manager_ref_, kIOHIDOptionsTypeNone);
+
+}
+
+HidServiceMac* HidServiceMac::InstanceFromContext(void* context) {

[Mark Mentovai, 2014/02/12 19:35:59]

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!

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

Done.

+  return reinterpret_cast<HidServiceMac*>(context);
+}
+
+void HidServiceMac::AddDeviceCallback(void* context,
+                                      IOReturn result,
+                                      void* sender,
+                                      IOHIDDeviceRef ref) {

[Mark Mentovai, 2014/02/12 19:35:59]

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.

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

Done.

+  HidServiceMac* service = InstanceFromContext(context);
+
+  // Takes ownership of ref. Will be released inside PlatformDeviceAdd.
+  CFRetain(ref);
+  service->message_loop_->PostTask(
+      FROM_HERE,
+      base::Bind(&HidServiceMac::PlatformAddDevice,
+                 base::Unretained(service),
+                 base::Unretained(ref)));
+}
+
+void HidServiceMac::RemoveDeviceCallback(void* context,
+                                         IOReturn result,
+                                         void* sender,
+                                         IOHIDDeviceRef ref) {
+  HidServiceMac* service = InstanceFromContext(context);
+
+  // Takes ownership of ref. Will be released inside PlatformDeviceRemove.
+  CFRetain(ref);
+  service->message_loop_->PostTask(
+      FROM_HERE,
+      base::Bind(&HidServiceMac::PlatformRemoveDevice,
+                 base::Unretained(service),
+                 base::Unretained(ref)));
+}
+
+IOHIDDeviceRef HidServiceMac::FindDevice(std::string id) {

[Mark Mentovai, 2014/02/12 19:35:59]

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.

[Mark Mentovai, 2014/02/12 19:35:59]

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?

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

Leaving this question open for now.

+  base::ScopedCFTypeRef<CFSetRef> devices(
+      IOHIDManagerCopyDevices(hid_manager_ref_));
+  CFIndex count = CFSetGetCount(devices);

[Mark Mentovai, 2014/02/12 19:35:59]

Hmm…this is kind of cumbersome.

+  scoped_ptr<IOHIDDeviceRef[]> device_refs(new IOHIDDeviceRef[count]);
+  CFSetGetValues(devices, (const void **)(device_refs.get()));
+
+  for (CFIndex i = 0; i < count; i++) {

[Mark Mentovai, 2014/02/12 19:35:59]

Usually use preincrement. Line 170 also.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Preinc...

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

Done.

+    int32_t int_property = 0;
+    if (GetHidIntProperty(device_refs[i], CFSTR(kIOHIDLocationIDKey),
+                       &int_property)) {

[Mark Mentovai, 2014/02/12 19:35:59]

Weird indentation.

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

Ew.

+      if (id == base::HexEncode(&int_property, sizeof(int_property))) {

[Mark Mentovai, 2014/02/12 19:35:59]

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.

+        return device_refs[i];
+      }
+    }
+  }
+
+  return NULL;
+}
+
+void HidServiceMac::Enumerate() {
+  base::ScopedCFTypeRef<CFSetRef> devices(
+      IOHIDManagerCopyDevices(hid_manager_ref_));
+  CFIndex count = CFSetGetCount(devices);

[Mark Mentovai, 2014/02/12 19:35:59]

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.

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

Done.

+  scoped_ptr<IOHIDDeviceRef[]> device_refs(new IOHIDDeviceRef[count]);
+  CFSetGetValues(devices, (const void **)(device_refs.get()));
+
+  for (CFIndex i = 0; i < count; i++) {
+    // Takes ownership. Will be released inside PlatformDeviceAdd.

[Mark Mentovai, 2014/02/12 19:35:59]

The function is actually named PlatformAddDevice.

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

Done.

+    CFRetain(device_refs[i]);
+    PlatformAddDevice(device_refs[i]);
+  }
+}
+
+void HidServiceMac::PlatformAddDevice(IOHIDDeviceRef raw_ref) {
+  HidDeviceInfo device;
+  int32_t int_property = 0;
+  std::string str_property;
+
+  // Auto-release.

[Mark Mentovai, 2014/02/12 19:35:59]

“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).

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

Done.

+  base::ScopedCFTypeRef<IOHIDDeviceRef> ref(raw_ref);
+
+  // Unique identifier for HID device.
+  if (GetHidIntProperty(ref, CFSTR(kIOHIDLocationIDKey), &int_property)) {
+    device.device_id = base::HexEncode(&int_property, sizeof(int_property));
+  } else {
+    // Not an available device.
+    return;
+  }
+
+  if (GetHidIntProperty(ref, CFSTR(kIOHIDVendorIDKey), &int_property)) {

[Mark Mentovai, 2014/02/12 19:35:59]

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.

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

Done.

+    device.vendor_id = int_property;
+  }
+  if (GetHidIntProperty(ref, CFSTR(kIOHIDProductIDKey), &int_property)) {
+    device.product_id = int_property;
+  }
+  if (GetHidIntProperty(ref, CFSTR(kIOHIDPrimaryUsageKey), &int_property)) {
+    device.usage = int_property;
+  }
+  if (GetHidIntProperty(ref, CFSTR(kIOHIDPrimaryUsagePageKey), &int_property)) {
+    device.usage_page = int_property;
+  }
+  if (GetHidIntProperty(ref, CFSTR(kIOHIDMaxInputReportSizeKey),
+                        &int_property)) {
+    device.input_report_size = int_property;
+  }
+  if (GetHidIntProperty(ref, CFSTR(kIOHIDMaxOutputReportSizeKey),
+                        &int_property)) {
+    device.output_report_size = int_property;
+  }
+  if (GetHidIntProperty(ref, CFSTR(kIOHIDMaxFeatureReportSizeKey),
+                     &int_property)) {

[Mark Mentovai, 2014/02/12 19:35:59]

Weird indentation

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

Done.

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

Done.

+    device.feature_report_size = int_property;
+  }
+  if (GetHidStringProperty(ref, CFSTR(kIOHIDProductKey), &str_property)) {
+    device.product_name = str_property;
+  }
+  if (GetHidStringProperty(ref, CFSTR(kIOHIDSerialNumberKey), &str_property)) {
+    device.serial_number = str_property;
+  }
+  HidService::AddDevice(device);
+}
+
+void HidServiceMac::PlatformRemoveDevice(IOHIDDeviceRef raw_ref) {
+  std::string device_id;
+  int32_t int_property = 0;
+  // Auto-release.

[Mark Mentovai, 2014/02/12 19:35:59]

Nope.

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

Done.

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

Done.

+  base::ScopedCFTypeRef<IOHIDDeviceRef> ref(raw_ref);
+  if (!GetHidIntProperty(ref, CFSTR(kIOHIDLocationIDKey), &int_property)) {
+    return;
+  }
+  device_id = base::HexEncode(&int_property, sizeof(int_property));
+  HidService::RemoveDevice(device_id);
+}
+
+scoped_refptr<HidConnection>
+HidServiceMac::Connect(std::string device_id) {
+  if (!ContainsKey(devices_, device_id))
+    return NULL;
+
+  IOHIDDeviceRef ref = FindDevice(device_id);
+  if (ref == NULL)
+    return NULL;
+  return scoped_refptr<HidConnection>(
+      new HidConnectionMac(this, devices_[device_id], ref));
+}
+
+}  // namespace device