chrome.hid: enrich model with report IDs

device/hid/hid_connection.cc

Index: device/hid/hid_connection.cc
diff --git a/device/hid/hid_connection.cc b/device/hid/hid_connection.cc
index c134bf2702e96aca40ca0cc58b376d9b151a1aa2..c75a4f4f398395fa0cd7e4cf1dfc6a3eac7eb803 100644
--- a/device/hid/hid_connection.cc
+++ b/device/hid/hid_connection.cc
@@ -4,8 +4,184 @@
 
 #include "device/hid/hid_connection.h"
 
+#include <algorithm>
+
 namespace device {
 
+namespace {
+
+const uint8_t kNullReportId = 0x00;
+const uint8_t kInvalidReportId = 0xFF;

[Ken Rockot(use gerrit already), 2014/06/19 19:29:56]

Shouldn't this be kAnyReportId given the usage?

[jracle (use Gerrit), 2014/06/19 21:39:36]

Right, that's indeed what it means.

On 2014/06/19 19:29:56, Ken Rockot wrote:

+
+struct CollectionHasReportId {
+  explicit CollectionHasReportId(const uint8_t report_id)
+      : report_id_(report_id) {}
+
+  bool operator()(const HidCollectionInfo& info) const {
+    if (info.report_ids.size() == 0 || report_id_ == kNullReportId)
+      return false;
+
+    if (report_id_ == kInvalidReportId)
+      return true;
+
+    return std::find(info.report_ids.begin(),
+                     info.report_ids.end(),
+                     report_id_) != info.report_ids.end();
+  }
+
+ private:
+  const uint8_t report_id_;
+};
+
+struct CollectionIsProtected {
+  bool operator()(const HidCollectionInfo& info) const {
+    return info.usage.IsProtected();
+  }
+};
+
+bool HasReportId(const HidDeviceInfo& device_info,

[Ken Rockot(use gerrit already), 2014/06/19 19:29:56]

This doesn't quite do what the name implies. How about FindCollectionByReportId?

Also no default arguments please. It's fine if NULL implies that the info need
not be returned, but force callers to pass it explicitly.

If you wanted to, you could define a wrapper like

bool HasReportId(const HidDeviceInfo& device_info) {
  return FindCollectionByReportId(device_info, kAnyReportId, NULL);
}

I think that would clarify what's going on in all of this.

[jracle (use Gerrit), 2014/06/19 21:39:36]

Thanks so much for noticing it!

See, indeed default arguments in this case are dangerous : I merged 2 functions
which indeed have 2 meaning, and you've found them..

That's a great review.

On 2014/06/19 19:29:56, Ken Rockot wrote:

+                 const uint8_t report_id = kInvalidReportId,
+                 HidCollectionInfo* collection_info = NULL) {
+  std::vector<HidCollectionInfo>::const_iterator collection_iter =
+      std::find_if(device_info.collections.begin(),
+                   device_info.collections.end(),
+                   CollectionHasReportId(report_id));
+  if (collection_iter != device_info.collections.end()) {
+    if (collection_info) {
+      *collection_info = *collection_iter;
+    }
+    return true;
+  }
+
+  return false;
+}
+
+bool HasProtectedCollection(const HidDeviceInfo& device_info) {
+  return std::find_if(device_info.collections.begin(),
+                      device_info.collections.end(),
+                      CollectionIsProtected()) != device_info.collections.end();
+}
+
+}  // namespace
+
+HidConnection::HidConnection(const HidDeviceInfo& device_info)
+    : device_info_(device_info) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  has_protected_collection_ = HasProtectedCollection(device_info);
+  has_report_id_ = HasReportId(device_info);

[jracle (use Gerrit), 2014/06/11 11:20:04]

I prefer to cache this info..

[Ken Rockot(use gerrit already), 2014/06/19 19:29:56]

Sure, sounds fine to me.

[jracle (use Gerrit), 2014/06/19 21:39:36]

Thx.

On 2014/06/19 19:29:56, Ken Rockot wrote:

+}
+
+HidConnection::~HidConnection() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+}
+
+void HidConnection::Read(scoped_refptr<net::IOBufferWithSize> buffer,
+                         const IOCallback& callback) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  if (device_info_.max_input_report_size == 0) {
+    // The device does not support input reports.
+    callback.Run(false, 0);
+    return;
+  }
+  int expected_buffer_size = device_info_.max_input_report_size;
+  if (!has_report_id()) {
+    expected_buffer_size--;
+  }
+  if (buffer->size() < expected_buffer_size) {
+    // Receive buffer is too small.
+    callback.Run(false, 0);
+    return;
+  }
+
+  PlatformRead(buffer, callback);
+}
+
+void HidConnection::Write(uint8_t report_id,
+                          scoped_refptr<net::IOBufferWithSize> buffer,
+                          const IOCallback& callback) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  if (device_info_.max_output_report_size == 0) {
+    // The device does not support output reports.
+    callback.Run(false, 0);
+    return;
+  }
+  if (IsReportIdProtected(report_id)) {

[jracle (use Gerrit), 2014/06/11 11:20:04]

We need to prevent R/W of protected reports.

+    callback.Run(false, 0);
+    return;
+  }
+
+  PlatformWrite(report_id, buffer, callback);
+}
+
+void HidConnection::GetFeatureReport(
+    uint8_t report_id,
+    scoped_refptr<net::IOBufferWithSize> buffer,
+    const IOCallback& callback) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  if (device_info_.max_feature_report_size == 0) {
+    // The device does not support feature reports.
+    callback.Run(false, 0);
+    return;
+  }
+  if (IsReportIdProtected(report_id)) {
+    callback.Run(false, 0);
+    return;
+  }
+  int expected_buffer_size = device_info_.max_feature_report_size;
+  if (!has_report_id()) {
+    expected_buffer_size--;
+  }
+  if (buffer->size() < expected_buffer_size) {
+    // Receive buffer is too small.
+    callback.Run(false, 0);
+    return;
+  }
+
+  PlatformGetFeatureReport(report_id, buffer, callback);
+}
+
+void HidConnection::SendFeatureReport(
+    uint8_t report_id,
+    scoped_refptr<net::IOBufferWithSize> buffer,
+    const IOCallback& callback) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  if (device_info_.max_feature_report_size == 0) {
+    // The device does not support feature reports.
+    callback.Run(false, 0);
+    return;
+  }
+  if (IsReportIdProtected(report_id)) {
+    callback.Run(false, 0);
+    return;
+  }
+
+  PlatformSendFeatureReport(report_id, buffer, callback);
+}
+
+bool HidConnection::FilterInputReport(

[jracle (use Gerrit), 2014/06/11 11:20:04]

This is new filtering method, used together with next one and matchers available
in anonymous namespace.

+    scoped_refptr<net::IOBufferWithSize> buffer,
+    const IOCallback& callback) {
+  if (buffer->size() == 0) {
+    return false;
+  }
+
+  if (IsReportIdProtected(buffer->data()[0])) {
+    callback.Run(true, 0);
+    return true;
+  }
+
+  return false;
+}
+
+bool HidConnection::IsReportIdProtected(const uint8_t report_id) {
+  HidCollectionInfo collection_info;
+  if (HasReportId(device_info_, report_id, &collection_info)) {
+    return collection_info.usage.IsProtected();
+  }
+
+  return has_protected_collection();
+}
+
 PendingHidReport::PendingHidReport() {}
 
 PendingHidReport::~PendingHidReport() {}
