Listen to XI_HierarchyChanged events and call setxkbmap when needed.

Listen to XI_HierarchyChanged events and call setxkbmap when needed.

BUG=chromium-os:15851
BUG=chromium-os:15516
BUG=84694
TEST=manually done: Log in, change the layout from US Qwerty (US) to US Extended (EXTD), focus Omnibox, press AltGr+a, confirm á is shown,  plug a USB keyboard, press AltGr+a, confirm the character again, press ctrl+alt+f2 to show VT2, press ctrl+alt+f1 to go back to X, press AltGr+a, confirm the char again, close lid, open lid, press AltGr+a, confirm the char again.
TEST=On login screen, change the keyboard layout to EXTD, add "sleep 30" to /etc/init/udev-*.conf files, reboot, on login screen, wait for ~30 seconds, press AltGr+a, confirm á is shown.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=87585

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=87756

[sadrul, 2011-06-01 14:55:19]

http://codereview.chromium.org/6975057/diff/1/chrome/browser/chromeos/xinput_...
File chrome/browser/chromeos/xinput_hierarchy_changed_event_listener.cc (right):

http://codereview.chromium.org/6975057/diff/1/chrome/browser/chromeos/xinput_...
chrome/browser/chromeos/xinput_hierarchy_changed_event_listener.cc:34: unsigned
char mask[(XI_LASTEVENT + 7) / 8] = {0};
mask[XIMaskLen(XI_LASTEVENT)]

http://codereview.chromium.org/6975057/diff/1/chrome/browser/chromeos/xinput_...
File chrome/browser/chromeos/xinput_hierarchy_changed_event_listener.h (right):

http://codereview.chromium.org/6975057/diff/1/chrome/browser/chromeos/xinput_...
chrome/browser/chromeos/xinput_hierarchy_changed_event_listener.h:9: #if
defined(HAVE_XINPUT2)
As you already know, HAVE_XINPUT2 is going away. Besides, it's better to exclude
the file in .gypi file rather than wrapping all of the file inside #if define's.

