Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
446693005:
Support non-US layout for rewriting Numpad key events. (Closed)
|
Created:
6 years, 4 months ago by Shu Chen Modified:
6 years, 4 months ago CC:
chromium-reviews, stevenjb+watch_chromium.org, davemoore+watch_chromium.org, oshima+watch_chromium.org, nkostylev+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionSupport non-US layout for rewriting Numpad key events.
BUG=335644
TEST=Verified on linux_chromeos.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=288324
Patch Set 1 #Patch Set 2 : handled Shift+NumpadKey. #
Total comments: 4
Patch Set 3 : fixed test failures, change to safer solution to use set_character(). #
Total comments: 4
Patch Set 4 : modified per review comments. #
Total comments: 5
Patch Set 5 : nit. #Messages
Total messages: 13 (0 generated)
Hi Oshima, can you please review this cl? Thanks
+yusukes@ for risks of breaking certain external keyboards (for NumLock stuff).
I'm not familiar with keyboard stuff, so I'll approve if yusukes@ approves.
LGTM w/ comments below: https://codereview.chromium.org/446693005/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/events/event_rewriter.cc (right): https://codereview.chromium.org/446693005/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/events/event_rewriter.cc:215: // The X11 keycode for numpad keys should always NOT be changed because Would you mind adding tests for this? (numpad, numpad+shift, and non-numpad cases). Since the change is very subtle, it's probably difficult to prevent regressions unless you add some tests now. https://codereview.chromium.org/446693005/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/events/event_rewriter.cc:217: // The original X11 keycode should be correct to get the correct keysym. This works because ui:: keycode is for accelerators and X11's is for text input, right? If so, please mention that.
Yusuke, thanks for you review. Can you please take another look at the latest patch set? I've changed the solution to solve the problem. The necessity of adding test is reduced. Thanks, Shu https://codereview.chromium.org/446693005/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/events/event_rewriter.cc (right): https://codereview.chromium.org/446693005/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/events/event_rewriter.cc:215: // The X11 keycode for numpad keys should always NOT be changed because On 2014/08/08 01:04:09, Yusuke Sato wrote: > Would you mind adding tests for this? (numpad, numpad+shift, and non-numpad > cases). > > Since the change is very subtle, it's probably difficult to prevent regressions > unless you add some tests now. > Actually there are test failure for this change. I've changed the solution to much safer one, which avoid diverge between native_event's keycode and ui::KeyEvent's key_code. To touch the new logic by tests, we should setup non-US keyboard layout on X11 server, which is hard. So I would prefer to skip the test for the new solution. https://codereview.chromium.org/446693005/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/events/event_rewriter.cc:217: // The original X11 keycode should be correct to get the correct keysym. On 2014/08/08 01:04:09, Yusuke Sato wrote: > This works because ui:: keycode is for accelerators and X11's is for text input, > right? If so, please mention that. With new solution, no need to add such comments.
LGTM https://codereview.chromium.org/446693005/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/events/event_rewriter.cc (right): https://codereview.chromium.org/446693005/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/events/event_rewriter.cc:236: numpad_xevent.xkey.state &= ~ShiftMask; Please explain (as code comment) why we need to remove ShiftMask. I already know because I chatted with you but other reviewers might not know. https://codereview.chromium.org/446693005/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/events/event_rewriter.cc:237: numpad_xevent.xkey.state |= Mod2Mask; Mention that Mod2Mask is NumLock to make the code easier to read?
Thanks for staying late for review, Yusuke. Oshima, can you please approve this cl? Thanks https://codereview.chromium.org/446693005/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/events/event_rewriter.cc (right): https://codereview.chromium.org/446693005/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/events/event_rewriter.cc:236: numpad_xevent.xkey.state &= ~ShiftMask; On 2014/08/08 06:00:32, Yusuke Sato wrote: > Please explain (as code comment) why we need to remove ShiftMask. I already know > because I chatted with you but other reviewers might not know. Done. https://codereview.chromium.org/446693005/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/events/event_rewriter.cc:237: numpad_xevent.xkey.state |= Mod2Mask; On 2014/08/08 06:00:32, Yusuke Sato wrote: > Mention that Mod2Mask is NumLock to make the code easier to read? Done.
lgtm with nits https://codereview.chromium.org/446693005/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/events/event_rewriter.cc (right): https://codereview.chromium.org/446693005/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/events/event_rewriter.cc:215: unsigned int original_x11_keycode = xkeyevent.xkey.keycode; nit: move this after line 226 (closer to where this is really used) https://codereview.chromium.org/446693005/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/events/event_rewriter.cc:240: numpad_xevent.xkey.state |= Mod2Mask; // Always set NumLock mask. nit: two spaces between ; and //
https://codereview.chromium.org/446693005/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/events/event_rewriter.cc (right): https://codereview.chromium.org/446693005/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/events/event_rewriter.cc:215: unsigned int original_x11_keycode = xkeyevent.xkey.keycode; On 2014/08/08 07:06:08, oshima wrote: > nit: move this after line 226 (closer to where this is really used) That will result in wrong behavior. Please notice xkeyevent.xkey.keycode will be changed at line 221. https://codereview.chromium.org/446693005/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/events/event_rewriter.cc:240: numpad_xevent.xkey.state |= Mod2Mask; // Always set NumLock mask. On 2014/08/08 07:06:08, oshima wrote: > nit: two spaces between ; and // Done.
The CQ bit was checked by shuchen@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/shuchen@chromium.org/446693005/80001
slgtm https://codereview.chromium.org/446693005/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/events/event_rewriter.cc (right): https://codereview.chromium.org/446693005/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/events/event_rewriter.cc:215: unsigned int original_x11_keycode = xkeyevent.xkey.keycode; On 2014/08/08 07:10:57, Shu Chen wrote: > On 2014/08/08 07:06:08, oshima wrote: > > nit: move this after line 226 (closer to where this is really used) > > That will result in wrong behavior. Please notice xkeyevent.xkey.keycode will be > changed at line 221. oh, sorry I missed that. please ignore.
Message was sent while issue was closed.
Change committed as 288324 |
