Clean up HID backend and API.

device/hid/hid_service_mac.cc

Index: device/hid/hid_service_mac.cc
diff --git a/device/hid/hid_service_mac.cc b/device/hid/hid_service_mac.cc
index 12da7cc2d0c46afb4ad152b423c553ef7388e4f8..e71a301e23862ebee00d072cd79b1e191d4635cd 100644
--- a/device/hid/hid_service_mac.cc
+++ b/device/hid/hid_service_mac.cc
@@ -1,250 +1,188 @@
-// Copyright (c) 2014 The Chromium Authors. All rights reserved.
+// Copyright 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>
-#include <set>
+#include <CoreFoundation/CoreFoundation.h>
+#include <IOKit/hid/IOHIDManager.h>
+
 #include <string>
+#include <vector>
 
 #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/message_loop/message_loop_proxy.h"
+#include "base/stl_util.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>
-#include <IOKit/hid/IOHIDManager.h>
 
 namespace device {
 
 class HidServiceMac;
 
-HidServiceMac::HidServiceMac() : enumeration_runloop_init_(true, false) {
-  base::ThreadRestrictions::AssertIOAllowed();
-
-  hid_manager_ref_.reset(IOHIDManagerCreate(kCFAllocatorDefault,
-                                            kIOHIDOptionsTypeNone));
-  if (!hid_manager_ref_ ||
-      CFGetTypeID(hid_manager_ref_) != IOHIDManagerGetTypeID()) {
-    LOG(ERROR) << "Failed to initialize HidManager";
-    return;
-  }
-  CFRetain(hid_manager_ref_);
-
-  // Register for plug/unplug notifications.
-  IOHIDManagerSetDeviceMatching(hid_manager_ref_.get(), NULL);
-  IOHIDManagerRegisterDeviceMatchingCallback(
-      hid_manager_ref_.get(),
-      &HidServiceMac::AddDeviceCallback,
-      this);
-  IOHIDManagerRegisterDeviceRemovalCallback(
-      hid_manager_ref_.get(),
-      &HidServiceMac::RemoveDeviceCallback,
-      this);
+namespace {
 
-  // Blocking operation to enumerate all the pre-existing devices.
-  Enumerate();
+typedef std::vector<IOHIDDeviceRef> HidDeviceList;
 
-  // 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.
-  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* HidServiceFromContext(void* context) {
+  return static_cast<HidServiceMac*>(context);
 }
 
-HidServiceMac::~HidServiceMac() {
-  enumeration_runloop_thread_->message_loop()->PostTask(
-        FROM_HERE,
-        base::Bind(&HidServiceMac::UnscheduleRunLoop, base::Unretained(this)));
+// Callback for CFSetApplyFunction as used by EnumerateHidDevices.
+void HidEnumerationBackInserter(const void* value, void* context) {
+  HidDeviceList* devices = static_cast<HidDeviceList*>(context);
+  const IOHIDDeviceRef device =
+      static_cast<IOHIDDeviceRef>(const_cast<void*>(value));
+  devices->push_back(device);
+}
 
-  enumeration_runloop_thread_->Stop();
+void EnumerateHidDevices(IOHIDManagerRef hid_manager,
+                         HidDeviceList* device_list) {
+  DCHECK(device_list->size() == 0);
+  base::ScopedCFTypeRef<CFSetRef> devices(IOHIDManagerCopyDevices(hid_manager));
+  if (devices)
+    CFSetApplyFunction(devices, HidEnumerationBackInserter, device_list);
 }
 
-void HidServiceMac::ScheduleRunLoop() {
-  enumeration_runloop_ = CFRunLoopGetCurrent();
+}  // namespace
 
-  IOHIDManagerScheduleWithRunLoop(
-      hid_manager_ref_,
-      enumeration_runloop_,
-      kCFRunLoopDefaultMode);
+HidServiceMac::HidServiceMac() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  message_loop_ = base::MessageLoopProxy::current();
+  DCHECK(message_loop_);
+  hid_manager_.reset(IOHIDManagerCreate(NULL, 0));
+  if (!hid_manager_) {
+    LOG(ERROR) << "Failed to initialize HidManager";
+    return;
+  }
+  DCHECK(CFGetTypeID(hid_manager_) == IOHIDManagerGetTypeID());
+  IOHIDManagerOpen(hid_manager_, kIOHIDOptionsTypeNone);
+  IOHIDManagerSetDeviceMatching(hid_manager_, NULL);
 
-  IOHIDManagerOpen(hid_manager_ref_, kIOHIDOptionsTypeNone);
+  // Enumerate all the currently known devices.
+  Enumerate();
 
-  enumeration_runloop_init_.Signal();
+  // Register for plug/unplug notifications.
+  StartWatchingDevices();
 }
 
-void HidServiceMac::UnscheduleRunLoop() {
-  IOHIDManagerUnscheduleFromRunLoop(
-        hid_manager_ref_,
-        enumeration_runloop_,
-        kCFRunLoopDefaultMode);
-
-  IOHIDManagerClose(hid_manager_ref_, kIOHIDOptionsTypeNone);
+HidServiceMac::~HidServiceMac() { StopWatchingDevices(); }

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

I mentioned this before at
https://codereview.chromium.org/161823002/diff/720001/device/hid/hid_service_...

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

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.

[Mark Mentovai, 2014/02/24 20:23:36]

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

 
+void HidServiceMac::StartWatchingDevices() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  IOHIDManagerRegisterDeviceMatchingCallback(
+      hid_manager_, &AddDeviceCallback, this);
+  IOHIDManagerRegisterDeviceRemovalCallback(
+      hid_manager_, &RemoveDeviceCallback, this);
+  IOHIDManagerScheduleWithRunLoop(
+      hid_manager_, CFRunLoopGetMain(), kCFRunLoopDefaultMode);
 }
 
-HidServiceMac* HidServiceMac::InstanceFromContext(void* context) {
-  return reinterpret_cast<HidServiceMac*>(context);
+void HidServiceMac::StopWatchingDevices() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  if (!hid_manager_)
+    return;
+  IOHIDManagerUnscheduleFromRunLoop(
+      hid_manager_, CFRunLoopGetMain(), kCFRunLoopDefaultMode);
+  IOHIDManagerClose(hid_manager_, kIOHIDOptionsTypeNone);
 }
 
 void HidServiceMac::AddDeviceCallback(void* context,
                                       IOReturn result,
                                       void* sender,
-                                      IOHIDDeviceRef ref) {
-  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)));
+                                      IOHIDDeviceRef hid_device) {
+  DCHECK(CFRunLoopGetMain() == CFRunLoopGetCurrent());
+  HidServiceMac* service = HidServiceFromContext(context);
+  service->message_loop_->PostTask(FROM_HERE,
+                                   base::Bind(&HidServiceMac::PlatformAddDevice,
+                                              base::Unretained(service),
+                                              base::Unretained(hid_device)));
 }
 
 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);
