Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2046193002:
[DomKey] Expose Korean special keys on Korean keyboard layout only (Closed)
Description[DomKey] Expose Korean special keys on Korean keyboard layout only
Korean keyboard has special keys "HangulMode" and "HanjaMode", we should
only expose them when the system layout is Korean. Otherwise we should
return "Unidentified".
This matches Firefox and windows virtual key.
Note: This also works with JIS keyboard + Korean layout, because we are
mapping DomKey based on KeyboardCode instead of DomCode.
BUG=612736
Committed: https://crrev.com/bdcfb5a1816f1bad537d7611c7ccb972b550339c
Cr-Commit-Position: refs/heads/master@{#399783}
Patch Set 1 #
Total comments: 16
Patch Set 2 : Wez's review #
Messages
Total messages: 12 (4 generated)
chongz@chromium.org changed reviewers: + wez@chromium.org
wez@ PTAL at this Korean DomKey CL, thanks!
lgtm https://codereview.chromium.org/2046193002/diff/1/ui/events/keycodes/platform... File ui/events/keycodes/platform_key_map_win.cc (right): https://codereview.chromium.org/2046193002/diff/1/ui/events/keycodes/platform... ui/events/keycodes/platform_key_map_win.cc:258: HKL layout) { nit: I'd suggest calling this LanguageSpecificOemKeyboardCodeToDomKey(). It'd also help to have a comment e.g. "Disambiguates the meaning of certain non-printable keys which have different meanings under different languages, but use the same VKEY code." https://codereview.chromium.org/2046193002/diff/1/ui/events/keycodes/platform... ui/events/keycodes/platform_key_map_win.cc:259: WORD langID = LOWORD(layout); nit: langID->language_id, or just |language| https://codereview.chromium.org/2046193002/diff/1/ui/events/keycodes/platform... ui/events/keycodes/platform_key_map_win.cc:260: WORD primaryLangID = PRIMARYLANGID(langID); nit: primary_language_id or just |primary_language| https://codereview.chromium.org/2046193002/diff/1/ui/events/keycodes/platform... ui/events/keycodes/platform_key_map_win.cc:262: // https://crbug.com/612694 nit: I'd suggest putting this comment after the Korean "if" block, otherwise it looks like the comment refers to this block. https://codereview.chromium.org/2046193002/diff/1/ui/events/keycodes/platform... ui/events/keycodes/platform_key_map_win.cc:267: case VKEY_HANJA: // Same as VKEY_KANJI. Do these comments make sense in the context of Korean - VKEY_KANA and KANJI are Japanese-layout names for those OEM values, right? https://codereview.chromium.org/2046193002/diff/1/ui/events/keycodes/platform... ui/events/keycodes/platform_key_map_win.cc:277: // 1. Look up general key map. nit: Suggest "Most |key_codes| have the same meaning regardless of |layout|." and then "Look up a |layout|-specific meaning for |key_code|." below. https://codereview.chromium.org/2046193002/diff/1/ui/events/keycodes/platform... File ui/events/keycodes/platform_key_map_win_unittest.cc (right): https://codereview.chromium.org/2046193002/diff/1/ui/events/keycodes/platform... ui/events/keycodes/platform_key_map_win_unittest.cc:291: TEST_F(PlatformKeyMapTest, KoreanSpecialDomKey) { nit: Suggest just KoreanDomKey or KoreanSpecificKeys https://codereview.chromium.org/2046193002/diff/1/ui/events/keycodes/platform... ui/events/keycodes/platform_key_map_win_unittest.cc:299: // US layout does not have these keys. nit: "US English should not return values for these keys."
https://codereview.chromium.org/2046193002/diff/1/ui/events/keycodes/platform... File ui/events/keycodes/platform_key_map_win.cc (right): https://codereview.chromium.org/2046193002/diff/1/ui/events/keycodes/platform... ui/events/keycodes/platform_key_map_win.cc:258: HKL layout) { On 2016/06/10 22:55:09, Wez wrote: > nit: I'd suggest calling this LanguageSpecificOemKeyboardCodeToDomKey(). > > It'd also help to have a comment e.g. "Disambiguates the meaning of certain > non-printable keys which have different meanings under different languages, but > use the same VKEY code." Done. https://codereview.chromium.org/2046193002/diff/1/ui/events/keycodes/platform... ui/events/keycodes/platform_key_map_win.cc:259: WORD langID = LOWORD(layout); On 2016/06/10 22:55:09, Wez wrote: > nit: langID->language_id, or just |language| Done. https://codereview.chromium.org/2046193002/diff/1/ui/events/keycodes/platform... ui/events/keycodes/platform_key_map_win.cc:260: WORD primaryLangID = PRIMARYLANGID(langID); On 2016/06/10 22:55:09, Wez wrote: > nit: primary_language_id or just |primary_language| Done. https://codereview.chromium.org/2046193002/diff/1/ui/events/keycodes/platform... ui/events/keycodes/platform_key_map_win.cc:262: // https://crbug.com/612694 On 2016/06/10 22:55:09, Wez wrote: > nit: I'd suggest putting this comment after the Korean "if" block, otherwise it > looks like the comment refers to this block. Done. https://codereview.chromium.org/2046193002/diff/1/ui/events/keycodes/platform... ui/events/keycodes/platform_key_map_win.cc:267: case VKEY_HANJA: // Same as VKEY_KANJI. On 2016/06/10 22:55:09, Wez wrote: > Do these comments make sense in the context of Korean - VKEY_KANA and KANJI are > Japanese-layout names for those OEM values, right? Done. https://codereview.chromium.org/2046193002/diff/1/ui/events/keycodes/platform... ui/events/keycodes/platform_key_map_win.cc:277: // 1. Look up general key map. On 2016/06/10 22:55:09, Wez wrote: > nit: Suggest "Most |key_codes| have the same meaning regardless of |layout|." > and then "Look up a |layout|-specific meaning for |key_code|." below. Done. https://codereview.chromium.org/2046193002/diff/1/ui/events/keycodes/platform... File ui/events/keycodes/platform_key_map_win_unittest.cc (right): https://codereview.chromium.org/2046193002/diff/1/ui/events/keycodes/platform... ui/events/keycodes/platform_key_map_win_unittest.cc:291: TEST_F(PlatformKeyMapTest, KoreanSpecialDomKey) { On 2016/06/10 22:55:09, Wez wrote: > nit: Suggest just KoreanDomKey or KoreanSpecificKeys Done. https://codereview.chromium.org/2046193002/diff/1/ui/events/keycodes/platform... ui/events/keycodes/platform_key_map_win_unittest.cc:299: // US layout does not have these keys. On 2016/06/10 22:55:09, Wez wrote: > nit: "US English should not return values for these keys." Done.
The CQ bit was checked by chongz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from wez@chromium.org Link to the patchset: https://codereview.chromium.org/2046193002/#ps20001 (title: "Wez's review")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2046193002/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
CQ bit was unchecked
Message was sent while issue was closed.
Description was changed from ========== [DomKey] Expose Korean special keys on Korean keyboard layout only Korean keyboard has special keys "HangulMode" and "HanjaMode", we should only expose them when the system layout is Korean. Otherwise we should return "Unidentified". This matches Firefox and windows virtual key. Note: This also works with JIS keyboard + Korean layout, because we are mapping DomKey based on KeyboardCode instead of DomCode. BUG=612736 ========== to ========== [DomKey] Expose Korean special keys on Korean keyboard layout only Korean keyboard has special keys "HangulMode" and "HanjaMode", we should only expose them when the system layout is Korean. Otherwise we should return "Unidentified". This matches Firefox and windows virtual key. Note: This also works with JIS keyboard + Korean layout, because we are mapping DomKey based on KeyboardCode instead of DomCode. BUG=612736 Committed: https://crrev.com/bdcfb5a1816f1bad537d7611c7ccb972b550339c Cr-Commit-Position: refs/heads/master@{#399783} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/bdcfb5a1816f1bad537d7611c7ccb972b550339c Cr-Commit-Position: refs/heads/master@{#399783}
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:20001) has been created in https://codereview.chromium.org/2061123005/ by tommycli@chromium.org. The reason for reverting is: Breaks Windows Unit DrMemory tests. Sorry. https://build.chromium.org/p/chromium.memory.fyi/builders/Windows%20Unit%20(D... First breaking build: https://build.chromium.org/p/chromium.memory.fyi/builders/Windows%20Unit%20(D... Error: UNADDRESSABLE ACCESS: reading 0x00001d2c-0x00001d30 4 byte(s) within 0x00001d2c-0x00001d30 # 0 system call NtUserLoadKeyboardLayoutEx UNICODE_STRING.Buffer # 1 USER32.dll!SetSystemCursor +0x6c (0x75780272 <USER32.dll+0x70272>) # 2 USER32.dll!OpenWindowStationW +0xa7b (0x7576babf <USER32.dll+0x5babf>) # 3 USER32.dll!LoadKeyboardLayoutW +0x14 (0x7576bb2c <USER32.dll+0x5bb2c>) # 4 ui::PlatformKeyMapTest_KoreanSpecificKeys_Test::TestBody [ui\events\keycodes\platform_key_map_win_unittest.cc:309] # 5 testing::internal::HandleExceptionsInMethodIfSupported<> [testing\gtest\src\gtest.cc:2458] Note: @0:00:18.236 in thread 3168 Suppression (error hash=#54506E6D47A9FB57#): For more info on using suppressions see http://dev.chromium.org/developers/how-tos/using-drmemory#TOC-Suppressing-err... { UNADDRESSABLE ACCESS name=<insert_a_suppression_name_here> system call NtUserLoadKeyboardLayoutEx UNICODE_STRING.Buffer USER32.dll!SetSystemCursor USER32.dll!OpenWindowStationW USER32.dll!LoadKeyboardLayoutW *!ui::PlatformKeyMapTest_KoreanSpecificKeys_Test::TestBody *!testing::internal::HandleExceptionsInMethodIfSupported<> } UNADDRESSABLE ACCESS: reading 0x00001d28-0x00001d2a 2 byte(s) within 0x00001d28-0x00001d2a # 0 system call NtUserLoadKeyboardLayoutEx UNICODE_STRING.Length # 1 USER32.dll!SetSystemCursor +0x6c (0x75780272 <USER32.dll+0x70272>) # 2 USER32.dll!OpenWindowStationW +0xa7b (0x7576babf <USER32.dll+0x5babf>) # 3 USER32.dll!LoadKeyboardLayoutW +0x14 (0x7576bb2c <USER32.dll+0x5bb2c>) # 4 ui::PlatformKeyMapTest_KoreanSpecificKeys_Test::TestBody [ui\events\keycodes\platform_key_map_win_unittest.cc:309] # 5 testing::internal::HandleExceptionsInMethodIfSupported<> [testing\gtest\src\gtest.cc:2458] Note: @0:00:18.236 in thread 3168 Suppression (error hash=#EBB09B57A8C7AF19#): For more info on using suppressions see http://dev.chromium.org/developers/how-tos/using-drmemory#TOC-Suppressing-err... { UNADDRESSABLE ACCESS name=<insert_a_suppression_name_here> system call NtUserLoadKeyboardLayoutEx UNICODE_STRING.Length USER32.dll!SetSystemCursor USER32.dll!OpenWindowStationW USER32.dll!LoadKeyboardLayoutW *!ui::PlatformKeyMapTest_KoreanSpecificKeys_Test::TestBody *!testing::internal::HandleExceptionsInMethodIfSupported<> }. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
