Move X11 device processing to worker threads

Move X11 device processing to worker threads

This moves blocking operations from the UI thread to worker threads.

BUG=428464
Committed: https://crrev.com/3a2200e8fcdc83989c6ee200fbdce5958ccd9011
Cr-Commit-Position: refs/heads/master@{#303891}

[dnicoara, 2014-11-05 19:28:43]

dnicoara@chromium.org changed reviewers:
+ sadrul@chromium.org

[dnicoara, 2014-11-05 19:28:44]

PTAL

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

https://codereview.chromium.org/688163004/diff/1/ui/events/devices/device_uti...
File ui/events/devices/device_util_linux.cc (right):

https://codereview.chromium.org/688163004/diff/1/ui/events/devices/device_uti...
ui/events/devices/device_util_linux.cc:19: std::string event_node =
path.BaseName().value();
Can you add CHECK_NE(TYPE_UI, MessageLoop::current()->type()) here?

https://codereview.chromium.org/688163004/diff/1/ui/events/platform/x11/x11_h...
File ui/events/platform/x11/x11_hotplug_event_handler.cc (right):

https://codereview.chromium.org/688163004/diff/1/ui/events/platform/x11/x11_h...
ui/events/platform/x11/x11_hotplug_event_handler.cc:62: XIDeviceInfo device;
Instead of using X11 structs in different threads, can we extract the relevant
information in this struct? I feel like that would be safer.

https://codereview.chromium.org/688163004/diff/1/ui/events/platform/x11/x11_h...
ui/events/platform/x11/x11_hotplug_event_handler.cc:201: XIAnyClassInfo*
class_info = x11_device.classes[j];
Is it safe to access the XIDeviceInfo::classes here in a non-UI thread?

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

I feel a bit dirty for having to create wrapper structs. I was looking if there
was a way to copy the X11 structs but couldn't find anything.

https://codereview.chromium.org/688163004/diff/1/ui/events/devices/device_uti...
File ui/events/devices/device_util_linux.cc (right):

https://codereview.chromium.org/688163004/diff/1/ui/events/devices/device_uti...
ui/events/devices/device_util_linux.cc:19: std::string event_node =
path.BaseName().value();
On 2014/11/06 21:05:27, sadrul wrote:
> Can you add CHECK_NE(TYPE_UI, MessageLoop::current()->type()) here?

Worker threads don't have a message loop. Is there anything else that you know
of, that I could try?

As a side note, I'm wondering why the ThreadRestrictions assertions weren't
triggered?

https://codereview.chromium.org/688163004/diff/1/ui/events/platform/x11/x11_h...
File ui/events/platform/x11/x11_hotplug_event_handler.cc (right):

https://codereview.chromium.org/688163004/diff/1/ui/events/platform/x11/x11_h...
ui/events/platform/x11/x11_hotplug_event_handler.cc:62: XIDeviceInfo device;
On 2014/11/06 21:05:28, sadrul wrote:
> Instead of using X11 structs in different threads, can we extract the relevant
> information in this struct? I feel like that would be safer.

Done.

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

https://codereview.chromium.org/688163004/diff/1/ui/events/platform/x11/x11_h...
ui/events/platform/x11/x11_hotplug_event_handler.cc:201: XIAnyClassInfo*
class_info = x11_device.classes[j];
On 2014/11/06 21:05:27, sadrul wrote:
> Is it safe to access the XIDeviceInfo::classes here in a non-UI thread?

No :( ... followed your advice.

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

https://codereview.chromium.org/688163004/diff/1/ui/events/devices/device_uti...
File ui/events/devices/device_util_linux.cc (right):

https://codereview.chromium.org/688163004/diff/1/ui/events/devices/device_uti...
ui/events/devices/device_util_linux.cc:19: std::string event_node =
path.BaseName().value();
On 2014/11/10 19:34:51, dnicoara wrote:
> On 2014/11/06 21:05:27, sadrul wrote:
> > Can you add CHECK_NE(TYPE_UI, MessageLoop::current()->type()) here?
> 
> Worker threads don't have a message loop. Is there anything else that you know
> of, that I could try?

Perhaps CHECK(!MessageLoopForUI::IsCurrent())?

> 
> As a side note, I'm wondering why the ThreadRestrictions assertions weren't
> triggered?

I was wondering that too ... I have no idea. Perhaps because we happen to do
this before we set the restrictions?

https://codereview.chromium.org/688163004/diff/20001/ui/events/platform/x11/x...
File ui/events/platform/x11/x11_hotplug_event_handler.cc (right):

https://codereview.chromium.org/688163004/diff/20001/ui/events/platform/x11/x...
ui/events/platform/x11/x11_hotplug_event_handler.cc:58: struct ValuatorClassInfo
{
Add a comment explaining that this mirrors XIValuatorClassInfo and is needed
because worker thread

https://codereview.chromium.org/688163004/diff/20001/ui/events/platform/x11/x...
ui/events/platform/x11/x11_hotplug_event_handler.cc:214: for (auto device_info :
device_infos) {
I think you want 'const auto&'?

https://codereview.chromium.org/688163004/diff/20001/ui/events/platform/x11/x...
ui/events/platform/x11/x11_hotplug_event_handler.cc:248: for (DeviceInfo
device_info : device_infos) {
const &

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

https://codereview.chromium.org/688163004/diff/20001/ui/events/platform/x11/x...
ui/events/platform/x11/x11_hotplug_event_handler.cc:298: void
HandleHotplugEvent(const std::vector<DeviceInfo>& devices,
Do you mind adding 'InWorker' in the function name here (and above) to make this
more clear?

https://codereview.chromium.org/688163004/diff/20001/ui/events/platform/x11/x...
ui/events/platform/x11/x11_hotplug_event_handler.cc:342: XInternAtom(display,
"Abs MT Position Y", False);
Consider using ui::X11AtomCache

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

https://codereview.chromium.org/688163004/diff/20001/ui/events/platform/x11/x...
File ui/events/platform/x11/x11_hotplug_event_handler.cc (right):

https://codereview.chromium.org/688163004/diff/20001/ui/events/platform/x11/x...
ui/events/platform/x11/x11_hotplug_event_handler.cc:58: struct ValuatorClassInfo
{
On 2014/11/11 18:29:51, sadrul wrote:
> Add a comment explaining that this mirrors XIValuatorClassInfo and is needed
> because worker thread

Done.

https://codereview.chromium.org/688163004/diff/20001/ui/events/platform/x11/x...
ui/events/platform/x11/x11_hotplug_event_handler.cc:214: for (auto device_info :
device_infos) {
On 2014/11/11 18:29:51, sadrul wrote:
> I think you want 'const auto&'?

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

https://codereview.chromium.org/688163004/diff/20001/ui/events/platform/x11/x...
ui/events/platform/x11/x11_hotplug_event_handler.cc:248: for (DeviceInfo
device_info : device_infos) {
On 2014/11/11 18:29:51, sadrul wrote:
> const &
> 
> Use auto in both here and above, or in neither place.

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'.

https://codereview.chromium.org/688163004/diff/20001/ui/events/platform/x11/x...
ui/events/platform/x11/x11_hotplug_event_handler.cc:298: void
HandleHotplugEvent(const std::vector<DeviceInfo>& devices,
On 2014/11/11 18:29:51, sadrul wrote:
> Do you mind adding 'InWorker' in the function name here (and above) to make
this
> more clear?

Done.

https://codereview.chromium.org/688163004/diff/20001/ui/events/platform/x11/x...
ui/events/platform/x11/x11_hotplug_event_handler.cc:342: XInternAtom(display,
"Abs MT Position Y", False);
On 2014/11/11 18:29:51, sadrul wrote:
> Consider using ui::X11AtomCache

Done.

Thanks for pointing that out.

[sadrul, 2014-11-11 22:57:30]

LGTM

[dnicoara, 2014-11-12 18:40:43]

The CQ bit was checked by dnicoara@chromium.org

[commit-bot: I haz the power, 2014-11-12 18:42:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/688163004/60001

[commit-bot: I haz the power, 2014-11-12 19:07:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-11-12 19:07:34]

Try jobs failed on following builders:
  linux_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu_trigger...)
  linux_chromium_rel_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sadrul, 2014-11-12 19:29:43]

https://codereview.chromium.org/688163004/diff/60001/ui/events/platform/x11/x...
File ui/events/platform/x11/x11_hotplug_event_handler.cc (right):

https://codereview.chromium.org/688163004/diff/60001/ui/events/platform/x11/x...
ui/events/platform/x11/x11_hotplug_event_handler.cc:47: "Abs MT Position Y",
Ooops. Looks like this needs a NULL at the end.

[dnicoara, 2014-11-12 19:46:11]

https://codereview.chromium.org/688163004/diff/60001/ui/events/platform/x11/x...
File ui/events/platform/x11/x11_hotplug_event_handler.cc (right):

https://codereview.chromium.org/688163004/diff/60001/ui/events/platform/x11/x...
ui/events/platform/x11/x11_hotplug_event_handler.cc:47: "Abs MT Position Y",
On 2014/11/12 19:29:43, sadrul wrote:
> Ooops. Looks like this needs a NULL at the end.

Done.

Ooops, good catch!

[dnicoara, 2014-11-12 20:06:45]

The CQ bit was checked by dnicoara@chromium.org

[commit-bot: I haz the power, 2014-11-12 20:08:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/688163004/100001

[commit-bot: I haz the power, 2014-11-12 20:52:42]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2014-11-12 20:53:28]

Patchset 6 (id:??) landed as
https://crrev.com/3a2200e8fcdc83989c6ee200fbdce5958ccd9011
Cr-Commit-Position: refs/heads/master@{#303891}