Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(34)

Issue 1585193002: Build key map DomCodeToKey() for Windows (Closed)

Created:
4 years, 11 months ago by chongz
Modified:
4 years, 10 months ago
CC:
chromium-reviews
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

To convert DomCode+EventFlags to DomKey we cannot use windows' build-in API ToUnicode/ToUnicodeEx directly because it will consume the dead keys in keyboard buffer. This CL enumerates all key combinations and build a key map in advance using ToUnicodeEx, so future lookup won't go through keyboard buffer. Note that the lookup table only contains printable characters, so non-printable characters will still use the default fallback in KeyEvent::ApplyLayout(). BUG=564678 Committed: https://crrev.com/7c406f8ec5f2a67b66bf363ea4e7aff5e698da3b Cr-Commit-Position: refs/heads/master@{#375812}

Patch Set 1 #

Patch Set 2 : Cleaned API, added unittest #

Total comments: 18

Patch Set 3 : dtapuska's review, added more tests, handles non-printable key combination #

Total comments: 58

Patch Set 4 : Wez's review #

Total comments: 30

Patch Set 5 : wez's review 2 #

Total comments: 7

Patch Set 6 : dtapuska's review #

Total comments: 14

Patch Set 7 : Wez's review 3 #

Total comments: 16

Patch Set 8 : Wez's review 4 #

Total comments: 4

Patch Set 9 : Remove DCHECK from UpdateLayout #

Total comments: 2

Patch Set 10 : Fix unittest #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+461 lines, -0 lines) Patch
M ui/events/BUILD.gn View 1 2 3 4 5 6 2 chunks +3 lines, -0 lines 0 comments Download
M ui/events/event.cc View 1 2 3 4 5 6 7 2 chunks +6 lines, -0 lines 0 comments Download
M ui/events/events.gyp View 1 2 3 4 5 6 1 chunk +2 lines, -0 lines 0 comments Download
M ui/events/events_unittests.gyp View 1 2 3 4 5 6 1 chunk +1 line, -0 lines 0 comments Download
A ui/events/keycodes/platform_key_map_win.h View 1 2 3 4 5 6 7 1 chunk +59 lines, -0 lines 0 comments Download
A ui/events/keycodes/platform_key_map_win.cc View 1 2 3 4 5 6 7 8 9 1 chunk +216 lines, -0 lines 2 comments Download
A ui/events/keycodes/platform_key_map_win_unittest.cc View 1 2 3 4 5 6 1 chunk +174 lines, -0 lines 0 comments Download

Messages

