Open HID connections asynchronously.

By making HidService::Connect return its result asynchronously platform
specific details such as whether device nodes must be opened on a
different thread (due to blocking) or requesting access from the Chrome
OS permission broker can be abstracted away.

BUG=422540

Review URL: https://codereview.chromium.org/660573007

Cr-Commit-Position: refs/heads/master@{#299950}

device/hid/hid_connection_unittest.cc

@@ -21,14 +21,39 @@ namespace {
 
 using net::IOBufferWithSize;
 
-class TestCompletionCallback {
+class TestConnectCallback {
  public:
-  TestCompletionCallback()
-      : read_callback_(base::Bind(&TestCompletionCallback::SetReadResult,
-                                  base::Unretained(this))),
-        write_callback_(base::Bind(&TestCompletionCallback::SetWriteResult,
+  TestConnectCallback()
+      : callback_(base::Bind(&TestConnectCallback::SetConnection,
                              base::Unretained(this))) {}
-  ~TestCompletionCallback() {}
+  ~TestConnectCallback() {}
+
+  void SetConnection(scoped_refptr<HidConnection> connection) {
+    connection_ = connection;
+    run_loop_.Quit();
+  }
+
+  scoped_refptr<HidConnection> WaitForConnection() {
+    run_loop_.Run();
+    return connection_;
+  }
+
+  const HidService::ConnectCallback& callback() { return callback_; }
+
+ private:
+  HidService::ConnectCallback callback_;
+  base::RunLoop run_loop_;
+  scoped_refptr<HidConnection> connection_;
+};
+
+class TestIoCallback {
+ public:
+  TestIoCallback()
+      : read_callback_(
+            base::Bind(&TestIoCallback::SetReadResult, base::Unretained(this))),
+        write_callback_(base::Bind(&TestIoCallback::SetWriteResult,
+                                   base::Unretained(this))) {}
+  ~TestIoCallback() {}
 
   void SetReadResult(bool success,
                      scoped_refptr<net::IOBuffer> buffer,
@@ -128,7 +153,9 @@ class HidConnectionTest : public testing::Test {
 TEST_F(HidConnectionTest, ReadWrite) {
   if (!UsbTestGadget::IsTestEnabled()) return;
 
-  scoped_refptr<HidConnection> conn = service_->Connect(device_id_);
+  TestConnectCallback connect_callback;
+  service_->Connect(device_id_, connect_callback.callback());
+  scoped_refptr<HidConnection> conn = connect_callback.WaitForConnection();
   ASSERT_TRUE(conn.get());
 
   for (int i = 0; i < 8; ++i) {
@@ -138,11 +165,11 @@ TEST_F(HidConnectionTest, ReadWrite) {
       buffer->data()[j] = i + j - 1;
     }
 
-    TestCompletionCallback write_callback;
+    TestIoCallback write_callback;
     conn->Write(buffer, buffer->size(), write_callback.write_callback());
     ASSERT_TRUE(write_callback.WaitForResult());
 
-    TestCompletionCallback read_callback;
+    TestIoCallback read_callback;
     conn->Read(read_callback.read_callback());
     ASSERT_TRUE(read_callback.WaitForResult());
     ASSERT_EQ(9UL, read_callback.size());

device/hid/hid_service.h

@@ -20,6 +20,9 @@ class HidConnection;
 
 class HidService {
  public:
+  typedef base::Callback<void(scoped_refptr<HidConnection> connection)>
+      ConnectCallback;
+
   static HidService* GetInstance(
       scoped_refptr<base::SingleThreadTaskRunner> file_task_runner,
       scoped_refptr<base::SingleThreadTaskRunner> ui_task_runner);
@@ -31,15 +34,10 @@ class HidService {
   // Returns |true| if successful or |false| if |device_id| is invalid.
   bool GetDeviceInfo(const HidDeviceId& device_id, HidDeviceInfo* info) const;
 
-#if defined(OS_CHROMEOS)
-  // Requests access to the given device from the Chrome OS permission broker.
-  virtual void RequestAccess(
-      const HidDeviceId& device_id,
-      const base::Callback<void(bool success)>& callback) = 0;
-#endif
-
-  virtual scoped_refptr<HidConnection> Connect(
-      const HidDeviceId& device_id) = 0;
+  // Opens a connection to a device. The callback will be run with null on
+  // failure.
+  virtual void Connect(const HidDeviceId& device_id,
+                       const ConnectCallback& callback) = 0;
 
  protected:
   friend class HidConnectionTest;

device/hid/hid_service_linux.cc

@@ -38,75 +38,90 @@ const char kHIDName[] = "HID_NAME";
 const char kHIDUnique[] = "HID_UNIQ";
 const char kSysfsReportDescriptorKey[] = "report_descriptor";
 
-}  // namespace
-
-HidServiceLinux::HidServiceLinux(
-    scoped_refptr<base::SingleThreadTaskRunner> ui_task_runner)
-    : ui_task_runner_(ui_task_runner),
-      weak_factory_(this) {
-  base::ThreadRestrictions::AssertIOAllowed();
-  task_runner_ = base::ThreadTaskRunnerHandle::Get();
-  DeviceMonitorLinux* monitor = DeviceMonitorLinux::GetInstance();
-  monitor->AddObserver(this);
-  monitor->Enumerate(
-      base::Bind(&HidServiceLinux::OnDeviceAdded, weak_factory_.GetWeakPtr()));
+#if defined(OS_CHROMEOS)
+void OnRequestAccessComplete(
+    scoped_refptr<base::SingleThreadTaskRunner> reply_task_runner,
+    const base::Callback<void(bool success)>& callback,
+    bool success) {
+  reply_task_runner->PostTask(FROM_HERE, base::Bind(callback, success));
 }
 
-#if defined(OS_CHROMEOS)
-void HidServiceLinux::RequestAccess(
-    const HidDeviceId& device_id,
+void RequestAccess(
+    const std::string& device_node,
+    scoped_refptr<base::SingleThreadTaskRunner> reply_task_runner,
     const base::Callback<void(bool success)>& callback) {
   bool success = false;
-  ScopedUdevDevicePtr device =
-      DeviceMonitorLinux::GetInstance()->GetDeviceFromPath(
-          device_id);
-
-  if (device) {
-    const char* dev_node = udev_device_get_devnode(device.get());
 
   if (base::SysInfo::IsRunningOnChromeOS()) {
     chromeos::PermissionBrokerClient* client =
         chromeos::DBusThreadManager::Get()->GetPermissionBrokerClient();
     DCHECK(client) << "Could not get permission broker client.";
     if (client) {
-        ui_task_runner_->PostTask(
-            FROM_HERE,
-            base::Bind(&chromeos::PermissionBrokerClient::RequestPathAccess,
-                       base::Unretained(client),
-                       std::string(dev_node),
+      client->RequestPathAccess(
+          device_node,
           -1,
-                       base::Bind(&HidServiceLinux::OnRequestAccessComplete,
-                                  weak_factory_.GetWeakPtr(),
-                                  callback)));
+          base::Bind(OnRequestAccessComplete, reply_task_runner, callback));
       return;
     }
   } else {
     // Not really running on Chrome OS, declare success.
     success = true;
   }
-  }
-  task_runner_->PostTask(FROM_HERE, base::Bind(callback, success));
+
+  reply_task_runner->PostTask(FROM_HERE, base::Bind(callback, success));
 }
 #endif
 
-scoped_refptr<HidConnection> HidServiceLinux::Connect(
-    const HidDeviceId& device_id) {
-  HidDeviceInfo device_info;
-  if (!GetDeviceInfo(device_id, &device_info))
-    return NULL;
+}  // namespace
+
+HidServiceLinux::HidServiceLinux(
+    scoped_refptr<base::SingleThreadTaskRunner> ui_task_runner)
+    : ui_task_runner_(ui_task_runner),
+      weak_factory_(this) {
+  base::ThreadRestrictions::AssertIOAllowed();
+  task_runner_ = base::ThreadTaskRunnerHandle::Get();
+  DeviceMonitorLinux* monitor = DeviceMonitorLinux::GetInstance();
+  monitor->AddObserver(this);
+  monitor->Enumerate(
+      base::Bind(&HidServiceLinux::OnDeviceAdded, weak_factory_.GetWeakPtr()));
+}
+
+void HidServiceLinux::Connect(const HidDeviceId& device_id,
+                              const ConnectCallback& callback) {
+  DCHECK(thread_checker_.CalledOnValidThread());
 
   ScopedUdevDevicePtr device =
       DeviceMonitorLinux::GetInstance()->GetDeviceFromPath(
-          device_info.device_id);
-
-  if (device) {
-    const char* dev_node = udev_device_get_devnode(device.get());
-    if (dev_node) {
-      return new HidConnectionLinux(device_info, dev_node);
+          device_id);
+  if (!device) {
+    task_runner_->PostTask(FROM_HERE, base::Bind(callback, nullptr));
+    return;
   }
+
+  const char* device_node = udev_device_get_devnode(device.get());
+  if (!device_node) {
+    task_runner_->PostTask(FROM_HERE, base::Bind(callback, nullptr));
+    return;
   }
 
-  return NULL;
+  base::Callback<void(bool success)> finish_connect =
+      base::Bind(&HidServiceLinux::FinishConnect,
+                 weak_factory_.GetWeakPtr(),
+                 device_id,
+                 std::string(device_node),
+                 callback);
+
+#if defined(OS_CHROMEOS)
+  ui_task_runner_->PostTask(FROM_HERE,
+                            base::Bind(RequestAccess,
+                                       std::string(device_node),
+                                       task_runner_,
+                                       finish_connect));
+#else
+  // Use the task runner to preserve the asynchronous behavior of this call on
+  // non-Chrome OS platforms.
+  task_runner_->PostTask(FROM_HERE, base::Bind(finish_connect, true));
+#endif
 }
 
 HidServiceLinux::~HidServiceLinux() {
@@ -195,10 +210,22 @@ void HidServiceLinux::OnDeviceRemoved(udev_device* device) {
   }
 }
 
-void HidServiceLinux::OnRequestAccessComplete(
-    const base::Callback<void(bool success)>& callback,
+void HidServiceLinux::FinishConnect(
+    const HidDeviceId& device_id,
+    const std::string device_node,
+    const base::Callback<void(scoped_refptr<HidConnection>)>& callback,
     bool success) {
-  task_runner_->PostTask(FROM_HERE, base::Bind(callback, success));
+  DCHECK(thread_checker_.CalledOnValidThread());
+  if (!success) {
+    callback.Run(nullptr);
+  }
+
+  const auto& map_entry = devices().find(device_id);
+  if (map_entry == devices().end()) {
+    callback.Run(nullptr);
+  }
+
+  callback.Run(new HidConnectionLinux(map_entry->second, device_node));
 }
 
 }  // namespace device

device/hid/hid_service_linux.h

@@ -23,14 +23,8 @@ class HidServiceLinux : public HidService,
  public:
   HidServiceLinux(scoped_refptr<base::SingleThreadTaskRunner> ui_task_runner);
 
-#if defined(OS_CHROMEOS)
-  virtual void RequestAccess(
-      const HidDeviceId& device_id,
-      const base::Callback<void(bool success)>& callback) override;
-#endif
-
-  virtual scoped_refptr<HidConnection> Connect(const HidDeviceId& device_id)
-      override;
+  virtual void Connect(const HidDeviceId& device_id,
+                       const ConnectCallback& callback) override;
 
   // Implements DeviceMonitorLinux::Observer:
   virtual void OnDeviceAdded(udev_device* device) override;
@@ -39,8 +33,9 @@ class HidServiceLinux : public HidService,
  private:
   virtual ~HidServiceLinux();
 
-  void OnRequestAccessComplete(
-      const base::Callback<void(bool success)>& callback,
+  void FinishConnect(const HidDeviceId& device_id,
+                     const std::string device_node,
+                     const ConnectCallback& callback,
                      bool success);
 
   scoped_refptr<base::SingleThreadTaskRunner> task_runner_;

device/hid/hid_service_mac.cc

@@ -259,13 +259,14 @@ void HidServiceMac::RemoveDevices() {
   }
 }
 
-scoped_refptr<HidConnection> HidServiceMac::Connect(
-    const HidDeviceId& device_id) {
+void HidServiceMac::Connect(const HidDeviceId& device_id,
+                            const ConnectCallback& callback) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   const auto& map_entry = devices().find(device_id);
   if (map_entry == devices().end()) {
-    return NULL;
+    task_runner_->PostTask(FROM_HERE, base::Bind(callback, nullptr));
+    return;
   }
   const HidDeviceInfo& device_info = map_entry->second;
 
@@ -275,7 +276,8 @@ scoped_refptr<HidConnection> HidServiceMac::Connect(
       IORegistryEntryFromPath(kIOMasterPortDefault, service_path));
   if (!service.get()) {
     VLOG(1) << "IOService not found for path: " << device_id;
-    return NULL;
+    task_runner_->PostTask(FROM_HERE, base::Bind(callback, nullptr));
+    return;
   }
 
   base::ScopedCFTypeRef<IOHIDDeviceRef> hid_device(
@@ -284,11 +286,15 @@ scoped_refptr<HidConnection> HidServiceMac::Connect(
   if (result != kIOReturnSuccess) {
     VLOG(1) << "Failed to open device: "
             << base::StringPrintf("0x%04x", result);
-    return NULL;
+    task_runner_->PostTask(FROM_HERE, base::Bind(callback, nullptr));
+    return;
   }
 
-  return make_scoped_refptr(new HidConnectionMac(
-      hid_device.release(), device_info, file_task_runner_));
+  task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(callback,
+                 make_scoped_refptr(new HidConnectionMac(
+                     hid_device.release(), device_info, file_task_runner_))));
 }
 
 }  // namespace device

device/hid/hid_service_mac.h

@@ -27,8 +27,8 @@ class HidServiceMac : public HidService {
  public:
   HidServiceMac(scoped_refptr<base::SingleThreadTaskRunner> ui_task_runner);
 
-  virtual scoped_refptr<HidConnection> Connect(const HidDeviceId& device_id)
-      override;
+  virtual void Connect(const HidDeviceId& device_id,
+                       const ConnectCallback& connect) override;
 
  private:
   virtual ~HidServiceMac();

device/hid/hid_service_win.cc

@@ -6,9 +6,13 @@
 
 #include <cstdlib>
 
+#include "base/bind.h"
 #include "base/files/file.h"
+#include "base/location.h"
+#include "base/single_thread_task_runner.h"
 #include "base/stl_util.h"
 #include "base/strings/sys_string_conversions.h"
+#include "base/thread_task_runner_handle.h"
 #include "base/threading/thread_restrictions.h"
 #include "device/hid/hid_connection_win.h"
 #include "device/hid/hid_device_info.h"
@@ -37,6 +41,8 @@ const char kHIDClass[] = "HIDClass";
 
 HidServiceWin::HidServiceWin() {
   base::ThreadRestrictions::AssertIOAllowed();
+  task_runner_ = base::ThreadTaskRunnerHandle::Get();
+  DCHECK(task_runner_.get());
   Enumerate();
 }
 
@@ -304,17 +310,22 @@ void HidServiceWin::GetDevices(std::vector<HidDeviceInfo>* devices) {
   HidService::GetDevices(devices);
 }
 
-scoped_refptr<HidConnection> HidServiceWin::Connect(
-    const HidDeviceId& device_id) {
-  HidDeviceInfo device_info;
-  if (!GetDeviceInfo(device_id, &device_info))
-    return NULL;
+void HidServiceWin::Connect(const HidDeviceId& device_id,
+                            const ConnectCallback& callback) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  const auto& map_entry = devices().find(device_id);
+  if (map_entry == devices().end()) {
+    task_runner_->PostTask(FROM_HERE, base::Bind(callback, nullptr));
+    return;
+  }
+  const HidDeviceInfo& device_info = map_entry->second;
+
   scoped_refptr<HidConnectionWin> connection(new HidConnectionWin(device_info));
   if (!connection->available()) {
-    PLOG(ERROR) << "Failed to open device.";
-    return NULL;
+    PLOG(ERROR) << "Failed to open device";
+    connection = nullptr;
   }
-  return connection;
+  task_runner_->PostTask(FROM_HERE, base::Bind(callback, connection));
 }
 
 }  // namespace device

device/hid/hid_service_win.h

@@ -32,8 +32,8 @@ class HidServiceWin : public HidService {
 
   virtual void GetDevices(std::vector<HidDeviceInfo>* devices) override;
 
-  virtual scoped_refptr<HidConnection> Connect(const HidDeviceId& device_id)
-      override;
+  virtual void Connect(const HidDeviceId& device_id,
+                       const ConnectCallback& callback) override;
 
  private:
   virtual ~HidServiceWin();
@@ -50,6 +50,8 @@ class HidServiceWin : public HidService {
   void PlatformAddDevice(const std::string& device_path);
   void PlatformRemoveDevice(const std::string& device_path);
 
+  scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
+
   DISALLOW_COPY_AND_ASSIGN(HidServiceWin);
 };
 

extensions/browser/api/hid/hid_api.cc

@@ -141,38 +141,28 @@ bool HidConnectFunction::Prepare() {
 }
 
 void HidConnectFunction::AsyncWorkStart() {
-  if (!device_manager_->GetDeviceInfo(parameters_->device_id, &device_info_)) {
+  HidDeviceInfo device_info;
+  if (!device_manager_->GetDeviceInfo(parameters_->device_id, &device_info)) {
     CompleteWithError(kErrorInvalidDeviceId);
     return;
   }
 
-  if (!device_manager_->HasPermission(extension(), device_info_)) {
+  if (!device_manager_->HasPermission(extension(), device_info)) {
     LOG(WARNING) << "Insufficient permissions to access device.";
     CompleteWithError(kErrorPermissionDenied);
     return;
   }
 
-#if defined(OS_CHROMEOS)
   HidService* hid_service = device::DeviceClient::Get()->GetHidService();
   DCHECK(hid_service);
-  hid_service->RequestAccess(
-      device_info_.device_id,
-      base::Bind(&HidConnectFunction::OnRequestAccessComplete, this));
-#else
-  OnRequestAccessComplete(true);
-#endif
-}
 
-void HidConnectFunction::OnRequestAccessComplete(bool success) {
-  if (!success) {
-    CompleteWithError(kErrorPermissionDenied);
-    return;
-  }
+  hid_service->Connect(
+      device_info.device_id,
+      base::Bind(&HidConnectFunction::OnConnectComplete, this));
+}
 
-  HidService* hid_service = device::DeviceClient::Get()->GetHidService();
-  DCHECK(hid_service);
-  scoped_refptr<HidConnection> connection =
-      hid_service->Connect(device_info_.device_id);
+void HidConnectFunction::OnConnectComplete(
+    scoped_refptr<HidConnection> connection) {
   if (!connection.get()) {
     CompleteWithError(kErrorFailedToOpenDevice);
     return;

extensions/browser/api/hid/hid_api.h

@@ -78,10 +78,9 @@ class HidConnectFunction : public HidAsyncApiFunction {
  private:
   virtual ~HidConnectFunction();
 
-  void OnRequestAccessComplete(bool success);
+  void OnConnectComplete(scoped_refptr<device::HidConnection> connection);
 
   scoped_ptr<core_api::hid::Connect::Params> parameters_;
-  device::HidDeviceInfo device_info_;
 
   DISALLOW_COPY_AND_ASSIGN(HidConnectFunction);
 };