@@ -14,9 +190,59 @@ PendingHidRead::PendingHidRead() {}
 
 PendingHidRead::~PendingHidRead() {}
 
-HidConnection::HidConnection(const HidDeviceInfo& device_info)
-    : device_info_(device_info) {}
+HidConnection2::HidConnection2(const HidDeviceInfo& device_info)
+    : HidConnection(device_info) {
+}
+
+HidConnection2::~HidConnection2() {
+  Flush();
+}
+
+void HidConnection2::PlatformRead(scoped_refptr<net::IOBufferWithSize> buffer,

[Ken Rockot(use gerrit already), 2014/06/19 19:29:56]

Ah, I see why you created this temporary class now. Um.

[jracle (use Gerrit), 2014/06/19 21:39:36]

Yes sorry, this was to eliminate duplication.

Hence, name is not acceptable as is, of course.

I didn't proceed further refactoring prior to your comments, and was not
inspired to find a relevant name for this class if it has to remain for this CL.

Any idea? :)

On 2014/06/19 19:29:56, Ken Rockot wrote:

+                                  const IOCallback& callback) {
+  PendingHidRead pending_read;
+  pending_read.buffer = buffer;
+  pending_read.callback = callback;
+  pending_reads_.push(pending_read);
+  ProcessReadQueue();
+}
+
+void HidConnection2::ProcessInputReport(
+    scoped_refptr<net::IOBufferWithSize> buffer) {
+  DCHECK(thread_checker().CalledOnValidThread());
+  PendingHidReport report;
+  report.buffer = buffer;
+  pending_reports_.push(report);
+  ProcessReadQueue();
+}
+
+void HidConnection2::Flush() {
+  while (!pending_reads_.empty()) {
+    pending_reads_.front().callback.Run(false, 0);
+    pending_reads_.pop();
+  }
+}
+
+void HidConnection2::ProcessReadQueue() {
+  DCHECK(thread_checker().CalledOnValidThread());
+  while (pending_reads_.size() && pending_reports_.size()) {
+    PendingHidRead read = pending_reads_.front();
+    pending_reads_.pop();
+    PendingHidReport report = pending_reports_.front();
+
+    if (read.buffer->size() < report.buffer->size()) {
+      read.callback.Run(false, report.buffer->size());
+    } else {
+      memcpy(read.buffer->data(), report.buffer->data(), report.buffer->size());
+      pending_reports_.pop();
+
+      if (FilterInputReport(report.buffer, read.callback)) {
+        return;
+      }
 
-HidConnection::~HidConnection() {}
+      read.callback.Run(true, report.buffer->size());
+    }
+  }
+}
 
 }  // namespace device