Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1585193002:
Build key map DomCodeToKey() for Windows (Closed)
DescriptionTo 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
Messages
Total messages: 51 (14 generated)
Description was changed from ========== Added WindowsKeyboardLookup class BUG=564678 ========== to ========== Map virtual keys to unicode characters without breaking keyboard buffer. Use static methods WindowsKeyboardLookup::ToUnicode WindowsKeyboardLookup::ToUnicodeWithLayout BUG=564678 ==========
chongz@chromium.org changed reviewers: + dtapuska@chromium.org
Please help me to check if I'm doing it in the right way. Also I'm still working on the tests.
Cleaned up API and added sample unit tests as per our discussion PTAL. TODO: Fix numlock and add more test cases
looking better; is this passing the dom key tests now? (before you devote more time to the unit tests we want to make sure it is working correctly for dom key) https://codereview.chromium.org/1585193002/diff/20001/ui/events/events.gyp File ui/events/events.gyp (left): https://codereview.chromium.org/1585193002/diff/20001/ui/events/events.gyp#ol... ui/events/events.gyp:388: } any idea why this line changed? https://codereview.chromium.org/1585193002/diff/20001/ui/events/events.gyp File ui/events/events.gyp (right): https://codereview.chromium.org/1585193002/diff/20001/ui/events/events.gyp#ne... ui/events/events.gyp:69: 'keycodes/keyboard_lookup_win.cc', There was a comment about the gn file; do you need to update that too? https://codereview.chromium.org/1585193002/diff/20001/ui/events/keycodes/keyb... File ui/events/keycodes/keyboard_lookup_win.cc (right): https://codereview.chromium.org/1585193002/diff/20001/ui/events/keycodes/keyb... ui/events/keycodes/keyboard_lookup_win.cc:12: const EventFlags WindowsKeyboardLookup::event_flags_index[] = { Move to annonymous scope; and add a static_assert that arraysize(event_flags_index) < the number of modifiers defined in WIndowsKeyboardLookup.. https://codereview.chromium.org/1585193002/diff/20001/ui/events/keycodes/keyb... ui/events/keycodes/keyboard_lookup_win.cc:160: static WindowsKeyboardLookup* instance = nullptr; You can actually get rid of the GetInstance and just use a member variable. static WindowsKeyboardLookup s_instance; And then call s_instance.CheckOrLoadLayout in the VirtualKeyToCurrentLayoutDomKey then you'll avoid two calls to GetKeyboardLayout(0) on the first call. https://codereview.chromium.org/1585193002/diff/20001/ui/events/keycodes/keyb... File ui/events/keycodes/keyboard_lookup_win.h (right): https://codereview.chromium.org/1585193002/diff/20001/ui/events/keycodes/keyb... ui/events/keycodes/keyboard_lookup_win.h:1: // Copyright (c) 2015 The Chromium Authors. All rights reserved. 2016 https://codereview.chromium.org/1585193002/diff/20001/ui/events/keycodes/keyb... ui/events/keycodes/keyboard_lookup_win.h:19: static WindowsKeyboardLookup* Create(HKL alayout); You likely want to hold return this as a scoped_ptr so ownership is clear.. https://codereview.chromium.org/1585193002/diff/20001/ui/events/keycodes/keyb... ui/events/keycodes/keyboard_lookup_win.h:31: size_t GetEventFlagsIndex(int event_flags) const; I think these three things can just be in anonymous scope namespace in the impl file. https://codereview.chromium.org/1585193002/diff/20001/ui/events/keycodes/keyb... File ui/events/keycodes/keyboard_lookup_win_unittest.cc (right): https://codereview.chromium.org/1585193002/diff/20001/ui/events/keycodes/keyb... ui/events/keycodes/keyboard_lookup_win_unittest.cc:1: // Copyright (c) 2015 The Chromium Authors. All rights reserved. 2016 https://codereview.chromium.org/1585193002/diff/20001/ui/events/keycodes/keyb... ui/events/keycodes/keyboard_lookup_win_unittest.cc:49: WindowsKeyboardLookup *lookup = WindowsKeyboardLookup::Create(us_iden); use a scoped_ptr for the keyboard lookup...
Yes it passes the tests. Will work on the comments and build more test cases https://codereview.chromium.org/1585193002/diff/20001/ui/events/events.gyp File ui/events/events.gyp (left): https://codereview.chromium.org/1585193002/diff/20001/ui/events/events.gyp#ol... ui/events/events.gyp:388: } On 2016/01/21 01:59:35, dtapuska wrote: > any idea why this line changed? Because my editor will force a new line at EOF... I will remove it https://codereview.chromium.org/1585193002/diff/20001/ui/events/events.gyp File ui/events/events.gyp (right): https://codereview.chromium.org/1585193002/diff/20001/ui/events/events.gyp#ne... ui/events/events.gyp:69: 'keycodes/keyboard_lookup_win.cc', On 2016/01/21 01:59:36, dtapuska wrote: > There was a comment about the gn file; do you need to update that too? Yeah I guess so, I will look into the gn file https://codereview.chromium.org/1585193002/diff/20001/ui/events/keycodes/keyb... File ui/events/keycodes/keyboard_lookup_win.cc (right): https://codereview.chromium.org/1585193002/diff/20001/ui/events/keycodes/keyb... ui/events/keycodes/keyboard_lookup_win.cc:12: const EventFlags WindowsKeyboardLookup::event_flags_index[] = { On 2016/01/21 01:59:36, dtapuska wrote: > Move to annonymous scope; and add a static_assert that > arraysize(event_flags_index) < the number of modifiers defined in > WIndowsKeyboardLookup.. Acknowledged. https://codereview.chromium.org/1585193002/diff/20001/ui/events/keycodes/keyb... ui/events/keycodes/keyboard_lookup_win.cc:160: static WindowsKeyboardLookup* instance = nullptr; On 2016/01/21 01:59:36, dtapuska wrote: > You can actually get rid of the GetInstance and just use a member variable. > > static WindowsKeyboardLookup s_instance; > > And then call s_instance.CheckOrLoadLayout in the > VirtualKeyToCurrentLayoutDomKey > > then you'll avoid two calls to GetKeyboardLayout(0) on the first call. Acknowledged. https://codereview.chromium.org/1585193002/diff/20001/ui/events/keycodes/keyb... File ui/events/keycodes/keyboard_lookup_win.h (right): https://codereview.chromium.org/1585193002/diff/20001/ui/events/keycodes/keyb... ui/events/keycodes/keyboard_lookup_win.h:1: // Copyright (c) 2015 The Chromium Authors. All rights reserved. On 2016/01/21 01:59:36, dtapuska wrote: > 2016 Acknowledged. https://codereview.chromium.org/1585193002/diff/20001/ui/events/keycodes/keyb... ui/events/keycodes/keyboard_lookup_win.h:19: static WindowsKeyboardLookup* Create(HKL alayout); On 2016/01/21 01:59:36, dtapuska wrote: > You likely want to hold return this as a scoped_ptr so ownership is clear.. Will just expose constructor https://codereview.chromium.org/1585193002/diff/20001/ui/events/keycodes/keyb... ui/events/keycodes/keyboard_lookup_win.h:31: size_t GetEventFlagsIndex(int event_flags) const; On 2016/01/21 01:59:36, dtapuska wrote: > I think these three things can just be in anonymous scope namespace in the impl > file. Acknowledged. https://codereview.chromium.org/1585193002/diff/20001/ui/events/keycodes/keyb... File ui/events/keycodes/keyboard_lookup_win_unittest.cc (right): https://codereview.chromium.org/1585193002/diff/20001/ui/events/keycodes/keyb... ui/events/keycodes/keyboard_lookup_win_unittest.cc:1: // Copyright (c) 2015 The Chromium Authors. All rights reserved. On 2016/01/21 01:59:36, dtapuska wrote: > 2016 Acknowledged. https://codereview.chromium.org/1585193002/diff/20001/ui/events/keycodes/keyb... ui/events/keycodes/keyboard_lookup_win_unittest.cc:49: WindowsKeyboardLookup *lookup = WindowsKeyboardLookup::Create(us_iden); On 2016/01/21 01:59:36, dtapuska wrote: > use a scoped_ptr for the keyboard lookup... Acknowledged.
I've updated based on last review, and also 1. Added full tests for US keyboard and French keyboard 2. Handles fall-back for non-printable key combination, will check with Gary to see if I'm doing it correctly
dtapuska@chromium.org changed reviewers: + wez@chromium.org
looks good https://codereview.chromium.org/1585193002/diff/40001/ui/events/keycodes/keyb... File ui/events/keycodes/keyboard_lookup_win.h (right): https://codereview.chromium.org/1585193002/diff/40001/ui/events/keycodes/keyb... ui/events/keycodes/keyboard_lookup_win.h:25: static WindowsKeyboardLookup s_instance; I'd define this static inside the VirtualKeyToCurrentLayoutDomKey itself.
Can we update the CL description to be a little clearer - "not break keyboard buffer" doesn't really give much of a hint as to why whatever we were doing before would "break" anything. thanks!
Description was changed from ========== Map virtual keys to unicode characters without breaking keyboard buffer. Use static methods WindowsKeyboardLookup::ToUnicode WindowsKeyboardLookup::ToUnicodeWithLayout BUG=564678 ========== to ========== To convert KeyboardCode+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 combinations and build a lookup table 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 ==========
On 2016/01/28 18:45:33, Wez wrote: > Can we update the CL description to be a little clearer - "not break keyboard > buffer" doesn't really give much of a hint as to why whatever we were doing > before would "break" anything. > > thanks! I've updated the description, sorry for the confusion...
https://codereview.chromium.org/1585193002/diff/40001/ui/events/keycodes/keyb... File ui/events/keycodes/keyboard_lookup_win.cc (right): https://codereview.chromium.org/1585193002/diff/40001/ui/events/keycodes/keyb... ui/events/keycodes/keyboard_lookup_win.cc:15: const EventFlags event_flags_index[] = { nit: Use a compile-time check to ensure that this is the same size as kMaxKeyboardModifier? https://codereview.chromium.org/1585193002/diff/40001/ui/events/keycodes/keyb... ui/events/keycodes/keyboard_lookup_win.cc:16: EF_CAPS_LOCK_ON, nit: Why have you moved CAPS_LOCK_ON to appear first here, rather than preserving the order in which these flags appear in the ui::EventFlags enum? https://codereview.chromium.org/1585193002/diff/40001/ui/events/keycodes/keyb... ui/events/keycodes/keyboard_lookup_win.cc:45: // Trying to match system_event_state_lookup.cc. This comment should be more specific about the behaviour we are trying to "match". Imagine if system_event_state_lookup.cc got moved or removed, for example. Also, it's not clear why we need this logic if you're saying it already exists somewhere else in Chromium :) https://codereview.chromium.org/1585193002/diff/40001/ui/events/keycodes/keyb... ui/events/keycodes/keyboard_lookup_win.cc:51: keyboard_state[VK_RSHIFT] &= ~0x80; Why do we set the non-located key state, but clear both the located & non-located states..? https://codereview.chromium.org/1585193002/diff/40001/ui/events/keycodes/keyb... ui/events/keycodes/keyboard_lookup_win.cc:74: // Windows does not have a specific key code for AltGr. This comment seems misplaced.. looks like it belongs outside the if block? If you're trying to explain why we don't clear Alt+Control when AltGr is released then that's a different question. Also, note that AltGr is specifically represented as RightAlt+LeftControl within Windows, whereas you're only setting the non-located Alt+Control flags here; this may lead to weird/incorrect results for AltGr and/or Alt+Control combinations. https://codereview.chromium.org/1585193002/diff/40001/ui/events/keycodes/keyb... ui/events/keycodes/keyboard_lookup_win.cc:102: // TODO(chongz): EF_EXTENDED ??? https://codereview.chromium.org/1585193002/diff/40001/ui/events/keycodes/keyb... ui/events/keycodes/keyboard_lookup_win.cc:107: WindowsKeyboardLookup WindowsKeyboardLookup::s_instance; I think this will cause a static initializer, so use LazyInstance here instead? https://codereview.chromium.org/1585193002/diff/40001/ui/events/keycodes/keyb... ui/events/keycodes/keyboard_lookup_win.cc:110: : keyboard_layout_(nullptr) { nit: HKL is a typedef of HANDLE, for which INVALID_HANDLE is the value usually used to indicate it's not valid. Looking at the HKL-related APIs, though, it seems that zero is actually the normal error-case return value... So I'd suggest keyboard_layout_(0) or keyboard_layout_(INVALID_HANDLE) here, basically. https://codereview.chromium.org/1585193002/diff/40001/ui/events/keycodes/keyb... ui/events/keycodes/keyboard_lookup_win.cc:112: <= WindowsKeyboardLookup::kMaxKeyboardModifier, "Need larger table"); nit: Change this to an equality check; there's no reason to let the flags index size get out of sync w/ the constant's value. nit: This also seems a weird place to assert this; is it not possible to assert it in the anonymous namespace, for example, instead? https://codereview.chromium.org/1585193002/diff/40001/ui/events/keycodes/keyb... ui/events/keycodes/keyboard_lookup_win.cc:123: s_instance.CheckOrLoadLayout(::GetKeyboardLayout(0)); What happens if this gets called by code from different threads? https://codereview.chromium.org/1585193002/diff/40001/ui/events/keycodes/keyb... ui/events/keycodes/keyboard_lookup_win.cc:130: return DomKey::NONE; This if() will be optimized out by the compiler, since there are no valid values of the KeyboardCode enum that are >= kMaxVirtualKeyCode - you basically have to assume that the caller didn't supply you with garbage at this point. You could still DCHECK_GT(kMaxVirtualKeyCode, vkcode) to protect against someone adding a valid KeyboardCode value that exceeds your maximum, but I'm not sure it's worth it. https://codereview.chromium.org/1585193002/diff/40001/ui/events/keycodes/keyb... ui/events/keycodes/keyboard_lookup_win.cc:134: // Trying to match Firefox's behavior. This doesn't look right; if I pressed a key w/out modifiers, and it had no DomKey mapping then why would I then try to map it w/ Shift+AltGr+CapsLock on before trying any other values? nit: If you remove the static then you can have this become: const int flags_to_try[] = { flags, .... , EF_NONE }; for (auto flags : flags_to_try) { ... if (key != DomKey::NONE) break; } to simplify this function. https://codereview.chromium.org/1585193002/diff/40001/ui/events/keycodes/keyb... ui/events/keycodes/keyboard_lookup_win.cc:154: BYTE original_keyboard_state[256]; nit: Style-guide prefers that variables are declared immediately before they are used, i.e. in this case you'd declare *and* initialize |keyboard_state_to_restore|, and then declare & memset() the |keyboard_state| _inside_ the outer for() loop - memset()ing between keyboard modifiers costs more but means you can get rid of the complex modifier un-setting logic. https://codereview.chromium.org/1585193002/diff/40001/ui/events/keycodes/keyb... ui/events/keycodes/keyboard_lookup_win.cc:162: wchar_t unibuff[5]; nit: Please given this a descriptive name, as per style guide! https://codereview.chromium.org/1585193002/diff/40001/ui/events/keycodes/keyb... ui/events/keycodes/keyboard_lookup_win.cc:168: // Dead key, repeat twice to get character representation. nit: IIRC you can always get a dead-key to generate a character representation by injecting VK_SPACE immediately afterward, which feels easier to grok here. https://codereview.chromium.org/1585193002/diff/40001/ui/events/keycodes/keyb... ui/events/keycodes/keyboard_lookup_win.cc:174: unibuff[0]); nit: Indentation looks off; run git cl format on the CL? https://codereview.chromium.org/1585193002/diff/40001/ui/events/keycodes/keyb... ui/events/keycodes/keyboard_lookup_win.cc:176: // Ignores legacy non-printalbe control characters. typo This comment is confusing; IIUC what you're saying is that this case handles the single-generated-character case, but only if it was not a non-printable control character? nit: Do we need to cope w/ return-values of >1, e.g. are there any keyboard layouts that generate GB13080 code points (guessing not, but you never know...)? https://codereview.chromium.org/1585193002/diff/40001/ui/events/keycodes/keyb... File ui/events/keycodes/keyboard_lookup_win.h (right): https://codereview.chromium.org/1585193002/diff/40001/ui/events/keycodes/keyb... ui/events/keycodes/keyboard_lookup_win.h:17: class EVENTS_BASE_EXPORT WindowsKeyboardLookup { This class encapsulates the key-map, in effect - could it just be WindowsKeymap? https://codereview.chromium.org/1585193002/diff/40001/ui/events/keycodes/keyb... ui/events/keycodes/keyboard_lookup_win.h:19: WindowsKeyboardLookup(HKL alayout); explicit https://codereview.chromium.org/1585193002/diff/40001/ui/events/keycodes/keyb... ui/events/keycodes/keyboard_lookup_win.h:23: DomKey VirtualKeyToDomKey(KeyboardCode vkcode, int event_flags) const; nit: Please add brief comments to explain why we have both of these. Within the src/ui/ code we refer to the virtual key codes as KeyboardCode (as opposed to DomKey or DomCode) so perhaps this should be KeyboardCodeToDomKey()? It's the case that there is not a 1:1 mapping from virtual key codes to DOM key values, so I'd recommend changing this to provide DomCodeToKey() and DomCodeToKeyboardCode() instead, and then have a separate OemScancodeToDomCode() to fetch the DOM |code| value based on the actual key scancode, as well as a "legacy fallback" KeyboardCodeToDomCode() helper akin to what you have here. That said, perhaps Dave has an alternative layout in mind? Also note that in general it's not possible to map back and forth between KeyboardCode, DomCode, DomKey and OEM scancode without being stateful. https://codereview.chromium.org/1585193002/diff/40001/ui/events/keycodes/keyb... ui/events/keycodes/keyboard_lookup_win.h:30: void CheckOrLoadLayout(HKL alayout); nit: Why |alayout| rather than |layout|? Why is this "CheckOrLoadLayout"? What does it mean to "check" the layout? https://codereview.chromium.org/1585193002/diff/40001/ui/events/keycodes/keyb... ui/events/keycodes/keyboard_lookup_win.h:32: static const size_t kMaxVirtualKeyCode = 256; nit: This isn't the maximum virtual key code value, but the _count_ of possible virtual key code values - the _maximum_ key code value is 255. Can you get away with using int rather than size_t here, since these aren't sizes of in-memory structures? We general prefer to use signed types unless we really have to use unsigned for some specific reason, to reduce scope for issues arising from signed/unsigned mismatches. https://codereview.chromium.org/1585193002/diff/40001/ui/events/keycodes/keyb... ui/events/keycodes/keyboard_lookup_win.h:33: static const size_t kMaxKeyboardModifier = 1 << 8; nit: This isn't the maximum modifier, it's the number of different modifier combinations, given eight discrete modifier flags that we take account of. https://codereview.chromium.org/1585193002/diff/40001/ui/events/keycodes/keyb... ui/events/keycodes/keyboard_lookup_win.h:36: DomKey lookup_table_[kMaxVirtualKeyCode][kMaxKeyboardModifier]; This means we're creating a table with 256*256 entries, each of which is 4 bytes in size, i.e. a 256KB table, which seems excessive - are all of the modifiers you're considering necessary to take into account? https://codereview.chromium.org/1585193002/diff/40001/ui/events/keycodes/keyb... ui/events/keycodes/keyboard_lookup_win.h:37: }; DISALLOW_COPY_AND_ASSIGN() on this class.
https://codereview.chromium.org/1585193002/diff/40001/ui/events/keycodes/keyb... File ui/events/keycodes/keyboard_lookup_win.h (right): https://codereview.chromium.org/1585193002/diff/40001/ui/events/keycodes/keyb... ui/events/keycodes/keyboard_lookup_win.h:23: DomKey VirtualKeyToDomKey(KeyboardCode vkcode, int event_flags) const; On 2016/01/29 00:22:24, Wez wrote: > nit: Please add brief comments to explain why we have both of these. > > Within the src/ui/ code we refer to the virtual key codes as KeyboardCode (as > opposed to DomKey or DomCode) so perhaps this should be KeyboardCodeToDomKey()? > > It's the case that there is not a 1:1 mapping from virtual key codes to DOM key > values, so I'd recommend changing this to provide DomCodeToKey() and > DomCodeToKeyboardCode() instead, and then have a separate OemScancodeToDomCode() > to fetch the DOM |code| value based on the actual key scancode, as well as a > "legacy fallback" KeyboardCodeToDomCode() helper akin to what you have here. > > That said, perhaps Dave has an alternative layout in mind? > > Also note that in general it's not possible to map back and forth between > KeyboardCode, DomCode, DomKey and OEM scancode without being stateful. Ok I do see how we can make this as an API that maps DomCodeToDomKey that would be similar to the DomCodeToUsLayoutDomKey API. To do this we could enumerate all possible DOMCodes we support in: https://code.google.com/p/chromium/codesearch#chromium/src/ui/events/keycodes... Grab the scan code and call ToUnicode on that. We'd then have some sort of map of [DOMCode + modifiers] -> DOMKey Can you explain the 1:1 mapping? I know it is true that multiple VKEYs will map to one DomKey; for example Numpad4 and 4 will both produce DomKey4. But I do think that one VKEY won't ever map to multiple DomKeys is always true. It is true that multiple scan codes do map to the same DOMCode. If we base this on the DomCode then we can handle non-printable characters as well; for example calling in to handle the non-printable map here: https://code.google.com/p/chromium/codesearch#chromium/src/ui/events/keycodes... Chong can you investigate if ToUnicode works well when we enumerate the scancodes? Or do we have to map the scancodes to vkeys (via MapVirtualKey) and then call ToUnicode on that result.
Thanks for the detailed review! I will start working on the fix... https://codereview.chromium.org/1585193002/diff/40001/ui/events/keycodes/keyb... File ui/events/keycodes/keyboard_lookup_win.cc (right): https://codereview.chromium.org/1585193002/diff/40001/ui/events/keycodes/keyb... ui/events/keycodes/keyboard_lookup_win.cc:15: const EventFlags event_flags_index[] = { On 2016/01/29 00:22:24, Wez wrote: > nit: Use a compile-time check to ensure that this is the same size as > kMaxKeyboardModifier? Tried here but kMaxKeyboardModifier is private... Should I make it public? https://codereview.chromium.org/1585193002/diff/40001/ui/events/keycodes/keyb... ui/events/keycodes/keyboard_lookup_win.cc:16: EF_CAPS_LOCK_ON, On 2016/01/29 00:22:23, Wez wrote: > nit: Why have you moved CAPS_LOCK_ON to appear first here, rather than > preserving the order in which these flags appear in the ui::EventFlags enum? Sorry I didn't notice that the order was changed on Jan. 08... Will match EventFlags. https://codereview.chromium.org/1585193002/diff/40001/ui/events/keycodes/keyb... ui/events/keycodes/keyboard_lookup_win.cc:45: // Trying to match system_event_state_lookup.cc. On 2016/01/29 00:22:23, Wez wrote: > This comment should be more specific about the behaviour we are trying to > "match". Imagine if system_event_state_lookup.cc got moved or removed, for > example. Also, it's not clear why we need this logic if you're saying it > already exists somewhere else in Chromium :) Maybe // Trying to recover keyboard_state using the same // logic as we querying event_flags from native OS. // See system_event_state_lookup.cc. But with the AltGr change its not matching anymore... https://codereview.chromium.org/1585193002/diff/40001/ui/events/keycodes/keyb... ui/events/keycodes/keyboard_lookup_win.cc:51: keyboard_state[VK_RSHIFT] &= ~0x80; On 2016/01/29 00:22:23, Wez wrote: > Why do we set the non-located key state, but clear both the located & > non-located states..? Will do memset() to reset state... https://codereview.chromium.org/1585193002/diff/40001/ui/events/keycodes/keyb... ui/events/keycodes/keyboard_lookup_win.cc:74: // Windows does not have a specific key code for AltGr. On 2016/01/29 00:22:23, Wez wrote: > This comment seems misplaced.. looks like it belongs outside the if block? > > If you're trying to explain why we don't clear Alt+Control when AltGr is > released then that's a different question. > > Also, note that AltGr is specifically represented as RightAlt+LeftControl within > Windows, whereas you're only setting the non-located Alt+Control flags here; > this may lead to weird/incorrect results for AltGr and/or Alt+Control > combinations. Sorry I didn't know that before, will change to RightAlt+LeftControl. https://codereview.chromium.org/1585193002/diff/40001/ui/events/keycodes/keyb... ui/events/keycodes/keyboard_lookup_win.cc:102: // TODO(chongz): EF_EXTENDED On 2016/01/29 00:22:23, Wez wrote: > ??? Not necessary, will remove... https://codereview.chromium.org/1585193002/diff/40001/ui/events/keycodes/keyb... ui/events/keycodes/keyboard_lookup_win.cc:107: WindowsKeyboardLookup WindowsKeyboardLookup::s_instance; On 2016/01/29 00:22:23, Wez wrote: > I think this will cause a static initializer, so use LazyInstance here instead? Acknowledged. https://codereview.chromium.org/1585193002/diff/40001/ui/events/keycodes/keyb... ui/events/keycodes/keyboard_lookup_win.cc:110: : keyboard_layout_(nullptr) { On 2016/01/29 00:22:23, Wez wrote: > nit: HKL is a typedef of HANDLE, for which INVALID_HANDLE is the value usually > used to indicate it's not valid. Looking at the HKL-related APIs, though, it > seems that zero is actually the normal error-case return value... > > So I'd suggest keyboard_layout_(0) or keyboard_layout_(INVALID_HANDLE) here, > basically. Acknowledged. https://codereview.chromium.org/1585193002/diff/40001/ui/events/keycodes/keyb... ui/events/keycodes/keyboard_lookup_win.cc:112: <= WindowsKeyboardLookup::kMaxKeyboardModifier, "Need larger table"); On 2016/01/29 00:22:23, Wez wrote: > nit: Change this to an equality check; there's no reason to let the flags index > size get out of sync w/ the constant's value. > > nit: This also seems a weird place to assert this; is it not possible to assert > it in the anonymous namespace, for example, instead? Maybe make kMaxKeyboardModifier public and move to top? https://codereview.chromium.org/1585193002/diff/40001/ui/events/keycodes/keyb... ui/events/keycodes/keyboard_lookup_win.cc:123: s_instance.CheckOrLoadLayout(::GetKeyboardLayout(0)); On 2016/01/29 00:22:23, Wez wrote: > What happens if this gets called by code from different threads? Should I add a lock or only allow it to be used by a certain thread? https://codereview.chromium.org/1585193002/diff/40001/ui/events/keycodes/keyb... ui/events/keycodes/keyboard_lookup_win.cc:130: return DomKey::NONE; On 2016/01/29 00:22:24, Wez wrote: > This if() will be optimized out by the compiler, since there are no valid values > of the KeyboardCode enum that are >= kMaxVirtualKeyCode - you basically have to > assume that the caller didn't supply you with garbage at this point. > > You could still DCHECK_GT(kMaxVirtualKeyCode, vkcode) to protect against someone > adding a valid KeyboardCode value that exceeds your maximum, but I'm not sure > it's worth it. Will assume the input is valid. https://codereview.chromium.org/1585193002/diff/40001/ui/events/keycodes/keyb... ui/events/keycodes/keyboard_lookup_win.cc:134: // Trying to match Firefox's behavior. On 2016/01/29 00:22:23, Wez wrote: > This doesn't look right; if I pressed a key w/out modifiers, and it had no > DomKey mapping then why would I then try to map it w/ Shift+AltGr+CapsLock on > before trying any other values? > > nit: If you remove the static then you can have this become: > > const int flags_to_try[] = { flags, .... , EF_NONE }; > for (auto flags : flags_to_try) { > ... > if (key != DomKey::NONE) > break; > } > > to simplify this function. If there is no modifiers (event_flags == EF_NONE) I will try to map to EF_NONE & (EF_SHIFT_DOWN | EF_ALTGR_DOWN | EF_CAPS_LOCK_ON) first, which is still EF_NONE... Ya it's confusing, maybe change to const int flags_to_try[] = { flags, flags & (EF_SHIFT_DOWN | EF_ALTGR_DOWN | EF_CAPS_LOCK_ON), flags & (EF_SHIFT_DOWN | EF_CAPS_LOCK_ON), EF_NONE, } https://codereview.chromium.org/1585193002/diff/40001/ui/events/keycodes/keyb... ui/events/keycodes/keyboard_lookup_win.cc:154: BYTE original_keyboard_state[256]; On 2016/01/29 00:22:23, Wez wrote: > nit: Style-guide prefers that variables are declared immediately before they are > used, i.e. in this case you'd declare *and* initialize > |keyboard_state_to_restore|, and then declare & memset() the |keyboard_state| > _inside_ the outer for() loop - memset()ing between keyboard modifiers costs > more but means you can get rid of the complex modifier un-setting logic. Acknowledged. https://codereview.chromium.org/1585193002/diff/40001/ui/events/keycodes/keyb... ui/events/keycodes/keyboard_lookup_win.cc:162: wchar_t unibuff[5]; On 2016/01/29 00:22:23, Wez wrote: > nit: Please given this a descriptive name, as per style guide! Acknowledged. https://codereview.chromium.org/1585193002/diff/40001/ui/events/keycodes/keyb... ui/events/keycodes/keyboard_lookup_win.cc:168: // Dead key, repeat twice to get character representation. On 2016/01/29 00:22:23, Wez wrote: > nit: IIRC you can always get a dead-key to generate a character representation > by injecting VK_SPACE immediately afterward, which feels easier to grok here. Acknowledged. https://codereview.chromium.org/1585193002/diff/40001/ui/events/keycodes/keyb... ui/events/keycodes/keyboard_lookup_win.cc:174: unibuff[0]); On 2016/01/29 00:22:23, Wez wrote: > nit: Indentation looks off; run git cl format on the CL? Acknowledged. https://codereview.chromium.org/1585193002/diff/40001/ui/events/keycodes/keyb... ui/events/keycodes/keyboard_lookup_win.cc:176: // Ignores legacy non-printalbe control characters. On 2016/01/29 00:22:23, Wez wrote: > typo > > This comment is confusing; IIUC what you're saying is that this case handles the > single-generated-character case, but only if it was not a non-printable control > character? > > nit: Do we need to cope w/ return-values of >1, e.g. are there any keyboard > layouts that generate GB13080 code points (guessing not, but you never know...)? Yes we don't want those control characters, will re-format the if and comments. Will try to find a better way to handle the multiple character... https://codereview.chromium.org/1585193002/diff/40001/ui/events/keycodes/keyb... File ui/events/keycodes/keyboard_lookup_win.h (right): https://codereview.chromium.org/1585193002/diff/40001/ui/events/keycodes/keyb... ui/events/keycodes/keyboard_lookup_win.h:17: class EVENTS_BASE_EXPORT WindowsKeyboardLookup { On 2016/01/29 00:22:24, Wez wrote: > This class encapsulates the key-map, in effect - could it just be WindowsKeymap? Acknowledged. https://codereview.chromium.org/1585193002/diff/40001/ui/events/keycodes/keyb... ui/events/keycodes/keyboard_lookup_win.h:19: WindowsKeyboardLookup(HKL alayout); On 2016/01/29 00:22:24, Wez wrote: > explicit Acknowledged. https://codereview.chromium.org/1585193002/diff/40001/ui/events/keycodes/keyb... ui/events/keycodes/keyboard_lookup_win.h:23: DomKey VirtualKeyToDomKey(KeyboardCode vkcode, int event_flags) const; On 2016/01/29 17:05:48, dtapuska wrote: > On 2016/01/29 00:22:24, Wez wrote: > > nit: Please add brief comments to explain why we have both of these. > > > > Within the src/ui/ code we refer to the virtual key codes as KeyboardCode (as > > opposed to DomKey or DomCode) so perhaps this should be > KeyboardCodeToDomKey()? > > > > It's the case that there is not a 1:1 mapping from virtual key codes to DOM > key > > values, so I'd recommend changing this to provide DomCodeToKey() and > > DomCodeToKeyboardCode() instead, and then have a separate > OemScancodeToDomCode() > > to fetch the DOM |code| value based on the actual key scancode, as well as a > > "legacy fallback" KeyboardCodeToDomCode() helper akin to what you have here. > > > > That said, perhaps Dave has an alternative layout in mind? > > > > Also note that in general it's not possible to map back and forth between > > KeyboardCode, DomCode, DomKey and OEM scancode without being stateful. > > Ok I do see how we can make this as an API that maps DomCodeToDomKey that would > be similar to the DomCodeToUsLayoutDomKey API. > > To do this we could enumerate all possible DOMCodes we support in: > > https://code.google.com/p/chromium/codesearch#chromium/src/ui/events/keycodes... > > Grab the scan code and call ToUnicode on that. > > We'd then have some sort of map of [DOMCode + modifiers] -> DOMKey > > Can you explain the 1:1 mapping? I know it is true that multiple VKEYs will map > to one DomKey; for example Numpad4 and 4 will both produce DomKey4. But I do > think that one VKEY won't ever map to multiple DomKeys is always true. > > It is true that multiple scan codes do map to the same DOMCode. > > If we base this on the DomCode then we can handle non-printable characters as > well; for example calling in to handle the non-printable map here: > https://code.google.com/p/chromium/codesearch#chromium/src/ui/events/keycodes... > > Chong can you investigate if ToUnicode works well when we enumerate the > scancodes? Or do we have to map the scancodes to vkeys (via MapVirtualKey) and > then call ToUnicode on that result. The static function VirtualKeyToCurrentLayoutDomKey() was expected to be used in Chrome since we always use current layout, and the other one VirtualKeyToDomKey() was to provide a way for testing different keyboard layout without changing system layout. Sure I will try changing the API and check if ToUnicode (and MapVirtualKey) works well, especially for custom key remapping. https://codereview.chromium.org/1585193002/diff/40001/ui/events/keycodes/keyb... ui/events/keycodes/keyboard_lookup_win.h:25: static WindowsKeyboardLookup s_instance; On 2016/01/28 15:27:55, dtapuska wrote: > I'd define this static inside the VirtualKeyToCurrentLayoutDomKey itself. Acknowledged. https://codereview.chromium.org/1585193002/diff/40001/ui/events/keycodes/keyb... ui/events/keycodes/keyboard_lookup_win.h:30: void CheckOrLoadLayout(HKL alayout); On 2016/01/29 00:22:24, Wez wrote: > nit: Why |alayout| rather than |layout|? > > Why is this "CheckOrLoadLayout"? What does it mean to "check" the layout? It will load layout only when it's different from the current one... I will move the check out and change it to LoadLayout() https://codereview.chromium.org/1585193002/diff/40001/ui/events/keycodes/keyb... ui/events/keycodes/keyboard_lookup_win.h:32: static const size_t kMaxVirtualKeyCode = 256; On 2016/01/29 00:22:24, Wez wrote: > nit: This isn't the maximum virtual key code value, but the _count_ of possible > virtual key code values - the _maximum_ key code value is 255. > > Can you get away with using int rather than size_t here, since these aren't > sizes of in-memory structures? We general prefer to use signed types unless we > really have to use unsigned for some specific reason, to reduce scope for issues > arising from signed/unsigned mismatches. Acknowledged. https://codereview.chromium.org/1585193002/diff/40001/ui/events/keycodes/keyb... ui/events/keycodes/keyboard_lookup_win.h:33: static const size_t kMaxKeyboardModifier = 1 << 8; On 2016/01/29 00:22:24, Wez wrote: > nit: This isn't the maximum modifier, it's the number of different modifier > combinations, given eight discrete modifier flags that we take account of. Acknowledged. https://codereview.chromium.org/1585193002/diff/40001/ui/events/keycodes/keyb... ui/events/keycodes/keyboard_lookup_win.h:36: DomKey lookup_table_[kMaxVirtualKeyCode][kMaxKeyboardModifier]; On 2016/01/29 00:22:24, Wez wrote: > This means we're creating a table with 256*256 entries, each of which is 4 bytes > in size, i.e. a 256KB table, which seems excessive - are all of the modifiers > you're considering necessary to take into account? Yes it's too large. I guess for each DomCode I can only store valid modifiers entries in a dynamic allocated array. https://codereview.chromium.org/1585193002/diff/40001/ui/events/keycodes/keyb... ui/events/keycodes/keyboard_lookup_win.h:37: }; On 2016/01/29 00:22:24, Wez wrote: > DISALLOW_COPY_AND_ASSIGN() on this class. Acknowledged.
Hi Wez, I've fixed the CL based on your review, PTAL. Thanks! https://codereview.chromium.org/1585193002/diff/40001/ui/events/keycodes/keyb... File ui/events/keycodes/keyboard_lookup_win.cc (right): https://codereview.chromium.org/1585193002/diff/40001/ui/events/keycodes/keyb... ui/events/keycodes/keyboard_lookup_win.cc:74: // Windows does not have a specific key code for AltGr. On 2016/01/29 20:25:49, chongz wrote: > On 2016/01/29 00:22:23, Wez wrote: > > This comment seems misplaced.. looks like it belongs outside the if block? > > > > If you're trying to explain why we don't clear Alt+Control when AltGr is > > released then that's a different question. > > > > Also, note that AltGr is specifically represented as RightAlt+LeftControl > within > > Windows, whereas you're only setting the non-located Alt+Control flags here; > > this may lead to weird/incorrect results for AltGr and/or Alt+Control > > combinations. > > Sorry I didn't know that before, will change to RightAlt+LeftControl. I'm not sure why but only the non-located Alt+Control will produce the right key. https://codereview.chromium.org/1585193002/diff/40001/ui/events/keycodes/keyb... ui/events/keycodes/keyboard_lookup_win.cc:123: s_instance.CheckOrLoadLayout(::GetKeyboardLayout(0)); On 2016/01/29 20:25:49, chongz wrote: > On 2016/01/29 00:22:23, Wez wrote: > > What happens if this gets called by code from different threads? > > Should I add a lock or only allow it to be used by a certain thread? Still, should I add a lock or only allow it to be used by a certain thread? https://codereview.chromium.org/1585193002/diff/40001/ui/events/keycodes/keyb... ui/events/keycodes/keyboard_lookup_win.cc:176: // Ignores legacy non-printalbe control characters. On 2016/01/29 20:25:49, chongz wrote: > On 2016/01/29 00:22:23, Wez wrote: > > typo > > > > This comment is confusing; IIUC what you're saying is that this case handles > the > > single-generated-character case, but only if it was not a non-printable > control > > character? > > > > nit: Do we need to cope w/ return-values of >1, e.g. are there any keyboard > > layouts that generate GB13080 code points (guessing not, but you never > know...)? > > Yes we don't want those control characters, will re-format the if and comments. > > Will try to find a better way to handle the multiple character... Added TODO to handle multiple characters later... https://codereview.chromium.org/1585193002/diff/40001/ui/events/keycodes/keyb... File ui/events/keycodes/keyboard_lookup_win.h (right): https://codereview.chromium.org/1585193002/diff/40001/ui/events/keycodes/keyb... ui/events/keycodes/keyboard_lookup_win.h:23: DomKey VirtualKeyToDomKey(KeyboardCode vkcode, int event_flags) const; On 2016/01/29 20:25:50, chongz wrote: > On 2016/01/29 17:05:48, dtapuska wrote: > > On 2016/01/29 00:22:24, Wez wrote: > > > nit: Please add brief comments to explain why we have both of these. > > > > > > Within the src/ui/ code we refer to the virtual key codes as KeyboardCode > (as > > > opposed to DomKey or DomCode) so perhaps this should be > > KeyboardCodeToDomKey()? > > > > > > It's the case that there is not a 1:1 mapping from virtual key codes to DOM > > key > > > values, so I'd recommend changing this to provide DomCodeToKey() and > > > DomCodeToKeyboardCode() instead, and then have a separate > > OemScancodeToDomCode() > > > to fetch the DOM |code| value based on the actual key scancode, as well as a > > > "legacy fallback" KeyboardCodeToDomCode() helper akin to what you have here. > > > > > > That said, perhaps Dave has an alternative layout in mind? > > > > > > Also note that in general it's not possible to map back and forth between > > > KeyboardCode, DomCode, DomKey and OEM scancode without being stateful. > > > > Ok I do see how we can make this as an API that maps DomCodeToDomKey that > would > > be similar to the DomCodeToUsLayoutDomKey API. > > > > To do this we could enumerate all possible DOMCodes we support in: > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/ui/events/keycodes... > > > > Grab the scan code and call ToUnicode on that. > > > > We'd then have some sort of map of [DOMCode + modifiers] -> DOMKey > > > > Can you explain the 1:1 mapping? I know it is true that multiple VKEYs will > map > > to one DomKey; for example Numpad4 and 4 will both produce DomKey4. But I do > > think that one VKEY won't ever map to multiple DomKeys is always true. > > > > It is true that multiple scan codes do map to the same DOMCode. > > > > If we base this on the DomCode then we can handle non-printable characters as > > well; for example calling in to handle the non-printable map here: > > > https://code.google.com/p/chromium/codesearch#chromium/src/ui/events/keycodes... > > > > Chong can you investigate if ToUnicode works well when we enumerate the > > scancodes? Or do we have to map the scancodes to vkeys (via MapVirtualKey) and > > then call ToUnicode on that result. > > The static function VirtualKeyToCurrentLayoutDomKey() was expected to be used in > Chrome since we always use current layout, and the other one > VirtualKeyToDomKey() was to provide a way for testing different keyboard layout > without changing system layout. > > Sure I will try changing the API and check if ToUnicode (and MapVirtualKey) > works well, especially for custom key remapping. Changed it to DomCodeToKey(), also added DomCodeToKeyboardCode() and KeyboardCodeToDomCode(), not sure if we still need OemScancodeToDomCode()? https://codereview.chromium.org/1585193002/diff/40001/ui/events/keycodes/keyb... ui/events/keycodes/keyboard_lookup_win.h:36: DomKey lookup_table_[kMaxVirtualKeyCode][kMaxKeyboardModifier]; On 2016/01/29 20:25:50, chongz wrote: > On 2016/01/29 00:22:24, Wez wrote: > > This means we're creating a table with 256*256 entries, each of which is 4 > bytes > > in size, i.e. a 256KB table, which seems excessive - are all of the modifiers > > you're considering necessary to take into account? > > Yes it's too large. I guess for each DomCode I can only store valid modifiers > entries in a dynamic allocated array. Switched to the sparse map and ignored EF_COMMAND_DOWN, EF_NUM_LOCK_ON and EF_NUM_LOCK_ON to save memory. Now the map has 856 entries for English layout and 12 byte per entry, so it will be around 13KB plus std::map overhead. Next step is to handle EF_NUM_LOCK_ON and maybe add more fall-back logic to save more memory if necessary. (FYI 900 entries for French and 868 for Persian, LoadLayout() takes 20ms on my Win8 VM)
Description was changed from ========== To convert KeyboardCode+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 combinations and build a lookup table 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 ========== to ========== 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 ==========
https://codereview.chromium.org/1585193002/diff/60001/ui/events/keycodes/key_... File ui/events/keycodes/key_map_win.cc (right): https://codereview.chromium.org/1585193002/diff/60001/ui/events/keycodes/key_... ui/events/keycodes/key_map_win.cc:129: DomCode WindowsKeyMap::KeyboardCodeToDomCode(KeyboardCode key_code) const { Is this really required? I'm not sure if we need a keyboardCode->DomCode map If we don't need this map then we can reduce the size of the struct stored no? because we wouldn't need to store legacy_key_code https://codereview.chromium.org/1585193002/diff/60001/ui/events/keycodes/key_... ui/events/keycodes/key_map_win.cc:187: // TODO(chongz): Handle multiple characters case. What are we going to do with non-printable characters? Should the table support DomCode->DomKey for the non-printable range as well? https://codereview.chromium.org/1585193002/diff/60001/ui/events/keycodes/key_... File ui/events/keycodes/key_map_win.h (right): https://codereview.chromium.org/1585193002/diff/60001/ui/events/keycodes/key_... ui/events/keycodes/key_map_win.h:22: DomCode code; Why is code required here? https://codereview.chromium.org/1585193002/diff/60001/ui/events/keycodes/key_... ui/events/keycodes/key_map_win.h:24: KeyboardCode legacy_key_code; I question storing this field whether it is required... https://codereview.chromium.org/1585193002/diff/60001/ui/events/keycodes/key_... ui/events/keycodes/key_map_win.h:27: typedef std::map<std::pair<DomCode, int /*flags*/>, KeyMapEntry> KeyMap; can we get away with an unordered_map? https://codereview.chromium.org/1585193002/diff/60001/ui/events/keycodes/key_... ui/events/keycodes/key_map_win.h:34: // Convert a DomCode to a DomKey for the layout specified in constructor. We ideally should identify what |flags| are..
Hi Wez, can I have a review on the updated CL please? Thanks! https://codereview.chromium.org/1585193002/diff/60001/ui/events/keycodes/key_... File ui/events/keycodes/key_map_win.cc (right): https://codereview.chromium.org/1585193002/diff/60001/ui/events/keycodes/key_... ui/events/keycodes/key_map_win.cc:74: keyboard_state[VK_MENU] |= 0x80; I'm not sure why but only the non-located Alt+Control will produce the right key. https://codereview.chromium.org/1585193002/diff/60001/ui/events/keycodes/key_... ui/events/keycodes/key_map_win.cc:129: DomCode WindowsKeyMap::KeyboardCodeToDomCode(KeyboardCode key_code) const { On 2016/02/04 20:31:45, dtapuska wrote: > Is this really required? I'm not sure if we need a keyboardCode->DomCode map > > If we don't need this map then we can reduce the size of the struct stored no? > because we wouldn't need to store legacy_key_code I guess Wez wants it in case user only gives us KeyboardCode? I'm not sure if I should do the implementation now or only when it's actually used. https://codereview.chromium.org/1585193002/diff/60001/ui/events/keycodes/key_... ui/events/keycodes/key_map_win.cc:141: HKL current_layout = ::GetKeyboardLayout(0); Still, should I add a lock or only allow it to be used by a certain thread? https://codereview.chromium.org/1585193002/diff/60001/ui/events/keycodes/key_... ui/events/keycodes/key_map_win.cc:187: // TODO(chongz): Handle multiple characters case. On 2016/02/04 20:31:45, dtapuska wrote: > What are we going to do with non-printable characters? > > Should the table support DomCode->DomKey for the non-printable range as well? I will add (copy?) the US non-printable table here later, currently it's relying on the default fall-back since ToUnicodeEx only works for printable characters. https://codereview.chromium.org/1585193002/diff/60001/ui/events/keycodes/key_... File ui/events/keycodes/key_map_win.h (right): https://codereview.chromium.org/1585193002/diff/60001/ui/events/keycodes/key_... ui/events/keycodes/key_map_win.h:27: typedef std::map<std::pair<DomCode, int /*flags*/>, KeyMapEntry> KeyMap; On 2016/02/04 20:31:46, dtapuska wrote: > can we get away with an unordered_map? Not sure which one is better, the map will usually have 900 entries, and LoadLayout() takes 20ms on my Win8 VM. https://codereview.chromium.org/1585193002/diff/60001/ui/events/keycodes/key_... ui/events/keycodes/key_map_win.h:34: // Convert a DomCode to a DomKey for the layout specified in constructor. On 2016/02/04 20:31:45, dtapuska wrote: > We ideally should identify what |flags| are.. Ya I will add comment about EventFlags here
https://codereview.chromium.org/1585193002/diff/60001/ui/events/keycodes/key_... File ui/events/keycodes/key_map_win.cc (right): https://codereview.chromium.org/1585193002/diff/60001/ui/events/keycodes/key_... ui/events/keycodes/key_map_win.cc:31: const EventFlags flags_index[] = { nit: This is the list of interesting modifier flags, so call it |modifier_flags|? or just |modifiers|? |flags_index| doesn't help the reader understand what it's for. https://codereview.chromium.org/1585193002/diff/60001/ui/events/keycodes/key_... ui/events/keycodes/key_map_win.cc:41: const int kMaxEventFlagsIndex = (1 << arraysize(flags_index)) - 1; This still isn't the maximum index of the flags_index ;) It's the total number of possible flag combinations, so perhaps call it kModifierFlagsCombinations? https://codereview.chromium.org/1585193002/diff/60001/ui/events/keycodes/key_... ui/events/keycodes/key_map_win.cc:43: int GetEventFlagsIndex(int flags) { I don't see this used anywhere? https://codereview.chromium.org/1585193002/diff/60001/ui/events/keycodes/key_... ui/events/keycodes/key_map_win.cc:52: int EventFlagsFromIndex(int index) { nit: Suggest GetModifierFlags(int combination) if you follow the "combination" and "modifiers" naming above. https://codereview.chromium.org/1585193002/diff/60001/ui/events/keycodes/key_... ui/events/keycodes/key_map_win.cc:74: keyboard_state[VK_MENU] |= 0x80; On 2016/02/04 at 21:10:25, chongz wrote: > I'm not sure why but only the non-located Alt+Control will produce the right key. OK, sorry for the misleading comment earlier! Not sure the exact mechanics of that hack, unfortunately. https://codereview.chromium.org/1585193002/diff/60001/ui/events/keycodes/key_... ui/events/keycodes/key_map_win.cc:93: WindowsKeyMap::WindowsKeyMap() : keyboard_layout_(0) {} nit: You can set this via inline initializer in the header. https://codereview.chromium.org/1585193002/diff/60001/ui/events/keycodes/key_... ui/events/keycodes/key_map_win.cc:95: WindowsKeyMap::WindowsKeyMap(HKL layout) : WindowsKeyMap() { See above re inline initializer for |keyboard_layout_| https://codereview.chromium.org/1585193002/diff/60001/ui/events/keycodes/key_... ui/events/keycodes/key_map_win.cc:116: if (key != DomKey::NONE) nit: You're doing this if() even if you didn't change |key| - make the if (it != ....) a block if() and put this inside it. https://codereview.chromium.org/1585193002/diff/60001/ui/events/keycodes/key_... ui/events/keycodes/key_map_win.cc:129: DomCode WindowsKeyMap::KeyboardCodeToDomCode(KeyboardCode key_code) const { On 2016/02/04 at 21:10:25, chongz wrote: > On 2016/02/04 20:31:45, dtapuska wrote: > > Is this really required? I'm not sure if we need a keyboardCode->DomCode map > > > > If we don't need this map then we can reduce the size of the struct stored no? > > because we wouldn't need to store legacy_key_code > > I guess Wez wants it in case user only gives us KeyboardCode? I'm not sure if I should do the implementation now or only when it's actually used. Let's add it if/when it's actually used! I was thinking just of synthetic events from JS, but I'm not sure we will even hit the WindowsKeyMap in that case. https://codereview.chromium.org/1585193002/diff/60001/ui/events/keycodes/key_... ui/events/keycodes/key_map_win.cc:138: DomKey WindowsKeyMap::DomCodeToSystemLayoutKey(DomCode code, int flags) { static methods should be prefixed with a "// static" comment line. https://codereview.chromium.org/1585193002/diff/60001/ui/events/keycodes/key_... ui/events/keycodes/key_map_win.cc:140: LAZY_INSTANCE_INITIALIZER; Note that the LazyInstance header says it "really only makes sense as a global variable". https://codereview.chromium.org/1585193002/diff/60001/ui/events/keycodes/key_... ui/events/keycodes/key_map_win.cc:141: HKL current_layout = ::GetKeyboardLayout(0); On 2016/02/04 at 21:10:25, chongz wrote: > Still, should I add a lock or only allow it to be used by a certain thread? GetKeyboardLayout(0) gets the current layout for the thread on which it is called - so just adding a lock could mean that calling code with multiple threads ends up swapping back and forth between layouts, if the layouts differ. It'd also required holding the lock while you populate the map, which is a non-trivially slow operation during which you'd be blocking any other threads that happened to call this API. Given that layouts are per-thread, could we require that the caller creates a WindowsKeyMap instance on the stack before starting the MessageLoop, and having a static CodeAndFlagsToDomKey() method (replacing DomCodeToSystemLayoutKey) that fetches it from thread-local-storage, refreshes it if necessary, and calls DomCode[AndFlags]ToKey() on it? https://codereview.chromium.org/1585193002/diff/60001/ui/events/keycodes/key_... File ui/events/keycodes/key_map_win.h (right): https://codereview.chromium.org/1585193002/diff/60001/ui/events/keycodes/key_... ui/events/keycodes/key_map_win.h:27: typedef std::map<std::pair<DomCode, int /*flags*/>, KeyMapEntry> KeyMap; On 2016/02/04 at 21:10:25, chongz wrote: > On 2016/02/04 20:31:46, dtapuska wrote: > > can we get away with an unordered_map? > > Not sure which one is better, the map will usually have 900 entries, and LoadLayout() takes 20ms on my Win8 VM. Unless we need ordering, and I don't think we do, I'd use unordered_map. We can trim down the map by establishing a lookup order e.g. if we don't find an entry for Code+Shift then we look up just Code and use that mapping, if present. Since the map should never exceed ~1000 entries we should be able to reserve buckets up-front and avoid reallocation & rehashing? Rather than the KeyMapEntry struct, can we just store two maps, one from Code+flags->Key and one from Code+flags->keyCode? https://codereview.chromium.org/1585193002/diff/60001/ui/events/keycodes/key_... ui/events/keycodes/key_map_win.h:38: KeyboardCode DomCodeToKeyboardCode(DomCode code) const; Don't we need |flags| passing in to this? e.g. the same DomCode generates VK_NUMPAD_5 or VK_CLEAR depending on the NumLock flag. https://codereview.chromium.org/1585193002/diff/60001/ui/events/keycodes/key_... ui/events/keycodes/key_map_win.h:41: DomCode KeyboardCodeToDomCode(KeyboardCode key_code) const; Is this used anywhere other than the test? If it's only for testing then name it <foo>ForTest so that presubmit will trap any cases where it's used in non-test code. If it's used in non-test code then should it also output the necessary |flags| for that particular DomCode to generate the desired |key_code| (see above)? https://codereview.chromium.org/1585193002/diff/60001/ui/events/keycodes/key_... ui/events/keycodes/key_map_win.h:52: void LoadLayout(HKL layout); nit: Rename this UpdateLayout() and have it check whether |keyboard_layout_ == layout| and early-exit if so?
https://codereview.chromium.org/1585193002/diff/60001/ui/events/keycodes/key_... File ui/events/keycodes/key_map_win.cc (right): https://codereview.chromium.org/1585193002/diff/60001/ui/events/keycodes/key_... ui/events/keycodes/key_map_win.cc:141: HKL current_layout = ::GetKeyboardLayout(0); On 2016/02/09 00:18:57, Wez wrote: > On 2016/02/04 at 21:10:25, chongz wrote: > > Still, should I add a lock or only allow it to be used by a certain thread? > > GetKeyboardLayout(0) gets the current layout for the thread on which it is > called - so just adding a lock could mean that calling code with multiple > threads ends up swapping back and forth between layouts, if the layouts differ. > It'd also required holding the lock while you populate the map, which is a > non-trivially slow operation during which you'd be blocking any other threads > that happened to call this API. > > Given that layouts are per-thread, could we require that the caller creates a > WindowsKeyMap instance on the stack before starting the MessageLoop, and having > a static CodeAndFlagsToDomKey() method (replacing DomCodeToSystemLayoutKey) that > fetches it from thread-local-storage, refreshes it if necessary, and calls > DomCode[AndFlags]ToKey() on it? Likely this code is probably only going to execute on one thread or so inside each process. We should create the TLS WindowsKeyMap lazily not at stack startup. Alternatively we can use a global ref counted WindowsKeyMap with an instance per each locale encountered; and have TLS point at the global map. We'd need to lock on the global table to lookup; and make WindowsKeyMap immutable. But then at least we'd share the ref across multiple threads if that does occur.
On 2016/02/09 16:46:39, dtapuska wrote: > https://codereview.chromium.org/1585193002/diff/60001/ui/events/keycodes/key_... > File ui/events/keycodes/key_map_win.cc (right): > > https://codereview.chromium.org/1585193002/diff/60001/ui/events/keycodes/key_... > ui/events/keycodes/key_map_win.cc:141: HKL current_layout = > ::GetKeyboardLayout(0); > On 2016/02/09 00:18:57, Wez wrote: > > On 2016/02/04 at 21:10:25, chongz wrote: > > > Still, should I add a lock or only allow it to be used by a certain thread? > > > > GetKeyboardLayout(0) gets the current layout for the thread on which it is > > called - so just adding a lock could mean that calling code with multiple > > threads ends up swapping back and forth between layouts, if the layouts > differ. > > It'd also required holding the lock while you populate the map, which is a > > non-trivially slow operation during which you'd be blocking any other threads > > that happened to call this API. > > > > Given that layouts are per-thread, could we require that the caller creates a > > WindowsKeyMap instance on the stack before starting the MessageLoop, and > having > > a static CodeAndFlagsToDomKey() method (replacing DomCodeToSystemLayoutKey) > that > > fetches it from thread-local-storage, refreshes it if necessary, and calls > > DomCode[AndFlags]ToKey() on it? > > Likely this code is probably only going to execute on one thread or so inside > each process. We should create the TLS WindowsKeyMap lazily not at stack > startup. Alternatively we can use a global ref counted WindowsKeyMap with an > instance per each locale encountered; and have TLS point at the global map. We'd > need to lock on the global table to lookup; and make WindowsKeyMap immutable. > But then at least we'd share the ref across multiple threads if that does occur. From my testing the WindowsKeyMap is always called by the same processId and one threadId even with multiple tabs and windows. It's called in 'desktop_window_tree_host_x11.cc', 'x11_window.cc', 'window_tree_host_x11.cc ' and 'event_utils.cc', so I guess normally we convert 'NativeEvent' to 'KeyEvent' in host and then send it to blink? Anyways I will make it per thread just in case.
https://codereview.chromium.org/1585193002/diff/60001/ui/events/keycodes/key_... File ui/events/keycodes/key_map_win.cc (right): https://codereview.chromium.org/1585193002/diff/60001/ui/events/keycodes/key_... ui/events/keycodes/key_map_win.cc:141: HKL current_layout = ::GetKeyboardLayout(0); On 2016/02/09 at 16:46:39, dtapuska wrote: > On 2016/02/09 00:18:57, Wez wrote: > > On 2016/02/04 at 21:10:25, chongz wrote: > > > Still, should I add a lock or only allow it to be used by a certain thread? > > > > GetKeyboardLayout(0) gets the current layout for the thread on which it is > > called - so just adding a lock could mean that calling code with multiple > > threads ends up swapping back and forth between layouts, if the layouts differ. > > It'd also required holding the lock while you populate the map, which is a > > non-trivially slow operation during which you'd be blocking any other threads > > that happened to call this API. > > > > Given that layouts are per-thread, could we require that the caller creates a > > WindowsKeyMap instance on the stack before starting the MessageLoop, and having > > a static CodeAndFlagsToDomKey() method (replacing DomCodeToSystemLayoutKey) that > > fetches it from thread-local-storage, refreshes it if necessary, and calls > > DomCode[AndFlags]ToKey() on it? > > Likely this code is probably only going to execute on one thread or so inside each process. We should create the TLS WindowsKeyMap lazily not at stack startup. Alternatively we can use a global ref counted WindowsKeyMap with an instance per each locale encountered; and have TLS point at the global map. We'd need to lock on the global table to lookup; and make WindowsKeyMap immutable. But then at least we'd share the ref across multiple threads if that does occur. On-demand thread-local instance SGTM - do we have a way of doing that without them leaking, though? If scanning the keymap is really slow then maintaining a cache would be useful; I expect it to be rare for a user to have a huge number of keymaps that they cycle through, but switching amongst a small number of layouts is common.
Hi wez, I've updated the CL, PTAL, thanks! https://codereview.chromium.org/1585193002/diff/80001/ui/events/keycodes/key_... File ui/events/keycodes/key_map_win.cc (right): https://codereview.chromium.org/1585193002/diff/80001/ui/events/keycodes/key_... ui/events/keycodes/key_map_win.cc:86: static base::ThreadLocalStorage::StaticSlot s_tls_key_map = TLS_INITIALIZER; I'm not sure I'm using it in the right way? https://codereview.chromium.org/1585193002/diff/80001/ui/events/keycodes/key_... ui/events/keycodes/key_map_win.cc:124: s_tls_key_map.Initialize(ThreadCleanupFunction); Should I add a lock here? Currently it's only used by the host application, but maybe someone will try to race with it in the future? https://codereview.chromium.org/1585193002/diff/80001/ui/events/keycodes/key_... ui/events/keycodes/key_map_win.cc:144: key_map_.reserve(1000); Could be less if we can use some fallback for the map later. https://codereview.chromium.org/1585193002/diff/80001/ui/events/keycodes/key_... File ui/events/keycodes/key_map_win.h (right): https://codereview.chromium.org/1585193002/diff/80001/ui/events/keycodes/key_... ui/events/keycodes/key_map_win.h:24: KeyMap; Currently only store DomCode+flags => DomKey, will add DomCode+flags => KeyboardCode when we actually need it. https://codereview.chromium.org/1585193002/diff/80001/ui/events/keycodes/key_... ui/events/keycodes/key_map_win.h:38: static DomKey CodeAndFlagsToDomKey(DomCode code, int flags /*EventFlags*/); I'm not sure how to name the above two functions... They are doing the similar thing but one is static?
https://codereview.chromium.org/1585193002/diff/80001/ui/events/keycodes/key_... File ui/events/keycodes/key_map_win.cc (right): https://codereview.chromium.org/1585193002/diff/80001/ui/events/keycodes/key_... 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 wrote: > I'm not sure I'm using it in the right way? From the other patterns this seems fine; although they do seem to use a lock to initialize the TLS. https://codereview.chromium.org/1585193002/diff/80001/ui/events/keycodes/key_... File ui/events/keycodes/key_map_win.h (right): https://codereview.chromium.org/1585193002/diff/80001/ui/events/keycodes/key_... ui/events/keycodes/key_map_win.h:38: static DomKey CodeAndFlagsToDomKey(DomCode code, int flags /*EventFlags*/); On 2016/02/10 01:25:10, chongz wrote: > I'm not sure how to name the above two functions... They are doing the similar > thing but one is static? Personally I'd name it DomCodeAndFlagsToDomKey and DomCodeAndFlagsToDomKeyStatic
Changed to use LazyInstance+TLS. PTAL, thanks!
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#new... ui/events/event.cc:41: #include "ui/events/keycodes/key_map_win.h" key_map_win.{cc|h} should be renamed to match the class name, or vice versa. e.g. you might rename the class to PlatformKeyMap, and then rename the files to platform_key_map_win.*, if you think we might create similar helper classes for other platforms - calling code would just see the platform-specific implementation as PlatformKeyMap under whatever build config said the platform is. https://codereview.chromium.org/1585193002/diff/100001/ui/events/keycodes/key... File ui/events/keycodes/key_map_win.cc (right): https://codereview.chromium.org/1585193002/diff/100001/ui/events/keycodes/key... ui/events/keycodes/key_map_win.cc:34: EF_SHIFT_DOWN, nit: This indentation doesn't match DomCodeEntry, above. https://codereview.chromium.org/1585193002/diff/100001/ui/events/keycodes/key... ui/events/keycodes/key_map_win.cc:74: if (flags & EF_NUM_LOCK_ON) nit: Consider adding a block comment to explain the difference in values we set for toggle and non-toggle modifiers. https://codereview.chromium.org/1585193002/diff/100001/ui/events/keycodes/key... ui/events/keycodes/key_map_win.cc:87: void TLSCleanupFunction(void* data) { nit: Style-guide prefers that even abbreviations or acronyms be CamelCased, which would make this TlsCleanupFunction. However, I'd also recommend renaming this to e.g. CleanupKeyMapTls(void*) or something, i.e. a name written to describe what it _does_ rather than what it _is_. https://codereview.chromium.org/1585193002/diff/100001/ui/events/keycodes/key... ui/events/keycodes/key_map_win.cc:88: WindowsKeyMap* key_map = static_cast<WindowsKeyMap*>(data); reinterpret_cast<> would be more appropriate here, I think, since this cast is from one pointer type to another. https://codereview.chromium.org/1585193002/diff/100001/ui/events/keycodes/key... ui/events/keycodes/key_map_win.cc:89: if (key_map) This if() is redundant - delete is fine to call on nullptr. https://codereview.chromium.org/1585193002/diff/100001/ui/events/keycodes/key... ui/events/keycodes/key_map_win.cc:109: // Trying to match Firefox's behavior and UIEvents DomKey guidelines. nit: Indentation looks wrong here? Is this what git cl format does? https://codereview.chromium.org/1585193002/diff/100001/ui/events/keycodes/key... ui/events/keycodes/key_map_win.cc:152: // Usually the map has ~1000 entries. nit: Have we actually verified exactly how many we get for a few sample layouts? https://codereview.chromium.org/1585193002/diff/100001/ui/events/keycodes/key... File ui/events/keycodes/key_map_win.h (right): https://codereview.chromium.org/1585193002/diff/100001/ui/events/keycodes/key... ui/events/keycodes/key_map_win.h:21: typedef std::unordered_map<std::pair<int /*DomCode*/, int /*EventFlags*/>, Why use an int here rather than DomCode? Feels like it would be cleaner to typedef the pair<DomCode,flags> to some helpful name and then typedef the unordered_map to work with that. Can this typedef be moved inside the WindowsKeyMap, so that it's not defined at the top-level of the ui namespace? Finally, KeyMap is a very generic name; this specific map is a DomCodeToKey map, effectively. You wouldn't use it e.g. to map from Key to Code or Code to KeyboardCode, for example. https://codereview.chromium.org/1585193002/diff/100001/ui/events/keycodes/key... ui/events/keycodes/key_map_win.h:26: class EVENTS_BASE_EXPORT WindowsKeyMap { nit: Aside from the explicit HKL constructor, the API of this class isn't actually OS-specific - it maps from generic DomCode+ui::EventFlags to DomKey, and the static API doesn't even require the Windows-specific HKL to be specified, so should we call this PlatformKeyMap in case we decided to add Linux or Mac equivalents later? https://codereview.chromium.org/1585193002/diff/100001/ui/events/keycodes/key... ui/events/keycodes/key_map_win.h:29: explicit WindowsKeyMap(HKL layout) : keyboard_layout_(0) { As per style-guide, don't declare method bodies inline, except for trivial getters/setters. https://codereview.chromium.org/1585193002/diff/100001/ui/events/keycodes/key... ui/events/keycodes/key_map_win.h:39: int flags /*EventFlags*/); I'd suggest renaming flags -> event_flags or even ui_event_flags and updating the comment to say something like "Returns the DomKey 'meaning' of |code| in the context of specified |ui_event_flags|." https://codereview.chromium.org/1585193002/diff/100001/ui/events/keycodes/key... ui/events/keycodes/key_map_win.h:42: WindowsKeyMap() : keyboard_layout_(0) {} See above re inlining. https://codereview.chromium.org/1585193002/diff/100001/ui/events/keycodes/key... ui/events/keycodes/key_map_win.h:46: HKL keyboard_layout_; This can use C++11 inline initialization: HKL keyboard_layout_ = 0; so that you don't need to initialize it explicitly in the constructor.
Hi Wez, I've renamed the file and moved typedefs inside. PTAL, thanks! https://codereview.chromium.org/1585193002/diff/120001/ui/events/keycodes/pla... File ui/events/keycodes/platform_key_map_win.cc (right): https://codereview.chromium.org/1585193002/diff/120001/ui/events/keycodes/pla... ui/events/keycodes/platform_key_map_win.cc:34: EF_SHIFT_DOWN, Yes cl-format wants 4-space indentation. I guess this belongs to 'Braced Initializer List Format'? https://google.github.io/styleguide/cppguide.html#Braced_Initializer_List_Format https://codereview.chromium.org/1585193002/diff/120001/ui/events/keycodes/pla... ui/events/keycodes/platform_key_map_win.cc:124: flags, Same as above. https://codereview.chromium.org/1585193002/diff/120001/ui/events/keycodes/pla... ui/events/keycodes/platform_key_map_win.cc:166: code_key_map_.reserve(500); Exact number for some sample layouts without Command, NumLock and ScrollLock. https://codereview.chromium.org/1585193002/diff/120001/ui/events/keycodes/pla... File ui/events/keycodes/platform_key_map_win.h (right): https://codereview.chromium.org/1585193002/diff/120001/ui/events/keycodes/pla... ui/events/keycodes/platform_key_map_win.h:21: class EVENTS_BASE_EXPORT PlatformKeyMap { Currently other platforms have their own way of getting DomKey. But I have discovered some issues, if it cannot be fixed I probably have to build the KeyMap for them as well. https://codereview.chromium.org/1585193002/diff/120001/ui/events/keycodes/pla... ui/events/keycodes/platform_key_map_win.h:48: struct DomCodeEventFlagsPairHash { Have to do this because we only have PairHash for int
LGTM once the remaining nits are dealt with. https://codereview.chromium.org/1585193002/diff/120001/ui/events/keycodes/pla... File ui/events/keycodes/platform_key_map_win.cc (right): https://codereview.chromium.org/1585193002/diff/120001/ui/events/keycodes/pla... ui/events/keycodes/platform_key_map_win.cc:92: // which is just one process and one thread. nit: This comment looks like it applies to the function, whereas it actually applies to the implementation in general - I'd suggest putting it next to the actual use of the platform_key_map_tls_lazy global, so that it's obvious at that point why we are using TLS. https://codereview.chromium.org/1585193002/diff/120001/ui/events/keycodes/pla... ui/events/keycodes/platform_key_map_win.cc:96: } nit: I wonder if you could use std::default_deleter<PlatformKeyMap*>::operator() rather than have to provide your own deleter function. https://codereview.chromium.org/1585193002/diff/120001/ui/events/keycodes/pla... ui/events/keycodes/platform_key_map_win.cc:108: platform_key_map_tls_lazy = LAZY_INSTANCE_INITIALIZER; nit: Suggest using the g_ prefix as suggested by style-guide, since that is a commonly used pattern in chromium. https://codereview.chromium.org/1585193002/diff/120001/ui/events/keycodes/pla... ui/events/keycodes/platform_key_map_win.cc:162: keyboard_layout_ = layout; nit: Suggest adding a comment here mentioning that this operation is expensive and that we can optimize it, w/ a bug filed for that, e.g. "// TODO(...): Optimize layout switching (see crbug.com/...)" Two optimizations we should consider are: 1. Caching previously-used layouts. 2. Avoiding adding entries to the map if the "fallback" entry has the same value. https://codereview.chromium.org/1585193002/diff/120001/ui/events/keycodes/pla... ui/events/keycodes/platform_key_map_win.cc:183: if (rv < 0) { The ToUnicodeEx specification says that it returns -1 for dead-keys, so this should be rv == -1 to guard against potential updates to that API adding new -ve return values. https://codereview.chromium.org/1585193002/diff/120001/ui/events/keycodes/pla... ui/events/keycodes/platform_key_map_win.cc:190: DCHECK(rv == 2); nit: DCHECK_EQ How will this code behave if the second call returns something other than 2? If it may misbehave in some way then consider using CHECK instead. https://codereview.chromium.org/1585193002/diff/120001/ui/events/keycodes/pla... File ui/events/keycodes/platform_key_map_win.h (right): https://codereview.chromium.org/1585193002/diff/120001/ui/events/keycodes/pla... ui/events/keycodes/platform_key_map_win.h:29: int ui_event_flags /*EventFlags*/) const; nit: No need for the EventFlags comment. https://codereview.chromium.org/1585193002/diff/120001/ui/events/keycodes/pla... ui/events/keycodes/platform_key_map_win.h:35: // 2. Or keyboard layout of current thread was changed. nit: Suggest replacing lines 3-5 with just: " // Updates a per-thread key map cache whenever the layout changes." which should be sufficient to get across that it's a non-trivial operation in some cases. nit: Related to this, should we provide an explicit function to trigger a layout update, that we can call in response to WM_INPUTLANGCHANGE, for example, so as to front-load the work ahead of any actual input? https://codereview.chromium.org/1585193002/diff/120001/ui/events/keycodes/pla... ui/events/keycodes/platform_key_map_win.h:38: int ui_event_flags /*EventFlags*/); nit: No need for the EventFlags comment here, please - should be obvious from the parameter name & context. https://codereview.chromium.org/1585193002/diff/120001/ui/events/keycodes/pla... ui/events/keycodes/platform_key_map_win.h:48: struct DomCodeEventFlagsPairHash { On 2016/02/12 at 16:04:03, chongz wrote: > Have to do this because we only have PairHash for int So ugly! OK... how about using std::pair<int,int> as the typedef, with a comment that we're treating DomCode as an int for simplicity? https://codereview.chromium.org/1585193002/diff/120001/ui/events/keycodes/pla... ui/events/keycodes/platform_key_map_win.h:58: DomCodeToKeyMap code_key_map_; nit: Suggest calling this code_to_key_map_ or just code_to_key_ (that it is a map should be obvious from context and type name)
chongz@chromium.org changed reviewers: + sadrul@chromium.org
Hi sadrul, can I have a review on ui/events/event.cc please? Hi dtapuska, can you take a look at the updated CL please? Thanks! https://codereview.chromium.org/1585193002/diff/140001/ui/events/keycodes/pla... File ui/events/keycodes/platform_key_map_win.cc (right): https://codereview.chromium.org/1585193002/diff/140001/ui/events/keycodes/pla... ui/events/keycodes/platform_key_map_win.cc:100: return new (instance) base::ThreadLocalStorage::Slot(CleanupKeyMapTls); Didn't find way to use std::default_delete... I guess std::default_delete::operator() has to be static in order to be pass in here? https://codereview.chromium.org/1585193002/diff/140001/ui/events/keycodes/pla... ui/events/keycodes/platform_key_map_win.cc:193: DCHECK_EQ(rv, 2); If |rv| is not 2 the result DomKey for this Dead Key would probably be wrong, but I guess we'd better not crash the app?
On 2016/02/16 20:18:59, chongz wrote: > Hi sadrul, can I have a review on ui/events/event.cc please? > > Hi dtapuska, can you take a look at the updated CL please? > > Thanks! > > https://codereview.chromium.org/1585193002/diff/140001/ui/events/keycodes/pla... > File ui/events/keycodes/platform_key_map_win.cc (right): > > https://codereview.chromium.org/1585193002/diff/140001/ui/events/keycodes/pla... > ui/events/keycodes/platform_key_map_win.cc:100: return new (instance) > base::ThreadLocalStorage::Slot(CleanupKeyMapTls); > Didn't find way to use std::default_delete... I guess > std::default_delete::operator() has to be static in order to be pass in here? > > https://codereview.chromium.org/1585193002/diff/140001/ui/events/keycodes/pla... > ui/events/keycodes/platform_key_map_win.cc:193: DCHECK_EQ(rv, 2); > If |rv| is not 2 the result DomKey for this Dead Key would probably be wrong, > but I guess we'd better not crash the app? lgtm
https://codereview.chromium.org/1585193002/diff/140001/ui/events/keycodes/pla... File ui/events/keycodes/platform_key_map_win.cc (right): https://codereview.chromium.org/1585193002/diff/140001/ui/events/keycodes/pla... 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 wrote: > Didn't find way to use std::default_delete... I guess std::default_delete::operator() has to be static in order to be pass in here? Yes, of course, you're right. :( What I really had in mind was using T::operator delete() (i.e. the function form of delete) here, but I'm not sure that that is possible either. https://codereview.chromium.org/1585193002/diff/140001/ui/events/keycodes/pla... ui/events/keycodes/platform_key_map_win.cc:193: DCHECK_EQ(rv, 2); On 2016/02/16 at 20:18:59, chongz wrote: > If |rv| is not 2 the result DomKey for this Dead Key would probably be wrong, but I guess we'd better not crash the app? If you don't expect the app to crash in that case, why are you DCHECK_EQ()ing here? The DCHECK expresses an invariant at this point in the code. It doesn't seem obvious to me that ToUnicodeEx() _should_ always return 2 in this case, even though I suspect in practice that it does. So perhaps you instead just want to ignore |rv| not being equal to two?
stamp lgtm
https://codereview.chromium.org/1585193002/diff/160001/ui/events/keycodes/pla... File ui/events/keycodes/platform_key_map_win.cc (right): https://codereview.chromium.org/1585193002/diff/160001/ui/events/keycodes/pla... ui/events/keycodes/platform_key_map_win.cc:100: return new (instance) base::ThreadLocalStorage::Slot(CleanupKeyMapTls); Still haven't find a good way to do it... Will fix it once I found a solution. https://codereview.chromium.org/1585193002/diff/160001/ui/events/keycodes/pla... ui/events/keycodes/platform_key_map_win.cc:193: if (rv == 2) { Sorry I got some misunderstanding about DCHECK, actually it shouldn't be used here. Removed.
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, dtapuska@chromium.org, sadrul@chromium.org Link to the patchset: https://codereview.chromium.org/1585193002/#ps160001 (title: "Remove DCHECK from UpdateLayout")
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
The CQ bit was unchecked by commit-bot@chromium.org
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/...)
The CQ bit was checked by chongz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dtapuska@chromium.org, sadrul@chromium.org, wez@chromium.org Link to the patchset: https://codereview.chromium.org/1585193002/#ps180001 (title: "Fix unittest")
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
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/7c406f8ec5f2a67b66bf363ea4e7aff5e698da3b Cr-Commit-Position: refs/heads/master@{#375812}
Message was sent while issue was closed.
brucedawson@chromium.org changed reviewers: + brucedawson@chromium.org
Message was sent while issue was closed.
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); 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?
Message was sent while issue was closed.
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?
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! |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
