Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1072193004:
Take GetCharacterFromKeyCode() out behind the barn. (Closed)
|
Created:
5 years, 8 months ago by kpschoedel Modified:
5 years, 3 months ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, creis+watch_chromium.org, tdresser+watch_chromium.org, tfarina, nasko+codewatch_chromium.org, darin-cc_chromium.org, mkwst+moarreviews-renderer_chromium.org, kalyank, jdduke+watch_chromium.org, ozone-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove GetCharacterFromKeyCode().
Remaining uses of GetCharacterFromKeyCode() are replaced with
DomCodeToUsLayoutMeaning() to make explicit that the mapping uses a
fixed US English layout and not the user's current layout, and to use
ui::DomCode in preference to the legacy Windows-based ui::KeyboardCode.
BUG=444048
Committed: https://crrev.com/121b8680676123f617ebc69770cbb114c030fbd3
Cr-Commit-Position: refs/heads/master@{#326553}
Committed: https://crrev.com/db1a9b2be68e046fdad81b4f17deb9fdc515e94e
Cr-Commit-Position: refs/heads/master@{#347147}
Patch Set 1 #
Total comments: 6
Patch Set 2 : address review comments (Wez) #Patch Set 3 : cleanup #Patch Set 4 : fix test #Patch Set 5 : rebase #
Messages
Total messages: 40 (15 generated)
kpschoedel@chromium.org changed reviewers: + sadrul@chromium.org, wez@chromium.org
Could you explain in your CL description, what are you doing in this change? From the subject only, I have no idea. Thanks!
Great to get rid of this, but it seems we're using these US-layout hard-coded APIs in code that is already in-use, so are we regressing anything here - or are those code paths already broken in being hard-coded to US English right now? https://codereview.chromium.org/1072193004/diff/1/ui/events/ozone/layout/xkb/... File ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc (right): https://codereview.chromium.org/1072193004/diff/1/ui/events/ozone/layout/xkb/... ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc:791: // If the Control key is down, only allow C0 characters to be returned, C0 characters..? https://codereview.chromium.org/1072193004/diff/1/ui/events/test/event_genera... File ui/events/test/event_generator.cc (right): https://codereview.chromium.org/1072193004/diff/1/ui/events/test/event_genera... ui/events/test/event_generator.cc:544: ui::UsLayoutKeyboardCodeToDomCode(key_code), flags); This changes the behaviour slightly - e.g. the top-level alpha key on a PC keyboard will generate Q (US English) or A (French layout) with the old code, since it's based on the VKEY, while this implementation will always generate Q. Is that an issue? More generally, is the fact that this code path is US English only an issue right now? https://codereview.chromium.org/1072193004/diff/1/ui/events/test/event_genera... ui/events/test/event_generator.cc:557: TestKeyEvent keyev(native_event, flags); nit: Hadn't noticed this before... should we be (ab)using TestKeyEvent in this way? nit: Also, it looks like this chunk of code could be simplified to remove key_press, and to avoid overwriting key_code, pretty easily, to make things more readable.
On 2015/04/21 22:08:46, Wez wrote: > Great to get rid of this, but it seems we're using these US-layout hard-coded > APIs in code that is already in-use, so are we regressing anything here - or are > those code paths already broken in being hard-coded to US English right now? GetCharacterFromKeyCode() [delete from keyboard_code_conversion.cc here] was hard-coded US English. There should be no functional changes from this CL.
lgtm
https://codereview.chromium.org/1072193004/diff/1/ui/events/ozone/layout/xkb/... File ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc (right): https://codereview.chromium.org/1072193004/diff/1/ui/events/ozone/layout/xkb/... ui/events/ozone/layout/xkb/xkb_keyboard_layout_engine.cc:791: // If the Control key is down, only allow C0 characters to be returned, On 2015/04/21 22:08:46, Wez wrote: > C0 characters..? Standardese for ASCII control characters. Changed the comment. https://codereview.chromium.org/1072193004/diff/1/ui/events/test/event_genera... File ui/events/test/event_generator.cc (right): https://codereview.chromium.org/1072193004/diff/1/ui/events/test/event_genera... ui/events/test/event_generator.cc:544: ui::UsLayoutKeyboardCodeToDomCode(key_code), flags); On 2015/04/21 22:08:46, Wez wrote: > This changes the behaviour slightly - e.g. the top-level alpha key on a PC > keyboard will generate Q (US English) or A (French layout) with the old code, > since it's based on the VKEY, while this implementation will always generate Q. > > Is that an issue? More generally, is the fact that this code path is US English > only an issue right now? GetCharacterFromKeyCode() was a hard-coded US layout as well. |DomCodeToUsLayoutCharacter(UsLayoutKeyboardCodeToDomCode())| should always return the same character as |GetCharacterFromKeyCode()| did, but I'll write a temporary test to verify that before landing this change. https://codereview.chromium.org/1072193004/diff/1/ui/events/test/event_genera... ui/events/test/event_generator.cc:557: TestKeyEvent keyev(native_event, flags); On 2015/04/21 22:08:46, Wez wrote: > nit: Hadn't noticed this before... should we be (ab)using TestKeyEvent in this > way? > > nit: Also, it looks like this chunk of code could be simplified to remove > key_press, and to avoid overwriting key_code, pretty easily, to make things more > readable. I've planned to visit this for crbug.com/444045
rs 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/1072193004/#ps20001 (title: "address review comments (Wez)")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1072193004/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
There are two instances where the DomCodeToUsLayoutCharacter(UsLayoutKeyboardCodeToDomCode()) combination differs from GetCharacterFromKeyCode(). First, for VKEY_DELETE, GetCharacterFromKeyCode() returned NUL, while the new code returns DEL. I suspect this was an oversight in GetCharacterFromKeyCode(), but will change to match if any tests break. Second, for VKEY_SEPARATOR, GetCharacterFromKeyCode() returns ‘,’ while the new code returns NUL, because there is no DOM Level 3 |code| representation for this. Fortunately there seems to be no relevant use of VKEY_SEPARATOR.
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 sadrul@chromium.org, wez@chromium.org Link to the patchset: https://codereview.chromium.org/1072193004/#ps40001 (title: "cleanup")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1072193004/40001
kpschoedel@chromium.org changed reviewers: + jam@chromium.org
+jam@ for content/renderer/render_view_browsertest.cc (Reassign if desired; you appear most often in the files' commit log.)
On 2015/04/22 16:06:59, kpschoedel wrote: > There are two instances where the > DomCodeToUsLayoutCharacter(UsLayoutKeyboardCodeToDomCode()) combination differs > from GetCharacterFromKeyCode(). > > First, for VKEY_DELETE, GetCharacterFromKeyCode() returned NUL, while the new > code returns DEL. I suspect this was an oversight in GetCharacterFromKeyCode(), > but will change to match if any tests break. SGTM; probably better to update the tests? > Second, for VKEY_SEPARATOR, GetCharacterFromKeyCode() returns ‘,’ while the new > code returns NUL, because there is no DOM Level 3 |code| representation for > this. Fortunately there seems to be no relevant use of VKEY_SEPARATOR. VKEY_SEPARATOR is the decimal key on the numpad, when in NumLock mode, IIRC, so DOM |code| will call that "NumpadDecimal". We should actually be generating a |code| of NumpadDecimal for that key even in non-NumLock mode (when its |keyCode| will be VKEY_DELETE, but its flags indicate it's a numpad key).
On 2015/04/22 17:43:30, kpschoedel wrote: > +jam@ for content/renderer/render_view_browsertest.cc > (Reassign if desired; you appear most often in the files' commit log.) 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 sadrul@chromium.org, wez@chromium.org Link to the patchset: https://codereview.chromium.org/1072193004/#ps60001 (title: "fix test")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1072193004/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/1072193004/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/121b8680676123f617ebc69770cbb114c030fbd3 Cr-Commit-Position: refs/heads/master@{#326553}
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.chromium.org/1085733007/ by kpschoedel@chromium.org. The reason for reverting is: MenuControllerMnemonicTestMnemonicMatch.MnemonicMatch failure in http://master.chrome.corp.google.com:8011/builders/cros%20trunk/builds/32987 https://code.google.com/p/chromium/issues/detail?id=480638 BUG=480638,444048.
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/1072193004/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1072193004/80001
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
The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org, jam@chromium.org, wez@chromium.org Link to the patchset: https://codereview.chromium.org/1072193004/#ps80001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1072193004/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1072193004/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/db1a9b2be68e046fdad81b4f17deb9fdc515e94e Cr-Commit-Position: refs/heads/master@{#347147} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
