[DomKey] Use VK->DomKey mapping for non-printable keys

ui/events/keycodes/platform_key_map_win_unittest.cc

Index: ui/events/keycodes/platform_key_map_win_unittest.cc
diff --git a/ui/events/keycodes/platform_key_map_win_unittest.cc b/ui/events/keycodes/platform_key_map_win_unittest.cc
index 44cbd58671528618ed25d93cee00ba9f4dfe800f..b43ee9a40159058a3ee5b62a5a88b71c27441b18 100644
--- a/ui/events/keycodes/platform_key_map_win_unittest.cc
+++ b/ui/events/keycodes/platform_key_map_win_unittest.cc
@@ -247,4 +247,162 @@ TEST_F(PlatformKeyMapTest, NumPad) {
   }
 }
 
+TEST_F(PlatformKeyMapTest, NonPrintableKey) {
+  HKL layout = ::LoadKeyboardLayout(LAYOUT_US, 0);
+  PlatformKeyMap keymap(layout);
+
+  // This table is a copy of |NonPrintableKeyEntry|, which must be sorted by
+  // |key_code|.

[Wez, 2016/06/01 20:49:04]

It doesn't seem to be all that useful to test the production version of the
table against something that is just a direct copy of that table (and which must
be kept up-to-date with it!).

I'd suggest instead iterating over the scancodes and calculating (1) the
corresponding VKEY under the US layout and (2) the DomKey that it maps to
according to DomKeyFromNativeImpl - then you can re-run the DomKeyFromNativeImpl
calls but this time just pass in VKEY codes with bogus scancodes, and verify
that the function returns the same results.

It wouldn't be a strict test that the entire mapping is valid, but it'd be more
meaningful, since you'd be verifying the key property that the scancode can be
omitted or mismatched, and the VKEY will be used instead.

[chongz, 2016/06/01 23:09:42]

If I understand correctly you are suggesting using
|DomCodeToUsLayoutDomKey()|(dom_code=>key) to test against
|DomKeyFromNativeImpl()|(VK=>key)?

Currently we rely on fallback |DomCodeToUsLayoutDomKey()| for non-printable
keys, and this CL is trying to add a VK->DomKey map to get rid of the fallback.

To make the process more clear can I iterating over the |dom_code| in
|dom_us_layout_data.h::kNonPrintableCodeMap[]|, and map the |dom_code| to
|scan_code| then US layout |VK|?

I think ideally |DomKeyFromNativeImpl()| should only use |VK+flags|, even if
|VK| is missing we should map |scan_code| to |VK| and still use the VK->DomKey
table.

Note: |DomCodeToUsLayoutDomKey()| has issue on US layout,
e.g. DomCode::PAUSE+Ctrl should map to DomKey::CANCEL, but actually it does not
consider modifiers.
But for testing purpose it should be fine since it covers DomCode::ABORT, which
will hopefully be mapped to VKEY_CANCEL and DomKey::CANCEL.

[Wez, 2016/06/03 00:00:51]

Something along those lines, yes.
Right and we expect DomCodeToUsLayoutDomKey() to match what we would expect the
platform to do, so it seems reasonable to use it to do limited (just to US
English layouts) tests to verify our platform-specific handling, given that we
have scancode<->DomCode and can use MapVirtualKey for scancode<->VKEY under the
US layout.
Yes, I think that makes sense.
Agreed - we should be able to do that as soon as the printable-character map is
switched over to be keyed off of VK+flags :)
Ah yes - perhaps we can patch that up in a follow-up CL, or at least add a note
that it is broken?

