Move modifier remapping code from X to Ash

Move modifier remapping code from X to Ash.

Doing modifier remapping in Ash would be better for the following reasons:

- We can write full unit tests of the feature without relying on Autotests. The tests are included in this CL.
- We can debug the remapping feature using Ash running on Goobuntu.
- Much easier to maintain. We can remove 4k lines of auto-generated XKB script, /build/$BOARD/usr/share/X11/xkb/symbols/chromeos, which is difficult to read and understand. We no longer need to upgrade the 4k script every time when xorg-server is upgraded.
- Easier to add more mappings. For example, now it's trivial to add a feature like 'remapping Search to ESC'.
- We can also delete another auto-generated file in Chrome, src/chrome/browser/chromeos/input_method/xkeyboard_data.h.

BUG=115112
TEST=manual and unit_tests

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

[Yusuke Sato, 2012-05-23 13:26:29]

Zach, could you review chrome/browser/chromeos/ part?

[Yusuke Sato, 2012-05-23 15:54:06]

+derat
Could you review KeyRewriter part?

[Daniel Erat, 2012-05-23 16:37:32]

http://codereview.chromium.org/10383301/diff/10001/chrome/browser/ui/views/as...
File chrome/browser/ui/views/ash/key_rewriter.cc (right):

http://codereview.chromium.org/10383301/diff/10001/chrome/browser/ui/views/as...
chrome/browser/ui/views/ash/key_rewriter.cc:45: unsigned int native_keysyms[4]; 
// left, right, shift+left, shift+right.
nit: use KeySym instead?

http://codereview.chromium.org/10383301/diff/10001/chrome/browser/ui/views/as...
chrome/browser/ui/views/ash/key_rewriter.cc:62: unsigned int native_flag;
nit: native_modifiers?

http://codereview.chromium.org/10383301/diff/10001/chrome/browser/ui/views/as...
chrome/browser/ui/views/ash/key_rewriter.cc:64: const char* key_name;
nit: pref_name?

