Move device/usb classes from the FILE thread to UI thread.

chrome/browser/devtools/device/usb/android_usb_browsertest.cc

Index: chrome/browser/devtools/device/usb/android_usb_browsertest.cc
diff --git a/chrome/browser/devtools/device/usb/android_usb_browsertest.cc b/chrome/browser/devtools/device/usb/android_usb_browsertest.cc
index c7f99ed6dccf78c937fd6e3dde81d8567d3f7723..dce7715d3e6330c3667064eaae12dc698350a0f2 100644
--- a/chrome/browser/devtools/device/usb/android_usb_browsertest.cc
+++ b/chrome/browser/devtools/device/usb/android_usb_browsertest.cc
@@ -29,7 +29,6 @@ using device::UsbEndpointDirection;
 using device::UsbInterfaceDescriptor;
 using device::UsbService;
 using device::UsbSynchronizationType;
-using device::UsbTransferCallback;
 using device::UsbTransferType;
 using device::UsbUsageType;
 
@@ -127,15 +126,22 @@ class MockUsbDeviceHandle : public UsbDeviceHandle {
 
   virtual void Close() override { device_ = nullptr; }
 
-  bool SetConfiguration(int configuration_value) override { return true; }
+  void SetConfiguration(int configuration_value,
+                        const ResultCallback& callback) override {
+    NOTIMPLEMENTED();
+  }
 
-  bool ClaimInterface(int interface_number) override {
-    if (device_->claimed_interfaces_.find(interface_number) !=
-        device_->claimed_interfaces_.end())
-      return false;
+  void ClaimInterface(int interface_number,
+                      const ResultCallback& callback) override {
+    bool success = false;
+    if (device_->claimed_interfaces_.find(interface_number) ==
+        device_->claimed_interfaces_.end()) {
+      device_->claimed_interfaces_.insert(interface_number);
+      success = true;
+    }
 
-    device_->claimed_interfaces_.insert(interface_number);
-    return true;
+    base::MessageLoop::current()->PostTask(FROM_HERE,
+                                           base::Bind(callback, success));
   }
 
   bool ReleaseInterface(int interface_number) override {
@@ -147,35 +153,34 @@ class MockUsbDeviceHandle : public UsbDeviceHandle {
     return true;
   }
 
-  virtual bool SetInterfaceAlternateSetting(int interface_number,
-                                            int alternate_setting) override {
-    return true;
+  void SetInterfaceAlternateSetting(int interface_number,
+                                    int alternate_setting,
+                                    const ResultCallback& callback) override {
+    NOTIMPLEMENTED();
   }
 
-  virtual bool ResetDevice() override { return true; }
-  bool GetStringDescriptor(uint8_t string_id,
-                           base::string16* content) override {
-    return false;
+  void ResetDevice(const ResultCallback& callback) override {
+    NOTIMPLEMENTED();
   }
 
   // Async IO. Can be called on any thread.
-  virtual void ControlTransfer(UsbEndpointDirection direction,
-                               TransferRequestType request_type,
-                               TransferRecipient recipient,
-                               uint8 request,
-                               uint16 value,
-                               uint16 index,
-                               net::IOBuffer* buffer,
-                               size_t length,
-                               unsigned int timeout,
-                               const UsbTransferCallback& callback) override {}
-
-  virtual void BulkTransfer(UsbEndpointDirection direction,
-                            uint8 endpoint,
-                            net::IOBuffer* buffer,
-                            size_t length,
-                            unsigned int timeout,
-                            const UsbTransferCallback& callback) override {
+  void ControlTransfer(UsbEndpointDirection direction,
+                       TransferRequestType request_type,
+                       TransferRecipient recipient,
+                       uint8 request,
+                       uint16 value,
+                       uint16 index,
+                       scoped_refptr<net::IOBuffer> buffer,
+                       size_t length,
+                       unsigned int timeout,
+                       const TransferCallback& callback) override {}
+
+  void BulkTransfer(UsbEndpointDirection direction,
+                    uint8 endpoint,
+                    scoped_refptr<net::IOBuffer> buffer,
+                    size_t length,
+                    unsigned int timeout,
+                    const TransferCallback& callback) override {
     if (direction == device::USB_DIRECTION_OUTBOUND) {
       if (remaining_body_length_ == 0) {
         std::vector<uint32> header(6);
@@ -201,11 +206,10 @@ class MockUsbDeviceHandle : public UsbDeviceHandle {
       device::UsbTransferStatus status =
           broken_ ? device::USB_TRANSFER_ERROR : device::USB_TRANSFER_COMPLETED;
       base::MessageLoop::current()->PostTask(
-          FROM_HERE,
-          base::Bind(callback, status, scoped_refptr<net::IOBuffer>(), 0));
+          FROM_HERE, base::Bind(callback, status, nullptr, 0));
       ProcessQueries();
     } else if (direction == device::USB_DIRECTION_INBOUND) {
-      queries_.push(Query(callback, make_scoped_refptr(buffer), length));
+      queries_.push(Query(callback, buffer, length));
       ProcessQueries();
     }
   }
@@ -336,36 +340,34 @@ class MockUsbDeviceHandle : public UsbDeviceHandle {
 
   }
 
-  virtual void InterruptTransfer(UsbEndpointDirection direction,
-                                 uint8 endpoint,
-                                 net::IOBuffer* buffer,
-                                 size_t length,
-                                 unsigned int timeout,
-                                 const UsbTransferCallback& callback) override {
-  }
+  void InterruptTransfer(UsbEndpointDirection direction,
+                         uint8 endpoint,
+                         scoped_refptr<net::IOBuffer> buffer,
+                         size_t length,
+                         unsigned int timeout,
+                         const TransferCallback& callback) override {}
 
-  virtual void IsochronousTransfer(
-      UsbEndpointDirection direction,
-      uint8 endpoint,
-      net::IOBuffer* buffer,
-      size_t length,
-      unsigned int packets,
-      unsigned int packet_length,
-      unsigned int timeout,
-      const UsbTransferCallback& callback) override {}
+  void IsochronousTransfer(UsbEndpointDirection direction,
+                           uint8 endpoint,
+                           scoped_refptr<net::IOBuffer> buffer,
+                           size_t length,
+                           unsigned int packets,
+                           unsigned int packet_length,
+                           unsigned int timeout,
+                           const TransferCallback& callback) override {}
 
  protected:
   virtual ~MockUsbDeviceHandle() {}
 
   struct Query {
-    UsbTransferCallback callback;
+    TransferCallback callback;
     scoped_refptr<net::IOBuffer> buffer;
     size_t size;
 
-    Query(UsbTransferCallback callback,
+    Query(TransferCallback callback,
           scoped_refptr<net::IOBuffer> buffer,
           int size)
-        : callback(callback), buffer(buffer), size(size) {};
+        : callback(callback), buffer(buffer), size(size) {}
   };
 
   scoped_refptr<MockUsbDevice<T> > device_;
@@ -381,7 +383,13 @@ class MockUsbDeviceHandle : public UsbDeviceHandle {
 template <class T>
 class MockUsbDevice : public UsbDevice {
  public:
-  MockUsbDevice() : UsbDevice(0, 0, 0) {
+  MockUsbDevice()
+      : UsbDevice(0,
+                  0,
+                  0,
+                  base::UTF8ToUTF16(kDeviceManufacturer),
+                  base::UTF8ToUTF16(kDeviceModel),
+                  base::UTF8ToUTF16(kDeviceSerial)) {
     UsbEndpointDescriptor bulk_in;
     bulk_in.address = 0x81;
     bulk_in.direction = device::USB_DIRECTION_INBOUND;
@@ -406,32 +414,17 @@ class MockUsbDevice : public UsbDevice {
     config_desc_.interfaces.push_back(interface_desc);
   }
 
-  virtual scoped_refptr<UsbDeviceHandle> Open() override {
-    return new MockUsbDeviceHandle<T>(this);
+  void Open(const OpenCallback& callback) override {
+    base::MessageLoop::current()->PostTask(
+        FROM_HERE, base::Bind(callback, make_scoped_refptr(
+                                            new MockUsbDeviceHandle<T>(this))));
   }
 
-  virtual const UsbConfigDescriptor* GetConfiguration() override {
+  const UsbConfigDescriptor* GetConfiguration() override {
     return T::kConfigured ? &config_desc_ : nullptr;
   }
 
-  virtual bool GetManufacturer(base::string16* manufacturer) override {
-    *manufacturer = base::UTF8ToUTF16(kDeviceManufacturer);
-    return true;
-  }
-
-  virtual bool GetProduct(base::string16* product) override {
-    *product = base::UTF8ToUTF16(kDeviceModel);
-    return true;
-  }
-
-  virtual bool GetSerialNumber(base::string16* serial) override {
-    *serial = base::UTF8ToUTF16(kDeviceSerial);
-    return true;
-  }
-
-  virtual bool Close(scoped_refptr<UsbDeviceHandle> handle) override {
-    return true;
-  }
+  bool Close(scoped_refptr<UsbDeviceHandle> handle) override { return true; }

[vkuzkokov, 2015/04/15 14:15:46]

nit: keep it on three separate lines.

[Reilly Grant (use Gerrit), 2015/04/15 20:03:47]

I'll try but clang-format likes to put such definitions on one line.

 
   std::set<int> claimed_interfaces_;
 
@@ -453,52 +446,34 @@ class MockUsbService : public UsbService {
     return nullptr;
   }
 
-  void GetDevices(std::vector<scoped_refptr<UsbDevice>>* devices) override {
-    STLClearObject(devices);
-    std::copy(devices_.begin(), devices_.end(), back_inserter(*devices));
+  void GetDevices(const GetDevicesCallback& callback) override {
+    callback.Run(devices_);
   }
 
   std::vector<scoped_refptr<UsbDevice> > devices_;
 };
 
-class MockBreakingUsbService : public UsbService {
+class MockBreakingUsbService : public MockUsbService {
  public:
-  scoped_refptr<UsbDevice> GetDeviceById(uint32 unique_id) override {
-    NOTIMPLEMENTED();
-    return nullptr;
-  }
-
-  void GetDevices(std::vector<scoped_refptr<UsbDevice>>* devices) override {
-    STLClearObject(devices);
-    devices->push_back(new MockUsbDevice<BreakingAndroidTraits>());
+  MockBreakingUsbService() {
+    devices_.clear();
+    devices_.push_back(new MockUsbDevice<BreakingAndroidTraits>());
   }
 };
 
-class MockNoConfigUsbService : public UsbService {
+class MockNoConfigUsbService : public MockUsbService {
  public:
-  scoped_refptr<UsbDevice> GetDeviceById(uint32 unique_id) override {
-    NOTIMPLEMENTED();
-    return nullptr;
-  }
-
-  void GetDevices(std::vector<scoped_refptr<UsbDevice>>* devices) override {
-    STLClearObject(devices);
-    devices->push_back(new MockUsbDevice<AndroidTraits>());
-    devices->push_back(new MockUsbDevice<NoConfigTraits>());
+  MockNoConfigUsbService() {
+    devices_.push_back(new MockUsbDevice<NoConfigTraits>());
   }
 };
 
-class MockUsbServiceForCheckingTraits : public UsbService {
+class MockUsbServiceForCheckingTraits : public MockUsbService {
  public:
   MockUsbServiceForCheckingTraits() : step_(0) {}
 
-  scoped_refptr<UsbDevice> GetDeviceById(uint32 unique_id) override {
-    NOTIMPLEMENTED();
-    return nullptr;
-  }
-
-  void GetDevices(std::vector<scoped_refptr<UsbDevice>>* devices) override {
-    STLClearObject(devices);
+  void GetDevices(const GetDevicesCallback& callback) override {
+    std::vector<scoped_refptr<UsbDevice>> devices;
     // This switch should be kept in sync with
     // AndroidUsbBrowserTest::DeviceCountChanged.
     switch (step_) {
@@ -507,19 +482,20 @@ class MockUsbServiceForCheckingTraits : public UsbService {
         break;
       case 1:
         // Android device.
-        devices->push_back(new MockUsbDevice<AndroidTraits>());
+        devices.push_back(new MockUsbDevice<AndroidTraits>());
         break;
       case 2:
         // Android and non-android device.
-        devices->push_back(new MockUsbDevice<AndroidTraits>());
-        devices->push_back(new MockUsbDevice<NonAndroidTraits>());
+        devices.push_back(new MockUsbDevice<AndroidTraits>());
+        devices.push_back(new MockUsbDevice<NonAndroidTraits>());
         break;
       case 3:
         // Non-android device.
-        devices->push_back(new MockUsbDevice<NonAndroidTraits>());
+        devices.push_back(new MockUsbDevice<NonAndroidTraits>());
         break;
     }
     step_++;
+    callback.Run(devices);
   }
 
  private:
@@ -548,15 +524,7 @@ class AndroidUsbDiscoveryTest : public InProcessBrowserTest {
       : scheduler_invoked_(0) {
   }
   void SetUpOnMainThread() override {
-    scoped_refptr<content::MessageLoopRunner> runner =
-        new content::MessageLoopRunner;
-
-    BrowserThread::PostTaskAndReply(
-        BrowserThread::FILE,
-        FROM_HERE,
-        base::Bind(&AndroidUsbDiscoveryTest::SetUpService, this),
-        runner->QuitClosure());
-    runner->Run();
+    mock_usb_service_.reset(CreateMockService());
 
     adb_bridge_ =
         DevToolsAndroidBridge::Factory::GetForProfile(browser()->profile());
@@ -579,23 +547,10 @@ class AndroidUsbDiscoveryTest : public InProcessBrowserTest {
     BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, request);
   }
 
-  virtual void SetUpService() {
-    UsbService::SetInstanceForTest(new MockUsbService());
-  }
-
-  void TearDownOnMainThread() override {
-    scoped_refptr<content::MessageLoopRunner> runner =
-        new content::MessageLoopRunner;
-    UsbService* service = nullptr;
-    BrowserThread::PostTaskAndReply(
-        BrowserThread::FILE,
-        FROM_HERE,
-        base::Bind(&UsbService::SetInstanceForTest, service),
-        runner->QuitClosure());
-    runner->Run();
-  }
+  virtual MockUsbService* CreateMockService() { return new MockUsbService(); }
 
   scoped_refptr<content::MessageLoopRunner> runner_;
+  scoped_ptr<MockUsbService> mock_usb_service_;
   DevToolsAndroidBridge* adb_bridge_;
   int scheduler_invoked_;
 };
@@ -613,22 +568,22 @@ class AndroidUsbCountTest : public AndroidUsbDiscoveryTest {
 
 class AndroidUsbTraitsTest : public AndroidUsbDiscoveryTest {
  protected:
-  void SetUpService() override {
-    UsbService::SetInstanceForTest(new MockUsbServiceForCheckingTraits());
+  MockUsbService* CreateMockService() override {
+    return new MockUsbServiceForCheckingTraits();
   }
 };
 
 class AndroidBreakingUsbTest : public AndroidUsbDiscoveryTest {
  protected:
-  void SetUpService() override {
-    UsbService::SetInstanceForTest(new MockBreakingUsbService());
+  MockUsbService* CreateMockService() override {
+    return new MockBreakingUsbService();
   }
 };
 
 class AndroidNoConfigUsbTest : public AndroidUsbDiscoveryTest {
  protected:
-  void SetUpService() override {
-    UsbService::SetInstanceForTest(new MockNoConfigUsbService());
+  MockUsbService* CreateMockService() override {
+    return new MockNoConfigUsbService();
   }
 };
 
@@ -662,10 +617,7 @@ class MockListListener : public DevToolsAndroidBridge::DeviceListListener {
 class MockCountListener : public DevToolsAndroidBridge::DeviceCountListener {
  public:
   explicit MockCountListener(DevToolsAndroidBridge* adb_bridge)
-      : adb_bridge_(adb_bridge),
-        reposts_left_(10),
-        invoked_(0) {
-  }
+      : adb_bridge_(adb_bridge), invoked_(0) {}
 
   void DeviceCountChanged(int count) override {
     ++invoked_;
@@ -673,33 +625,9 @@ class MockCountListener : public DevToolsAndroidBridge::DeviceCountListener {
     Shutdown();
   }
 
-  void Shutdown() {
-    ShutdownOnUIThread();
-  };
-
-  void ShutdownOnUIThread() {
-    DCHECK_CURRENTLY_ON(BrowserThread::UI);
-    if (reposts_left_-- == 0) {
-      base::MessageLoop::current()->Quit();
-    } else {
-      BrowserThread::PostTask(
-          BrowserThread::FILE,
-          FROM_HERE,
-          base::Bind(&MockCountListener::ShutdownOnFileThread,
-                     base::Unretained(this)));
-    }
-  }
-
-  void ShutdownOnFileThread() {
-    DCHECK_CURRENTLY_ON(BrowserThread::FILE);
-    BrowserThread::PostTask(BrowserThread::UI,
-                            FROM_HERE,
-                            base::Bind(&MockCountListener::ShutdownOnUIThread,
-                                       base::Unretained(this)));
-  }
+  void Shutdown() { base::MessageLoop::current()->Quit(); }

[vkuzkokov, 2015/04/15 14:15:46]

Thread-hopping serves a purpose in test. We check that device counting will not
be rescheduled after we remove last listener. As far as I understand we can
remove hopping to FILE though.

[Reilly Grant (use Gerrit), 2015/04/15 20:03:47]

I think this is tested by the AndroidUsbCountTests that check
scheduler_invoked_. The code here just bounces back and forth 10 times before
quitting. There are no checks performed.

[vkuzkokov, 2015/04/15 21:58:06]

That last check is enough. Bouncing is needed in case there is a task posted
that would cause extra scheduling.

[Reilly Grant (use Gerrit), 2015/04/15 22:26:44]

The FILE thread isn't used anymore so I can just use IsIdleForTesting to check
that nothing has been scheduled on the UI thread.

 
   DevToolsAndroidBridge* adb_bridge_;
-  int reposts_left_;
   int invoked_;
 };