chrome.hid: enrich model with report IDs

device/hid/hid_connection.h

 // 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_CONNECTION_H_
 #define DEVICE_HID_HID_CONNECTION_H_
 
 #include <stdint.h>
 
+#include <queue>
+
 #include "base/callback.h"
 #include "base/memory/ref_counted.h"
+#include "base/threading/thread_checker.h"
 #include "device/hid/hid_device_info.h"
 #include "net/base/io_buffer.h"
 
 namespace device {
 
 class HidConnection : public base::RefCountedThreadSafe<HidConnection> {
  public:
   typedef base::Callback<void(bool success, size_t size)> IOCallback;
 
-  virtual void Read(scoped_refptr<net::IOBufferWithSize> buffer,
-                    const IOCallback& callback) = 0;
-  virtual void Write(uint8_t report_id,
-                     scoped_refptr<net::IOBufferWithSize> buffer,
-                     const IOCallback& callback) = 0;
-  virtual void GetFeatureReport(uint8_t report_id,
-                                scoped_refptr<net::IOBufferWithSize> buffer,
-                                const IOCallback& callback) = 0;
-  virtual void SendFeatureReport(uint8_t report_id,
-                                 scoped_refptr<net::IOBufferWithSize> buffer,
-                                 const IOCallback& callback) = 0;
+  const HidDeviceInfo& device_info() const { return device_info_; }
+  bool has_protected_collection() const { return has_protected_collection_; }
+  bool has_report_id() const { return has_report_id_; }
+  const base::ThreadChecker& thread_checker() const { return thread_checker_; }
 
-  const HidDeviceInfo& device_info() const { return device_info_; }
+  void Read(scoped_refptr<net::IOBufferWithSize> buffer,

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

I made those methods the single entry-point, and create abstract Platform*
methods called within them. Purpose is to concentrate report validation logic
here, as a must-do step.

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

Great!

+            const IOCallback& callback);
+  void Write(uint8_t report_id,
+             scoped_refptr<net::IOBufferWithSize> buffer,
+             const IOCallback& callback);
+  void GetFeatureReport(uint8_t report_id,
+                        scoped_refptr<net::IOBufferWithSize> buffer,
+                        const IOCallback& callback);
+  void SendFeatureReport(uint8_t report_id,
+                         scoped_refptr<net::IOBufferWithSize> buffer,
+                         const IOCallback& callback);
 
  protected:
   friend class base::RefCountedThreadSafe<HidConnection>;
-  friend struct HidDeviceInfo;
 
   explicit HidConnection(const HidDeviceInfo& device_info);
   virtual ~HidConnection();
 
+  virtual void PlatformRead(scoped_refptr<net::IOBufferWithSize> buffer,
+                            const IOCallback& callback) = 0;
+  virtual void PlatformWrite(uint8_t report_id,
+                             scoped_refptr<net::IOBufferWithSize> buffer,
+                             const IOCallback& callback) = 0;
+  virtual void PlatformGetFeatureReport(
+      uint8_t report_id,
+      scoped_refptr<net::IOBufferWithSize> buffer,
+      const IOCallback& callback) = 0;
+  virtual void PlatformSendFeatureReport(
+      uint8_t report_id,
+      scoped_refptr<net::IOBufferWithSize> buffer,
+      const IOCallback& callback) = 0;
+
+  bool FilterInputReport(scoped_refptr<net::IOBufferWithSize> buffer,

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

Can this be private?

+                         const IOCallback& callback);
+
  private:
+  bool IsReportIdProtected(const uint8_t report_id);
+
   const HidDeviceInfo device_info_;
+  bool has_report_id_;
+  bool has_protected_collection_;
+  base::ThreadChecker thread_checker_;
 
   DISALLOW_COPY_AND_ASSIGN(HidConnection);
 };
 
 struct PendingHidReport {

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

I'm very incomfortable with those 2 structs. Their naming is also bizarre
(Pending?) (where is HID stuff in a buffer?).

There should be a means to merge them with structs used in Windows.

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

I agree they could be cleaned up, but I'd recommend waiting for a future CL.

Trying to unify this right now is making the CL way more complicated than it
needs to be. What if for now HidConnection just exposed a protoected
CompleteRead method, which receives a filled IOBufferWithSize and an IOCallback.
This method can perform the filtering and call the callback if allowed.

Then each platform can just issue a CompleteRead(buffer, callback) instead of
running the callback where they're doing it now.

FilterInputReport can then be private, there's no need for a refactor of
HidConnection or pending report structures, no need to unify Windows+Linux+Mac
queueing.

(I think this is all true - sorry if I'm missing something)

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

Agree we should keep them in this CL, that'd be too many changes at a time.

It's a bit late for me to try out your proposal in my brain, but wild guess is
that you've got a good catch there.

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

   PendingHidReport();
   ~PendingHidReport();
 
   scoped_refptr<net::IOBufferWithSize> buffer;
 };
 
 struct PendingHidRead {
   PendingHidRead();
   ~PendingHidRead();
 
   scoped_refptr<net::IOBufferWithSize> buffer;
   HidConnection::IOCallback callback;
 };
 
+class HidConnection2 : public HidConnection {

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

This is of course 'temporary code' before refactoring.
We can try to move this code in parent class, or find relevant name for this
class, which is base class used by HidConnectionLinux and HidConnectionMac.

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

I am very confused by this.

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

Code duplication removal.

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

+ protected:
+  explicit HidConnection2(const HidDeviceInfo& device_info);
+  virtual ~HidConnection2();
+
+  // HidConnection implementation.
+  virtual void PlatformRead(scoped_refptr<net::IOBufferWithSize> buffer,
+                            const IOCallback& callback) OVERRIDE;
+
+  void ProcessInputReport(scoped_refptr<net::IOBufferWithSize> buffer);
+  void Flush();
+
+ private:
+  void ProcessReadQueue();
+
+  std::queue<PendingHidReport> pending_reports_;

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

see above comments

+  std::queue<PendingHidRead> pending_reads_;
+};
+
 }  // namespace device
 
 #endif  // DEVICE_HID_HID_CONNECTION_H_