Move X11 device processing to worker threads

ui/events/platform/x11/x11_hotplug_event_handler.cc

Index: ui/events/platform/x11/x11_hotplug_event_handler.cc
diff --git a/ui/events/platform/x11/x11_hotplug_event_handler.cc b/ui/events/platform/x11/x11_hotplug_event_handler.cc
index 3ce944efc3ebf3ea54e8701ea5d26a5bd7120c24..f5369f3f6f6903d201fd1d26048d61571ef8c121 100644
--- a/ui/events/platform/x11/x11_hotplug_event_handler.cc
+++ b/ui/events/platform/x11/x11_hotplug_event_handler.cc
@@ -4,6 +4,7 @@
 
 #include "ui/events/platform/x11/x11_hotplug_event_handler.h"
 
+#include <X11/Xatom.h>
 #include <X11/extensions/XInput.h>
 #include <X11/extensions/XInput2.h>
 
@@ -13,11 +14,17 @@
 #include <string>
 #include <vector>
 
+#include "base/bind.h"
 #include "base/command_line.h"
+#include "base/location.h"
 #include "base/logging.h"
 #include "base/process/launch.h"
+#include "base/single_thread_task_runner.h"
 #include "base/strings/string_util.h"
 #include "base/sys_info.h"
+#include "base/thread_task_runner_handle.h"
+#include "base/threading/worker_pool.h"
+#include "ui/events/devices/device_data_manager.h"
 #include "ui/events/devices/device_hotplug_event_observer.h"
 #include "ui/events/devices/device_util_linux.h"
 #include "ui/events/devices/input_device.h"
@@ -35,6 +42,36 @@ const char kATKeyboardName[] = "AT Translated Set 2 keyboard";
 // The prefix of xinput devices corresponding to CrOS EC internal keyboards.
 const char kCrosEcKeyboardPrefix[] = "cros-ec";
 
