chrome.hid: enrich model with report IDs

device/hid/hid_connection_linux.cc

Index: device/hid/hid_connection_linux.cc
diff --git a/device/hid/hid_connection_linux.cc b/device/hid/hid_connection_linux.cc
index 331e3f30856adede0451301adee8ff70d1442fff..060373c923020befebfe693315a6fc8345e9fcdc 100644
--- a/device/hid/hid_connection_linux.cc
+++ b/device/hid/hid_connection_linux.cc
@@ -46,8 +46,6 @@ scoped_refptr<net::IOBufferWithSize> CopyBufferWithReportId(
 HidConnectionLinux::HidConnectionLinux(HidDeviceInfo device_info,
                                        std::string dev_node)
     : HidConnection(device_info) {
-  DCHECK(thread_checker_.CalledOnValidThread());
-
   int flags = base::File::FLAG_OPEN |
               base::File::FLAG_READ |
               base::File::FLAG_WRITE;
@@ -88,49 +86,13 @@ HidConnectionLinux::HidConnectionLinux(HidDeviceInfo device_info,
 }
 
 HidConnectionLinux::~HidConnectionLinux() {
-  DCHECK(thread_checker_.CalledOnValidThread());
   Disconnect();
+  Flush();
 }
 
-void HidConnectionLinux::OnFileCanReadWithoutBlocking(int fd) {
-  DCHECK(thread_checker_.CalledOnValidThread());
-  DCHECK_EQ(fd, device_file_.GetPlatformFile());
-
-  uint8 buffer[1024] = {0};
-  int bytes_read =
-      HANDLE_EINTR(read(device_file_.GetPlatformFile(), buffer, 1024));
-  if (bytes_read < 0) {
-    if (errno == EAGAIN) {
-      return;
-    }
-    VPLOG(1) << "Read failed";
-    Disconnect();
-    return;
-  }
-
-  PendingHidReport report;
-  report.buffer = new net::IOBufferWithSize(bytes_read);
-  memcpy(report.buffer->data(), buffer, bytes_read);
-  pending_reports_.push(report);
-  ProcessReadQueue();
-}
-
-void HidConnectionLinux::OnFileCanWriteWithoutBlocking(int fd) {}
-
-void HidConnectionLinux::Disconnect() {
-  DCHECK(thread_checker_.CalledOnValidThread());
-  device_file_watcher_.StopWatchingFileDescriptor();
-  device_file_.Close();
-  while (!pending_reads_.empty()) {
-    PendingHidRead pending_read = pending_reads_.front();
-    pending_reads_.pop();
-    pending_read.callback.Run(false, 0);
-  }
-}
-
-void HidConnectionLinux::Read(scoped_refptr<net::IOBufferWithSize> buffer,
-                              const IOCallback& callback) {
-  DCHECK(thread_checker_.CalledOnValidThread());
+void HidConnectionLinux::PlatformRead(
+    scoped_refptr<net::IOBufferWithSize> buffer,
+    const IOCallback& callback) {
   PendingHidRead pending_read;
   pending_read.buffer = buffer;
   pending_read.callback = callback;
@@ -138,10 +100,10 @@ void HidConnectionLinux::Read(scoped_refptr<net::IOBufferWithSize> buffer,
   ProcessReadQueue();
 }
 
-void HidConnectionLinux::Write(uint8_t report_id,
-                               scoped_refptr<net::IOBufferWithSize> buffer,
-                               const IOCallback& callback) {
-  DCHECK(thread_checker_.CalledOnValidThread());
+void HidConnectionLinux::PlatformWrite(
+    uint8_t report_id,
+    scoped_refptr<net::IOBufferWithSize> buffer,
+    const IOCallback& callback) {
   // If report ID is non-zero, insert it into a new copy of the buffer.
   if (report_id != 0)
     buffer = CopyBufferWithReportId(buffer, report_id);
@@ -156,12 +118,10 @@ void HidConnectionLinux::Write(uint8_t report_id,
   }
 }
 
-void HidConnectionLinux::GetFeatureReport(
+void HidConnectionLinux::PlatformGetFeatureReport(
     uint8_t report_id,
     scoped_refptr<net::IOBufferWithSize> buffer,
     const IOCallback& callback) {
-  DCHECK(thread_checker_.CalledOnValidThread());
-
   if (buffer->size() == 0) {
     callback.Run(false, 0);
     return;
@@ -180,11 +140,10 @@ void HidConnectionLinux::GetFeatureReport(
   }
 }
 
-void HidConnectionLinux::SendFeatureReport(
+void HidConnectionLinux::PlatformSendFeatureReport(
     uint8_t report_id,
     scoped_refptr<net::IOBufferWithSize> buffer,
     const IOCallback& callback) {
-  DCHECK(thread_checker_.CalledOnValidThread());
   if (report_id != 0)
     buffer = CopyBufferWithReportId(buffer, report_id);
   int result = ioctl(device_file_.GetPlatformFile(),
@@ -198,17 +157,70 @@ void HidConnectionLinux::SendFeatureReport(
   }
 }
 
+void HidConnectionLinux::OnFileCanReadWithoutBlocking(int fd) {
+  DCHECK(thread_checker().CalledOnValidThread());
+  DCHECK_EQ(fd, device_file_.GetPlatformFile());
+
+  uint8 raw_buffer[1024] = {0};
+  int bytes_read =
+      HANDLE_EINTR(read(device_file_.GetPlatformFile(), raw_buffer, 1024));
+  if (bytes_read < 0) {
+    if (errno == EAGAIN) {
+      return;
+    }
+    VPLOG(1) << "Read failed";
+    Disconnect();
+    return;
+  }
+
+  scoped_refptr<net::IOBufferWithSize> buffer =
+      new net::IOBufferWithSize(bytes_read);
+  memcpy(buffer->data(), raw_buffer, bytes_read);
+
+  ProcessInputReport(buffer);
+}
+
+void HidConnectionLinux::OnFileCanWriteWithoutBlocking(int fd) {
+}
+
+void HidConnectionLinux::Disconnect() {
+  DCHECK(thread_checker().CalledOnValidThread());
+  device_file_watcher_.StopWatchingFileDescriptor();
+  device_file_.Close();
+
+  Flush();
+}
+
+void HidConnectionLinux::Flush() {
+  while (!pending_reads_.empty()) {
+    pending_reads_.front().callback.Run(false, 0);
+    pending_reads_.pop();
+  }
+}
+
+void HidConnectionLinux::ProcessInputReport(
+    scoped_refptr<net::IOBufferWithSize> buffer) {
+  DCHECK(thread_checker().CalledOnValidThread());
+  PendingHidReport report;
+  report.buffer = buffer;
+  pending_reports_.push(report);
+  ProcessReadQueue();
+}
+
 void HidConnectionLinux::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 (report.buffer->size() > read.buffer->size()) {
+
+    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();
-      read.callback.Run(true, report.buffer->size());
+
+      CompleteRead(report.buffer, read.callback);

[Ken Rockot(use gerrit already), 2014/06/30 15:19:46]

Let me propose one final significant change. What if CompleteRead returned a
boolean indicating whether or not it called the callback.

Then in the case where a report is filtered, CompleteRead won't actually call
the callback at all; instead it will just return false, and the platform
implementation can requeue the pending read.

This has the nice effect of API users not getting lots of empty reports, which I
think will be kind of annoying othwerwise (imagine monitoring a keyboard's
battery life, and constantly having to check if the HID report you received was
empty because the user keeps typing on the keyboard and generating filtered
input reports.)

[jracle (use Gerrit), 2014/06/30 15:38:39]

That's true!..

On 2014/06/30 15:19:46, Ken Rockot wrote:

[jracle (use Gerrit), 2014/06/30 16:00:40]

We might also get rid of FilterInputReport helper, no?
I'm a bit uncomfortable with it.

On 2014/06/30 15:38:39, jracle wrote:

     }
   }
 }