Total messages: 51 (14 generated)
chongz
Please help me to check if I'm doing it in the right way. Also I'm ...
4 years, 11 months ago (2016-01-14 15:20:00 UTC) #3
chongz
Cleaned up API and added sample unit tests as per our discussion PTAL. TODO: Fix ...
4 years, 11 months ago (2016-01-21 00:39:56 UTC) #4
dtapuska
looking better; is this passing the dom key tests now? (before you devote more time ...
4 years, 11 months ago (2016-01-21 01:59:36 UTC) #5
chongz
Yes it passes the tests. Will work on the comments and build more test cases ...
4 years, 11 months ago (2016-01-21 15:47:40 UTC) #6
chongz
I've updated based on last review, and also 1. Added full tests for US keyboard ...
4 years, 11 months ago (2016-01-27 22:49:29 UTC) #7
dtapuska
looks good https://codereview.chromium.org/1585193002/diff/40001/ui/events/keycodes/keyboard_lookup_win.h File ui/events/keycodes/keyboard_lookup_win.h (right): https://codereview.chromium.org/1585193002/diff/40001/ui/events/keycodes/keyboard_lookup_win.h#newcode25 ui/events/keycodes/keyboard_lookup_win.h:25: static WindowsKeyboardLookup s_instance; I'd define this static ...
4 years, 10 months ago (2016-01-28 15:27:55 UTC) #9
Wez
Can we update the CL description to be a little clearer - "not break keyboard ...
4 years, 10 months ago (2016-01-28 18:45:33 UTC) #10
chongz
On 2016/01/28 18:45:33, Wez wrote: > Can we update the CL description to be a ...
4 years, 10 months ago (2016-01-28 19:50:24 UTC) #12
Wez
https://codereview.chromium.org/1585193002/diff/40001/ui/events/keycodes/keyboard_lookup_win.cc File ui/events/keycodes/keyboard_lookup_win.cc (right): https://codereview.chromium.org/1585193002/diff/40001/ui/events/keycodes/keyboard_lookup_win.cc#newcode15 ui/events/keycodes/keyboard_lookup_win.cc:15: const EventFlags event_flags_index[] = { nit: Use a compile-time ...
4 years, 10 months ago (2016-01-29 00:22:24 UTC) #13
dtapuska
https://codereview.chromium.org/1585193002/diff/40001/ui/events/keycodes/keyboard_lookup_win.h File ui/events/keycodes/keyboard_lookup_win.h (right): https://codereview.chromium.org/1585193002/diff/40001/ui/events/keycodes/keyboard_lookup_win.h#newcode23 ui/events/keycodes/keyboard_lookup_win.h:23: DomKey VirtualKeyToDomKey(KeyboardCode vkcode, int event_flags) const; On 2016/01/29 00:22:24, ...
4 years, 10 months ago (2016-01-29 17:05:49 UTC) #14
chongz
Thanks for the detailed review! I will start working on the fix... https://codereview.chromium.org/1585193002/diff/40001/ui/events/keycodes/keyboard_lookup_win.cc File ui/events/keycodes/keyboard_lookup_win.cc ...
4 years, 10 months ago (2016-01-29 20:25:50 UTC) #15
chongz
Hi Wez, I've fixed the CL based on your review, PTAL. Thanks! https://codereview.chromium.org/1585193002/diff/40001/ui/events/keycodes/keyboard_lookup_win.cc File ui/events/keycodes/keyboard_lookup_win.cc ...
4 years, 10 months ago (2016-02-02 21:14:04 UTC) #16
dtapuska
https://codereview.chromium.org/1585193002/diff/60001/ui/events/keycodes/key_map_win.cc File ui/events/keycodes/key_map_win.cc (right): https://codereview.chromium.org/1585193002/diff/60001/ui/events/keycodes/key_map_win.cc#newcode129 ui/events/keycodes/key_map_win.cc:129: DomCode WindowsKeyMap::KeyboardCodeToDomCode(KeyboardCode key_code) const { Is this really required? ...
4 years, 10 months ago (2016-02-04 20:31:46 UTC) #18
chongz
Hi Wez, can I have a review on the updated CL please? Thanks! https://codereview.chromium.org/1585193002/diff/60001/ui/events/keycodes/key_map_win.cc File ...
4 years, 10 months ago (2016-02-04 21:10:26 UTC) #19
Wez
https://codereview.chromium.org/1585193002/diff/60001/ui/events/keycodes/key_map_win.cc File ui/events/keycodes/key_map_win.cc (right): https://codereview.chromium.org/1585193002/diff/60001/ui/events/keycodes/key_map_win.cc#newcode31 ui/events/keycodes/key_map_win.cc:31: const EventFlags flags_index[] = { nit: This is the ...
4 years, 10 months ago (2016-02-09 00:18:58 UTC) #20
dtapuska
https://codereview.chromium.org/1585193002/diff/60001/ui/events/keycodes/key_map_win.cc File ui/events/keycodes/key_map_win.cc (right): https://codereview.chromium.org/1585193002/diff/60001/ui/events/keycodes/key_map_win.cc#newcode141 ui/events/keycodes/key_map_win.cc:141: HKL current_layout = ::GetKeyboardLayout(0); On 2016/02/09 00:18:57, Wez wrote: ...
4 years, 10 months ago (2016-02-09 16:46:39 UTC) #21
chongz
On 2016/02/09 16:46:39, dtapuska wrote: > https://codereview.chromium.org/1585193002/diff/60001/ui/events/keycodes/key_map_win.cc > File ui/events/keycodes/key_map_win.cc (right): > > https://codereview.chromium.org/1585193002/diff/60001/ui/events/keycodes/key_map_win.cc#newcode141 > ...
4 years, 10 months ago (2016-02-09 19:42:12 UTC) #22
Wez
https://codereview.chromium.org/1585193002/diff/60001/ui/events/keycodes/key_map_win.cc File ui/events/keycodes/key_map_win.cc (right): https://codereview.chromium.org/1585193002/diff/60001/ui/events/keycodes/key_map_win.cc#newcode141 ui/events/keycodes/key_map_win.cc:141: HKL current_layout = ::GetKeyboardLayout(0); On 2016/02/09 at 16:46:39, dtapuska ...
4 years, 10 months ago (2016-02-09 22:40:01 UTC) #23
chongz
Hi wez, I've updated the CL, PTAL, thanks! https://codereview.chromium.org/1585193002/diff/80001/ui/events/keycodes/key_map_win.cc File ui/events/keycodes/key_map_win.cc (right): https://codereview.chromium.org/1585193002/diff/80001/ui/events/keycodes/key_map_win.cc#newcode86 ui/events/keycodes/key_map_win.cc:86: static ...
4 years, 10 months ago (2016-02-10 01:25:10 UTC) #24
dtapuska
https://codereview.chromium.org/1585193002/diff/80001/ui/events/keycodes/key_map_win.cc File ui/events/keycodes/key_map_win.cc (right): https://codereview.chromium.org/1585193002/diff/80001/ui/events/keycodes/key_map_win.cc#newcode86 ui/events/keycodes/key_map_win.cc:86: static base::ThreadLocalStorage::StaticSlot s_tls_key_map = TLS_INITIALIZER; On 2016/02/10 01:25:10, chongz ...
4 years, 10 months ago (2016-02-10 21:56:34 UTC) #25
chongz
Changed to use LazyInstance+TLS. PTAL, thanks!
4 years, 10 months ago (2016-02-11 16:57:06 UTC) #26
Wez
https://codereview.chromium.org/1585193002/diff/100001/ui/events/event.cc File ui/events/event.cc (right): https://codereview.chromium.org/1585193002/diff/100001/ui/events/event.cc#newcode41 ui/events/event.cc:41: #include "ui/events/keycodes/key_map_win.h" key_map_win.{cc|h} should be renamed to match the ...
4 years, 10 months ago (2016-02-11 23:14:45 UTC) #27
chongz
Hi Wez, I've renamed the file and moved typedefs inside. PTAL, thanks! https://codereview.chromium.org/1585193002/diff/120001/ui/events/keycodes/platform_key_map_win.cc File ui/events/keycodes/platform_key_map_win.cc ...
4 years, 10 months ago (2016-02-12 16:04:03 UTC) #28
Wez
LGTM once the remaining nits are dealt with. https://codereview.chromium.org/1585193002/diff/120001/ui/events/keycodes/platform_key_map_win.cc File ui/events/keycodes/platform_key_map_win.cc (right): https://codereview.chromium.org/1585193002/diff/120001/ui/events/keycodes/platform_key_map_win.cc#newcode92 ui/events/keycodes/platform_key_map_win.cc:92: // ...
4 years, 10 months ago (2016-02-12 21:29:38 UTC) #29
chongz
Hi sadrul, can I have a review on ui/events/event.cc please? Hi dtapuska, can you take ...
4 years, 10 months ago (2016-02-16 20:18:59 UTC) #31
dtapuska
On 2016/02/16 20:18:59, chongz wrote: > Hi sadrul, can I have a review on ui/events/event.cc ...
4 years, 10 months ago (2016-02-16 20:24:59 UTC) #32
Wez
https://codereview.chromium.org/1585193002/diff/140001/ui/events/keycodes/platform_key_map_win.cc File ui/events/keycodes/platform_key_map_win.cc (right): https://codereview.chromium.org/1585193002/diff/140001/ui/events/keycodes/platform_key_map_win.cc#newcode100 ui/events/keycodes/platform_key_map_win.cc:100: return new (instance) base::ThreadLocalStorage::Slot(CleanupKeyMapTls); On 2016/02/16 at 20:18:59, chongz ...
4 years, 10 months ago (2016-02-16 22:25:49 UTC) #33
sadrul
stamp lgtm
4 years, 10 months ago (2016-02-17 00:20:26 UTC) #34
chongz
https://codereview.chromium.org/1585193002/diff/160001/ui/events/keycodes/platform_key_map_win.cc File ui/events/keycodes/platform_key_map_win.cc (right): https://codereview.chromium.org/1585193002/diff/160001/ui/events/keycodes/platform_key_map_win.cc#newcode100 ui/events/keycodes/platform_key_map_win.cc:100: return new (instance) base::ThreadLocalStorage::Slot(CleanupKeyMapTls); Still haven't find a good ...
4 years, 10 months ago (2016-02-17 01:21:53 UTC) #35
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1585193002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1585193002/160001
4 years, 10 months ago (2016-02-17 02:56:32 UTC) #38
commit-bot: I haz the power
Try jobs failed on following builders: win8_chromium_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/builds/104295)
4 years, 10 months ago (2016-02-17 03:50:45 UTC) #40
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1585193002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1585193002/180001
4 years, 10 months ago (2016-02-17 05:00:24 UTC) #43
commit-bot: I haz the power
Committed patchset #10 (id:180001)
4 years, 10 months ago (2016-02-17 06:35:08 UTC) #45
commit-bot: I haz the power
Patchset 10 (id:??) landed as https://crrev.com/7c406f8ec5f2a67b66bf363ea4e7aff5e698da3b Cr-Commit-Position: refs/heads/master@{#375812}
4 years, 10 months ago (2016-02-17 06:36:34 UTC) #47
brucedawson
https://codereview.chromium.org/1585193002/diff/180001/ui/events/keycodes/platform_key_map_win.cc File ui/events/keycodes/platform_key_map_win.cc (right): https://codereview.chromium.org/1585193002/diff/180001/ui/events/keycodes/platform_key_map_win.cc#newcode172 ui/events/keycodes/platform_key_map_win.cc:172: ::GetKeyboardState(keyboard_state_to_restore); The /analyze builder points out that this function ...
4 years, 10 months ago (2016-02-19 19:17:54 UTC) #49
Wez
https://codereview.chromium.org/1585193002/diff/180001/ui/events/keycodes/platform_key_map_win.cc File ui/events/keycodes/platform_key_map_win.cc (right): https://codereview.chromium.org/1585193002/diff/180001/ui/events/keycodes/platform_key_map_win.cc#newcode172 ui/events/keycodes/platform_key_map_win.cc:172: ::GetKeyboardState(keyboard_state_to_restore); On 2016/02/19 at 19:17:54, brucedawson wrote: > The ...
4 years, 10 months ago (2016-02-19 21:26:42 UTC) #50
chongz
4 years, 10 months ago (2016-02-19 21:34:07 UTC) #51
Message was sent while issue was closed.
On 2016/02/19 21:26:42, Wez wrote:
>
https://codereview.chromium.org/1585193002/diff/180001/ui/events/keycodes/pla...
> File ui/events/keycodes/platform_key_map_win.cc (right):
> 
>
https://codereview.chromium.org/1585193002/diff/180001/ui/events/keycodes/pla...
> ui/events/keycodes/platform_key_map_win.cc:172:
> ::GetKeyboardState(keyboard_state_to_restore);
> On 2016/02/19 at 19:17:54, brucedawson wrote:
> > The /analyze builder points out that this function can fail, and if it does
> then keyboard_state_to_restore will be uninitialized, and read from.
> > 
> > I don't know whether GetKeyboardState() can *actually* fail - /analyze is
> notoriously unreliable/uninformative on this sort of information - but perhaps
> an if (!GetKeyboardState()) return; would be a good idea?
> 
> Agreed that that seems a sensible precaution - at least then we'll see
messed-up
> keyboard events rather than some other subtle bug.
> 
> Might it even be appropriate to CHECK if we're asserting that it can never
fail?

Sure, the CL is here http://crrev.com/1716913002
PTAL, thanks!

Powered by Google App Engine
This is Rietveld 408576698