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..d14358502c195bc437e60214c20eeddefeee3797 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,95 @@ 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 ValuatorClassInfo {

[sadrul, 2014/11/11 18:29:51]

Add a comment explaining that this mirrors XIValuatorClassInfo and is needed
because worker thread

[dnicoara, 2014/11/11 20:49:05]

Done.

+  ValuatorClassInfo(const XIValuatorClassInfo& info)
+      : label(info.label),
+        max(info.max),
+        min(info.min),
+        mode(info.mode),
+        number(info.number) {}
+
+  Atom label;
+  double max;
+  double min;
+  int mode;
+  int number;
+};
+
+struct TouchClassInfo {
+  TouchClassInfo(const XITouchClassInfo& info)
+      : mode(info.mode) {}
+
+  int mode;
+};
+
+struct DeviceInfo {
+  DeviceInfo(const XIDeviceInfo& device, const base::FilePath& path)
+      : id(device.deviceid),
+        name(device.name),
+        use(device.use),
+        enabled(device.enabled),
+        path(path) {
+    for (int i = 0; i < device.num_classes; ++i) {
+      switch (device.classes[i]->type) {
+        case XIValuatorClass:
+          valuator_class_infos.push_back(ValuatorClassInfo(
+              *reinterpret_cast<XIValuatorClassInfo*>(device.classes[i])));
+          break;
+#if defined(USE_XI2_MT)
+        case XITouchClass:
+          touch_class_infos.push_back(TouchClassInfo(
+              *reinterpret_cast<XITouchClassInfo*>(device.classes[i])));
+          break;
+#endif
+        default:
+          break;
+      }
+    }
+  }
+
+  // Unique device identifier.
+  int id;
+
+  // Internal device name.
+  std::string name;
+
+  // Device type (ie: XIMasterPointer)
+  int use;
+
+  // Specifies if the device is enabled and can send events.
+  bool enabled;
+
+  // Path to the actual device (ie: /dev/input/eventXX)
+  base::FilePath path;
+
+  std::vector<ValuatorClassInfo> valuator_class_infos;
+
+#if defined(USE_XI2_MT)
+  std::vector<TouchClassInfo> touch_class_infos;
+#endif
+};
+
+// 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 +155,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 +175,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 +190,31 @@ 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);
-}
+  std::string path;
+  // Make sure the returned value is a string.
+  if (actual_type == XA_STRING && actual_format == 8)
+    path = reinterpret_cast<char*>(data);
 
-}  // namespace
-
-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) {

[sadrul, 2014/11/11 18:29:51]

I think you want 'const auto&'?

[dnicoara, 2014/11/11 20:49:06]

Done, you're right, I need to learn the details for c++11 features.

+    if (!device_info.enabled || device_info.use != XISlaveKeyboard)
       continue;  // Assume all keyboards are keyboard slaves
-    std::string device_name(x11_devices[i].name);
+    std::string device_name(device_info.name);
     base::TrimWhitespaceASCII(device_name, base::TRIM_TRAILING, &device_name);
     if (IsTestKeyboard(device_name))
       continue;  // Skip test devices.
@@ -144,78 +227,134 @@ void X11HotplugEventHandler::HandleKeyboardDevices(
       type = InputDeviceType::INPUT_DEVICE_UNKNOWN;
     }
     devices.push_back(
-        KeyboardDevice(x11_devices[i].deviceid, type, device_name));
+        KeyboardDevice(device_info.id, 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 (DeviceInfo device_info : device_infos) {

[sadrul, 2014/11/11 18:29:51]

const &

Use auto in both here and above, or in neither place.

[dnicoara, 2014/11/11 20:49:05]

Done.

I've used the actual type for clarity. I don't feel strongly about this, so let
me know if you'd like 'auto'.

+    if (!device_info.enabled || device_info.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];
-
-      if (class_info->type == XIValuatorClass) {
-        XIValuatorClassInfo* valuator_info =
-            reinterpret_cast<XIValuatorClassInfo*>(class_info);
-
-        if (valuator_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) {
-          // Ignore Y axis valuator with unexpected properties
-          if (valuator_info->number == 1 && valuator_info->mode == Absolute &&
-              valuator_info->min == 0.0) {
-            max_y = valuator_info->max;
-          }
+    for (ValuatorClassInfo valuator : device_info.valuator_class_infos) {
+      if (display_state.mt_position_x == valuator.label) {
+        // Ignore X axis valuator with unexpected properties
+        if (valuator.number == 0 && valuator.mode == Absolute &&
+            valuator.min == 0.0) {
+          max_x = valuator.max;
+        }
+      } else if (display_state.mt_position_y == valuator.label) {
+        // Ignore Y axis valuator with unexpected properties
+        if (valuator.number == 1 && valuator.mode == Absolute &&
+            valuator.min == 0.0) {
+          max_y = valuator.max;
         }
       }
+    }
+
 #if defined(USE_XI2_MT)
-      if (class_info->type == XITouchClass) {
-        XITouchClassInfo* touch_info =
-            reinterpret_cast<XITouchClassInfo*>(class_info);
-        is_direct_touch = touch_info->mode == XIDirectTouch;
-      }
-#endif
+    for (TouchClassInfo info : device_info.touch_class_infos) {
+      is_direct_touch = info.mode == XIDirectTouch;
     }
+#endif
 
     // 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);
       // |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,
+          device_info.id,
           type,
-          name,
+          device_info.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,

[sadrul, 2014/11/11 18:29:51]

Do you mind adding 'InWorker' in the function name here (and above) to make this
more clear?

[dnicoara, 2014/11/11 20:49:06]

Done.

+                        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);

[sadrul, 2014/11/11 18:29:51]

Consider using ui::X11AtomCache

[dnicoara, 2014/11/11 20:49:06]

Done.

Thanks for pointing that out.

+
+  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