Usb backend refactor step 2: Separate Usb Device Handle and Usb Device (deprecate)

chrome/browser/usb/usb_service.h

Index: chrome/browser/usb/usb_service.h
diff --git a/chrome/browser/usb/usb_service.h b/chrome/browser/usb/usb_service.h
index f2cfd3cf7f5e3bbc21d35601d9c59fb3c1f0329e..8ce95142cdca5db0da8c43941ae59f1c9d15d382 100644
--- a/chrome/browser/usb/usb_service.h
+++ b/chrome/browser/usb/usb_service.h
@@ -10,65 +10,59 @@
 #include <vector>
 
 #include "base/basictypes.h"
+#include "base/threading/non_thread_safe.h"
 #include "base/threading/platform_thread.h"
 #include "chrome/browser/usb/usb_device.h"
 #include "components/browser_context_keyed_service/browser_context_keyed_service.h"
 #include "third_party/libusb/src/libusb/libusb.h"
 
+class UsbContext;
+class UsbDeviceStub;
 class UsbEventHandler;
+class RefCountedPlatformUsbDevice;
+
 typedef libusb_context* PlatformUsbContext;
 
 // The USB service handles creating and managing an event handler thread that is
-// used to manage and dispatch USB events. It is also responsbile for device
-// discovery on the system, which allows it to re-use device handles to prevent
-// competition for the same USB device.
+// used to manage and dispatch USB events. It is also responsible for device
+// discovery on the system.
 class UsbService : public BrowserContextKeyedService {
  public:
   UsbService();
   virtual ~UsbService();
 
-  // Cleanup must be invoked before the service is destroyed. It interrupts the
-  // event handling thread and disposes of open devices.
-  void Cleanup();
-
+  // The following methods must be called on FILE thread:

[pfeldman, 2013/07/22 08:39:45]

UsbService is created and deleted on UI thread. You can't call its methods on
FILE thread since you are introducing a race condition.

You either need to make it ref-counted + wrap it for use with
BrowserContextKeyedService (as I do in
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/dev...)
or you need to make it only work UI thread only.

I actually think that latter is preferred. You would essentially need to
marshall its public methods to FILE thread first in usb_service.cc, but it
should not be a big deal given that your API has callbacks for return values. As
a benefit, all clients become simpler - they no longer need to do marshalling as
you do in usb_api.cc.

[Bei Zhang, 2013/07/23 17:58:44]

This is the summary of our discussion about this:
1. We determined that the enumeration of the usb devices will require blocking
IOs that is not allowed on UI thread or IO thread.
2. We shall not have more than one UsbService because libusb does not actually
support that.

In conclusion, we will make UsbService a Singleton that works mostly on FILE
thread but construct/destructs on UI thread.


On 2013/07/22 08:39:45, pfeldman wrote:

+  //
   // Find all of the devices attached to the system that are identified by
   // |vendor_id| and |product_id|, inserting them into |devices|. Clears
   // |devices| before use. Calls |callback| once |devices| is populated.
   void FindDevices(const uint16 vendor_id,
                    const uint16 product_id,
                    int interface_id,
-                   std::vector<scoped_refptr<UsbDevice> >* devices,
+                   std::vector<int>* devices,
                    const base::Callback<void()>& callback);
 
-  // Find all of the devices attached to the system, inserting them into
+  // Get all of the devices attached to the system, inserting them into
   // |devices|. Clears |devices| before use.
-  void EnumerateDevices(std::vector<scoped_refptr<UsbDevice> >* devices);
+  void GetDevices(std::vector<int>* devices);

[pfeldman, 2013/07/22 08:39:45]

I don't think this integer handle makes sense. There is a number of operations
available on the device (on libusb_device) without opening it.
libusb_get_device_descriptor and libusb_get_active_config_descriptor would be
good examples. Your UsbDeviceStub captures (not yet opened) device well and I
think you should expose it to the API. I.e. I think your API could reflect
libusb API better:

UsbService -> context, singleton
UsbDevice -> libusb_device
UsbDeviceHandle -> libusb_device_handle
UsbInterface -> libusb_device_handle with claimed interface

those types would use RAII well.

Another idea is that although UsbDevice is not thread-safe, it could be operated
on any IO thread. Like WeakPtr - client picks thread with its first call and
sticks to it. libusb_device is thread-safe, so it should be doable.

[Bei Zhang, 2013/07/23 17:58:44]

Yes, that will be exactly what the step 3 does.

On 2013/07/22 08:39:45, pfeldman wrote:

+
+  // Open a device for further communication.
+  scoped_refptr<UsbDevice> OpenDevice(int device);
 
   // This function should not be called by normal code. It is invoked by a
   // UsbDevice's Close function and disposes of the associated platform handle.
   void CloseDevice(scoped_refptr<UsbDevice> device);
 
+  // Schedule an update to USB device info.
+  void ScheduleRefreshDevices();
+
  private:
-  // RefCountedPlatformUsbDevice takes care of managing the underlying reference
-  // count of a single PlatformUsbDevice. This allows us to construct things
-  // like vectors of RefCountedPlatformUsbDevices and not worry about having to
-  // explicitly unreference them after use.
-  class RefCountedPlatformUsbDevice {
-   public:
-    explicit RefCountedPlatformUsbDevice(PlatformUsbDevice device);
-    RefCountedPlatformUsbDevice(const RefCountedPlatformUsbDevice& other);
-    virtual ~RefCountedPlatformUsbDevice();
-    PlatformUsbDevice device();
-
-   private:
-    PlatformUsbDevice device_;
-  };
-
-  typedef std::vector<RefCountedPlatformUsbDevice> DeviceVector;
+  // BrowserContextKeyedService:
+  virtual void Shutdown() OVERRIDE;
 
   // Return true if |device|'s vendor and product identifiers match |vendor_id|
   // and |product_id|.
-  static bool DeviceMatches(PlatformUsbDevice device,
+  static bool DeviceMatches(const UsbDeviceStub* device,
                             const uint16 vendor_id,
                             const uint16 product_id);
 
@@ -78,26 +72,27 @@ class UsbService : public BrowserContextKeyedService {
   // FindDevices.
   void FindDevicesImpl(const uint16 vendor_id,
                        const uint16 product_id,
-                       std::vector<scoped_refptr<UsbDevice> >* devices,
+                       std::vector<int>* devices,
                        const base::Callback<void()>& callback,
                        bool success);
 
-  // Populates |output| with the result of enumerating all attached USB devices.
-  void EnumerateDevicesImpl(DeviceVector* output);
+  // Enumerate USB devices from OS and Update devices_ map.
+  void RefreshDevices();
+
+  scoped_refptr<UsbContext> context_;
 
-  // If a UsbDevice wrapper corresponding to |device| has already been created,
-  // returns it. Otherwise, opens the device, creates a wrapper, and associates
-  // the wrapper with the device internally.
-  UsbDevice* LookupOrCreateDevice(PlatformUsbDevice device);
+  // The next id of device. Can only be accessed from FILE thread.
+  int next_unique_id_;
 
-  PlatformUsbContext context_;
-  UsbEventHandler* event_handler_;
+  bool device_enumeration_scheduled_;
 
-  // The devices_ map contains scoped_refptrs to all open devices, indicated by
-  // their vendor and product id. This allows for reusing an open device without
-  // creating another platform handle for it.
-  typedef std::map<PlatformUsbDevice, scoped_refptr<UsbDevice> > DeviceMap;
+  // The devices_ map contains all connected devices.
+  // They are not to be used directly outside UsbService. Instead, FindDevice
+  // methods returns their id for accessing them.
+  typedef std::map<PlatformUsbDevice, UsbDeviceStub*> DeviceMap;
+  typedef std::map<int, UsbDeviceStub*> DeviceMapById;
   DeviceMap devices_;
+  DeviceMapById devices_by_id_;
 
   DISALLOW_COPY_AND_ASSIGN(UsbService);
 };