+  const struct TestCase {
+    KeyboardCode key_code;
+    DomKey key;
+  } kNonPrintableKeyTestCase[] = {
+      {VKEY_CANCEL, DomKey::CANCEL},
+      {VKEY_BACK, DomKey::BACKSPACE},
+      {VKEY_TAB, DomKey::TAB},
+      {VKEY_CLEAR, DomKey::CLEAR},
+      {VKEY_RETURN, DomKey::ENTER},
+      {VKEY_SHIFT, DomKey::SHIFT},
+      {VKEY_CONTROL, DomKey::CONTROL},
+      {VKEY_MENU, DomKey::ALT},
+      {VKEY_PAUSE, DomKey::PAUSE},
+      {VKEY_CAPITAL, DomKey::CAPS_LOCK},
+      // TODO(chongz): Map VKEY_KANA/VKEY_HANGUL based on layout.
+      // https://crbug.com/612736
+      {VKEY_KANA, DomKey::KANA_MODE},
+      // {VKEY_HANGUL, DomKey::HANGUL_MODE},
+      {VKEY_JUNJA, DomKey::JUNJA_MODE},
+      {VKEY_FINAL, DomKey::FINAL_MODE},
+      // TODO(chongz): Map VKEY_HANJA/VKEY_KANJI based on layout.
+      // https://crbug.com/612736
+      {VKEY_HANJA, DomKey::HANJA_MODE},
+      // {VKEY_KANJI, DomKey::KANJI_MODE},
+      {VKEY_ESCAPE, DomKey::ESCAPE},
+      {VKEY_CONVERT, DomKey::CONVERT},
+      {VKEY_NONCONVERT, DomKey::NON_CONVERT},
+      {VKEY_ACCEPT, DomKey::ACCEPT},
+      {VKEY_MODECHANGE, DomKey::MODE_CHANGE},
+      // VKEY_SPACE
+      {VKEY_PRIOR, DomKey::PAGE_UP},
+      {VKEY_NEXT, DomKey::PAGE_DOWN},
+      {VKEY_END, DomKey::END},
+      {VKEY_HOME, DomKey::HOME},
+      {VKEY_LEFT, DomKey::ARROW_LEFT},
+      {VKEY_UP, DomKey::ARROW_UP},
+      {VKEY_RIGHT, DomKey::ARROW_RIGHT},
+      {VKEY_DOWN, DomKey::ARROW_DOWN},
+      {VKEY_SELECT, DomKey::SELECT},
+      {VKEY_PRINT, DomKey::PRINT},
+      {VKEY_EXECUTE, DomKey::EXECUTE},
+      {VKEY_SNAPSHOT, DomKey::PRINT_SCREEN},
+      {VKEY_INSERT, DomKey::INSERT},
+      {VKEY_DELETE, DomKey::DEL},
+      {VKEY_HELP, DomKey::HELP},
+      // VKEY_0..9
+      // VKEY_A..Z
+      {VKEY_LWIN, DomKey::META},
+      // VKEY_COMMAND - The same as VKEY_LWIN.
+      {VKEY_RWIN, DomKey::META},
+      {VKEY_APPS, DomKey::CONTEXT_MENU},
+      {VKEY_SLEEP, DomKey::STANDBY},
+      // VKEY_NUMPAD0..9 - Handled by |NumPadKeyCodeToDomKey()|.
+      // VKEY_MULTIPLY, VKEY_ADD, VKEY_SEPARATOR, VKEY_SUBTRACT, VKEY_DECIMAL,
+      // VKEY_DIVIDE
+      {VKEY_F1, DomKey::F1},
+      {VKEY_F2, DomKey::F2},
+      {VKEY_F3, DomKey::F3},
+      {VKEY_F4, DomKey::F4},
+      {VKEY_F5, DomKey::F5},
+      {VKEY_F6, DomKey::F6},
+      {VKEY_F7, DomKey::F7},
+      {VKEY_F8, DomKey::F8},
+      {VKEY_F9, DomKey::F9},
+      {VKEY_F10, DomKey::F10},
+      {VKEY_F11, DomKey::F11},
+      {VKEY_F12, DomKey::F12},
+      {VKEY_F13, DomKey::F13},
+      {VKEY_F14, DomKey::F14},
+      {VKEY_F15, DomKey::F15},
+      {VKEY_F16, DomKey::F16},
+      {VKEY_F17, DomKey::F17},
+      {VKEY_F18, DomKey::F18},
+      {VKEY_F19, DomKey::F19},
+      {VKEY_F20, DomKey::F20},
+      {VKEY_F21, DomKey::F21},
+      {VKEY_F22, DomKey::F22},
+      {VKEY_F23, DomKey::F23},
+      {VKEY_F24, DomKey::F24},
+      {VKEY_NUMLOCK, DomKey::NUM_LOCK},
+      {VKEY_SCROLL, DomKey::SCROLL_LOCK},
+      {VKEY_LSHIFT, DomKey::SHIFT},
+      {VKEY_RSHIFT, DomKey::SHIFT},
+      {VKEY_LCONTROL, DomKey::CONTROL},
+      {VKEY_RCONTROL, DomKey::CONTROL},
+      {VKEY_LMENU, DomKey::ALT},
+      {VKEY_RMENU, DomKey::ALT},
+      {VKEY_BROWSER_BACK, DomKey::BROWSER_BACK},
+      {VKEY_BROWSER_FORWARD, DomKey::BROWSER_FORWARD},
+      {VKEY_BROWSER_REFRESH, DomKey::BROWSER_REFRESH},
+      {VKEY_BROWSER_STOP, DomKey::BROWSER_STOP},
+      {VKEY_BROWSER_SEARCH, DomKey::BROWSER_SEARCH},
+      {VKEY_BROWSER_FAVORITES, DomKey::BROWSER_FAVORITES},
+      {VKEY_BROWSER_HOME, DomKey::BROWSER_HOME},
+      {VKEY_VOLUME_MUTE, DomKey::AUDIO_VOLUME_MUTE},
+      {VKEY_VOLUME_DOWN, DomKey::AUDIO_VOLUME_DOWN},
+      {VKEY_VOLUME_UP, DomKey::AUDIO_VOLUME_UP},
+      {VKEY_MEDIA_NEXT_TRACK, DomKey::MEDIA_TRACK_NEXT},
+      {VKEY_MEDIA_PREV_TRACK, DomKey::MEDIA_TRACK_PREVIOUS},
+      {VKEY_MEDIA_STOP, DomKey::MEDIA_STOP},
+      {VKEY_MEDIA_PLAY_PAUSE, DomKey::MEDIA_PLAY_PAUSE},
+      {VKEY_MEDIA_LAUNCH_MAIL, DomKey::LAUNCH_MAIL},
+      {VKEY_MEDIA_LAUNCH_MEDIA_SELECT, DomKey::LAUNCH_MEDIA_PLAYER},
+      // TODO(chongz): Actually we don't know whether it's mapped to launch
+      // computer/calculator or not, better use a generic DomKey string.
+      // https://crbug.com/612743
+      {VKEY_MEDIA_LAUNCH_APP1, DomKey::LAUNCH_MY_COMPUTER},
+      {VKEY_MEDIA_LAUNCH_APP2, DomKey::LAUNCH_CALCULATOR},
+      // VKEY_OEM_1..8, 102, PLUS, COMMA, MINUS, PERIOD
+      {VKEY_ALTGR, DomKey::ALT_GRAPH},
+      {VKEY_PROCESSKEY, DomKey::PROCESS},
+      // VKEY_PACKET - Used to pass Unicode char, considered as printable key.
+
+      // TODO(chongz): Handle Japanese keyboard layout keys 0xF0..0xF5.
+      // https://crbug.com/612694
+      {VKEY_ATTN, DomKey::ATTN},
+      {VKEY_CRSEL, DomKey::CR_SEL},
+      {VKEY_EXSEL, DomKey::EX_SEL},
+      {VKEY_EREOF, DomKey::ERASE_EOF},
+      {VKEY_PLAY, DomKey::PLAY},
+      {VKEY_ZOOM, DomKey::ZOOM_TOGGLE},
+      // TODO(chongz): Handle VKEY_NONAME, VKEY_PA1
+      {VKEY_OEM_CLEAR, DomKey::CLEAR},
+  };
+
+  for (size_t i = 1; i < arraysize(kNonPrintableKeyTestCase); ++i) {

[Wez, 2016/06/01 20:49:04]

You're testing that your test-case is well-ordered, which doesn't seem to
actually be a useful property to your test - you really want to test that the
original table is well-ordered, surely?

[chongz, 2016/06/01 23:09:42]

Yes, because it must be ordered for binary search, and I didn't find a way to do
it using |static_assert<>|...

[Wez, 2016/06/03 00:00:51]

Right; my point was just that testing that a copy of the table is well-ordered,
rather than the table itself, may not be very helpful!

+    EXPECT_TRUE(kNonPrintableKeyTestCase[i - 1].key_code <
+                kNonPrintableKeyTestCase[i].key_code)
+        << kNonPrintableKeyTestCase[i].key_code;
+  }
+
+  for (const auto& test_case : kNonPrintableKeyTestCase) {
+    KeyboardCode key_code = test_case.key_code;
+    // We only use |key_code| for non-printable keys.
+    DomCode dom_code = DomCode::NONE;
+
+    EXPECT_EQ(test_case.key,
+              DomKeyFromNativeImpl(keymap, dom_code, key_code, EF_NONE))
+        << key_code;
+    EXPECT_EQ(test_case.key,
+              DomKeyFromNativeImpl(keymap, dom_code, key_code, EF_ALTGR_DOWN))
+        << key_code;
+    EXPECT_EQ(test_case.key,
+              DomKeyFromNativeImpl(keymap, dom_code, key_code, EF_CONTROL_DOWN))
+        << key_code;
+    EXPECT_EQ(test_case.key,
+              DomKeyFromNativeImpl(keymap, dom_code, key_code,
+                                   EF_ALTGR_DOWN | EF_CONTROL_DOWN))
+        << key_code;
+  }
+}
+
 }  // namespace ui