Use rewriting to make ChromeOS keyboard F<number> keys produce extended keycodes.

Use rewriting to make ChromeOS keyboard F<number> keys produce extended keycodes.

This makes the ChromeOS keyboard F<number> keys produce keycodes such as
BROWSER_BACK, VOLUME_UP, POWER, etc. We can then remove special-cases for
ChromeOS accelerator bindings, and allow the same bindings to work on all
platforms including ChromeOS. The ChromeOS keyboard, and external keyboards
with these extra keys will also rely on and fire the same bindings for these
keys, unifying the code path.

The other advantage of this, is that now ChromeOS does not need to bind
F<number> keys in a non-standard way. So, an external keyboard plugged
into a Chromebook can still use the F<number> keys in the same way as they
would when it was plugged into their desktop. This behaviour isn't yet
possible, as the event rewritter is not aware if the event came from the
ChromeOS keyboard or an external keyboard, but is a 1-line change once this
information is known.

Lastly, when the Search key acts as Function key option is enabled from
https://codereview.chromium.org/11421055/ then Search-1 through Search-0
produce the keycode F1 through F10, and Search-- and Search-= produce F11 and
F12. This allows applications to easily rely on these keys and consume them the
same way on desktop as on a Chromebook.

R=yusukes@chromium.org
BUG=162268
TEST=unit_tests:EventRewriter.TestRewriteFunctionKeys
Depends on: https://codereview.chromium.org/11421055/

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=170415

[Yusuke Sato, 2012-11-26 07:28:19]

https://codereview.chromium.org/11417144/diff/1/chrome/browser/ui/ash/event_r...
File chrome/browser/ui/ash/event_rewriter.cc (right):

https://codereview.chromium.org/11417144/diff/1/chrome/browser/ui/ash/event_r...
chrome/browser/ui/ash/event_rewriter.cc:764: // On a ChromeOS keyboard,
F<number> keys have special purposes. On other
What's the expected behavior when Stumpy is used with a generic PC keyboard?
Please add a comment for that case.

https://codereview.chromium.org/11417144/diff/1/chrome/browser/ui/ash/event_r...
chrome/browser/ui/ash/event_rewriter.cc:840: case XK_1:
This is very dirty, but probably we should check |native_event()->xkey.keycode|
here rather than X keysym because otherwise we are not able to send Shift+F1 to
an app. Please note that Shift+1 generates XK_exclam, not XK_1.

