Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
280413002:
Fix handling of rewritten-Alt + Button1 -> Button3. (Closed)
|
Created:
6 years, 7 months ago by kpschoedel Modified:
6 years, 6 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 Visibility:
Public. |
DescriptionMake Alt+Button1 -> Button3 respect remapped modifiers.
Update native modifier state to reflect the rewritten ui::Event state before the X11 mouse code uses it.
Also splits code for the various ui::Event subclasses out of RewriteEvent() for clarity.
BUG=372485
R=sadrul@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=274568
Patch Set 1 #
Total comments: 6
Patch Set 2 : Add unit test #
Total comments: 1
Patch Set 3 : rebase #Patch Set 4 : review comments #
Total comments: 5
Patch Set 5 : review comments #
Total comments: 2
Messages
Total messages: 22 (0 generated)
oshima, selected you as reviewer since you're affected. sadrul, fyi. https://codereview.chromium.org/280413002/diff/1/chrome/browser/chromeos/even... File chrome/browser/chromeos/events/event_rewriter.cc (right): https://codereview.chromium.org/280413002/diff/1/chrome/browser/chromeos/even... chrome/browser/chromeos/events/event_rewriter.cc:373: #if defined(USE_X11) adopted from ash::StickKeysHandler::AppendModifier() https://codereview.chromium.org/280413002/diff/1/chrome/browser/chromeos/even... chrome/browser/chromeos/events/event_rewriter.cc:733: UpdateX11EventMask(*flags, Essential to get the native flags for the remapped ui::EventFlags before testing them below; this was the proximate cause for the Alt+Button1 symptom. https://codereview.chromium.org/280413002/diff/1/chrome/browser/chromeos/even... File chrome/browser/chromeos/events/event_rewriter.h (right): https://codereview.chromium.org/280413002/diff/1/chrome/browser/chromeos/even... chrome/browser/chromeos/events/event_rewriter.h:150: // Rewrite a particular kind of event. Split these out of |RewriteEvent()| as in the sticky CL because the function was getting long.
I'm not familiar with the code, so I'll leave this to sadrul. Just one request. Can you briefly explain what was the regression in the bug description?
Can you include some details about the fix in the CL description too? And a test? https://codereview.chromium.org/280413002/diff/1/chrome/browser/chromeos/even... File chrome/browser/chromeos/events/event_rewriter.cc (right): https://codereview.chromium.org/280413002/diff/1/chrome/browser/chromeos/even... chrome/browser/chromeos/events/event_rewriter.cc:337: (status != ui::EVENT_REWRITE_CONTINUE)) { status here is always _CONTINUE, right? https://codereview.chromium.org/280413002/diff/1/chrome/browser/chromeos/even... chrome/browser/chromeos/events/event_rewriter.cc:338: if (status == ui::EVENT_REWRITE_CONTINUE) ditto
https://codereview.chromium.org/280413002/diff/1/chrome/browser/chromeos/even... File chrome/browser/chromeos/events/event_rewriter.cc (right): https://codereview.chromium.org/280413002/diff/1/chrome/browser/chromeos/even... chrome/browser/chromeos/events/event_rewriter.cc:337: (status != ui::EVENT_REWRITE_CONTINUE)) { On 2014/05/14 03:06:01, sadrul wrote: > status here is always _CONTINUE, right? Yes, I forgot to remove that when I copied this code back from the sticky keys CL.
https://codereview.chromium.org/280413002/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/events/event_rewriter.cc (right): https://codereview.chromium.org/280413002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/events/event_rewriter.cc:337: ui::KeyEvent* rewritten_key_event = new ui::KeyEvent(key_event); You need to set status to REWRITTEN here? Do we have a test that fails because of this omission? If not, can we add one? Let's instead early return CONTINUE above after line 336, and return REWRITTEN below in line ~356. (ditto for RewriteMouse and RewriteTouch cases)
On 2014/05/20 16:46:54, sadrul wrote: > You need to set status to REWRITTEN here? Do we have a test that fails because > of this omission? If not, can we add one? Currently part of EventRewriterAshTest would fail with that mistake.
Addressed latest comments.
https://codereview.chromium.org/280413002/diff/90001/chrome/browser/chromeos/... File chrome/browser/chromeos/events/event_rewriter.cc (right): https://codereview.chromium.org/280413002/diff/90001/chrome/browser/chromeos/... chrome/browser/chromeos/events/event_rewriter.cc:353: XKeyEvent* xkey = &(xev->xkey); Add a CHECK here that xev->type == KeyPress or KeyRelease https://codereview.chromium.org/280413002/diff/90001/chrome/browser/chromeos/... chrome/browser/chromeos/events/event_rewriter.cc:731: reinterpret_cast<unsigned int*>(&xievent->mods.effective)); Should this happen after |flags| is changed in lines 737-738 below (so, after line 751)?
https://codereview.chromium.org/280413002/diff/90001/chrome/browser/chromeos/... File chrome/browser/chromeos/events/event_rewriter.cc (right): https://codereview.chromium.org/280413002/diff/90001/chrome/browser/chromeos/... chrome/browser/chromeos/events/event_rewriter.cc:353: XKeyEvent* xkey = &(xev->xkey); On 2014/06/02 00:19:52, sadrul wrote: > Add a CHECK here that xev->type == KeyPress or KeyRelease Done. https://codereview.chromium.org/280413002/diff/90001/chrome/browser/chromeos/... chrome/browser/chromeos/events/event_rewriter.cc:731: reinterpret_cast<unsigned int*>(&xievent->mods.effective)); On 2014/06/02 00:19:52, sadrul wrote: > Should this happen after |flags| is changed in lines 737-738 below (so, after > line 751)? No, this is updating the native (X11) flags so that the test at 736 below uses the results of key rewriting, if there was any. One of the upcoming CLs (post support for XI2 key events) replaces this whole section with generic ui::Event tests. https://codereview.chromium.org/280413002/diff/110001/chrome/browser/chromeos... File chrome/browser/chromeos/events/event_rewriter.cc (right): https://codereview.chromium.org/280413002/diff/110001/chrome/browser/chromeos... chrome/browser/chromeos/events/event_rewriter.cc:140: {ui::EF_COMMAND_DOWN, Mod4Mask}, From rebase. https://codereview.chromium.org/280413002/diff/110001/chrome/browser/chromeos... File chrome/browser/chromeos/events/event_rewriter_unittest.cc (right): https://codereview.chromium.org/280413002/diff/110001/chrome/browser/chromeos... chrome/browser/chromeos/events/event_rewriter_unittest.cc:41: std::string GetExpectedResultAsString(ui::KeyboardCode ui_keycode, All changes to this file in this patch set are from rebasing.
LGTM https://codereview.chromium.org/280413002/diff/90001/chrome/browser/chromeos/... File chrome/browser/chromeos/events/event_rewriter.cc (right): https://codereview.chromium.org/280413002/diff/90001/chrome/browser/chromeos/... chrome/browser/chromeos/events/event_rewriter.cc:731: reinterpret_cast<unsigned int*>(&xievent->mods.effective)); On 2014/06/02 00:52:36, kpschoedel wrote: > On 2014/06/02 00:19:52, sadrul wrote: > > Should this happen after |flags| is changed in lines 737-738 below (so, after > > line 751)? > > No, this is updating the native (X11) flags so that the test at 736 below uses > the results of key rewriting, if there was any. In that case, do we need to UpdateX11EventMask() again after |flags| is changed in lines 737-738? > > One of the upcoming CLs (post support for XI2 key events) replaces this whole > section with generic ui::Event tests.
On 2014/06/02 00:58:47, sadrul wrote: > In that case, do we need to UpdateX11EventMask() again after |flags| is changed > in lines 737-738? No, the code there from the original (at 741-742) updates the native mask directly. I left it alone since it's all about to go away anyway.
The CQ bit was checked by kpschoedel@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kpschoedel@chromium.org/280413002/110001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
Oshima, PTAL (I forgot Sadrul isn't in OWNERS here).
derat, would you have time for a quick look at this?
lgtm
The CQ bit was checked by kpschoedel@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kpschoedel@chromium.org/280413002/110001
Message was sent while issue was closed.
Change committed as 274568 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
