Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1108893002:
Revise DOM Level 3 US Layout maps. (Closed)
|
Created:
5 years, 8 months ago by kpschoedel Modified:
5 years, 7 months ago Reviewers:
Wez CC:
chromium-reviews, jdduke+watch_chromium.org, tdresser+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRevise DOM Level 3 US Layout maps.
Changes to more closely match GetCharacterFromKeyCode().
BUG=44048
Committed: https://crrev.com/f44179f9d15cc019ffb235a936771c91db56e1ad
Cr-Commit-Position: refs/heads/master@{#329897}
Patch Set 1 #
Total comments: 8
Patch Set 2 : address review comments (wez@) #
Total comments: 2
Patch Set 3 : address review comments (wez@) #Patch Set 4 : rebase for keycodes/dom/ rename #
Messages
Total messages: 22 (7 generated)
kpschoedel@chromium.org changed reviewers: + wez@chromium.org
This CL has the table changes previously in https://codereview.chromium.org/1072193004/, plus VKEY_SEPARATOR per the last review comment there.
The CQ bit was checked by kpschoedel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1108893002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Looks like the BUG number is missing a digit?
https://codereview.chromium.org/1108893002/diff/1/ui/events/keycodes/dom_us_l... File ui/events/keycodes/dom_us_layout_data.h (right): https://codereview.chromium.org/1108893002/diff/1/ui/events/keycodes/dom_us_l... ui/events/keycodes/dom_us_layout_data.h:63: {DomCode::INTL_BACKSLASH, {'<', '>'}}, Why do we need this and INTL_YEN, below, if this is a US-only table? If we need these for some reason then note that this changes means that there's no single round-trip mapping for this table any more, since < and > exist on other keys under a US layout, for which this key has no meaning. I notice we don't have INTL_HASH in here, which is equivalent to these keys, and would also break the round-trip if added to this table? Suggest updating the comment on the table to clarify that it is used only by DomCodeToUsLayoutMeaning() to convert from a code to some sensible printable character. https://codereview.chromium.org/1108893002/diff/1/ui/events/keycodes/dom_us_l... ui/events/keycodes/dom_us_layout_data.h:596: // VKEYs with no directly corresponding DomCode, but a USB usage code: Why do we have a DomCode::<name> in the comment for these, if they have no corresponding DOM |code| value to map to? https://codereview.chromium.org/1108893002/diff/1/ui/events/keycodes/dom_us_l... ui/events/keycodes/dom_us_layout_data.h:598: // {DomCode::SEPARATOR, VKEY_SEPARATOR}, // 0x07009F Separator Remove this?
https://codereview.chromium.org/1108893002/diff/1/ui/events/keycodes/dom_us_l... File ui/events/keycodes/dom_us_layout_data.h (right): https://codereview.chromium.org/1108893002/diff/1/ui/events/keycodes/dom_us_l... ui/events/keycodes/dom_us_layout_data.h:63: {DomCode::INTL_BACKSLASH, {'<', '>'}}, On 2015/04/28 00:28:04, Wez wrote: > Why do we need this and INTL_YEN, below, if this is a US-only table? The keys are normally supported in US layouts, when a keyboard generating them is present. The pre-change value here ‘\|’ is what the XKB US layouts (from which I derived the table) generate, and has some history <http://commons.wikimedia.org/wiki/File:IBM_Model_F_XT.png>. The ‘<>’ value also has history <http://deskthority.net/wiki/File:LK201AA_top.jpg>, but the change here is because GetCharacterFromKeyCode() returns ‘<>’ for the corresponding VKEY_OEM_102. (MS helpfully says that VK_OEM_102 is ‘"<>" or "\|" on RT 102-key keyboard’.) I'd be comfortable using ‘\|’ and changing GetCharacterFromKeyCode() and the test to match. I'm pretty sure nothing actually depends on the mapping. > If we need these for some reason then note that this changes means that there's > no single round-trip mapping for this table any more, Right, but there has never been round-trip mapping for characters, because of numpad keys. > I notice we don't have INTL_HASH in here, which is equivalent to these keys, Added. It wasn't in keycode_converter_data.h, but should be now that we use it for DOM3 |code|, since 'IntlHash' exists there. > Suggest updating the comment on the table to clarify that it is used only by > DomCodeToUsLayoutMeaning() to convert from a code to some sensible printable > character. Done. https://codereview.chromium.org/1108893002/diff/1/ui/events/keycodes/dom_us_l... ui/events/keycodes/dom_us_layout_data.h:596: // VKEYs with no directly corresponding DomCode, but a USB usage code: On 2015/04/28 00:28:04, Wez wrote: > Why do we have a DomCode::<name> in the comment for these, if they have no > corresponding DOM |code| value to map to? Left over from the (scripted) table entries. Removed. https://codereview.chromium.org/1108893002/diff/1/ui/events/keycodes/dom_us_l... ui/events/keycodes/dom_us_layout_data.h:598: // {DomCode::SEPARATOR, VKEY_SEPARATOR}, // 0x07009F Separator On 2015/04/28 00:28:04, Wez wrote: > Remove this? Done.
https://codereview.chromium.org/1108893002/diff/1/ui/events/keycodes/dom_us_l... File ui/events/keycodes/dom_us_layout_data.h (right): https://codereview.chromium.org/1108893002/diff/1/ui/events/keycodes/dom_us_l... ui/events/keycodes/dom_us_layout_data.h:63: {DomCode::INTL_BACKSLASH, {'<', '>'}}, On 2015/04/28 17:03:45, kpschoedel wrote: > On 2015/04/28 00:28:04, Wez wrote: > > Why do we need this and INTL_YEN, below, if this is a US-only table? > > The keys are normally supported in US layouts, when a keyboard generating them > is present. The pre-change value here ‘\|’ is what the XKB US layouts (from > which I derived the table) generate, and has some history > <http://commons.wikimedia.org/wiki/File:IBM_Model_F_XT.png>. The ‘<>’ value also > has history <http://deskthority.net/wiki/File:LK201AA_top.jpg>, but the change > here is because GetCharacterFromKeyCode() returns ‘<>’ for the corresponding > VKEY_OEM_102. Mapping from VKEY to |code| is a legacy-compatibility special-case, so we should be best-effort about supporting it. To do this "properly" we'd need to know the keyboard's physical layout so we can un-pick the VKEY or OEM-scancode back into the "correct" USB/DOM-code value... :( > (MS helpfully says that VK_OEM_102 is ‘"<>" or "\|" on RT 102-key > keyboard’.) I'd be comfortable using ‘\|’ and changing GetCharacterFromKeyCode() > and the test to match. I'm pretty sure nothing actually depends on the mapping. > > > If we need these for some reason then note that this changes means that > there's > > no single round-trip mapping for this table any more, > > Right, but there has never been round-trip mapping for characters, because of > numpad keys. > > > I notice we don't have INTL_HASH in here, which is equivalent to these keys, > > Added. It wasn't in keycode_converter_data.h, but should be now that we use it > for DOM3 |code|, since 'IntlHash' exists there. > > > Suggest updating the comment on the table to clarify that it is used only by > > DomCodeToUsLayoutMeaning() to convert from a code to some sensible printable > > character. > > Done. https://codereview.chromium.org/1108893002/diff/20001/ui/events/keycodes/dom4... File ui/events/keycodes/dom4/keycode_converter_data.h (right): https://codereview.chromium.org/1108893002/diff/20001/ui/events/keycodes/dom4... ui/events/keycodes/dom4/keycode_converter_data.h:125: USB_KEYMAP(0x070032, 0x0000, 0x0000, 0xffff, "IntlHash", INTL_HASH), Ick... I forgot that the platforms are munging the USB codes for the two keys together into the same platform-specific codes. Note that we have a special-case for the USB-to-native code mapping (see https://code.google.com/p/chromium/codesearch#chromium/src/ui/events/keycodes...) to deal with this and a special-case for Mac - looks like that would be useful for DOM-code as well, for consistency? If we need to generate the right DOM-code strings for these platform values we'll need an ugly special-case for these keys that generates Backslash or IntlHash as the |code| values, dependent on the physical keyboard layout - gross. :(
https://codereview.chromium.org/1108893002/diff/1/ui/events/keycodes/dom_us_l... File ui/events/keycodes/dom_us_layout_data.h (right): https://codereview.chromium.org/1108893002/diff/1/ui/events/keycodes/dom_us_l... ui/events/keycodes/dom_us_layout_data.h:63: {DomCode::INTL_BACKSLASH, {'<', '>'}}, On 2015/05/05 00:16:00, Wez wrote: > On 2015/04/28 17:03:45, kpschoedel wrote: > > On 2015/04/28 00:28:04, Wez wrote: > > > Why do we need this and INTL_YEN, below, if this is a US-only table? > > > > The keys are normally supported in US layouts, when a keyboard generating them > > is present. The pre-change value here ‘\|’ is what the XKB US layouts (from > > which I derived the table) generate, and has some history > > <http://commons.wikimedia.org/wiki/File:IBM_Model_F_XT.png>. The ‘<>’ value > also > > has history <http://deskthority.net/wiki/File:LK201AA_top.jpg>, but the change > > here is because GetCharacterFromKeyCode() returns ‘<>’ for the corresponding > > VKEY_OEM_102. > > Mapping from VKEY to |code| is a legacy-compatibility special-case, so we should > be best-effort about supporting it. To do this "properly" we'd need to know the > keyboard's physical layout so we can un-pick the VKEY or OEM-scancode back into > the "correct" USB/DOM-code value... :( > > > (MS helpfully says that VK_OEM_102 is ‘"<>" or "\|" on RT 102-key > > keyboard’.) I'd be comfortable using ‘\|’ and changing > GetCharacterFromKeyCode() > > and the test to match. I'm pretty sure nothing actually depends on the > mapping. > > > > > If we need these for some reason then note that this changes means that > > there's > > > no single round-trip mapping for this table any more, > > > > Right, but there has never been round-trip mapping for characters, because of > > numpad keys. > > > > > I notice we don't have INTL_HASH in here, which is equivalent to these keys, > > > > Added. It wasn't in keycode_converter_data.h, but should be now that we use it > > for DOM3 |code|, since 'IntlHash' exists there. > > > > > Suggest updating the comment on the table to clarify that it is used only by > > > DomCodeToUsLayoutMeaning() to convert from a code to some sensible printable > > > character. > > > > Done. Acknowledged — will keep GetCharacterFromKeyCode()'s values. https://codereview.chromium.org/1108893002/diff/20001/ui/events/keycodes/dom4... File ui/events/keycodes/dom4/keycode_converter_data.h (right): https://codereview.chromium.org/1108893002/diff/20001/ui/events/keycodes/dom4... ui/events/keycodes/dom4/keycode_converter_data.h:125: USB_KEYMAP(0x070032, 0x0000, 0x0000, 0xffff, "IntlHash", INTL_HASH), On 2015/05/05 00:16:00, Wez wrote: > Ick... I forgot that the platforms are munging the USB codes for the two keys > together into the same platform-specific codes. > > Note that we have a special-case for the USB-to-native code mapping (see > https://code.google.com/p/chromium/codesearch#chromium/src/ui/events/keycodes...) > to deal with this and a special-case for Mac - looks like that would be useful > for DOM-code as well, for consistency? Done for DomCodeToNativeKeycode() just by calling UsbKeycodeToNativeKeycode(). CodeToNativeKeycode(), which predates the enum and takes a string, is now used only in tests, so I propose converting those and removing the function (in a separate CL). > If we need to generate the right DOM-code strings for these platform values > we'll need an ugly special-case for these keys that generates Backslash or > IntlHash as the |code| values, dependent on the physical keyboard layout - > gross. :( *Fortunately* I see no practical way to distinguish them.
Ping for this review.
lgtm
The CQ bit was checked by kpschoedel@chromium.org to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from wez@chromium.org Link to the patchset: https://codereview.chromium.org/1108893002/#ps60001 (title: "rebase for keycodes/dom/ rename")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1108893002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by kpschoedel@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1108893002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/f44179f9d15cc019ffb235a936771c91db56e1ad Cr-Commit-Position: refs/heads/master@{#329897} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
