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

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

 // 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.
 
 #include <algorithm>
 
 #include "base/containers/scoped_ptr_hash_map.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/devtools/device/adb/mock_adb_server.h"
 #include "chrome/browser/devtools/device/devtools_android_bridge.h"
@@ skipping 11 matching lines @@
 
 using content::BrowserThread;
 using device::UsbConfigDescriptor;
 using device::UsbDevice;
 using device::UsbDeviceHandle;
 using device::UsbEndpointDescriptor;
 using device::UsbEndpointDirection;
 using device::UsbInterfaceDescriptor;
 using device::UsbService;
 using device::UsbSynchronizationType;
-using device::UsbTransferCallback;
 using device::UsbTransferType;
 using device::UsbUsageType;
 
 namespace {
 
 struct NoConfigTraits {
   static const int kClass = 0xff;
   static const int kSubclass = 0x42;
   static const int kProtocol = 0x1;
   static const bool kBreaks = false;
@@ skipping 77 matching lines @@
         last_local_socket_(0),
         broken_(false) {
   }
 
   virtual scoped_refptr<UsbDevice> GetDevice() const override {
     return device_;
   }
 
   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 {
     if (device_->claimed_interfaces_.find(interface_number) ==
         device_->claimed_interfaces_.end())
       return false;
 
     device_->claimed_interfaces_.erase(interface_number);
     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 {}
+  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 {}
 
-  virtual void BulkTransfer(UsbEndpointDirection direction,
-                            uint8 endpoint,
-                            net::IOBuffer* buffer,
-                            size_t length,
-                            unsigned int timeout,
-                            const UsbTransferCallback& 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);
         memcpy(&header[0], buffer->data(), length);
         current_message_.reset(
             new AdbMessage(header[0], header[1], header[2], std::string()));
         remaining_body_length_ = header[3];
         uint32 magic = header[5];
         if ((current_message_->command ^ 0xffffffff) != magic) {
           DCHECK(false) << "Header checksum error";
           return;
         }
       } else {
         DCHECK(current_message_.get());
         current_message_->body += std::string(buffer->data(), length);
         remaining_body_length_ -= length;
       }
 
       if (remaining_body_length_ == 0) {
         ProcessIncoming();
       }
 
       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();
     }
   }
 
   template <class D>
   void append(D data) {
     std::copy(reinterpret_cast<char*>(&data),
               (reinterpret_cast<char*>(&data)) + sizeof(D),
               std::back_inserter(output_buffer_));
   }
@@ skipping 110 matching lines @@
                          output_buffer_.begin() + query.size);
     base::MessageLoop::current()->PostTask(
         FROM_HERE,
         base::Bind(query.callback,
                    device::USB_TRANSFER_COMPLETED,
                    query.buffer,
                    query.size));
 
   }
 