Similarly, I guess many i18n keyboard layouts that require ShiftMask to generate
XK_1 (such as http://en.wikipedia.org/wiki/Keyboard_layout#French ) are not able
to send F1 to an app. In that case, only Shift+F1 can be sent to an app.

(BTW, if you have a better solution for this, please let me know. Checking X
keycode is not a very clean solution... Since X keycode is actually defined
outside X11 as Wez mentioned, we might end up writing Chrome OS autotests to
prevent regressions. Currently X keycode is defined by Linux kernel module
called evdev, but X server might use other module in the future, or numbers
defined by the kernel might change in the future.)

https://codereview.chromium.org/11417144/diff/1/chrome/browser/ui/ash/event_r...
File chrome/browser/ui/ash/event_rewriter_unittest.cc (right):

https://codereview.chromium.org/11417144/diff/1/chrome/browser/ui/ash/event_r...
chrome/browser/ui/ash/event_rewriter_unittest.cc:132:
keycode_browser_back_(XKeysymToKeycode(display_, XF86XK_Back)),
Using XF86XK keysyms probably has the same issue as
https://code.google.com/p/chromium/issues/detail?id=158585 .

[danakj, 2012-11-26 17:50:38]

https://codereview.chromium.org/11417144/diff/1/chrome/browser/ui/ash/event_r...
File chrome/browser/ui/ash/event_rewriter.cc (right):

https://codereview.chromium.org/11417144/diff/1/chrome/browser/ui/ash/event_r...
chrome/browser/ui/ash/event_rewriter.cc:764: // On a ChromeOS keyboard,
F<number> keys have special purposes. On other
On 2012/11/26 07:28:19, Yusuke Sato wrote:
> What's the expected behavior when Stumpy is used with a generic PC keyboard?
> Please add a comment for that case.

Stumpy should not --has-chromeos-keyboard, so it should not do any rewriting
here on a stumpy. I can add a comment that "ChromeOS keyboard" means a
chromebook keyboard.

Hopefully I am understanding the flag correctly.

https://codereview.chromium.org/11417144/diff/1/chrome/browser/ui/ash/event_r...
File chrome/browser/ui/ash/event_rewriter_unittest.cc (right):

https://codereview.chromium.org/11417144/diff/1/chrome/browser/ui/ash/event_r...
chrome/browser/ui/ash/event_rewriter_unittest.cc:132:
keycode_browser_back_(XKeysymToKeycode(display_, XF86XK_Back)),
On 2012/11/26 07:28:19, Yusuke Sato wrote:
> Using XF86XK keysyms probably has the same issue as
> https://code.google.com/p/chromium/issues/detail?id=158585 .

Oh darn :( Okay, I'll make this work for now, and point to the bug.

[Yusuke Sato, 2012-11-26 18:59:59]

https://codereview.chromium.org/11417144/diff/1/chrome/browser/ui/ash/event_r...
File chrome/browser/ui/ash/event_rewriter.cc (right):

https://codereview.chromium.org/11417144/diff/1/chrome/browser/ui/ash/event_r...
chrome/browser/ui/ash/event_rewriter.cc:764: // On a ChromeOS keyboard,
F<number> keys have special purposes. On other
On 2012/11/26 17:50:38, danakj wrote:
> On 2012/11/26 07:28:19, Yusuke Sato wrote:
> > What's the expected behavior when Stumpy is used with a generic PC keyboard?
> > Please add a comment for that case.
> 
> Stumpy should not --has-chromeos-keyboard, so it should not do any rewriting
> here on a stumpy. I can add a comment that "ChromeOS keyboard" means a
> chromebook keyboard.
> 
> Hopefully I am understanding the flag correctly.

Sorry: I thought that HasChromeOSKeyboard() was a new function you added, but it
was not true. It was a function I added a while ago..

But anyway, calling the function wouldn't be good in this case because the
function returns false on Parrot (Acer $199 model) although the model has Chrome
OS style top-row keys. How about removing line 767-768 (and possibly rewriting
764-765)? Leaving the TODO comment seems to be good enough for now.


Details: The name might be a bit confusing, but --has-chromeos-keyborad is used
when the device has the same keyboard as Mario (i.e. Search instead of Caps
Lock). So Mario/Alex/ZGB/Lumpy/Lucas have the flag, but Parrot (Acer $199 model
which does have Caps Lock) and Stumpy do not.

[danakj, 2012-11-26 19:07:16]

https://codereview.chromium.org/11417144/diff/1/chrome/browser/ui/ash/event_r...
File chrome/browser/ui/ash/event_rewriter.cc (right):

https://codereview.chromium.org/11417144/diff/1/chrome/browser/ui/ash/event_r...
chrome/browser/ui/ash/event_rewriter.cc:764: // On a ChromeOS keyboard,
F<number> keys have special purposes. On other
On 2012/11/26 19:00:00, Yusuke Sato wrote:
> On 2012/11/26 17:50:38, danakj wrote:
> > On 2012/11/26 07:28:19, Yusuke Sato wrote:
> > > What's the expected behavior when Stumpy is used with a generic PC
keyboard?
> > > Please add a comment for that case.
> > 
> > Stumpy should not --has-chromeos-keyboard, so it should not do any rewriting
> > here on a stumpy. I can add a comment that "ChromeOS keyboard" means a
> > chromebook keyboard.
> > 
> > Hopefully I am understanding the flag correctly.
> 
> Sorry: I thought that HasChromeOSKeyboard() was a new function you added, but
it
> was not true. It was a function I added a while ago..
> 
> But anyway, calling the function wouldn't be good in this case because the
> function returns false on Parrot (Acer $199 model) although the model has
Chrome
> OS style top-row keys. How about removing line 767-768 (and possibly rewriting
> 764-765)? Leaving the TODO comment seems to be good enough for now.
> 
> 
> Details: The name might be a bit confusing, but --has-chromeos-keyborad is
used
> when the device has the same keyboard as Mario (i.e. Search instead of Caps
> Lock). So Mario/Alex/ZGB/Lumpy/Lucas have the flag, but Parrot (Acer $199
model
> which does have Caps Lock) and Stumpy do not.

Ok. I will had an EventSourceIsChromebookKeyboard() method with a TODO, that
just returns true for now. So we'll have  a clear path to check for
internal/external keyboards in the future.

Thanks for the explanation.

[danakj, 2012-11-26 21:08:38]

I left the unit test for now, and am trying it out with a trybot. Other feedback
is addressed now.

https://codereview.chromium.org/11417144/diff/1/chrome/browser/ui/ash/event_r...
File chrome/browser/ui/ash/event_rewriter.cc (right):

https://codereview.chromium.org/11417144/diff/1/chrome/browser/ui/ash/event_r...
chrome/browser/ui/ash/event_rewriter.cc:840: case XK_1:
On 2012/11/26 07:28:19, Yusuke Sato wrote:
> This is very dirty, but probably we should check
|native_event()->xkey.keycode|
> here rather than X keysym because otherwise we are not able to send Shift+F1
to
> an app. Please note that Shift+1 generates XK_exclam, not XK_1.
> 
> Similarly, I guess many i18n keyboard layouts that require ShiftMask to
generate
> XK_1 (such as http://en.wikipedia.org/wiki/Keyboard_layout#French ) are not
able
> to send F1 to an app. In that case, only Shift+F1 can be sent to an app.
> 
> (BTW, if you have a better solution for this, please let me know. Checking X
> keycode is not a very clean solution... Since X keycode is actually defined
> outside X11 as Wez mentioned, we might end up writing Chrome OS autotests to
> prevent regressions. Currently X keycode is defined by Linux kernel module
> called evdev, but X server might use other module in the future, or numbers
> defined by the kernel might change in the future.)

Ok this sounds reasonable. If we check that keycode == XKeySymToKeyCode(XK_1),
then Search-1 does the right thing on US keyboards. And Search-<whatever> will
send F1, where <whatever> would generate a "1" with some modifier set. That way
those keyboards can also pass F1 and Shift-F1 etc.

It's still really US-centric for F11/F12, as "-" and "=" might not be in the
same position beside "0", but it's a start.

Done.

[Yusuke Sato, 2012-11-27 01:56:53]

LGTM with some nits (but please wait for a comment from blumberg@).

https://codereview.chromium.org/11417144/diff/1008/ash/accelerators/accelerat...
File ash/accelerators/accelerator_table.cc (right):

https://codereview.chromium.org/11417144/diff/1008/ash/accelerators/accelerat...
ash/accelerators/accelerator_table.cc:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
I think you also have to update UMA code. Try this:

$ cd ~/chrome/src/
$ git grep -A 3 VKEY_F ash/accelerators/accelerator_controller.cc
ash/system/chromeos/keyboard_brightness_controller.cc
$ git grep UMA_ACCEL_ .

https://codereview.chromium.org/11417144/diff/1008/ash/accelerators/accelerat...
ash/accelerators/accelerator_table.cc:106: { true, ui::VKEY_MEDIA_LAUNCH_APP2,
ui::EF_NONE, TOGGLE_MAXIMIZED },
FYI, this changes the behavior of F3 key on a full-size Apple USB keyboard. This
is probably not a big deal though :p

https://codereview.chromium.org/11417144/diff/1008/chrome/browser/ui/ash/even...
File chrome/browser/ui/ash/event_rewriter.cc (right):

https://codereview.chromium.org/11417144/diff/1008/chrome/browser/ui/ash/even...
chrome/browser/ui/ash/event_rewriter.cc:913: } else if (xkeycode ==
minus_xkeycode_) {
Could you add a TODO comment to explain the i18n keyboard issue? For example, as
you might notice, when French keyboard is in use, minus_xkeycode_ and
number6_xkeycode_ have the same value.

http://en.wikipedia.org/wiki/File:KB_France.svg

https://codereview.chromium.org/11417144/diff/1008/chrome/browser/ui/ash/even...
chrome/browser/ui/ash/event_rewriter.cc:936: return false;
nit: move this line to line 933?

[danakj, 2012-11-27 03:14:25]

https://codereview.chromium.org/11417144/diff/1008/ash/accelerators/accelerat...
File ash/accelerators/accelerator_table.cc (right):

https://codereview.chromium.org/11417144/diff/1008/ash/accelerators/accelerat...
ash/accelerators/accelerator_table.cc:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
On 2012/11/27 01:56:54, Yusuke Sato wrote:
> I think you also have to update UMA code. Try this:
> 
> $ cd ~/chrome/src/
> $ git grep -A 3 VKEY_F ash/accelerators/accelerator_controller.cc
> ash/system/chromeos/keyboard_brightness_controller.cc
> $ git grep UMA_ACCEL_ .

Hm.. The actual key being pressed is still F4 and F5. I updated the code so that
the correct UMA events will happen again. But I didn't rename the UMA events,
since F4 and F5 (the actual keys!) seems more useful than MEDIA_LAUNCH_APP2 and
MEDIA_LAUNCH_APP1.

https://codereview.chromium.org/11417144/diff/1008/ash/accelerators/accelerat...
ash/accelerators/accelerator_table.cc:106: { true, ui::VKEY_MEDIA_LAUNCH_APP2,
ui::EF_NONE, TOGGLE_MAXIMIZED },
On 2012/11/27 01:56:54, Yusuke Sato wrote:
> FYI, this changes the behavior of F3 key on a full-size Apple USB keyboard.
This
> is probably not a big deal though :p

Ya, also the "calculator" button on my external keyboard. I considered using
another keycode instead for the maximize key, but this seemed to make the most
sense still.

https://codereview.chromium.org/11417144/diff/1008/chrome/browser/ui/ash/even...
File chrome/browser/ui/ash/event_rewriter.cc (right):

https://codereview.chromium.org/11417144/diff/1008/chrome/browser/ui/ash/even...
chrome/browser/ui/ash/event_rewriter.cc:913: } else if (xkeycode ==
minus_xkeycode_) {
On 2012/11/27 01:56:54, Yusuke Sato wrote:
> Could you add a TODO comment to explain the i18n keyboard issue? For example,
as
> you might notice, when French keyboard is in use, minus_xkeycode_ and
> number6_xkeycode_ have the same value.
> 
> http://en.wikipedia.org/wiki/File:KB_France.svg

Yep!

https://codereview.chromium.org/11417144/diff/1008/chrome/browser/ui/ash/even...
chrome/browser/ui/ash/event_rewriter.cc:936: return false;
On 2012/11/27 01:56:54, Yusuke Sato wrote:
> nit: move this line to line 933?

Done.

[Yusuke Sato, 2012-11-27 18:19:50]

https://codereview.chromium.org/11417144/diff/1008/ash/accelerators/accelerat...
File ash/accelerators/accelerator_table.cc (right):

https://codereview.chromium.org/11417144/diff/1008/ash/accelerators/accelerat...
ash/accelerators/accelerator_table.cc:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
Also need to fix ash/system/chromeos/keyboard_brightness_controller.cc,
probably?


On 2012/11/27 03:14:26, danakj wrote:
> On 2012/11/27 01:56:54, Yusuke Sato wrote:
> > I think you also have to update UMA code. Try this:
> > 
> > $ cd ~/chrome/src/
> > $ git grep -A 3 VKEY_F ash/accelerators/accelerator_controller.cc
> > ash/system/chromeos/keyboard_brightness_controller.cc
> > $ git grep UMA_ACCEL_ .
> 
> Hm.. The actual key being pressed is still F4 and F5. I updated the code so
that
> the correct UMA events will happen again. But I didn't rename the UMA events,
> since F4 and F5 (the actual keys!) seems more useful than MEDIA_LAUNCH_APP2
and
> MEDIA_LAUNCH_APP1.

[Yusuke Sato, 2012-11-27 19:38:16]

On 2012/11/27 18:19:50, Yusuke Sato wrote:
>
https://codereview.chromium.org/11417144/diff/1008/ash/accelerators/accelerat...
> File ash/accelerators/accelerator_table.cc (right):
> 
>
https://codereview.chromium.org/11417144/diff/1008/ash/accelerators/accelerat...
> ash/accelerators/accelerator_table.cc:1: // Copyright (c) 2012 The Chromium
> Authors. All rights reserved.
> Also need to fix ash/system/chromeos/keyboard_brightness_controller.cc,
> probably?

oops.. there seems to be more. please try:

~/chrome/src$ git grep -e 'Accel_.*_F'

> 
> 
> On 2012/11/27 03:14:26, danakj wrote:
> > On 2012/11/27 01:56:54, Yusuke Sato wrote:
> > > I think you also have to update UMA code. Try this:
> > > 
> > > $ cd ~/chrome/src/
> > > $ git grep -A 3 VKEY_F ash/accelerators/accelerator_controller.cc
> > > ash/system/chromeos/keyboard_brightness_controller.cc
> > > $ git grep UMA_ACCEL_ .
> > 
> > Hm.. The actual key being pressed is still F4 and F5. I updated the code so
> that
> > the correct UMA events will happen again. But I didn't rename the UMA
events,
> > since F4 and F5 (the actual keys!) seems more useful than MEDIA_LAUNCH_APP2
> and
> > MEDIA_LAUNCH_APP1.

[Yusuke Sato, 2012-11-27 22:16:58]

patch set 5 lgtm

Can you ping blumberg@ and explain him about the old and new behavior of the
top-row keys?


On 2012/11/27 19:38:16, Yusuke Sato wrote:
> On 2012/11/27 18:19:50, Yusuke Sato wrote:
> >
>
https://codereview.chromium.org/11417144/diff/1008/ash/accelerators/accelerat...
> > File ash/accelerators/accelerator_table.cc (right):
> > 
> >
>
https://codereview.chromium.org/11417144/diff/1008/ash/accelerators/accelerat...
> > ash/accelerators/accelerator_table.cc:1: // Copyright (c) 2012 The Chromium
> > Authors. All rights reserved.
> > Also need to fix ash/system/chromeos/keyboard_brightness_controller.cc,
> > probably?
> 
> oops.. there seems to be more. please try:
> 
> ~/chrome/src$ git grep -e 'Accel_.*_F'
> 
> > 
> > 
> > On 2012/11/27 03:14:26, danakj wrote:
> > > On 2012/11/27 01:56:54, Yusuke Sato wrote:
> > > > I think you also have to update UMA code. Try this:
> > > > 
> > > > $ cd ~/chrome/src/
> > > > $ git grep -A 3 VKEY_F ash/accelerators/accelerator_controller.cc
> > > > ash/system/chromeos/keyboard_brightness_controller.cc
> > > > $ git grep UMA_ACCEL_ .
> > > 
> > > Hm.. The actual key being pressed is still F4 and F5. I updated the code
so
> > that
> > > the correct UMA events will happen again. But I didn't rename the UMA
> events,
> > > since F4 and F5 (the actual keys!) seems more useful than
MEDIA_LAUNCH_APP2
> > and
> > > MEDIA_LAUNCH_APP1.

[danakj, 2012-11-28 20:14:52]

Hi Yusuke, PTAL.

I've found a way to determine if the keyboard is the chromebook keyboard. I
verified with "DISPLAY=:0 xinput list" on lumpy and alex as well that this
matches there. It seems to be the string used on internal laptop keyboards in
general.

[Yusuke Sato, 2012-11-28 20:39:17]

https://codereview.chromium.org/11417144/diff/9003/chrome/browser/ui/ash/even...
File chrome/browser/ui/ash/event_rewriter.cc (right):

https://codereview.chromium.org/11417144/diff/9003/chrome/browser/ui/ash/even...
chrome/browser/ui/ash/event_rewriter.cc:200: if
(LowerCaseEqualsASCII(device_name, "at translated set 2 keyboard"))
can you also support Chrome USB keyboard for Stumpy? In case you don't have the
keyboard, the ID is:

yusukes@yusukes:~/chrome/src$ xinput list | grep Samsung
    ↳ Samsung USB Keyboard                    	id=12	[slave  keyboard (3)]

[danakj, 2012-11-28 21:15:11]

Done! PTAL

[Yusuke Sato, 2012-11-28 21:21:39]

On 2012/11/28 21:15:11, danakj wrote:
> Done! PTAL

lgtm, thanks.

[danakj, 2012-11-28 21:24:00]

+sky for OWNERS

Sky, this is a followup to the last CL adding Search-bindings. This adds
Search-1 for F1, and so on.

[danakj, 2012-11-28 21:24:46]

+derat for event_rewriter

[sky, 2012-11-28 22:15:35]

LGTM

[Daniel Erat, 2012-11-28 22:23:03]

https://codereview.chromium.org/11417144/diff/10002/ash/accelerators/accelera...
File ash/accelerators/accelerator_table.cc (right):

https://codereview.chromium.org/11417144/diff/10002/ash/accelerators/accelera...
ash/accelerators/accelerator_table.cc:47: { true, ui::VKEY_MEDIA_LAUNCH_APP2,
ui::EF_CONTROL_DOWN, CYCLE_DISPLAY_MODE },
is there a standard "internet/media keyboard" layout that leads to
maximize->launch_app2 and cycle->launch_app1?  just curious about if this is
mostly arbitrary or deliberately chosen -- having app2 on the left seems a bit
strange.

https://codereview.chromium.org/11417144/diff/10002/chrome/browser/ui/ash/eve...
File chrome/browser/ui/ash/event_rewriter.cc (right):

https://codereview.chromium.org/11417144/diff/10002/chrome/browser/ui/ash/eve...
chrome/browser/ui/ash/event_rewriter.cc:205: return kDeviceChromebookKeyboard;
it's possible that i misunderstood the conversation about this earlier, but it
seems confusing to me that we use "ChromebookKeyboard" to refer to a
chrome-os-specific keyboard attached to a chromebox, but not to refer to
built-in keyboards attached to other devices that i would think of as
chromebooks.  can we use a better name for this?  maybe something like
kDeviceChromeKeyboard?

https://codereview.chromium.org/11417144/diff/10002/chrome/browser/ui/ash/eve...
chrome/browser/ui/ash/event_rewriter.cc:893: // Remap Search+2 to F2.
i'd probably remove these repetitive comments in favor of a single comment at
the beginning describing the general reasoning.  i'm wondering if it'd be best
to just do something like this, actually (untested and probably full of syntax
errors and bugs, but hopefully you'll get the idea):

  // Describes a remapping for a given key.
  struct KeyRemapping {
    KeySym source_keysym;
    KeySym target_keysym;
    ui::KeyboardCode target_keycode;
  };

  static const KeyRemapping kKeysymRemappings[] = {
    { XK_F1, XF86XK_Back, ui::VKEY_BROWSER_BACK },
    { XK_F2, XF86XK_Forward, ui::VKEY_BROWSER_FORWARD },
    // ...
  };

  for (int i = 0; i < arraysize(kKeysymRemappings); ++i) {
    const KeyRemapping& remapping = kKeysymRemappings[i];
    if (keysym == remapping.source_keysym) {
      remapped_native_keycode = GetNativeKeycode(remapping.target_keysym);
      remapped_keycode = remapping.target_keycode;
      break;
    }
  }

  // ...

  if (search_as_function_key && xkey->state & Mod4Mask) {
    static const KeyRemapping kKeycodeRemappings[] = { 
      { XK_1, XK_F1, ui::VKEY_F1 },
      { XK_2, XK_F2, ui::VKEY_F2 },
      // ...
    };

    for (int i = 0; i < arraysize(kKeycodeRemappings); ++i) {
      const KeyRemapping& remapping = kKeycodeRemappings[i];
      if (xkey->keycode == GetNativeKeycode(remapping.source_keysym)) {
        remapped_native_keycode = GetNativeKeycode(remapping.target_keysym);
        remapped_keycode = remapping.target_keyboard_code;
        remapped_native_state &= ~Mod4Mask;
        break;
      }
    }
  }

https://codereview.chromium.org/11417144/diff/10002/chrome/browser/ui/ash/eve...
File chrome/browser/ui/ash/event_rewriter.h (right):

https://codereview.chromium.org/11417144/diff/10002/chrome/browser/ui/ash/eve...
chrome/browser/ui/ash/event_rewriter.h:95: bool
EventSourceIsChromebookKeyboard(ui::KeyEvent* event);
nit: const argument and method

https://codereview.chromium.org/11417144/diff/10002/chrome/browser/ui/ash/eve...
chrome/browser/ui/ash/event_rewriter.h:195: unsigned int f1_xkeycode_;
yikes.  can this be switched to a single keysym-to-keycode map?  (maybe even
with an accessor that looks up keycodes and caches them as needed, so we don't
have to maintain a huge listing in the initialization code?  the Refresh method
could just clear the map.)

[danakj, 2012-11-29 01:50:54]

PTAL.

@yusukes: I have guarded all the "Search key as modifier" places also on
--has-chromeos-keyboard, as this seems to be the places we want it (ie not on
the acer model since it has a function key of its own)

https://codereview.chromium.org/11417144/diff/10002/ash/accelerators/accelera...
File ash/accelerators/accelerator_table.cc (right):

https://codereview.chromium.org/11417144/diff/10002/ash/accelerators/accelera...
ash/accelerators/accelerator_table.cc:47: { true, ui::VKEY_MEDIA_LAUNCH_APP2,
ui::EF_CONTROL_DOWN, CYCLE_DISPLAY_MODE },
On 2012/11/28 22:23:03, Daniel Erat wrote:
> is there a standard "internet/media keyboard" layout that leads to
> maximize->launch_app2 and cycle->launch_app1?  just curious about if this is
> mostly arbitrary or deliberately chosen -- having app2 on the left seems a bit
> strange.

I did that because APP1 was already bound to CYCLE_FORWARD_LINEAR (line 39). So
I just left it and added APP2 beside it.

https://codereview.chromium.org/11417144/diff/10002/chrome/browser/ui/ash/eve...
File chrome/browser/ui/ash/event_rewriter.cc (right):

https://codereview.chromium.org/11417144/diff/10002/chrome/browser/ui/ash/eve...
chrome/browser/ui/ash/event_rewriter.cc:205: return kDeviceChromebookKeyboard;
On 2012/11/28 22:23:03, Daniel Erat wrote:
> it's possible that i misunderstood the conversation about this earlier, but it
> seems confusing to me that we use "ChromebookKeyboard" to refer to a
> chrome-os-specific keyboard attached to a chromebox, but not to refer to
> built-in keyboards attached to other devices that i would think of as
> chromebooks.  can we use a better name for this?  maybe something like
> kDeviceChromeKeyboard?

Hm.. It should refer to ChromeOS keyboards that have a search key and top-row
for brightness etc, and no function-key of their own. Will rename this to
kDeviceChromeOSKeyboard and guard this on HasChromeOSKeyboard().

https://codereview.chromium.org/11417144/diff/10002/chrome/browser/ui/ash/eve...
chrome/browser/ui/ash/event_rewriter.cc:893: // Remap Search+2 to F2.
On 2012/11/28 22:23:03, Daniel Erat wrote:
> i'd probably remove these repetitive comments in favor of a single comment at
> the beginning describing the general reasoning.  i'm wondering if it'd be best
> to just do something like this, actually (untested and probably full of syntax
> errors and bugs, but hopefully you'll get the idea):
> 
>   // Describes a remapping for a given key.
>   struct KeyRemapping {
>     KeySym source_keysym;
>     KeySym target_keysym;
>     ui::KeyboardCode target_keycode;
>   };
> 
>   static const KeyRemapping kKeysymRemappings[] = {
>     { XK_F1, XF86XK_Back, ui::VKEY_BROWSER_BACK },
>     { XK_F2, XF86XK_Forward, ui::VKEY_BROWSER_FORWARD },
>     // ...
>   };
> 
>   for (int i = 0; i < arraysize(kKeysymRemappings); ++i) {
>     const KeyRemapping& remapping = kKeysymRemappings[i];
>     if (keysym == remapping.source_keysym) {
>       remapped_native_keycode = GetNativeKeycode(remapping.target_keysym);
>       remapped_keycode = remapping.target_keycode;
>       break;
>     }
>   }
> 
>   // ...
> 
>   if (search_as_function_key && xkey->state & Mod4Mask) {
>     static const KeyRemapping kKeycodeRemappings[] = { 
>       { XK_1, XK_F1, ui::VKEY_F1 },
>       { XK_2, XK_F2, ui::VKEY_F2 },
>       // ...
>     };
> 
>     for (int i = 0; i < arraysize(kKeycodeRemappings); ++i) {
>       const KeyRemapping& remapping = kKeycodeRemappings[i];
>       if (xkey->keycode == GetNativeKeycode(remapping.source_keysym)) {
>         remapped_native_keycode = GetNativeKeycode(remapping.target_keysym);
>         remapped_keycode = remapping.target_keyboard_code;
>         remapped_native_state &= ~Mod4Mask;
>         break;
>       }
>     }
>   }

Done.

https://codereview.chromium.org/11417144/diff/10002/chrome/browser/ui/ash/eve...
File chrome/browser/ui/ash/event_rewriter.h (right):

https://codereview.chromium.org/11417144/diff/10002/chrome/browser/ui/ash/eve...
chrome/browser/ui/ash/event_rewriter.h:95: bool
EventSourceIsChromebookKeyboard(ui::KeyEvent* event);
On 2012/11/28 22:23:03, Daniel Erat wrote:
> nit: const argument and method

Done. The argument ends up unusued anyhow, so I removed it.

https://codereview.chromium.org/11417144/diff/10002/chrome/browser/ui/ash/eve...
chrome/browser/ui/ash/event_rewriter.h:195: unsigned int f1_xkeycode_;
On 2012/11/28 22:23:03, Daniel Erat wrote:
> yikes.  can this be switched to a single keysym-to-keycode map?  (maybe even
> with an accessor that looks up keycodes and caches them as needed, so we don't
> have to maintain a huge listing in the initialization code?  the Refresh
method
> could just clear the map.)

Done.

[Daniel Erat, 2012-11-29 04:12:27]

https://codereview.chromium.org/11417144/diff/16001/chrome/browser/ui/ash/eve...
File chrome/browser/ui/ash/event_rewriter.cc (right):

https://codereview.chromium.org/11417144/diff/16001/chrome/browser/ui/ash/eve...
chrome/browser/ui/ash/event_rewriter.cc:865: {
NativeKeySymToNativeKeycode(XK_1), 0, Mod4Mask,
the keysym-to-keycode mappings can change (and hence RefreshKeycodes), right? 
it might be better to just use KeyboardRemappingByKeySym here and make
Rewrite...ByKeyCode look up the keycodes.

[danakj, 2012-11-29 04:34:14]

https://codereview.chromium.org/11417144/diff/16001/chrome/browser/ui/ash/eve...
File chrome/browser/ui/ash/event_rewriter.cc (right):

https://codereview.chromium.org/11417144/diff/16001/chrome/browser/ui/ash/eve...
chrome/browser/ui/ash/event_rewriter.cc:865: {
NativeKeySymToNativeKeycode(XK_1), 0, Mod4Mask,
On 2012/11/29 04:12:27, Daniel Erat wrote:
> the keysym-to-keycode mappings can change (and hence RefreshKeycodes), right? 
> it might be better to just use KeyboardRemappingByKeySym here and make
> Rewrite...ByKeyCode look up the keycodes.

Ahhh. Yes, good point. Thank you for catching that.

In order for the helper method to do that, it needs to be in the class. And it
takes things like KeySym as arguments, which means they need to be defined in
the header.

Including Xlib.h from the .h is a bad idea due to macros like RootWindow() etc.
So, I just defined KeySym and KeyCode in the .h file as subtypes of the
EventRewriter class.

Alternatively, we can do some hacky code to keep the helper functions outside of
the class. Let me know what you'd like please.

[Daniel Erat, 2012-11-29 04:54:23]

LGTM w/comments

https://codereview.chromium.org/11417144/diff/11015/chrome/browser/ui/ash/eve...
File chrome/browser/ui/ash/event_rewriter.cc (right):

https://codereview.chromium.org/11417144/diff/11015/chrome/browser/ui/ash/eve...
chrome/browser/ui/ash/event_rewriter.cc:286: base::hash_map<KeySym,
KeyCode>::iterator it;
nit: delete this; it doesn't look like you're using it

https://codereview.chromium.org/11417144/diff/11015/chrome/browser/ui/ash/eve...
chrome/browser/ui/ash/event_rewriter.cc:814: { XK_F11, 0, 0, XF86XK_PowerOff,
ui::VKEY_POWER, }
i don't think this is correct -- the power key on chrome os keyboards already
comes in as XF86XK_PowerOff, as far as i'm aware.  chrome doesn't handle it
directly, though; it receives d-bus signals about it from the power manager
(which monitors the input device directly; as i understand it, the acpi standard
doesn't report power button releases correctly.  if we just look at the X event,
we see a press followed immediately by a release, even if the button is still
held down)

https://codereview.chromium.org/11417144/diff/11015/chrome/browser/ui/ash/eve...
File chrome/browser/ui/ash/event_rewriter.h (right):

https://codereview.chromium.org/11417144/diff/11015/chrome/browser/ui/ash/eve...
chrome/browser/ui/ash/event_rewriter.h:105: typedef unsigned long KeySym;
this approach seems fine to me, although i'd probably move these outside of any
namespaces so they match the ones coming from Xlib

https://codereview.chromium.org/11417144/diff/11015/chrome/browser/ui/ash/eve...
chrome/browser/ui/ash/event_rewriter.h:117: // if possible, and returns the
remapped event values.
nit: s/returns/updates/, also add "Returns true if the event was rewritten."
(ditto for ByKeyCode below)

[danakj, 2012-11-30 03:07:40]

Thanks!

https://codereview.chromium.org/11417144/diff/11015/chrome/browser/ui/ash/eve...
File chrome/browser/ui/ash/event_rewriter.cc (right):

https://codereview.chromium.org/11417144/diff/11015/chrome/browser/ui/ash/eve...
chrome/browser/ui/ash/event_rewriter.cc:286: base::hash_map<KeySym,
KeyCode>::iterator it;
On 2012/11/29 04:54:23, Daniel Erat wrote:
> nit: delete this; it doesn't look like you're using it

Done.

https://codereview.chromium.org/11417144/diff/11015/chrome/browser/ui/ash/eve...
chrome/browser/ui/ash/event_rewriter.cc:814: { XK_F11, 0, 0, XF86XK_PowerOff,
ui::VKEY_POWER, }
On 2012/11/29 04:54:23, Daniel Erat wrote:
> i don't think this is correct -- the power key on chrome os keyboards already
> comes in as XF86XK_PowerOff, as far as i'm aware.  chrome doesn't handle it
> directly, though; it receives d-bus signals about it from the power manager
> (which monitors the input device directly; as i understand it, the acpi
standard
> doesn't report power button releases correctly.  if we just look at the X
event,
> we see a press followed immediately by a release, even if the button is still
> held down)

Done.

https://codereview.chromium.org/11417144/diff/11015/chrome/browser/ui/ash/eve...
File chrome/browser/ui/ash/event_rewriter.h (right):

https://codereview.chromium.org/11417144/diff/11015/chrome/browser/ui/ash/eve...
chrome/browser/ui/ash/event_rewriter.h:105: typedef unsigned long KeySym;
On 2012/11/29 04:54:23, Daniel Erat wrote:
> this approach seems fine to me, although i'd probably move these outside of
any
> namespaces so they match the ones coming from Xlib

Ah, okay. I was worried about "already defined" conflicts, but I see that
doesn't happen for typedefs. Moved them.

https://codereview.chromium.org/11417144/diff/11015/chrome/browser/ui/ash/eve...
chrome/browser/ui/ash/event_rewriter.h:117: // if possible, and returns the
remapped event values.
On 2012/11/29 04:54:23, Daniel Erat wrote:
> nit: s/returns/updates/, also add "Returns true if the event was rewritten."
> (ditto for ByKeyCode below)

Done.

[danakj, 2012-11-30 03:49:12]

https://codereview.chromium.org/11417144/diff/11015/chrome/browser/ui/ash/eve...
File chrome/browser/ui/ash/event_rewriter.h (right):

https://codereview.chromium.org/11417144/diff/11015/chrome/browser/ui/ash/eve...
chrome/browser/ui/ash/event_rewriter.h:105: typedef unsigned long KeySym;
On 2012/11/30 03:07:40, danakj wrote:
> On 2012/11/29 04:54:23, Daniel Erat wrote:
> > this approach seems fine to me, although i'd probably move these outside of
> any
> > namespaces so they match the ones coming from Xlib
> 
> Ah, okay. I was worried about "already defined" conflicts, but I see that
> doesn't happen for typedefs. Moved them.

Actually, it did cause compiler errors, so I moved it back into the class.

[commit-bot: I haz the power, 2012-11-30 04:07:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/11417144/4056

[commit-bot: I haz the power, 2012-11-30 07:27:54]

Change committed as 170415