http://codereview.chromium.org/10383301/diff/10001/chrome/browser/ui/views/as...
chrome/browser/ui/views/ash/key_rewriter.cc:75: PrefService* pref_service) {
nit: const PrefService&

http://codereview.chromium.org/10383301/diff/10001/chrome/browser/ui/views/as...
chrome/browser/ui/views/ash/key_rewriter.cc:86: bool IsRight(unsigned int
native_keysym) {
nit: KeySym

http://codereview.chromium.org/10383301/diff/10001/chrome/browser/ui/views/as...
chrome/browser/ui/views/ash/key_rewriter.cc:100: PrefService* GetPrefService() {
nit: const PrefService* ?

http://codereview.chromium.org/10383301/diff/10001/chrome/browser/ui/views/as...
chrome/browser/ui/views/ash/key_rewriter.cc:259: unsigned int
KeyRewriter::NativeKeySymToNativeKeycode(int keysym) {
nit: KeySym

http://codereview.chromium.org/10383301/diff/10001/chrome/browser/ui/views/as...
chrome/browser/ui/views/ash/key_rewriter.cc:370: bool rewritten = false;
nit: maybe remove this variable and just return false when pref_service is NULL
or OS_CHROMEOS isn't defined and true otherwise?  or are you planning to have
more complicated logic later where this variable is conditionally set at
different places in the body of this method?  right now, it looks like it's only
true on chrome os.

http://codereview.chromium.org/10383301/diff/10001/chrome/browser/ui/views/as...
chrome/browser/ui/views/ash/key_rewriter.cc:381: unsigned int
remapped_native_keycode = xkey->keycode;
nit: KeyCode

http://codereview.chromium.org/10383301/diff/10001/chrome/browser/ui/views/as...
chrome/browser/ui/views/ash/key_rewriter.cc:400: break;
nit: add a no-op default?  think that some compilers will warn if you don't.

http://codereview.chromium.org/10383301/diff/10001/chrome/browser/ui/views/as...
File chrome/browser/ui/views/ash/key_rewriter.h (right):

http://codereview.chromium.org/10383301/diff/10001/chrome/browser/ui/views/as...
chrome/browser/ui/views/ash/key_rewriter.h:93: unsigned int
NativeKeySymToNativeKeycode(int keysym);
X.h defines KeySym as a XID and KeyCode as an unsigned char.  should the
signature instead be:

  unsigned char NativeKeySymToNativeKeycode(unsigned long keysym);

?

http://codereview.chromium.org/10383301/diff/10001/chrome/browser/ui/views/as...
File chrome/browser/ui/views/ash/key_rewriter_unittest.cc (right):

http://codereview.chromium.org/10383301/diff/10001/chrome/browser/ui/views/as...
chrome/browser/ui/views/ash/key_rewriter_unittest.cc:581: const unsigned int
kKeycodeSuperL = XKeysymToKeycode(display, XK_Super_L);
nit: KeyCode instead of unsigned int for all of these?  (looks like Xlib is
inconsistent here, of course, but i'm pretty sure the protocol defines it as
8-bit)

http://codereview.chromium.org/10383301/diff/10001/chrome/browser/ui/views/as...
chrome/browser/ui/views/ash/key_rewriter_unittest.cc:593:
InitXKeyEvent(ui::VKEY_LWIN, 0, kKeycodeSuperL, 0U, &xev);
there's a bunch of duplicate code in this file.  do you think it'd be possible
to shorten it?  what about something like (untested):

  std::string GetRewrittenEventAsString(
      KeyRewriter* rewriter,
      ui::KeyboardCode ui_keycode,
      int ui_flags,    
      KeyCode x_keycode,
      unsigned int x_state) {
    XEvent xev;
    InitXKeyEvent(ui_keycode, ui_flags, x_keycode, x_state, &xev);
    aura::KeyEvent keyevent(&xev, false /* is_char */);
    rewriter->RewriteForTesting(&keyevent);
    return StringPrintf(
        "ui_keycode=%d ui_flags=%d ui_type=%d " 
        "x_keycode=%d x_state=%u x_type=%d",
        keyevent.key_code(), keyevent.flags(), keyevent.type(),
        xkey.keycode, xkey.state, xkey.type);
  }

  std::string GetExpectedResultAsString(
      ui::KeyboardCode ui_keycode,
      int ui_flags,
      ui::EventType ui_type,
      KeyCode x_keycode,
      unsigned int x_state,
      int x_type) {
    return StringPrintf(
        "ui_keycode=%d ui_flags=%d ui_type=%d "
        "x_keycode=%d x_state=%u x_type=%d",
        ui_keycode, ui_flags, ui_type, x_keycode, x_state, x_type);
  }

  EXPECT_EQ(
      GetExpectedResultAsString(
          ui::VKEY_LWIN, 0, ui::ET_KEY_PRESSED, kKeycodeSuperL, 0U, KeyPress),
      GetRewrittenEventAsString(
          &rewriter, ui::VKEY_LWIN, 0, kKeycodeSuperL, 0U));
  // etc.

Perhaps the kKeycode consts could also be moved into the base class so you don't
need to repeat that code at the beginning of every test, too.

http://codereview.chromium.org/10383301/diff/10001/chrome/browser/ui/views/as...
chrome/browser/ui/views/ash/key_rewriter_unittest.cc:761: const unsigned int
kKeycodeSuperL = XKeysymToKeycode(display, XK_Super_L);
nit: KeyCode (also for other unsigned ints in this file)

[Yusuke Sato, 2012-05-24 08:24:41]

Please take another look.

http://codereview.chromium.org/10383301/diff/10001/chrome/browser/ui/views/as...
File chrome/browser/ui/views/ash/key_rewriter.cc (right):

http://codereview.chromium.org/10383301/diff/10001/chrome/browser/ui/views/as...
chrome/browser/ui/views/ash/key_rewriter.cc:45: unsigned int native_keysyms[4]; 
// left, right, shift+left, shift+right.
On 2012/05/23 16:37:32, Daniel Erat wrote:
> nit: use KeySym instead?

Done.

http://codereview.chromium.org/10383301/diff/10001/chrome/browser/ui/views/as...
chrome/browser/ui/views/ash/key_rewriter.cc:62: unsigned int native_flag;
On 2012/05/23 16:37:32, Daniel Erat wrote:
> nit: native_modifiers?

Done.

http://codereview.chromium.org/10383301/diff/10001/chrome/browser/ui/views/as...
chrome/browser/ui/views/ash/key_rewriter.cc:64: const char* key_name;
On 2012/05/23 16:37:32, Daniel Erat wrote:
> nit: pref_name?

Done.

http://codereview.chromium.org/10383301/diff/10001/chrome/browser/ui/views/as...
chrome/browser/ui/views/ash/key_rewriter.cc:75: PrefService* pref_service) {
On 2012/05/23 16:37:32, Daniel Erat wrote:
> nit: const PrefService&

Done.

http://codereview.chromium.org/10383301/diff/10001/chrome/browser/ui/views/as...
chrome/browser/ui/views/ash/key_rewriter.cc:86: bool IsRight(unsigned int
native_keysym) {
On 2012/05/23 16:37:32, Daniel Erat wrote:
> nit: KeySym

Done.

http://codereview.chromium.org/10383301/diff/10001/chrome/browser/ui/views/as...
chrome/browser/ui/views/ash/key_rewriter.cc:100: PrefService* GetPrefService() {
On 2012/05/23 16:37:32, Daniel Erat wrote:
> nit: const PrefService* ?

Done.

http://codereview.chromium.org/10383301/diff/10001/chrome/browser/ui/views/as...
chrome/browser/ui/views/ash/key_rewriter.cc:259: unsigned int
KeyRewriter::NativeKeySymToNativeKeycode(int keysym) {
On 2012/05/23 16:37:32, Daniel Erat wrote:
> nit: KeySym

Done.

http://codereview.chromium.org/10383301/diff/10001/chrome/browser/ui/views/as...
chrome/browser/ui/views/ash/key_rewriter.cc:370: bool rewritten = false;
On 2012/05/23 16:37:32, Daniel Erat wrote:
> nit: maybe remove this variable and just return false when pref_service is
NULL
> or OS_CHROMEOS isn't defined and true otherwise?  or are you planning to have
> more complicated logic later where this variable is conditionally set at
> different places in the body of this method?  right now, it looks like it's
only
> true on chrome os.

Sure, removed the variable.

http://codereview.chromium.org/10383301/diff/10001/chrome/browser/ui/views/as...
chrome/browser/ui/views/ash/key_rewriter.cc:381: unsigned int
remapped_native_keycode = xkey->keycode;
On 2012/05/23 16:37:32, Daniel Erat wrote:
> nit: KeyCode

Done.

http://codereview.chromium.org/10383301/diff/10001/chrome/browser/ui/views/as...
chrome/browser/ui/views/ash/key_rewriter.cc:400: break;
On 2012/05/23 16:37:32, Daniel Erat wrote:
> nit: add a no-op default?  think that some compilers will warn if you don't.

Done.

http://codereview.chromium.org/10383301/diff/10001/chrome/browser/ui/views/as...
File chrome/browser/ui/views/ash/key_rewriter.h (right):

http://codereview.chromium.org/10383301/diff/10001/chrome/browser/ui/views/as...
chrome/browser/ui/views/ash/key_rewriter.h:93: unsigned int
NativeKeySymToNativeKeycode(int keysym);
On 2012/05/23 16:37:32, Daniel Erat wrote:
> X.h defines KeySym as a XID and KeyCode as an unsigned char.  should the
> signature instead be:
> 
>   unsigned char NativeKeySymToNativeKeycode(unsigned long keysym);
> 
> ?

Done.

http://codereview.chromium.org/10383301/diff/10001/chrome/browser/ui/views/as...
File chrome/browser/ui/views/ash/key_rewriter_unittest.cc (right):

http://codereview.chromium.org/10383301/diff/10001/chrome/browser/ui/views/as...
chrome/browser/ui/views/ash/key_rewriter_unittest.cc:581: const unsigned int
kKeycodeSuperL = XKeysymToKeycode(display, XK_Super_L);
On 2012/05/23 16:37:32, Daniel Erat wrote:
> nit: KeyCode instead of unsigned int for all of these?  (looks like Xlib is
> inconsistent here, of course, but i'm pretty sure the protocol defines it as
> 8-bit)

Done.

http://codereview.chromium.org/10383301/diff/10001/chrome/browser/ui/views/as...
chrome/browser/ui/views/ash/key_rewriter_unittest.cc:593:
InitXKeyEvent(ui::VKEY_LWIN, 0, kKeycodeSuperL, 0U, &xev);
On 2012/05/23 16:37:32, Daniel Erat wrote:
> there's a bunch of duplicate code in this file.  do you think it'd be possible
> to shorten it?  what about something like (untested):
> 
>   std::string GetRewrittenEventAsString(
>       KeyRewriter* rewriter,
>       ui::KeyboardCode ui_keycode,
>       int ui_flags,    
>       KeyCode x_keycode,
>       unsigned int x_state) {
>     XEvent xev;
>     InitXKeyEvent(ui_keycode, ui_flags, x_keycode, x_state, &xev);
>     aura::KeyEvent keyevent(&xev, false /* is_char */);
>     rewriter->RewriteForTesting(&keyevent);
>     return StringPrintf(
>         "ui_keycode=%d ui_flags=%d ui_type=%d " 
>         "x_keycode=%d x_state=%u x_type=%d",
>         keyevent.key_code(), keyevent.flags(), keyevent.type(),
>         xkey.keycode, xkey.state, xkey.type);
>   }
> 
>   std::string GetExpectedResultAsString(
>       ui::KeyboardCode ui_keycode,
>       int ui_flags,
>       ui::EventType ui_type,
>       KeyCode x_keycode,
>       unsigned int x_state,
>       int x_type) {
>     return StringPrintf(
>         "ui_keycode=%d ui_flags=%d ui_type=%d "
>         "x_keycode=%d x_state=%u x_type=%d",
>         ui_keycode, ui_flags, ui_type, x_keycode, x_state, x_type);
>   }
> 
>   EXPECT_EQ(
>       GetExpectedResultAsString(
>           ui::VKEY_LWIN, 0, ui::ET_KEY_PRESSED, kKeycodeSuperL, 0U, KeyPress),
>       GetRewrittenEventAsString(
>           &rewriter, ui::VKEY_LWIN, 0, kKeycodeSuperL, 0U));
>   // etc.
> 
> Perhaps the kKeycode consts could also be moved into the base class so you
don't
> need to repeat that code at the beginning of every test, too.

Thanks for the suggestion! Done.

http://codereview.chromium.org/10383301/diff/10001/chrome/browser/ui/views/as...
chrome/browser/ui/views/ash/key_rewriter_unittest.cc:761: const unsigned int
kKeycodeSuperL = XKeysymToKeycode(display, XK_Super_L);
On 2012/05/23 16:37:32, Daniel Erat wrote:
> nit: KeyCode (also for other unsigned ints in this file)

Done.

[Zachary Kuznia, 2012-05-24 09:50:30]

LGTM for chromeos/

[Daniel Erat, 2012-05-24 14:55:11]

lgtm

https://chromiumcodereview.appspot.com/10383301/diff/24002/chrome/browser/ui/...
File chrome/browser/ui/views/ash/key_rewriter.cc (right):

https://chromiumcodereview.appspot.com/10383301/diff/24002/chrome/browser/ui/...
chrome/browser/ui/views/ash/key_rewriter.cc:43: unsigned int native_modifier;
nit: s/modifier/modifiers/ (since there are multiple ones or-ed together)

[Yusuke Sato, 2012-05-25 01:34:15]

http://codereview.chromium.org/10383301/diff/24002/chrome/browser/ui/views/as...
File chrome/browser/ui/views/ash/key_rewriter.cc (right):

http://codereview.chromium.org/10383301/diff/24002/chrome/browser/ui/views/as...
chrome/browser/ui/views/ash/key_rewriter.cc:43: unsigned int native_modifier;
On 2012/05/24 14:55:12, Daniel Erat wrote:
> nit: s/modifier/modifiers/ (since there are multiple ones or-ed together)

Actually only one modifier mask is used per entry (Mod4Mask, ControlMask,
Mod1Mask, 0U, 0U). Let me keep this as-is for now.

[commit-bot: I haz the power, 2012-05-25 02:26:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yusukes@chromium.org/10383301/20003

[commit-bot: I haz the power, 2012-05-25 04:16:09]

Change committed as 138988