http://codereview.chromium.org/6975057/diff/1/chrome/browser/chromeos/xinput_...
chrome/browser/chromeos/xinput_hierarchy_changed_event_listener.h:22: class
XInputHierarchyChangedEventListener : public base::MessagePumpXObserver {
Please s/MessagePumpXObserver/MessageLoopForUI::Observer/

[Yusuke Sato, 2011-06-01 22:31:06]

Fixed. Please take another look.

http://codereview.chromium.org/6975057/diff/1/chrome/browser/chromeos/xinput_...
File chrome/browser/chromeos/xinput_hierarchy_changed_event_listener.cc (right):

http://codereview.chromium.org/6975057/diff/1/chrome/browser/chromeos/xinput_...
chrome/browser/chromeos/xinput_hierarchy_changed_event_listener.cc:34: unsigned
char mask[(XI_LASTEVENT + 7) / 8] = {0};
On 2011/06/01 14:55:19, sadrul wrote:
> mask[XIMaskLen(XI_LASTEVENT)]

Done.

http://codereview.chromium.org/6975057/diff/1/chrome/browser/chromeos/xinput_...
File chrome/browser/chromeos/xinput_hierarchy_changed_event_listener.h (right):

http://codereview.chromium.org/6975057/diff/1/chrome/browser/chromeos/xinput_...
chrome/browser/chromeos/xinput_hierarchy_changed_event_listener.h:9: #if
defined(HAVE_XINPUT2)
Removed all HAVE_XINPUT2 #if define()s.

On 2011/06/01 14:55:19, sadrul wrote:
> As you already know, HAVE_XINPUT2 is going away. Besides, it's better to
exclude
> the file in .gypi file rather than wrapping all of the file inside #if
define's.

http://codereview.chromium.org/6975057/diff/1/chrome/browser/chromeos/xinput_...
chrome/browser/chromeos/xinput_hierarchy_changed_event_listener.h:22: class
XInputHierarchyChangedEventListener : public base::MessagePumpXObserver {
Thanks! Fixed.

On 2011/06/01 14:55:19, sadrul wrote:
> Please s/MessagePumpXObserver/MessageLoopForUI::Observer/

[Daniel Kurtz, 2011-06-01 23:17:15]

The current implementation LGTM.

However, I recommend an optimization to eliminate lots of setxkbmap at vt switch
& powerd_suspend.  Unfortunately, the optimization has the side-affect that
setxkbmap will occur on discovery of both all slave devices: keyboards,
trackpads, keyboards, etc.

http://codereview.chromium.org/6975057/diff/4001/chrome/browser/chromeos/xinp...
File chrome/browser/chromeos/xinput_hierarchy_changed_event_listener.cc (right):

http://codereview.chromium.org/6975057/diff/4001/chrome/browser/chromeos/xinp...
chrome/browser/chromeos/xinput_hierarchy_changed_event_listener.cc:52: &&
(info->flags & XIDeviceEnabled)) {
This works, but as noted, it causes setxkbmap calls during VT switches, when
every X input device is disabled/enabled.

Instead, we could only apply the keyboard settings when a new slave device is
detected.  Unfortunately, I don't know how to determine if a new device is a
keyboard or a mouse, so this will apply the keyboard for both keyboards and
mice:

if ((event->flags & XISlaveAdded) && (info->use == XIFloatingSlave) &&
(info->flags & XISlaveAdded))


Ideally, we would apply the setting only when a slave keyboard was attached to
the master keyboard, but, unfortunately, we don't seem to get XISlaveAttached
events.

[sadrul, 2011-06-01 23:22:23]

LGTM on the parts I commented on (with a nit).

http://codereview.chromium.org/6975057/diff/4001/chrome/browser/chromeos/xinp...
File chrome/browser/chromeos/xinput_hierarchy_changed_event_listener.h (right):

http://codereview.chromium.org/6975057/diff/4001/chrome/browser/chromeos/xinp...
chrome/browser/chromeos/xinput_hierarchy_changed_event_listener.h:13: #include
"base/message_pump_glib_x_dispatch.h"
base/message_loop.h

[Yusuke Sato, 2011-06-02 01:05:10]

http://codereview.chromium.org/6975057/diff/4001/chrome/browser/chromeos/xinp...
File chrome/browser/chromeos/xinput_hierarchy_changed_event_listener.cc (right):

http://codereview.chromium.org/6975057/diff/4001/chrome/browser/chromeos/xinp...
chrome/browser/chromeos/xinput_hierarchy_changed_event_listener.cc:52: &&
(info->flags & XIDeviceEnabled)) {
On 2011/06/01 23:17:15, Daniel Kurtz wrote:
> This works, but as noted, it causes setxkbmap calls during VT switches, when
> every X input device is disabled/enabled.
> 
> Instead, we could only apply the keyboard settings when a new slave device is
> detected.  Unfortunately, I don't know how to determine if a new device is a
> keyboard or a mouse, so this will apply the keyboard for both keyboards and
> mice:
> 
> if ((event->flags & XISlaveAdded) && (info->use == XIFloatingSlave) &&
> (info->flags & XISlaveAdded))
> 
> 
> Ideally, we would apply the setting only when a slave keyboard was attached to
> the master keyboard, but, unfortunately, we don't seem to get XISlaveAttached
> events.

Done.

http://codereview.chromium.org/6975057/diff/4001/chrome/browser/chromeos/xinp...
File chrome/browser/chromeos/xinput_hierarchy_changed_event_listener.h (right):

http://codereview.chromium.org/6975057/diff/4001/chrome/browser/chromeos/xinp...
chrome/browser/chromeos/xinput_hierarchy_changed_event_listener.h:13: #include
"base/message_pump_glib_x_dispatch.h"
On 2011/06/01 23:22:23, sadrul wrote:
> base/message_loop.h

Done.

[Zachary Kuznia, 2011-06-02 05:25:00]

LGTM

http://codereview.chromium.org/6975057/diff/9001/chrome/browser/chromeos/xinp...
File chrome/browser/chromeos/xinput_hierarchy_changed_event_listener.cc (right):

http://codereview.chromium.org/6975057/diff/9001/chrome/browser/chromeos/xinp...
chrome/browser/chromeos/xinput_hierarchy_changed_event_listener.cc:24:
LOG(ERROR) << "X Input extension not available";
Should this be VLOG?  Also, could you appent the error code to this log?

http://codereview.chromium.org/6975057/diff/9001/chrome/browser/chromeos/xinp...
chrome/browser/chromeos/xinput_hierarchy_changed_event_listener.cc:113:
LOG(ERROR) << "XGetEventData failed";
VLOG?

[Yusuke Sato, 2011-06-02 05:50:19]

http://codereview.chromium.org/6975057/diff/9001/chrome/browser/chromeos/xinp...
File chrome/browser/chromeos/xinput_hierarchy_changed_event_listener.cc (right):

http://codereview.chromium.org/6975057/diff/9001/chrome/browser/chromeos/xinp...
chrome/browser/chromeos/xinput_hierarchy_changed_event_listener.cc:24:
LOG(ERROR) << "X Input extension not available";
On 2011/06/02 05:25:00, Zachary Kuznia wrote:
> Should this be VLOG?  Also, could you appent the error code to this log?

Done.

http://codereview.chromium.org/6975057/diff/9001/chrome/browser/chromeos/xinp...
chrome/browser/chromeos/xinput_hierarchy_changed_event_listener.cc:113:
LOG(ERROR) << "XGetEventData failed";
On 2011/06/02 05:25:00, Zachary Kuznia wrote:
> VLOG?

Done.

[Yusuke Sato, 2011-06-02 09:02:49]

I've reverted the change since it breaks Chrome OS ARM bot.
See http://code.google.com/p/chromium-os/issues/detail?id=15851 for details.
I'll fix the CL tomorrow.

[Yusuke Sato, 2011-06-03 02:29:47]

Added workaround for crbug.com/84694.

Even with the workaround, Chromium OS image for ARM _can_ handle the XInput2
events, since Chromium OS image for ARM is built entirely inside chroot and the
chroot always has XInput2.

The workaround is only for the Chrome OS ARM build bot which compiles Chrome OS
version of Chrome outside of chroot.

Please take another look.

On 2011/06/02 09:02:49, Yusuke Sato wrote:
> I've reverted the change since it breaks Chrome OS ARM bot.
> See http://code.google.com/p/chromium-os/issues/detail?id=15851 for details.
> I'll fix the CL tomorrow.

[Daniel Kurtz, 2011-06-03 05:36:16]

LGTM