-  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_;
   uint32 remaining_body_length_;
   scoped_ptr<AdbMessage> current_message_;
   std::vector<char> output_buffer_;
   std::queue<Query> queries_;
   base::ScopedPtrHashMap<int, MockLocalSocket> local_sockets_;
   int last_local_socket_;
   bool broken_;
 };
 
 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;
     bulk_in.maximum_packet_size = 512;
     bulk_in.transfer_type = device::USB_TRANSFER_BULK;
 
     UsbEndpointDescriptor bulk_out;
     bulk_out.address = 0x01;
     bulk_out.direction = device::USB_DIRECTION_OUTBOUND;
     bulk_out.maximum_packet_size = 512;
     bulk_out.transfer_type = device::USB_TRANSFER_BULK;
 
     UsbInterfaceDescriptor interface_desc;
     interface_desc.interface_number = 0;
     interface_desc.alternate_setting = 0;
     interface_desc.interface_class = T::kClass;
     interface_desc.interface_subclass = T::kSubclass;
     interface_desc.interface_protocol = T::kProtocol;
     interface_desc.endpoints.push_back(bulk_in);
     interface_desc.endpoints.push_back(bulk_out);
 
     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_;
 
  protected:
   virtual ~MockUsbDevice() {}
 
  private:
   UsbConfigDescriptor config_desc_;
 };
 
 class MockUsbService : public UsbService {
  public:
   MockUsbService() {
     devices_.push_back(new MockUsbDevice<AndroidTraits>());
   }
 
   scoped_refptr<UsbDevice> GetDeviceById(uint32 unique_id) override {
     NOTIMPLEMENTED();
     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_) {
       case 0:
         // No devices.
         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:
   int step_;
 };
 
 class DevToolsAndroidBridgeWarmUp
     : public DevToolsAndroidBridge::DeviceCountListener {
  public:
   DevToolsAndroidBridgeWarmUp(base::Closure closure,
                               DevToolsAndroidBridge* adb_bridge)
       : closure_(closure), adb_bridge_(adb_bridge) {}
 
   void DeviceCountChanged(int count) override {
     adb_bridge_->RemoveDeviceCountListener(this);
     closure_.Run();
   }
 
   base::Closure closure_;
   DevToolsAndroidBridge* adb_bridge_;
 };
 
 class AndroidUsbDiscoveryTest : public InProcessBrowserTest {
  protected:
   AndroidUsbDiscoveryTest()
       : 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());
     DCHECK(adb_bridge_);
     adb_bridge_->set_task_scheduler_for_test(base::Bind(
         &AndroidUsbDiscoveryTest::ScheduleDeviceCountRequest, this));
 
     scoped_refptr<UsbDeviceProvider> provider =
         new UsbDeviceProvider(browser()->profile());
 
     AndroidDeviceManager::DeviceProviders providers;
     providers.push_back(provider);
     adb_bridge_->set_device_providers_for_test(providers);
     runner_ = new content::MessageLoopRunner;
   }
 
   void ScheduleDeviceCountRequest(const base::Closure& request) {
     DCHECK_CURRENTLY_ON(BrowserThread::UI);
     scheduler_invoked_++;
     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_;
 };
 
 class AndroidUsbCountTest : public AndroidUsbDiscoveryTest {
  protected:
   void SetUpOnMainThread() override {
     AndroidUsbDiscoveryTest::SetUpOnMainThread();
     DevToolsAndroidBridgeWarmUp warmup(runner_->QuitClosure(), adb_bridge_);
     adb_bridge_->AddDeviceCountListener(&warmup);
     runner_->Run();
     runner_ = new content::MessageLoopRunner;
   }
 };
 
 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();
   }
 };
 
 class MockListListener : public DevToolsAndroidBridge::DeviceListListener {
  public:
   MockListListener(DevToolsAndroidBridge* adb_bridge,
                    const base::Closure& callback)
       : adb_bridge_(adb_bridge),
         callback_(callback) {
   }
@@ skipping 13 matching lines @@
     }
   }
 
   DevToolsAndroidBridge* adb_bridge_;
   base::Closure callback_;
 };
 
 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_;
     adb_bridge_->RemoveDeviceCountListener(this);
     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_;
 };
 
 class MockCountListenerWithReAdd : public MockCountListener {
  public:
   explicit MockCountListenerWithReAdd(
       DevToolsAndroidBridge* adb_bridge)
       : MockCountListener(adb_bridge),
         readd_count_(2) {
   }
@@ skipping 136 matching lines @@
   runner_->Run();
   EXPECT_EQ(2, listener.invoked_);
   EXPECT_EQ(listener.invoked_ - 1, scheduler_invoked_);
 }
 
 IN_PROC_BROWSER_TEST_F(AndroidUsbTraitsTest, TestDeviceCounting) {
   MockCountListenerForCheckingTraits listener(adb_bridge_);
   adb_bridge_->AddDeviceCountListener(&listener);
   runner_->Run();
 }