chrome.hid : enrich device info with Top-Level collections usages

device/hid/hid_report_descriptor.h

Index: device/hid/hid_report_descriptor.h
diff --git a/device/hid/hid_report_descriptor.h b/device/hid/hid_report_descriptor.h
new file mode 100644
index 0000000000000000000000000000000000000000..2d06b08641e54d844328f3ecde099b60dc311ba8
--- /dev/null
+++ b/device/hid/hid_report_descriptor.h
@@ -0,0 +1,803 @@
+// 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.
+
+#ifndef DEVICE_HID_HID_REPORT_DESCRIPTOR_H_
+#define DEVICE_HID_HID_REPORT_DESCRIPTOR_H_
+
+#include <sstream>
+#include <vector>
+
+#include "base/basictypes.h"
+#include "device/hid/hid_usage_and_page.h"
+
+namespace device {
+
+// An element of a HID report descriptor.
+class HidReportDescriptorItem {

[Ken Rockot(use gerrit already), 2014/04/15 21:18:37]

I think this is large enough that it deserves its own header and cc files.

[jracle (use Gerrit), 2014/04/16 16:39:19]

Sure, done.

+
+#pragma pack(push, 1)

[Ken Rockot(use gerrit already), 2014/04/15 21:18:37]

Please do not use packed structs to map onto a data buffer like this. Just
parse/unpack the data into standalone fields in the HRDI class. A little more
work up front, but much easier for any passerby or consumer to understand what's
going on.

[jracle (use Gerrit), 2014/04/16 16:39:19]

Got your point, I will refactor in next patch set.

+
+ public:
+  enum Type {
+    kTypeMain = 0,

[Ken Rockot(use gerrit already), 2014/04/15 21:18:37]

Please move Type into the next public block since none of the tag enums depend
on it. The fewer private/public blocks, the better.

+    kTypeGlobal = 1,
+    kTypeLocal = 2,
+    kTypeReserved = 3
+  };
+
+ private:
+  // Tags
+
+  enum MainTag {
+    kMainTagDefault = 0x00,       // 0000

[Ken Rockot(use gerrit already), 2014/04/15 21:18:37]

As these are only used internally, please move them out of the header. They can
live in an anonymous namespace.

+    kMainTagInput = 0x08,         // 1000
+    kMainTagOutput = 0x09,        // 1001
+    kMainTagFeature = 0x0B,       // 1011
+    kMainTagCollection = 0x0A,    // 1010
+    kMainTagEndCollection = 0x0C  // 1100
+  };
+
+  enum GlobalTag {
+    kGlobalTagUsagePage = 0x00,        // 0000
+    kGlobalTagLogicalMinimum = 0x01,   // 0001
+    kGlobalTagLogicalMaximum = 0x02,   // 0010
+    kGlobalTagPhysicalMinimum = 0x03,  // 0011
+    kGlobalTagPhysicalMaximum = 0x04,  // 0100
+    kGlobalTagUnitExponent = 0x05,     // 0101
+    kGlobalTagUnit = 0x06,             // 0110
+    kGlobalTagReportSize = 0x07,       // 0111
+    kGlobalTagReportId = 0x08,         // 1000
+    kGlobalTagReportCount = 0x09,      // 1001
+    kGlobalTagPush = 0x0A,             // 1010
+    kGlobalTagPop = 0x0B               // 1011
+  };
+
+  enum LocalTag {
+    kLocalTagUsage = 0x00,              // 0000
+    kLocalTagUsageMinimum = 0x01,       // 0001
+    kLocalTagUsageMaximum = 0x02,       // 0010
+    kLocalTagDesignatorIndex = 0x03,    // 0011
+    kLocalTagDesignatorMinimum = 0x04,  // 0100
+    kLocalTagDesignatorMaximum = 0x05,  // 0101
+    kLocalTagStringIndex = 0x07,        // 0111
+    kLocalTagStringMinimum = 0x08,      // 1000
+    kLocalTagStringMaximum = 0x09,      // 1001
+    kLocalTagDelimiter = 0x0A           // 1010
+  };
+
+  enum ReservedTag {
+    kReservedTagLong = 0xF  // 1111
+  };
+
+ public:
+  enum Tag {
+    kTagDefault = kMainTagDefault << 2 | kTypeMain,
+    kTagInput = kMainTagInput << 2 | kTypeMain,
+    kTagOutput = kMainTagOutput << 2 | kTypeMain,
+    kTagFeature = kMainTagFeature << 2 | kTypeMain,
+    kTagCollection = kMainTagCollection << 2 | kTypeMain,
+    kTagEndCollection = kMainTagEndCollection << 2 | kTypeMain,
+    kTagUsagePage = kGlobalTagUsagePage << 2 | kTypeGlobal,
+    kTagLogicalMinimum = kGlobalTagLogicalMinimum << 2 | kTypeGlobal,
+    kTagLogicalMaximum = kGlobalTagLogicalMaximum << 2 | kTypeGlobal,
+    kTagPhysicalMinimum = kGlobalTagPhysicalMinimum << 2 | kTypeGlobal,
+    kTagPhysicalMaximum = kGlobalTagPhysicalMaximum << 2 | kTypeGlobal,
+    kTagUnitExponent = kGlobalTagUnitExponent << 2 | kTypeGlobal,
+    kTagUnit = kGlobalTagUnit << 2 | kTypeGlobal,
+    kTagReportSize = kGlobalTagReportSize << 2 | kTypeGlobal,
+    kTagReportId = kGlobalTagReportId << 2 | kTypeGlobal,
+    kTagReportCount = kGlobalTagReportCount << 2 | kTypeGlobal,
+    kTagPush = kGlobalTagPush << 2 | kTypeGlobal,
+    kTagPop = kGlobalTagPop << 2 | kTypeGlobal,
+    kTagUsage = kLocalTagUsage << 2 | kTypeLocal,
+    kTagUsageMinimum = kLocalTagUsageMinimum << 2 | kTypeLocal,
+    kTagUsageMaximum = kLocalTagUsageMaximum << 2 | kTypeLocal,
+    kTagDesignatorIndex = kLocalTagDesignatorIndex << 2 | kTypeLocal,
+    kTagDesignatorMinimum = kLocalTagDesignatorMinimum << 2 | kTypeLocal,
+    kTagDesignatorMaximum = kLocalTagDesignatorMaximum << 2 | kTypeLocal,
+    kTagStringIndex = kLocalTagStringIndex << 2 | kTypeLocal,
+    kTagStringMinimum = kLocalTagStringMinimum << 2 | kTypeLocal,
+    kTagStringMaximum = kLocalTagStringMaximum << 2 | kTypeLocal,
+    kTagDelimiter = kLocalTagDelimiter << 2 | kTypeLocal,
+    kTagLong = kReservedTagLong << 2 | kTypeReserved
+  };
+
+ private:
+  friend std::ostream& operator<<(std::ostream& os, const Tag& tag) {

[Ken Rockot(use gerrit already), 2014/04/15 21:18:37]

Please do not inline this, and in fact it doesn't even need to live inside any
class. It can be in an anonymous namespace in some implementation file.

[jracle (use Gerrit), 2014/04/16 16:39:19]

Done

+    switch (tag) {
+      case kTagDefault:
+        os << "Default";
+        break;
+      case kTagInput:
+        os << "Input";
+        break;
+      case kTagOutput:
+        os << "Output";
+        break;
+      case kTagFeature:
+        os << "Feature";
+        break;
+      case kTagCollection:
+        os << "Collection";
+        break;
+      case kTagEndCollection:
+        os << "End Collection";
+        break;
+      case kTagUsagePage:
+        os << "Usage Page";
+        break;
+      case kTagLogicalMinimum:
+        os << "Logical Minimum";
+        break;
+      case kTagLogicalMaximum:
+        os << "Logical Maximum";
+        break;
+      case kTagPhysicalMinimum:
+        os << "Physical Minimum";
+        break;
+      case kTagPhysicalMaximum:
+        os << "Physical Maximum";
+        break;
+      case kTagUnitExponent:
+        os << "Unit Exponent";
+        break;
+      case kTagUnit:
+        os << "Unit";
+        break;
+      case kTagReportSize:
+        os << "Report Size";
+        break;
+      case kTagReportId:
+        os << "Report ID";
+        break;
+      case kTagReportCount:
+        os << "Report Count";
+        break;
+      case kTagPush:
+        os << "Push";
+        break;
+      case kTagPop:
+        os << "Pop";
+        break;
+      case kTagUsage:
+        os << "Usage";
+        break;
+      case kTagUsageMinimum:
+        os << "Usage Minimum";
+        break;
+      case kTagUsageMaximum:
+        os << "Usage Maximum";
+        break;
+      case kTagDesignatorIndex:
+        os << "Designator Index";
+        break;
+      case kTagDesignatorMinimum:
+        os << "Designator Minimum";
+        break;
+      case kTagDesignatorMaximum:
+        os << "Designator Maximum";
+        break;
+      case kTagStringIndex:
+        os << "String Index";
+        break;
+      case kTagStringMinimum:
+        os << "String Minimum";
+        break;
+      case kTagStringMaximum:
+        os << "String Maximum";
+        break;
+      case kTagDelimiter:
+        os << "Delimeter";
+        break;
+      case kTagLong:
+        os << "Long";
+        break;
+      default:
+        NOTREACHED();
+        break;
+    }
+
+    return os;
+  };
+
+ public:
+  // Headers
+
+  struct Header {

[Ken Rockot(use gerrit already), 2014/04/15 21:18:37]

I'm jumping around this CL a lot as I get a better understanding of it, so
please excuse any repetition. :)

I would much rather see HidReportDescriptorItem do all of the parsing work up
front and actually store its own (unpacked) fields from the result.

There is no reason (as far as I can tell) that it should need to expose the
detail of a long vs short header. It can just expose size, type, tag, yes? Then
these packed structs can disappear altogether.

[jracle (use Gerrit), 2014/04/16 16:39:19]

Got it, no point to show those details. Confusing.

+    uint8_t size : 2;
+    uint8_t type : 2;
+    uint8_t tag : 4;
+  };
+
+  struct LongHeader {
+    Header short_header;
+    uint8_t data_size;
+    uint8_t long_item_tag;
+  };
+
+  // Data
+
+  // (Short) Main items
+
+  struct Default {

[Ken Rockot(use gerrit already), 2014/04/15 21:18:37]

Default what? A more descriptive name may be in order here.

[jracle (use Gerrit), 2014/04/16 16:39:19]

Oops. Not meaningful.

+   private:
+    friend std::ostream& operator<<(std::ostream& os, const Default& data) {

[Ken Rockot(use gerrit already), 2014/04/15 21:18:37]

See the comment in Input_Output_Feature about moving these ostream operators out
of the structs altogether. I think it applies to every struct defined in this
file.

[jracle (use Gerrit), 2014/04/16 16:39:19]

Indeed.

+      return os;
+    }
+  };
+
+  struct Input_Output_Feature {

[Ken Rockot(use gerrit already), 2014/04/15 21:18:37]

InputOutputFeature, please

+    uint8_t data_or_constant : 1;

[Ken Rockot(use gerrit already), 2014/04/15 21:18:37]

Same as previous comments. I know it's a lot of fields, but please do not map
this directly onto the buffer. I'm OK with the bit fields in this case, but an
instance of this structure should just live inside of HRDI as a *copy* of
whatever information was in the descriptor buffer at construction time. 

+    uint8_t array_or_variable : 1;
+    uint8_t absolute_or_relative : 1;
+    uint8_t wrap : 1;
+    uint8_t linear : 1;
+    uint8_t preferred : 1;
+    uint8_t null : 1;
+    uint8_t reserved_1 : 1;
+    uint8_t bit_field_or_buffer : 1;
+    uint8_t reserved_2 : 1;
+
+   private:
+    friend std::ostream& operator<<(std::ostream& os,
+                                    const Input_Output_Feature& data) {

[Ken Rockot(use gerrit already), 2014/04/15 21:18:37]

Please do not inline this. In fact once again, this does not need to belong to
the struct at all. Please define it in an anonymous namespace in an
implementation file.

+      if (data.data_or_constant)
+        os << "Con";
+      else
+        os << "Dat";
+      if (data.array_or_variable)
+        os << "|Arr";
+      else
+        os << "|Var";
+      if (data.absolute_or_relative)
+        os << "|Abs";
+      else
+        os << "|Rel";
+      if (data.wrap)
+        os << "|Wrp";
+      else
+        os << "|NoWrp";
+      if (data.linear)
+        os << "|NoLin";
+      else
+        os << "|Lin";
+      if (data.preferred)
+        os << "|NoPrf";
+      else
+        os << "|Prf";
+      if (data.null)
+        os << "|Null";
+      else
+        os << "|NoNull";
+      if (data.bit_field_or_buffer)
+        os << "|Buff";
+      else
+        os << "|BitF";
+      return os;
+    }
+  };
+
+  struct Collection {
+

[Ken Rockot(use gerrit already), 2014/04/15 21:18:37]

Whitespace

+    enum CollectionType {
+      kCollectionTypePhysical,
+      kCollectionTypeApplication,
+      kCollectionTypeLogical,
+      kCollectionTypeReport,
+      kCollectionTypeNamedArray,
+      kCollectionTypeUsageSwitch,
+      kCollectionTypeUsageModifier,
+      kCollectionTypeReserved,
+      kCollectionTypeVendor
+    };
+
+    uint8_t value;
+
+    CollectionType collectionType() const {

[Ken Rockot(use gerrit already), 2014/04/15 21:18:37]

Please do not inline this. Same goes for the rest of these methods. Please move
all of these implementations to a cc file.

Also it's not a trivial accessor, so please rename GetCollectionType().

Actually, on second thought, don't store a uint8_t in Collection at all. Store a
CollectionType. If you want to keep the value-to-type mapping logic isolated to
Collection, define a static method, like:

static CollectionType GetCollectionTypeFromValue(uint8_t value);

and call that method wherever you're creating/initializing a Collection.

If you do this then collectionType() becomes a trivial accessor.

Also please note, camelCase is always wrong in Chrome code (it's only used in
blink/WebKit and code which implements their interfaces).

Please use lowercase + underscores for accessors. So in this case if you do make
collectionType a trivial accessor, it should be renamed to collection_type().
Otherwise it should be GetCollectionType().

+      switch (value) {
+        case 0x00:
+          return kCollectionTypePhysical;
+        case 0x01:
+          return kCollectionTypePhysical;
+        case 0x02:
+          return kCollectionTypePhysical;
+        case 0x03:
+          return kCollectionTypePhysical;
+        case 0x04:
+          return kCollectionTypePhysical;
+        case 0x05:
+          return kCollectionTypePhysical;
+        case 0x06:
+          return kCollectionTypePhysical;
+        default:
+          break;
+      }
+
+      if (0x80 < value && value < 0xFF)
+        return kCollectionTypeVendor;
+
+      return kCollectionTypeReserved;
+    }
+
+   private:
+    friend std::ostream& operator<<(std::ostream& os, const Collection& data) {
+      switch (data.collectionType()) {
+        case kCollectionTypePhysical:
+          os << "Physical";
+          break;
+        case kCollectionTypeApplication:
+          os << "Application";
+          break;
+        case kCollectionTypeLogical:
+          os << "Logical";
+          break;
+        case kCollectionTypeReport:
+          os << "Report";
+          break;
+        case kCollectionTypeNamedArray:
+          os << "Named Array";
+          break;
+        case kCollectionTypeUsageSwitch:
+          os << "Usage Switch";
+          break;
+        case kCollectionTypeUsageModifier:
+          os << "Usage Modifier";
+          break;
+        case kCollectionTypeReserved:
+          os << "Reserved";
+          break;
+        case kCollectionTypeVendor:
+          os << "Vendor";
+          break;
+        default:
+          NOTREACHED();
+          break;
+      }
+      return os;
+    }
+  };
+
+  struct EndCollection {
+   private:
+    friend std::ostream& operator<<(std::ostream& os,
+                                    const EndCollection& data) {
+      return os;
+    }
+  };
+
+  // (Short) Global Items
+
+  struct UsagePage {
+    uint16_t value;
+
+   private:
+    friend std::ostream& operator<<(std::ostream& os, const UsagePage& data) {
+      HidUsageAndPage::Page page = (HidUsageAndPage::Page)data.value;
+      os << page;
+      return os;
+    }
+  };
+
+  struct LogicalMinimum {
+    int32_t value;
+
+   private:
+    friend std::ostream& operator<<(std::ostream& os,
+                                    const LogicalMinimum& data) {
+      os << data.value;
+      return os;
+    }
+  };
+
+  struct LogicalMaximum {

[Ken Rockot(use gerrit already), 2014/04/15 21:18:37]

There's a lot of code duplication going on here and a lot of nested type
definitions. Maybe you could simplify with something like

struct Int32Data {
  int32_t value;
}

struct Uint32Data {
  uint32_t value;
}

of course in that case it becomes clear to me that there is no need for all
these types. They don't add any value to the code, so why not just store them in
Data as their underlying types. That is:

struct Data {
  ...
  int32_t logical_minimum;
  int32_t logical_maximum;
  ...
}

Until there is actually a need for code to treat these as different data types
(which there doesn't appear to be yet), it will be much cleaner to do it this
way.

+    int32_t value;
+
+   private:
+    friend std::ostream& operator<<(std::ostream& os,
+                                    const LogicalMaximum& data) {
+      os << data.value;
+      return os;
+    }
+  };
+
+  struct PhysicalMinimum {
+    int32_t value;
+
+   private:
+    friend std::ostream& operator<<(std::ostream& os,
+                                    const PhysicalMinimum& data) {
+      os << data.value;
+      return os;
+    }
+  };
+
+  struct PhysicalMaximum {
+    int32_t value;
+
+   private:
+    friend std::ostream& operator<<(std::ostream& os,
+                                    const PhysicalMaximum& data) {
+      os << data.value;
+      return os;
+    }
+  };
+
+  struct UnitExponent {
+    uint32_t value;
+
+   private:
+    friend std::ostream& operator<<(std::ostream& os,
+                                    const UnitExponent& data) {
+      os << data.value;
+      return os;
+    }
+  };
+
+  struct Unit {
+    uint32_t value;
+
+   private:
+    friend std::ostream& operator<<(std::ostream& os, const Unit& data) {
+      os << data.value;
+      return os;
+    }
+  };
+
+  struct ReportSize {
+    uint32_t value;
+
+   private:
+    friend std::ostream& operator<<(std::ostream& os, const ReportSize& data) {
+      os << data.value;
+      return os;
+    }
+  };
+
+  struct ReportId {
+    uint32_t value;
+
+   private:
+    friend std::ostream& operator<<(std::ostream& os, const ReportId& data) {
+      os << "0x" << std::hex << std::uppercase << data.value;
+      return os;
+    }
+  };
+
+  struct ReportCount {
+    uint32_t value;
+
+   private:
+    friend std::ostream& operator<<(std::ostream& os, const ReportCount& data) {
+      os << data.value;
+      return os;
+    }
+  };
+
+  struct Push {
+   private:
+    friend std::ostream& operator<<(std::ostream& os, const Push& data) {
+      return os;
+    }
+  };
+
+  struct Pop {
+   private:
+    friend std::ostream& operator<<(std::ostream& os, const Pop& data) {
+      return os;
+    }
+  };
+
+  // (Short) Local Items
+
+  struct Usage {
+    uint16_t value;
+
+   private:
+    friend std::ostream& operator<<(std::ostream& os, const Usage& data) {
+      os << "0x" << std::hex << std::uppercase << data.value;
+      return os;
+    }
+  };
+
+  struct UsageMinimum {
+    uint16_t value;
+
+   private:
+    friend std::ostream& operator<<(std::ostream& os,
+                                    const UsageMinimum& data) {
+      os << "0x" << std::hex << std::uppercase << data.value;
+      return os;
+    }
+  };
+
+  struct UsageMaximum {
+    uint16_t value;
+
+   private:
+    friend std::ostream& operator<<(std::ostream& os,
+                                    const UsageMaximum& data) {
+      os << "0x" << std::hex << std::uppercase << data.value;
+      return os;
+    }
+  };
+
+  struct DesignatorIndex {
+   private:
+    friend std::ostream& operator<<(std::ostream& os,
+                                    const DesignatorIndex& data) {
+      return os;
+    }
+  };
+
+  struct DesignatorMinimum {
+   private:
+    friend std::ostream& operator<<(std::ostream& os,
+                                    const DesignatorMinimum& data) {
+      return os;
+    }
+  };
+
+  struct DesignatorMaximum {
+   private:
+    friend std::ostream& operator<<(std::ostream& os,
+                                    const DesignatorMaximum& data) {
+      return os;
+    }
+  };
+
+  struct StringIndex {
+   private:
+    friend std::ostream& operator<<(std::ostream& os, const StringIndex& data) {
+      return os;
+    }
+  };
+
+  struct StringMinimum {
+   private:
+    friend std::ostream& operator<<(std::ostream& os,
+                                    const StringMinimum& data) {
+      return os;
+    }
+  };
+
+  struct StringMaximum {
+   private:
+    friend std::ostream& operator<<(std::ostream& os,
+                                    const StringMaximum& data) {
+      return os;
+    }
+  };
+
+  struct Delimiter {
+   private:
+    friend std::ostream& operator<<(std::ostream& os, const Delimiter& data) {
+      return os;
+    }
+  };
+
+  // (Long) Reserved items
+  // nothing for now
+
+  // Data union
+
+  struct Data {
+    union u {
+      // (Short) Main items
+      Default none;
+      Input_Output_Feature input;
+      Input_Output_Feature output;
+      Input_Output_Feature feature;
+      Collection collection;
+      EndCollection end_collection;
+      // (Short) Global items
+      UsagePage usage_page;
+      LogicalMinimum logical_minimum;
+      LogicalMaximum logical_maximum;
+      PhysicalMinimum physical_minimum;
+      PhysicalMaximum physical_maximum;
+      UnitExponent unit_exponent;
+      Unit unit;
+      ReportSize report_size;
+      ReportId report_id;
+      ReportCount report_count;
+      Push push;
+      Pop pop;
+      // (Short) Local items
+      Usage usage;
+      UsageMinimum usage_minimum;
+      UsageMaximum usage_maximum;
+      DesignatorIndex designator_index;
+      DesignatorMinimum designator_minimum;
+      DesignatorMaximum designator_maximum;
+      StringIndex string_index;
+      StringMinimum string_minimum;
+      StringMaximum string_maximum;
+      Delimiter delimiter;
+      // (Long) Reserved items
+      // nothing for now
+    };
+  };
+
+#pragma pack(pop)
+
+  HidReportDescriptorItem(const uint8_t* bytes,
+                          HidReportDescriptorItem* previous = NULL);

[Ken Rockot(use gerrit already), 2014/04/15 21:18:37]

Please, no default argument values. Please specify use two separate
constructors:

explicit HidReportDescriptorItem(const uint8_t* bytes);
HidReportDescriptorItem(const uint8_t* bytes, HidReportDescriptorItem...)

In fact I think you could just get rid of the default NULL value and force
callers to specify NULL.

Also, as was mentioned in other places, please do not just map structures on top
of |bytes|, and in turn, please do not even retain a pointer to |bytes|. Parse
the data from the buffer right away and forget about the buffer. And also please
document after doing this that the constructor does not take ownership of the
buffer.

Finally, it looks like (appropriately) these are only constructed and destroyed
by HidReportDescriptor. Please make the constructor and destructor private and
make HidReportDescriptor a friend.

[jracle (use Gerrit), 2014/04/16 16:39:19]

Done

+  ~HidReportDescriptorItem();
+
+  HidReportDescriptorItem* previous() const {

[Ken Rockot(use gerrit already), 2014/04/15 21:18:37]

Please make it clear that neither this item nor the caller owns the returned
pointer, but that any HidReportDescriptorItem is always owned by the
HidReportDescriptor that created it. This should be clarified for previous()
next() and parent().

+    return previous_;
+  };
+  HidReportDescriptorItem* next() const {
+    return next_;
+  };
+  HidReportDescriptorItem* parent() const {
+    return parent_;
+  };
+  size_t depth() const;

[Ken Rockot(use gerrit already), 2014/04/15 21:18:37]

GetDepth() please. Lowercase names are for trivial accessors only.

+
+  bool isLong() const;

[Ken Rockot(use gerrit already), 2014/04/15 21:18:37]

IsLong() please.

+  size_t headerSize() const;

[Ken Rockot(use gerrit already), 2014/04/15 21:18:37]

GetHeaderSize() please.

+  size_t payloadSize() const;

[Ken Rockot(use gerrit already), 2014/04/15 21:18:37]

GetPayloadSize() please.

+  size_t size() const;

[Ken Rockot(use gerrit already), 2014/04/15 21:18:37]

GetSize()

Also because none of these are trivial accessors, please document each of them
briefly here. Someone reading this code may not know what "IsLong?" is asking.
They may not know what "GetDepth" is returning.

+
+  Header* header() const;

[Ken Rockot(use gerrit already), 2014/04/15 21:18:37]

This (and longHeader) make me extremely uncomfortable. I would much rather you
add methods to access the underlying fields by value.

[jracle (use Gerrit), 2014/04/16 16:39:19]

Now private in next patch-set, and accessed by value.

+  LongHeader* longHeader() const;
+
+  Tag tag() const;

[Ken Rockot(use gerrit already), 2014/04/15 21:18:37]

GetTag() please.

+  Data* data() const;

[Ken Rockot(use gerrit already), 2014/04/15 21:18:37]

Since Data is a POD type, and you can change it so an instance is actually owned
by this class (rather than mapping the union on top of a raw buffer), you can
just return a copy here. Please do so, and then data() can be a trivial inlined
accessor.

[jracle (use Gerrit), 2014/04/16 16:39:19]

Data is actually known only for short items.
It has a max size of uint32_t in that case.
Hence GetShortData() in next patch set (I removed the union and my dumb structs)

+
+ private:
+  friend std::ostream& operator<<(std::ostream& os,
+                                  const HidReportDescriptorItem& item) {

[Ken Rockot(use gerrit already), 2014/04/15 21:18:37]

Please see base/strings/stringprintf.h for the base::StringPrintf function. I
think after all, this may be strongly preferred over using ostream operator
overloads and stringstreams.

[jracle (use Gerrit), 2014/04/16 16:39:19]

This I didn't catch yet, can you handle it in a next patch-set please? (or give
me more info on how to use it without creating 'ToString()' or 'Dump()' methods.

+    Tag tg = item.tag();
+    Data* dt = item.data();
+
+    std::ostringstream sstr;
+    sstr << tg;
+    sstr << " (";
+
+    long pos = sstr.tellp();
+    switch (tg) {
+      case kTagDefault:
+        sstr << *(Default*)dt;
+        break;
+      case kTagInput:
+      case kTagOutput:
+      case kTagFeature:
+        sstr << *(Input_Output_Feature*)dt;
+        break;
+      case kTagCollection:
+        sstr << *(Collection*)dt;
+        break;
+      case kTagEndCollection:
+        sstr << *(EndCollection*)dt;
+        break;
+      case kTagUsagePage:
+        sstr << *(UsagePage*)dt;
+        break;
+      case kTagLogicalMinimum:
+        sstr << *(LogicalMinimum*)dt;
+        break;
+      case kTagLogicalMaximum:
+        sstr << *(LogicalMaximum*)dt;
+        break;
+      case kTagPhysicalMinimum:
+        sstr << *(PhysicalMinimum*)dt;
+        break;
+      case kTagPhysicalMaximum:
+        sstr << *(PhysicalMaximum*)dt;
+        break;
+      case kTagUnitExponent:
+        sstr << *(UnitExponent*)dt;
+        break;
+      case kTagUnit:
+        sstr << *(Unit*)dt;
+        break;
+      case kTagReportSize:
+        sstr << *(ReportSize*)dt;
+        break;
+      case kTagReportId:
+        sstr << *(ReportId*)dt;
+        break;
+      case kTagReportCount:
+        sstr << *(ReportCount*)dt;
+        break;
+      case kTagPush:
+        sstr << *(Push*)dt;
+        break;
+      case kTagPop:
+        sstr << *(Pop*)dt;
+        break;
+      case kTagUsage:
+        sstr << *(Usage*)dt;
+        break;
+      case kTagUsageMinimum:
+        sstr << *(UsageMinimum*)dt;
+        break;
+      case kTagUsageMaximum:
+        sstr << *(UsageMaximum*)dt;
+        break;
+      case kTagDesignatorIndex:
+        sstr << *(DesignatorIndex*)dt;
+        break;
+      case kTagDesignatorMinimum:
+        sstr << *(DesignatorMinimum*)dt;
+        break;
+      case kTagDesignatorMaximum:
+        sstr << *(DesignatorMaximum*)dt;
+        break;
+      case kTagStringIndex:
+        sstr << *(StringIndex*)dt;
+        break;
+      case kTagStringMinimum:
+        sstr << *(StringMinimum*)dt;
+        break;
+      case kTagStringMaximum:
+        sstr << *(StringMaximum*)dt;
+        break;
+      case kTagDelimiter:
+        sstr << *(Delimiter*)dt;
+        break;
+      case kTagLong:
+        break;
+      default:
+        NOTREACHED();
+        break;
+    }
+
+    if (pos == sstr.tellp()) {
+      std::string str = sstr.str();
+      str.erase(str.end() - 2, str.end());
+      os << str;
+    } else {
+      os << sstr.str() << ")";
+    }
+
+    return os;
+  }
+
+  const uint8_t* bytes_;
+  HidReportDescriptorItem* previous_;
+  HidReportDescriptorItem* next_;
+  HidReportDescriptorItem* parent_;
+};
+
+// HID report descriptor.
+// See section 6.2.2 of HID specifications (v1.11).
+class HidReportDescriptor {
+
+ private:
+  static const char kIndentChar;
+
+ public:
+  HidReportDescriptor(const uint8_t* bytes, size_t size);
+  ~HidReportDescriptor();
+
+  const std::vector<HidReportDescriptorItem*>& items() const;

[Ken Rockot(use gerrit already), 2014/04/15 21:18:37]

items is a trivial accessor; please feel free to inline it here.

[jracle (use Gerrit), 2014/04/16 16:39:19]

OK

+  std::vector<HidUsageAndPage> topLevelCollections() const;

[Ken Rockot(use gerrit already), 2014/04/15 21:18:37]

GetTopLevelCollections() please. This is not a trivial accessor. Also please
consider having this populate a std::vector<HidUsageAndPage>* argument rather
than returning a vector.

[jracle (use Gerrit), 2014/04/16 16:39:19]

OK

+
+ private:
+  friend std::ostream& operator<<(std::ostream& os,
+                                  const HidReportDescriptor& descriptor) {

[Ken Rockot(use gerrit already), 2014/04/15 21:18:37]

Please do not inline this definition. In fact it doesn't need to belong to
HidReportDescriptor or be a friend. It can use items() to access.

Please move this to an anonymous namespace in the cc file.

+    for (std::vector<HidReportDescriptorItem*>::const_iterator items_iter =
+             descriptor.items_.begin();
+         items_iter != descriptor.items_.end();
+         ++items_iter) {
+      HidReportDescriptorItem* item = *items_iter;
+      size_t indentLevel = item->depth();
+      for (size_t i = 0; i < indentLevel; i++)
+        os << kIndentChar;
+      os << *item << std::endl;
+    }
+    return os;
+  }
+
+  const uint8_t* bytes_;
+  size_t size_;
+  std::vector<HidReportDescriptorItem*> items_;

[Ken Rockot(use gerrit already), 2014/04/15 21:18:37]

Please use linked_ptr here

std::vector<linked_ptr<HidReportDescriptorItem> > items_

[jracle (use Gerrit), 2014/04/16 16:39:19]

Indeed

+};
+
+}  // namespace device
+
+#endif  // DEVICE_HID_HID_REPORT_DESCRIPTOR_H_