chrome.hid: enrich model with report IDs

device/hid/hid_connection_win.cc

Index: device/hid/hid_connection_win.cc
diff --git a/device/hid/hid_connection_win.cc b/device/hid/hid_connection_win.cc
index 17448f071823c1d97a1d54a8275ac1f9d02f1727..ba73deb93016f21b2ed2fd7b13ca96e5d3a3c71a 100644
--- a/device/hid/hid_connection_win.cc
+++ b/device/hid/hid_connection_win.cc
@@ -27,8 +27,26 @@ extern "C" {
 #include <setupapi.h>
 #include <winioctl.h>
 
+#include <algorithm>
+
 namespace device {
 
+namespace {
+
+struct CollectionHasReportId {
+  bool operator()(const HidCollectionInfo& info) const {
+    return info.report_ids.size() > 0;
+  }
+};
+
+bool DeviceHasReportId(const HidDeviceInfo& device_info) {

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

Very nice.

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

Thx. Wish I could use lambdas and C++11 though.
Sorry for my baby-knowledge of C++03, I'm so used to lambdas in other languages
that I have hard time reminding me of good std:c++ patterns!!

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

+  return std::find_if(device_info.collections.begin(),
+                      device_info.collections.end(),
+                      CollectionHasReportId()) != device_info.collections.end();
+}
+
+}  // namespace
+
 struct PendingHidTransfer : public base::RefCounted<PendingHidTransfer>,
                             public base::win::ObjectWatcher::Delegate,
                             public base::MessageLoop::DestructionObserver {
@@ -136,7 +154,7 @@ HidConnectionWin::~HidConnectionWin() {
 void HidConnectionWin::Read(scoped_refptr<net::IOBufferWithSize> buffer,
                             const HidConnection::IOCallback& callback) {
   DCHECK(thread_checker_.CalledOnValidThread());
-  if (device_info().input_report_size == 0) {
+  if (device_info().max_input_report_size == 0) {
     // The device does not support input reports.
     callback.Run(false, 0);
     return;
@@ -145,9 +163,9 @@ void HidConnectionWin::Read(scoped_refptr<net::IOBufferWithSize> buffer,
   // This fairly awkward logic is correct: If Windows does not expect a device
   // to supply a report ID in its input reports, it requires the buffer to be
   // 1 byte larger than what the device actually sends.
-  int receive_buffer_size = device_info().input_report_size;
+  int receive_buffer_size = device_info().max_input_report_size;
   int expected_buffer_size = receive_buffer_size;
-  if (!device_info().has_report_id)
+  if (!DeviceHasReportId(device_info()))
     expected_buffer_size -= 1;

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

It's this adjust-by-1 logic which exists throughout the Windows code that I have
big concerns about.

It would be really awesome if you could do some validation of this (testing with
a device which uses report IDs and with one that doesn't).

But if you'd rather defer on that, as long as the logic in this change matches
the logic which existed before, I'm OK with leaving it as-is.

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

I totally follow you on that point, and will validate as you propose. I'm very
uncomfortable with that black-magic (though this would mean removing my great
DeviceHasReportId matcher ;).

Not kidding, I'll try to fix it in that CL (cause, even if I submit today - I
know I'm supposed to be off, just here for 2hs, I'll have probably to submit
other patch sets next week regarding protected reports filtering).

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

[jracle (use Gerrit), 2014/06/10 14:07:40]

Hi Ken,
 
  today I've ben given a special firmware to try out above logic, and it appears
that it is correct! Windows pads with a leading 0x00 reports without ID.

Yet, I'm also grouping logic for filtering out protected usages now, and hence
will commit tomorrow or the day after.

Cheers

On 2014/06/07 12:56:21, jracle wrote:

 
   if (buffer->size() < expected_buffer_size) {
@@ -173,7 +191,7 @@ void HidConnectionWin::Write(uint8_t report_id,
                              scoped_refptr<net::IOBufferWithSize> buffer,
                              const HidConnection::IOCallback& callback) {
   DCHECK(thread_checker_.CalledOnValidThread());
-  if (device_info().output_report_size == 0) {
+  if (device_info().max_output_report_size == 0) {
     // The device does not support output reports.
     callback.Run(false, 0);
     return;
@@ -202,15 +220,15 @@ void HidConnectionWin::GetFeatureReport(
     scoped_refptr<net::IOBufferWithSize> buffer,
     const IOCallback& callback) {
   DCHECK(thread_checker_.CalledOnValidThread());
-  if (device_info().feature_report_size == 0) {
+  if (device_info().max_feature_report_size == 0) {
     // The device does not support feature reports.
     callback.Run(false, 0);
     return;
   }
 
-  int receive_buffer_size = device_info().feature_report_size;
+  int receive_buffer_size = device_info().max_feature_report_size;
   int expected_buffer_size = receive_buffer_size;
-  if (!device_info().has_report_id)
+  if (!DeviceHasReportId(device_info()))
     expected_buffer_size -= 1;
   if (buffer->size() < expected_buffer_size) {
     callback.Run(false, 0);
@@ -242,7 +260,7 @@ void HidConnectionWin::SendFeatureReport(
     scoped_refptr<net::IOBufferWithSize> buffer,
     const IOCallback& callback) {
   DCHECK(thread_checker_.CalledOnValidThread());
-  if (device_info().feature_report_size == 0) {
+  if (device_info().max_feature_report_size == 0) {
     // The device does not support feature reports.
     callback.Run(false, 0);
     return;