chrome.hid: enrich model with report IDs

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 ed85ec2d5cc922c51397f5e09a6dba3168ebdd5a..222712d53afe3677ded342a5f8f63251c1643114 100644
--- a/device/hid/hid_service_mac.cc
+++ b/device/hid/hid_service_mac.cc
@@ -7,6 +7,7 @@
 #include <CoreFoundation/CoreFoundation.h>
 #include <IOKit/hid/IOHIDManager.h>
 
+#include <set>
 #include <string>
 #include <vector>
 
@@ -48,6 +49,22 @@ void EnumerateHidDevices(IOHIDManagerRef hid_manager,
     CFSetApplyFunction(devices, HidEnumerationBackInserter, device_list);
 }
 
+void GetReportIds(IOHIDElementRef element, std::set<int>& reportIDs) {
+  CFArrayRef children = IOHIDElementGetChildren(element);
+  if (!children)
+    return;
+  CFIndex childrenCount = CFArrayGetCount(children);
+  for (CFIndex j = 0; j < childrenCount; j++) {

[Ken Rockot(use gerrit already), 2014/06/06 20:04:43]

nit: ++j

[jracle (use Gerrit), 2014/06/07 12:56:21]

I would have bet :). Had that before, jeez! You're right, more performant

On 2014/06/06 20:04:43, Ken Rockot wrote:

+    IOHIDElementRef child =
+        (IOHIDElementRef)CFArrayGetValueAtIndex(children, j);

[Ken Rockot(use gerrit already), 2014/06/06 20:04:43]

static_cast please

[jracle (use Gerrit), 2014/06/07 12:56:22]

Sorry. C-ish stuff again

On 2014/06/06 20:04:43, Ken Rockot wrote:

+    uint32_t reportID = IOHIDElementGetReportID(child);
+    if (reportID) {
+      reportIDs.insert(reportID);
+    }
+    GetReportIds(child, reportIDs);
+  }
+}
+
 }  // namespace
 
 HidServiceMac::HidServiceMac() {
@@ -136,41 +153,52 @@ void HidServiceMac::PlatformAddDevice(IOHIDDeviceRef hid_device) {
   // Note that our ownership of hid_device is implied if calling this method.
   // It is balanced in PlatformRemoveDevice.
   DCHECK(thread_checker_.CalledOnValidThread());
-
   HidDeviceInfo device_info;
   device_info.device_id = hid_device;
   device_info.vendor_id =
       GetHidIntProperty(hid_device, CFSTR(kIOHIDVendorIDKey));
   device_info.product_id =
       GetHidIntProperty(hid_device, CFSTR(kIOHIDProductIDKey));
-  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));
-  CFTypeRef deviceUsagePairsRaw =
-      IOHIDDeviceGetProperty(hid_device, CFSTR(kIOHIDDeviceUsagePairsKey));
-  CFArrayRef deviceUsagePairs =
-      base::mac::CFCast<CFArrayRef>(deviceUsagePairsRaw);
-  CFIndex deviceUsagePairsCount = CFArrayGetCount(deviceUsagePairs);
-  for (CFIndex i = 0; i < deviceUsagePairsCount; i++) {
-    CFDictionaryRef deviceUsagePair = base::mac::CFCast<CFDictionaryRef>(
-        CFArrayGetValueAtIndex(deviceUsagePairs, i));
-    CFNumberRef usage_raw = base::mac::CFCast<CFNumberRef>(
-        CFDictionaryGetValue(deviceUsagePair, CFSTR(kIOHIDDeviceUsageKey)));
-    uint16_t usage;
-    CFNumberGetValue(usage_raw, kCFNumberSInt32Type, &usage);
-    CFNumberRef page_raw = base::mac::CFCast<CFNumberRef>(
-        CFDictionaryGetValue(deviceUsagePair, CFSTR(kIOHIDDeviceUsagePageKey)));
-    HidUsageAndPage::Page page;
-    CFNumberGetValue(page_raw, kCFNumberSInt32Type, &page);
-    device_info.usages.push_back(HidUsageAndPage(usage, page));
-  }
   device_info.product_name =
       GetHidStringProperty(hid_device, CFSTR(kIOHIDProductKey));
   device_info.serial_number =
       GetHidStringProperty(hid_device, CFSTR(kIOHIDSerialNumberKey));
