Fix some surprising reverse key mappings.

Fix some surprising reverse key mappings.

Added a guard in |chromeos::EventRewriter::RewriteKeyEvent()|.
Moved |ui::XKeyEventKeyCode()| to share it. Added unit test cases.

BUG=390263
TEST=EventRewriterTest.TestRewriteModifiersRemapMany
R=derat@chromium.org,sadrul@chromium.org

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

[Daniel Erat, 2014-07-07 21:07:02]

lgtm with some comments

https://codereview.chromium.org/378503007/diff/1/ui/events/keycodes/keyboard_...
File ui/events/keycodes/keyboard_code_conversion_x.cc (right):

https://codereview.chromium.org/378503007/diff/1/ui/events/keycodes/keyboard_...
ui/events/keycodes/keyboard_code_conversion_x.cc:1282: // SHIFT state is ignored
in the call to |XKeysymForWindowsKeyCode()| here
nit: don't need pipes around function names in comments; i think they're usually
just used around variable names like |flags| to avoid ambiguity

https://codereview.chromium.org/378503007/diff/1/ui/events/keycodes/keyboard_...
File ui/events/keycodes/keyboard_code_conversion_x.h (right):

https://codereview.chromium.org/378503007/diff/1/ui/events/keycodes/keyboard_...
ui/events/keycodes/keyboard_code_conversion_x.h:34: unsigned int
XKeyEventKeyCode(ui::KeyboardCode key_code,
does this need to be exported like all the other functions in this file?

how about renaming it to XKeyCodeForWindowsKeyCode() and s/key_code/keycode/ so
it matches up with the XKeysymForWindowsKeyCode() function above it? (although
it'd probably be cleaner if "WindowsKeyCode" were replaced with "UiKeyboardCode"
or just "KeyboardCode" in both functions)

also, you don't need ui:: here; this is already in the ui namespace.

[kpschoedel, 2014-07-08 14:40:22]

https://codereview.chromium.org/378503007/diff/1/ui/events/keycodes/keyboard_...
File ui/events/keycodes/keyboard_code_conversion_x.cc (right):

https://codereview.chromium.org/378503007/diff/1/ui/events/keycodes/keyboard_...
ui/events/keycodes/keyboard_code_conversion_x.cc:1282: // SHIFT state is ignored
in the call to |XKeysymForWindowsKeyCode()| here
On 2014/07/07 21:07:01, Daniel Erat wrote:
> nit: don't need pipes around function names in comments; i think they're
usually
> just used around variable names like |flags| to avoid ambiguity

Done. Thanks, I'll follow that in the future.

https://codereview.chromium.org/378503007/diff/1/ui/events/keycodes/keyboard_...
File ui/events/keycodes/keyboard_code_conversion_x.h (right):

https://codereview.chromium.org/378503007/diff/1/ui/events/keycodes/keyboard_...
ui/events/keycodes/keyboard_code_conversion_x.h:34: unsigned int
XKeyEventKeyCode(ui::KeyboardCode key_code,
On 2014/07/07 21:07:01, Daniel Erat wrote:
> does this need to be exported like all the other functions in this file?
> 
> how about renaming it to XKeyCodeForWindowsKeyCode() and s/key_code/keycode/
so
> it matches up with the XKeysymForWindowsKeyCode() function above it? (although
> it'd probably be cleaner if "WindowsKeyCode" were replaced with
"UiKeyboardCode"
> or just "KeyboardCode" in both functions)
> 
> also, you don't need ui:: here; this is already in the ui namespace.

Done.

I have work in progress under crbug.com/380349 to identify the two different
interpretations of ui::KeyboardCode (sometimes it's a physical location,
sometimes a Latin-mapped interpretation, a la Windows) and consistently name
functions accordingly.

[Daniel Erat, 2014-07-08 14:49:31]

https://codereview.chromium.org/378503007/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/events/event_rewriter.cc (right):

https://codereview.chromium.org/378503007/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/events/event_rewriter.cc:402: xkey->keycode =
XKeysymForWindowsKeyCode(
s/XKeysym/XKeyCode/

https://codereview.chromium.org/378503007/diff/20001/ui/events/keycodes/keybo...
File ui/events/keycodes/keyboard_code_conversion_x.h (right):

https://codereview.chromium.org/378503007/diff/20001/ui/events/keycodes/keybo...
ui/events/keycodes/keyboard_code_conversion_x.h:34: EVENTS_BASE_EXPORT unsigned
int XKeysymForWindowsKeyCode(KeyboardCode key_code,
s/XKeysym/XKeyCode/

[kpschoedel, 2014-07-08 16:44:50]

https://codereview.chromium.org/378503007/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/events/event_rewriter.cc (right):

https://codereview.chromium.org/378503007/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/events/event_rewriter.cc:402: xkey->keycode =
XKeysymForWindowsKeyCode(
On 2014/07/08 14:49:31, Daniel Erat wrote:
> s/XKeysym/XKeyCode/

Drat. Nothing broke because it was an unambiguous overload.

[Daniel Erat, 2014-07-08 17:10:17]

lgtm

[sadrul, 2014-07-10 15:29:46]

+garykac@ for ui/events/keycodes/ changes

[kpschoedel, 2014-07-14 18:41:21]

garykac, PTAL under ui/events/keycodes

https://codereview.chromium.org/378503007/diff/40001/ui/events/keycodes/keybo...
File ui/events/keycodes/keyboard_code_conversion_x.cc (right):

https://codereview.chromium.org/378503007/diff/40001/ui/events/keycodes/keybo...
ui/events/keycodes/keyboard_code_conversion_x.cc:1279: unsigned int
XKeyCodeForWindowsKeyCode(ui::KeyboardCode key_code,
Function moved from ui/events/test/events_test_utils_x11.cc — no changes to
code, only comments.

[garykac, 2014-07-14 20:37:27]

lgtm

[sadrul, 2014-07-14 20:42:33]

lgtm

[kpschoedel, 2014-07-14 21:26:39]

The CQ bit was checked by kpschoedel@chromium.org

[commit-bot: I haz the power, 2014-07-14 21:27:47]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kpschoedel@chromium.org/378503007/40001

[commit-bot: I haz the power, 2014-07-14 21:58:21]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/9...)
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/20...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win8_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win8_chromium_rel/bui...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/28446)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/32862)

[commit-bot: I haz the power, 2014-07-14 22:00:40]

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

[commit-bot: I haz the power, 2014-07-14 22:00:41]

Try jobs failed on following builders:
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/28459)

[Shu Chen, 2014-07-15 04:23:55]

https://codereview.chromium.org/378503007/diff/40001/ui/events/keycodes/keybo...
File ui/events/keycodes/keyboard_code_conversion_x.cc (right):

https://codereview.chromium.org/378503007/diff/40001/ui/events/keycodes/keybo...
ui/events/keycodes/keyboard_code_conversion_x.cc:1302: return
XKeysymToKeycode(display, XKeysymForWindowsKeyCode(key_code, false));
XKeysymForWindowsKeyCode() method doesn't consider other latin language layouts,
especially for VKEY_OEM_[1-8].

Although I'm not sure whether it would be a big deal to make accurate conversion
from VKEY to keysym, I have a little concern for shortcut key "Ctrl+Alt+/",
which is to show the key mapping on screen.
On US layout, "/" key is VK_OEM_2; but on FR layout, VK_OEM_2 is "." key with
":" keysym.
And DE layout, VK_OEM_2 is "\" key with "#" keysym.

[kpschoedel, 2014-07-15 14:35:05]

https://codereview.chromium.org/378503007/diff/40001/ui/events/keycodes/keybo...
File ui/events/keycodes/keyboard_code_conversion_x.cc (right):

https://codereview.chromium.org/378503007/diff/40001/ui/events/keycodes/keybo...
ui/events/keycodes/keyboard_code_conversion_x.cc:1302: return
XKeysymToKeycode(display, XKeysymForWindowsKeyCode(key_code, false));
On 2014/07/15 04:23:55, Shu Chen wrote:
> XKeysymForWindowsKeyCode() method doesn't consider other latin language
layouts,
> especially for VKEY_OEM_[1-8].
> 
> Although I'm not sure whether it would be a big deal to make accurate
conversion
> from VKEY to keysym,

I agree. Changing the keyboard event rewriting to a ui::EventRewriter made it
necessary to map back to the X keycode, because some event consumers still need
it, but after crbug.com/380349 that should no longer be necessary.

[Shu Chen, 2014-07-15 14:42:12]

https://codereview.chromium.org/378503007/diff/40001/ui/events/keycodes/keybo...
File ui/events/keycodes/keyboard_code_conversion_x.cc (right):

https://codereview.chromium.org/378503007/diff/40001/ui/events/keycodes/keybo...
ui/events/keycodes/keyboard_code_conversion_x.cc:1302: return
XKeysymToKeycode(display, XKeysymForWindowsKeyCode(key_code, false));
Got it. I think it's ok for a temporary fix. And you don't need my lgtm so
removing myself from the reviews.


On 2014/07/15 14:35:05, kpschoedel wrote:
> On 2014/07/15 04:23:55, Shu Chen wrote:
> > XKeysymForWindowsKeyCode() method doesn't consider other latin language
> layouts,
> > especially for VKEY_OEM_[1-8].
> > 
> > Although I'm not sure whether it would be a big deal to make accurate
> conversion
> > from VKEY to keysym,
> 
> I agree. Changing the keyboard event rewriting to a ui::EventRewriter made it
> necessary to map back to the X keycode, because some event consumers still
need
> it, but after crbug.com/380349 that should no longer be necessary.

[kpschoedel, 2014-07-15 17:32:40]

The CQ bit was checked by kpschoedel@chromium.org

[commit-bot: I haz the power, 2014-07-15 17:36:10]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kpschoedel@chromium.org/378503007/60001

[commit-bot: I haz the power, 2014-07-16 04:19:28]

Change committed as 283328