Automatically remap Command key on an Apple keyboard to Control [part 2 of 2 - Chrome part]

Automatically remap Command key on an Apple keyboard to Control [part 2 of 2 - Chrome part]

chrome/browser/chromeos/xinput_hierarchy_changed_event_listener.cc:

* Every time when a new device is added, notify the name of the device to KeyRewriter.
* Listens to XI_KeyPress and XI_KeyRelease events to the X root window, and notify the device ID of the event to KeyRewriter so that the rewriter could know the source of the next KeyPress/KeyRelease Core event.

Note that it's not possible to monitor both XI_KeyPress and core KeyPress events for a single X window. For example, if we monitor XI_KeyPress events in aura::RootWindowHostLinux (by calling XISelectEvents() for the root Aura window, xwindow_), it becomes impossible to receive KeyPress core events for |xwindow_| in RootWindowHostLinux. It's also impossible to convert XI_KeyPress into core KeyPress. That is the reason why xinput_hierarchy_changed_event_listener.cc is used to monitor XI_KeyPress/Release events.

chrome/browser/ui/views/ash/key_rewriter.cc:

* Rewrites Command key press on an Apple keyboard to Control key press, regardless of the user preference for remapping modifier keys.


Part 1 of 2 (Ash part): http://codereview.chromium.org/9838010/

BUG=chromium-os:25097
TEST=try

[Yusuke Sato, 2012-03-27 08:32:14]

Daniel, could you review this as well?

[Daniel Erat, 2012-03-27 16:19:35]

http://codereview.chromium.org/9854025/diff/7011/chrome/browser/chromeos/devi...
File chrome/browser/chromeos/device_hierarchy_observer.h (right):

http://codereview.chromium.org/9854025/diff/7011/chrome/browser/chromeos/devi...
chrome/browser/chromeos/device_hierarchy_observer.h:17: virtual void
DeviceAdded(int device_id) = 0;
do you also need a DeviceRemoved() method?

http://codereview.chromium.org/9854025/diff/7011/chrome/browser/chromeos/devi...
chrome/browser/chromeos/device_hierarchy_observer.h:22: virtual void
KeyPressedOrReleased(int device_id) = 0;
nit: rename to DeviceKeyPressedOrReleased() so there's a clue in the name as to
where this is coming from

http://codereview.chromium.org/9854025/diff/7011/chrome/browser/ui/views/ash/...
File chrome/browser/ui/views/ash/key_rewriter.cc (right):

http://codereview.chromium.org/9854025/diff/7011/chrome/browser/ui/views/ash/...
chrome/browser/ui/views/ash/key_rewriter.cc:20: #include
<X11/extensions/XInput2.h>
nit: these are supposed to come before the c++ headers, i think

http://codereview.chromium.org/9854025/diff/7011/chrome/browser/ui/views/ash/...
chrome/browser/ui/views/ash/key_rewriter.cc:33: const unsigned int
kKeyCodeSuperL = 133;  // Left Command or Windows key.
XKeysymToKeycode().  here's an example:
git.chromium.org/gitweb/?p=chromiumos/platform/window_manager.git;a=blob;f=x11/real_x_connection.cc;h=47a4b933e06731824779dd4e59600e2b22abc6ad;hb=HEAD#l1218

note that you should also refresh these when MappingNotify events are received:
tronche.com/gui/x/xlib/events/window-state-change/mapping.html

http://codereview.chromium.org/9854025/diff/7011/chrome/browser/ui/views/ash/...
chrome/browser/ui/views/ash/key_rewriter.cc:83: return kDevicePcKeyboard;
might be better to just use kDeviceUnknown here -- this could be something else
instead of a keyboard, right?

http://codereview.chromium.org/9854025/diff/7011/chrome/browser/ui/views/ash/...
chrome/browser/ui/views/ash/key_rewriter.cc:172: if (xkey->keycode ==
kKeyCodeSuperL)
instead of looking up the keycodes earlier, i think that you could call
XLookupKeysym() here

http://codereview.chromium.org/9854025/diff/7011/chrome/browser/ui/views/ash/...
chrome/browser/ui/views/ash/key_rewriter.cc:173: xkey->keycode =
kKeyCodeControlL;
this is a bit different than what i expected when i reviewed the previous
change.  should you also be updating the aura::KeyEvent fields?

http://codereview.chromium.org/9854025/diff/7011/chrome/browser/ui/views/ash/...
chrome/browser/ui/views/ash/key_rewriter.cc:197: LOG(WARNING) << "An Apple
keyboard '" << device_name << "' is connected: "
nit: get rid of this warning or switch it to a LOG(INFO) or VLOG

http://codereview.chromium.org/9854025/diff/7011/chrome/browser/ui/views/ash/...
File chrome/browser/ui/views/ash/key_rewriter_unittest.cc (right):