+typedef base::Callback<void(const std::vector<KeyboardDevice>&)>
+    KeyboardDeviceCallback;
+
+typedef base::Callback<void(const std::vector<TouchscreenDevice>&)>
+    TouchscreenDeviceCallback;
+
+// Used for updating the state on the UI thread once device information is
+// parsed on helper threads.
+struct UiCallbacks {
+  KeyboardDeviceCallback keyboard_callback;
+  TouchscreenDeviceCallback touchscreen_callback;
+};
+
+struct DeviceInfo {
+  DeviceInfo(const XIDeviceInfo& device, const base::FilePath& path)
+      : device(device), path(path) {}
+
+  XIDeviceInfo device;

[sadrul, 2014/11/06 21:05:28]

Instead of using X11 structs in different threads, can we extract the relevant
information in this struct? I feel like that would be safer.

[dnicoara, 2014/11/10 19:34:52]

Done.

Arr, you are right, I should have checked the contents of the X11 structs.

+
+  // Path to the actual device (ie: /dev/input/eventXX)
+  base::FilePath path;
+};
+
+// X11 display cache used on worker threads. This is filled on the UI thread and
+// passed in to the worker threads.
+struct DisplayState {
+  Atom mt_position_x;
+  Atom mt_position_y;
+};
+
 // Returns true if |name| is the name of a known keyboard device. Note, this may
 // return false negatives.
 bool IsKnownKeyboard(const std::string& name) {
@@ -59,23 +96,19 @@ bool IsTestKeyboard(const std::string& name) {
   return name.find("XTEST") != std::string::npos;
 }
 
-// We consider the touchscreen to be internal if it is an I2c device.
-// With the device id, we can query X to get the device's dev input
-// node eventXXX. Then we search all the dev input nodes registered
-// by I2C devices to see if we can find eventXXX.
-bool IsTouchscreenInternal(XDisplay* dpy, int device_id) {
-#if !defined(CHROMEOS)
-  return false;
+base::FilePath GetDevicePath(XDisplay* dpy, int device_id) {
+#if !defined(OS_CHROMEOS)
+  return base::FilePath();
 #else
   if (!base::SysInfo::IsRunningOnChromeOS())
-    return false;
+    return base::FilePath();
 #endif
 
   // Input device has a property "Device Node" pointing to its dev input node,
   // e.g.   Device Node (250): "/dev/input/event8"
   Atom device_node = XInternAtom(dpy, "Device Node", False);
   if (device_node == None)
-    return false;
+    return base::FilePath();
 
   Atom actual_type;
   int actual_format;
@@ -83,7 +116,7 @@ bool IsTouchscreenInternal(XDisplay* dpy, int device_id) {
   unsigned char* data;
   XDevice* dev = XOpenDevice(dpy, device_id);
   if (!dev)
-    return false;
+    return base::FilePath();
 
   if (XGetDeviceProperty(dpy,
                          dev,
@@ -98,40 +131,33 @@ bool IsTouchscreenInternal(XDisplay* dpy, int device_id) {
                          &bytes_after,
                          &data) != Success) {
     XCloseDevice(dpy, dev);
-    return false;
+    return base::FilePath();
   }
-  base::FilePath dev_node_path(reinterpret_cast<char*>(data));
-  XFree(data);
-  XCloseDevice(dpy, dev);
-
-  return ui::IsTouchscreenInternal(dev_node_path);
-}
 
-}  // namespace
+  std::string path;
+  // Make sure the returned value is a string.
+  if (actual_type == XA_STRING && actual_format == 8)
+    path = reinterpret_cast<char*>(data);
 
-X11HotplugEventHandler::X11HotplugEventHandler(
-    DeviceHotplugEventObserver* delegate)
-    : delegate_(delegate) {
-}
-
-X11HotplugEventHandler::~X11HotplugEventHandler() {
-}
+  XFree(data);
+  XCloseDevice(dpy, dev);
 
-void X11HotplugEventHandler::OnHotplugEvent() {
-  const XIDeviceList& device_list =
-      DeviceListCacheX11::GetInstance()->GetXI2DeviceList(gfx::GetXDisplay());
-  HandleTouchscreenDevices(device_list);
-  HandleKeyboardDevices(device_list);
+  return base::FilePath(path);
 }
 
-void X11HotplugEventHandler::HandleKeyboardDevices(
-    const XIDeviceList& x11_devices) {
+// Helper used to parse keyboard information. When it is done it uses
+// |reply_runner| and |callback| to update the state on the UI thread.
+void HandleKeyboardDevices(const std::vector<DeviceInfo>& device_infos,
+                           scoped_refptr<base::TaskRunner> reply_runner,
+                           const KeyboardDeviceCallback& callback) {
   std::vector<KeyboardDevice> devices;
 
-  for (int i = 0; i < x11_devices.count; i++) {
-    if (!x11_devices[i].enabled || x11_devices[i].use != XISlaveKeyboard)
+  for (auto device_info : device_infos) {
+    XIDeviceInfo x11_device = device_info.device;
+
+    if (!x11_device.enabled || x11_device.use != XISlaveKeyboard)
       continue;  // Assume all keyboards are keyboard slaves
-    std::string device_name(x11_devices[i].name);
+    std::string device_name(x11_device.name);
     base::TrimWhitespaceASCII(device_name, base::TRIM_TRAILING, &device_name);
     if (IsTestKeyboard(device_name))
       continue;  // Skip test devices.
@@ -144,43 +170,47 @@ void X11HotplugEventHandler::HandleKeyboardDevices(
       type = InputDeviceType::INPUT_DEVICE_UNKNOWN;
     }
     devices.push_back(
-        KeyboardDevice(x11_devices[i].deviceid, type, device_name));
+        KeyboardDevice(x11_device.deviceid, type, device_name));
   }
-  delegate_->OnKeyboardDevicesUpdated(devices);
+
+  reply_runner->PostTask(FROM_HERE, base::Bind(callback, devices));
 }
 
-void X11HotplugEventHandler::HandleTouchscreenDevices(
-    const XIDeviceList& x11_devices) {
+// Helper used to parse touchscreen information. When it is done it uses
+// |reply_runner| and |callback| to update the state on the UI thread.
+void HandleTouchscreenDevices(const std::vector<DeviceInfo>& device_infos,
+                              const DisplayState& display_state,
+                              scoped_refptr<base::TaskRunner> reply_runner,
+                              const TouchscreenDeviceCallback& callback) {
   std::vector<TouchscreenDevice> devices;
-  Display* display = gfx::GetXDisplay();
-  Atom valuator_x = XInternAtom(display, "Abs MT Position X", False);
-  Atom valuator_y = XInternAtom(display, "Abs MT Position Y", False);
-  if (valuator_x == None || valuator_y == None)
+  if (display_state.mt_position_x == None ||
+      display_state.mt_position_y == None)
     return;
 
   std::set<int> no_match_touchscreen;
-  for (int i = 0; i < x11_devices.count; i++) {
-    if (!x11_devices[i].enabled || x11_devices[i].use != XIFloatingSlave)
+  for (auto device_info : device_infos) {
+    XIDeviceInfo x11_device = device_info.device;
+    if (!x11_device.enabled || x11_device.use != XIFloatingSlave)
       continue;  // Assume all touchscreens are floating slaves
 
     double max_x = -1.0;
     double max_y = -1.0;
     bool is_direct_touch = false;
 
-    for (int j = 0; j < x11_devices[i].num_classes; j++) {
-      XIAnyClassInfo* class_info = x11_devices[i].classes[j];
+    for (int j = 0; j < x11_device.num_classes; j++) {
+      XIAnyClassInfo* class_info = x11_device.classes[j];

[sadrul, 2014/11/06 21:05:27]

Is it safe to access the XIDeviceInfo::classes here in a non-UI thread?

[dnicoara, 2014/11/10 19:34:52]

No :( ... followed your advice.

 
       if (class_info->type == XIValuatorClass) {
         XIValuatorClassInfo* valuator_info =
             reinterpret_cast<XIValuatorClassInfo*>(class_info);
 
-        if (valuator_x == valuator_info->label) {
+        if (display_state.mt_position_x == valuator_info->label) {
           // Ignore X axis valuator with unexpected properties
           if (valuator_info->number == 0 && valuator_info->mode == Absolute &&
               valuator_info->min == 0.0) {
             max_x = valuator_info->max;
           }
-        } else if (valuator_y == valuator_info->label) {
+        } else if (display_state.mt_position_y == valuator_info->label) {
           // Ignore Y axis valuator with unexpected properties
           if (valuator_info->number == 1 && valuator_info->mode == Absolute &&
               valuator_info->min == 0.0) {
@@ -200,22 +230,84 @@ void X11HotplugEventHandler::HandleTouchscreenDevices(
     // Touchscreens should have absolute X and Y axes, and be direct touch
     // devices.
     if (max_x > 0.0 && max_y > 0.0 && is_direct_touch) {
-      InputDeviceType type =
-          IsTouchscreenInternal(display, x11_devices[i].deviceid)
+      InputDeviceType type = IsTouchscreenInternal(device_info.path)
               ? InputDeviceType::INPUT_DEVICE_INTERNAL
               : InputDeviceType::INPUT_DEVICE_EXTERNAL;
-      std::string name(x11_devices[i].name);
+      std::string name(x11_device.name);
       // |max_x| and |max_y| are inclusive values, so we need to add 1 to get
       // the size.
       devices.push_back(TouchscreenDevice(
-          x11_devices[i].deviceid,
+          x11_device.deviceid,
           type,
           name,
           gfx::Size(max_x + 1, max_y + 1)));
     }
   }
 
-  delegate_->OnTouchscreenDevicesUpdated(devices);
+  reply_runner->PostTask(FROM_HERE, base::Bind(callback, devices));
+}
+
+// Called on a worker thread to parse the device information.
+void HandleHotplugEvent(const std::vector<DeviceInfo>& devices,
+                        const DisplayState& display_state,
+                        scoped_refptr<base::TaskRunner> reply_runner,
+                        const UiCallbacks& callbacks) {
+  HandleTouchscreenDevices(
+      devices, display_state, reply_runner, callbacks.touchscreen_callback);
+  HandleKeyboardDevices(devices, reply_runner, callbacks.keyboard_callback);
+}
+
+DeviceHotplugEventObserver* GetHotplugEventObserver() {
+  return DeviceDataManager::GetInstance();
+}
+
+void OnKeyboardDevices(const std::vector<KeyboardDevice>& devices) {
+  GetHotplugEventObserver()->OnKeyboardDevicesUpdated(devices);
+}
+
+void OnTouchscreenDevices(const std::vector<TouchscreenDevice>& devices) {
+  GetHotplugEventObserver()->OnTouchscreenDevicesUpdated(devices);
+}
+
+}  // namespace
+
+X11HotplugEventHandler::X11HotplugEventHandler() {
+}
+
+X11HotplugEventHandler::~X11HotplugEventHandler() {
+}
+
+void X11HotplugEventHandler::OnHotplugEvent() {
+  const XIDeviceList& device_list =
+      DeviceListCacheX11::GetInstance()->GetXI2DeviceList(gfx::GetXDisplay());
+  Display* display = gfx::GetXDisplay();
+
+  std::vector<DeviceInfo> device_infos;
+  for (int i = 0; i < device_list.count; ++i)
+    device_infos.push_back(DeviceInfo(
+        device_list[i], GetDevicePath(display, device_list[i].deviceid)));
+
+  // X11 is not thread safe, so first get all the required state.
+  DisplayState display_state;
+  display_state.mt_position_x =
+      XInternAtom(display, "Abs MT Position X", False);
+  display_state.mt_position_y =
+      XInternAtom(display, "Abs MT Position Y", False);
+
+  UiCallbacks callbacks;
+  callbacks.keyboard_callback = base::Bind(&OnKeyboardDevices);
+  callbacks.touchscreen_callback = base::Bind(&OnTouchscreenDevices);
+
+  // Parsing the device information may block, so delegate the operation to a
+  // worker thread. Once the device information is extracted the parsed devices
+  // will be returned via the callbacks.
+  base::WorkerPool::PostTask(FROM_HERE,
+                             base::Bind(&HandleHotplugEvent,
+                                        device_infos,
+                                        display_state,
+                                        base::ThreadTaskRunnerHandle::Get(),
+                                        callbacks),
+                             true /* task_is_slow */);
 }
 
 }  // namespace ui