+  device_info.max_input_report_size =
+      GetHidIntProperty(hid_device, CFSTR(kIOHIDMaxInputReportSizeKey));
+  device_info.max_output_report_size =
+      GetHidIntProperty(hid_device, CFSTR(kIOHIDMaxOutputReportSizeKey));
+  device_info.max_feature_report_size =
+      GetHidIntProperty(hid_device, CFSTR(kIOHIDMaxFeatureReportSizeKey));
+  CFMutableDictionaryRef collectionsMatch =

[Ken Rockot(use gerrit already), 2014/06/06 20:04:44]

Can you please move this blob of code into its own function (maybe in anonymous
namespace?) Specifically all the stuff related to matching collections and
populating a list.

Also please remember, we_use_underscores, not camelCase for variable names.

How about collections_filter instead of "collectionsMatch"

[jracle (use Gerrit), 2014/06/07 12:56:22]

It's like you were reading in my brains, fightening!
I've forgot to refactor this. To add to your comment, I gonna also look at
'hid_utils_mac.h/cc' ; I'm not the originator of that file, but am pretty
uncomfortable seeing it only used by hid_service_mac.cc. Will extract all that
in its anonymous namespace if you don't mind, and will remove that hid_utils_mac
stuff.

On 2014/06/06 20:04:44, Ken Rockot wrote:

+      CFDictionaryCreateMutable(kCFAllocatorDefault,
+                                0,
+                                &kCFTypeDictionaryKeyCallBacks,
+                                &kCFTypeDictionaryValueCallBacks);
+  int type = kIOHIDElementTypeCollection;

[Ken Rockot(use gerrit already), 2014/06/06 20:04:44]

How about:

const int kCollectionTypeVlaue = kIOHIDElementTypeCollection

[jracle (use Gerrit), 2014/06/07 12:56:21]

sure

On 2014/06/06 20:04:44, Ken Rockot wrote:

+  CFNumberRef typeCFNumberRef =

[Ken Rockot(use gerrit already), 2014/06/06 20:04:43]

How about

CFNumberRef collection_type_id = CFNumberCreate(...);

[jracle (use Gerrit), 2014/06/07 12:56:21]

sorry, that's awful variable name I wished to rename.. and forgot to do it!

On 2014/06/06 20:04:43, Ken Rockot wrote:

+      CFNumberCreate(kCFAllocatorDefault, kCFNumberIntType, &type);
+  CFDictionarySetValue(
+      collectionsMatch, CFSTR(kIOHIDElementTypeKey), typeCFNumberRef);
+  CFRelease(typeCFNumberRef);
+  CFArrayRef collections = IOHIDDeviceCopyMatchingElements(
+      hid_device, collectionsMatch, kIOHIDOptionsTypeNone);
+  CFIndex collectionsCount = CFArrayGetCount(collections);
+  for (CFIndex i = 0; i < collectionsCount; i++) {
+    IOHIDElementRef collection =
+        (IOHIDElementRef)CFArrayGetValueAtIndex(collections, i);

[Ken Rockot(use gerrit already), 2014/06/06 20:04:43]

static_cast

[jracle (use Gerrit), 2014/06/07 12:56:21]

sure

On 2014/06/06 20:04:43, Ken Rockot wrote:

+    DCHECK(IOHIDElementGetType(collection) == kIOHIDElementTypeCollection);

[Ken Rockot(use gerrit already), 2014/06/06 20:04:43]

Short of the world imploding or IOKit being fundamentally broken, can this check
really ever fail? If not, please remove.

[jracle (use Gerrit), 2014/06/07 12:56:21]

Let me think half a sec.. That's indeed stupid defensive code from my part oops.

On 2014/06/06 20:04:43, Ken Rockot wrote:

+    // Top-Level Collection has no parent
+    if (IOHIDElementGetParent(collection) == 0) {
+      HidCollectionInfo collection_info;
+      HidUsageAndPage::Page page =
+          (HidUsageAndPage::Page)IOHIDElementGetUsagePage(collection);

[Ken Rockot(use gerrit already), 2014/06/06 20:04:44]

static_cast

[jracle (use Gerrit), 2014/06/07 12:56:21]

yes

On 2014/06/06 20:04:44, Ken Rockot wrote:

+      uint16_t usage = IOHIDElementGetUsage(collection);
+      collection_info.usage = HidUsageAndPage(usage, page);
+      // Explore children recursively and retrieve their report IDs
+      GetReportIds(collection, collection_info.report_ids);
+      device_info.collections.push_back(collection_info);
+    }
+  }
   AddDevice(device_info);
 }