http://codereview.chromium.org/9854025/diff/7011/chrome/browser/ui/views/ash/...
chrome/browser/ui/views/ash/key_rewriter_unittest.cc:13: #include
<X11/keysymdef.h>
nit: move these above the c++ includes

[Daniel Erat, 2012-03-31 01:24:08]

I uploaded a copy of this change and applied my own feedback at
http://codereview.chromium.org/9963027/ .

http://codereview.chromium.org/9854025/diff/7011/chrome/browser/chromeos/devi...
File chrome/browser/chromeos/device_hierarchy_observer.h (right):

http://codereview.chromium.org/9854025/diff/7011/chrome/browser/chromeos/devi...
chrome/browser/chromeos/device_hierarchy_observer.h:17: virtual void
DeviceAdded(int device_id) = 0;
On 2012/03/27 16:19:35, Daniel Erat wrote:
> do you also need a DeviceRemoved() method?

Done.

http://codereview.chromium.org/9854025/diff/7011/chrome/browser/chromeos/devi...
chrome/browser/chromeos/device_hierarchy_observer.h:22: virtual void
KeyPressedOrReleased(int device_id) = 0;
On 2012/03/27 16:19:35, Daniel Erat wrote:
> nit: rename to DeviceKeyPressedOrReleased() so there's a clue in the name as
to
> where this is coming from

Done.

http://codereview.chromium.org/9854025/diff/7011/chrome/browser/ui/views/ash/...
File chrome/browser/ui/views/ash/key_rewriter.cc (right):

http://codereview.chromium.org/9854025/diff/7011/chrome/browser/ui/views/ash/...
chrome/browser/ui/views/ash/key_rewriter.cc:20: #include
<X11/extensions/XInput2.h>
On 2012/03/27 16:19:35, Daniel Erat wrote:
> nit: these are supposed to come before the c++ headers, i think

Done.

http://codereview.chromium.org/9854025/diff/7011/chrome/browser/ui/views/ash/...
chrome/browser/ui/views/ash/key_rewriter.cc:33: const unsigned int
kKeyCodeSuperL = 133;  // Left Command or Windows key.
On 2012/03/27 16:19:35, Daniel Erat wrote:
> XKeysymToKeycode().  here's an example:
>
git.chromium.org/gitweb/?p=chromiumos/platform/window_manager.git;a=blob;f=x11/real_x_connection.cc;h=47a4b933e06731824779dd4e59600e2b22abc6ad;hb=HEAD#l1218
> 
> note that you should also refresh these when MappingNotify events are
received:
> tronche.com/gui/x/xlib/events/window-state-change/mapping.html

Done (via http://codereview.chromium.org/9958036/).

http://codereview.chromium.org/9854025/diff/7011/chrome/browser/ui/views/ash/...
chrome/browser/ui/views/ash/key_rewriter.cc:83: return kDevicePcKeyboard;
On 2012/03/27 16:19:35, Daniel Erat wrote:
> might be better to just use kDeviceUnknown here -- this could be something
else
> instead of a keyboard, right?

Done.

http://codereview.chromium.org/9854025/diff/7011/chrome/browser/ui/views/ash/...
chrome/browser/ui/views/ash/key_rewriter.cc:172: if (xkey->keycode ==
kKeyCodeSuperL)
On 2012/03/27 16:19:35, Daniel Erat wrote:
> instead of looking up the keycodes earlier, i think that you could call
> XLookupKeysym() here

Done.

http://codereview.chromium.org/9854025/diff/7011/chrome/browser/ui/views/ash/...
chrome/browser/ui/views/ash/key_rewriter.cc:173: xkey->keycode =
kKeyCodeControlL;
On 2012/03/27 16:19:35, Daniel Erat wrote:
> this is a bit different than what i expected when i reviewed the previous
> change.  should you also be updating the aura::KeyEvent fields?

Done.

http://codereview.chromium.org/9854025/diff/7011/chrome/browser/ui/views/ash/...
chrome/browser/ui/views/ash/key_rewriter.cc:197: LOG(WARNING) << "An Apple
keyboard '" << device_name << "' is connected: "
On 2012/03/27 16:19:35, Daniel Erat wrote:
> nit: get rid of this warning or switch it to a LOG(INFO) or VLOG

Done.

http://codereview.chromium.org/9854025/diff/7011/chrome/browser/ui/views/ash/...
File chrome/browser/ui/views/ash/key_rewriter_unittest.cc (right):

http://codereview.chromium.org/9854025/diff/7011/chrome/browser/ui/views/ash/...
chrome/browser/ui/views/ash/key_rewriter_unittest.cc:13: #include
<X11/keysymdef.h>
On 2012/03/27 16:19:35, Daniel Erat wrote:
> nit: move these above the c++ includes

Done.