+                                         IOHIDDeviceRef hid_device) {
+  DCHECK(CFRunLoopGetMain() == CFRunLoopGetCurrent());
+  HidServiceMac* service = HidServiceFromContext(context);
   service->message_loop_->PostTask(
       FROM_HERE,
       base::Bind(&HidServiceMac::PlatformRemoveDevice,
                  base::Unretained(service),
-                 base::Unretained(ref)));
-}
-
-IOHIDDeviceRef HidServiceMac::FindDevice(std::string id) {
-  base::ScopedCFTypeRef<CFSetRef> devices(
-      IOHIDManagerCopyDevices(hid_manager_ref_));
-  CFIndex count = CFSetGetCount(devices);
-  scoped_ptr<IOHIDDeviceRef[]> device_refs(new IOHIDDeviceRef[count]);
-  CFSetGetValues(devices, (const void **)(device_refs.get()));
-
-  for (CFIndex i = 0; i < count; i++) {
-    int32_t int_property = 0;
-    if (GetHidIntProperty(device_refs[i], CFSTR(kIOHIDLocationIDKey),
-                       &int_property)) {
-      if (id == base::HexEncode(&int_property, sizeof(int_property))) {
-        return device_refs[i];
-      }
-    }
-  }
-
-  return NULL;
+                 base::Unretained(hid_device)));
 }
 
 void HidServiceMac::Enumerate() {
-  base::ScopedCFTypeRef<CFSetRef> devices(
-      IOHIDManagerCopyDevices(hid_manager_ref_));
-  CFIndex count = CFSetGetCount(devices);
-  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.
-    CFRetain(device_refs[i]);
-    PlatformAddDevice(device_refs[i]);
+  DCHECK(thread_checker_.CalledOnValidThread());
+  HidDeviceList devices;
+  EnumerateHidDevices(hid_manager_, &devices);
+  for (HidDeviceList::const_iterator iter = devices.begin();
+       iter != devices.end();
+       ++iter) {
+    IOHIDDeviceRef hid_device = *iter;
+    PlatformAddDevice(hid_device);
   }
 }
 
-void HidServiceMac::PlatformAddDevice(IOHIDDeviceRef raw_ref) {
-  HidDeviceInfo device;
-  int32_t int_property = 0;
-  std::string str_property;
-
-  // Auto-release.
-  base::ScopedCFTypeRef<IOHIDDeviceRef> ref(raw_ref);
-
+void HidServiceMac::PlatformAddDevice(IOHIDDeviceRef hid_device) {
   // 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.
+  DCHECK(thread_checker_.CalledOnValidThread());
+
+  HidDeviceInfo device_info;
+  device_info.device_id = GenerateDeviceId();
+  device_info.vendor_id =
+      GetHidIntProperty(hid_device, CFSTR(kIOHIDVendorIDKey));
+  device_info.product_id =
+      GetHidIntProperty(hid_device, CFSTR(kIOHIDProductIDKey));
+  device_info.usage =
+      GetHidIntProperty(hid_device, CFSTR(kIOHIDPrimaryUsageKey));
+  device_info.usage_page =
+      GetHidIntProperty(hid_device, CFSTR(kIOHIDPrimaryUsagePageKey));
+  device_info.input_report_size =
+      GetHidIntProperty(hid_device, CFSTR(kIOHIDMaxInputReportSizeKey));
+  device_info.output_report_size =
+      GetHidIntProperty(hid_device, CFSTR(kIOHIDMaxOutputReportSizeKey));
+  device_info.feature_report_size =
+      GetHidIntProperty(hid_device, CFSTR(kIOHIDMaxFeatureReportSizeKey));
+  device_info.product_name =
+      GetHidStringProperty(hid_device, CFSTR(kIOHIDProductKey));
+  device_info.serial_number =
+      GetHidStringProperty(hid_device, CFSTR(kIOHIDSerialNumberKey));
+
+  if (!platform_resource_map_.Add(device_info.device_id, hid_device))
     return;
-  }
-
-  if (GetHidIntProperty(ref, CFSTR(kIOHIDVendorIDKey), &int_property)) {
-    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)) {
-    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);
+  CFRetain(hid_device);
+  AddDevice(device_info);
 }
 
-void HidServiceMac::PlatformRemoveDevice(IOHIDDeviceRef raw_ref) {
-  std::string device_id;
-  int32_t int_property = 0;
-  // Auto-release.
-  base::ScopedCFTypeRef<IOHIDDeviceRef> ref(raw_ref);
-  if (!GetHidIntProperty(ref, CFSTR(kIOHIDLocationIDKey), &int_property)) {
+void HidServiceMac::PlatformRemoveDevice(IOHIDDeviceRef hid_device) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+
+  HidDeviceId device_id;
+  if (!platform_resource_map_.GetIdByResource(hid_device, &device_id))
     return;
-  }
-  device_id = base::HexEncode(&int_property, sizeof(int_property));
-  HidService::RemoveDevice(device_id);
+  platform_resource_map_.RemoveById(device_id);
+  CFRelease(hid_device);

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

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.

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

Done.

+
+  RemoveDevice(device_id);
 }
 
-scoped_refptr<HidConnection>
-HidServiceMac::Connect(std::string device_id) {
-  if (!ContainsKey(devices_, device_id))
+scoped_refptr<HidConnection> HidServiceMac::Connect(HidDeviceId device_id) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  HidDeviceInfo device_info;
+  if (!GetInfo(device_id, &device_info))
     return NULL;
-
-  IOHIDDeviceRef ref = FindDevice(device_id);
-  if (ref == NULL)
+  IOHIDDeviceRef hid_device;
+  if (!platform_resource_map_.GetResourceById(device_id, &hid_device))
+    return NULL;
+  if (hid_device == NULL)
     return NULL;
+
   return scoped_refptr<HidConnection>(
-      new HidConnectionMac(this, devices_[device_id], ref));
+      new HidConnectionMac(this, device_info, hid_device));
 }
 
 }  // namespace device