Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
598733002:
Support search as a sticky key. (Closed)
|
Created:
6 years, 3 months ago by David Tseng Modified:
6 years, 2 months ago CC:
chromium-reviews, extensions-reviews_chromium.org, sadrul, nkostylev+watch_chromium.org, plundblad+watch_chromium.org, aboxhall+watch_chromium.org, yuzo+watch_chromium.org, dtseng+watch_chromium.org, arv+watch_chromium.org, oshima+watch_chromium.org, chromium-apps-reviews_chromium.org, kalyank, stevenjb+watch_chromium.org, ben+ash_chromium.org, dmazzoni+watch_chromium.org, dmazzoni, Peter Lundblad Base URL:
https://chromium.googlesource.com/chromium/src.git@lkcr Project:
chromium Visibility:
Public. |
DescriptionSupport search as a sticky key.
As an important modifier on Chrome OS, this cl makes it possible to use sequenced and locked sticky key modes with the search key.
BUG=416964
TEST=manual
Committed: https://crrev.com/151aef63430e196c0bdbf1ad1e0706a5c8e484a9
Cr-Commit-Position: refs/heads/master@{#297054}
Patch Set 1 : #Patch Set 2 : #
Total comments: 6
Patch Set 3 : Address comments. #Patch Set 4 : Add Search+Left test. #
Messages
Total messages: 30 (15 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
dtseng@chromium.org changed reviewers: + tengs@chromium.org
tengs: please take a quick look. I have yet to add tests, but will be happy to do so once you agree with this cl. I don't know if there was a particular reason why search was left out of the sticky keys feature, but running this by you first.
dtseng@chromium.org changed reviewers: + finnur@chromium.org
+ finnur@. I made some changes related to sticky key as it relates to extension commands. The behavior, I think we want here is to avoid further rewriting of keys after sticky key has finished processing it. For example, search+left if hit in sequence (search key down and key up, followed by left arrow, key down and key up) would trigger the extensions command API for Search+Left.
https://codereview.chromium.org/598733002/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/events/event_rewriter.cc (right): https://codereview.chromium.org/598733002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/events/event_rewriter.cc:403: IsExtensionCommandRegistered(state.key_code, state.flags); This block should probably be below line 405. https://codereview.chromium.org/598733002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/events/event_rewriter.cc:408: // binding the command. This comment seems a little out of place here. Why is search (note: Search) referred to here specifically? It wouldn't be the only modifier we don't want to rewrite, right? https://codereview.chromium.org/598733002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/events/event_rewriter.cc:410: if (!(key_event.flags() & ui::EF_FINAL)) { The indentation here is now wrong, but we anyway prefer: if (!foo && !bar) ... ... over ... if (!foo) { if (!bar) { ...
Patchset #3 (id:80001) has been deleted
Patchset #3 (id:100001) has been deleted
Patchset #3 (id:120001) has been deleted
Patchset #3 (id:140001) has been deleted
https://codereview.chromium.org/598733002/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/events/event_rewriter.cc (right): https://codereview.chromium.org/598733002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/events/event_rewriter.cc:403: IsExtensionCommandRegistered(state.key_code, state.flags); On 2014/09/24 10:48:41, Finnur wrote: > This block should probably be below line 405. Done. https://codereview.chromium.org/598733002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/events/event_rewriter.cc:408: // binding the command. On 2014/09/24 10:48:41, Finnur wrote: > This comment seems a little out of place here. Why is search (note: Search) > referred to here specifically? It wouldn't be the only modifier we don't want to > rewrite, right? Was fixated on Search for the fix; rephrased to be much clearer. https://codereview.chromium.org/598733002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/events/event_rewriter.cc:410: if (!(key_event.flags() & ui::EF_FINAL)) { On 2014/09/24 10:48:41, Finnur wrote: > The indentation here is now wrong, but we anyway prefer: > > if (!foo && !bar) > ... > > ... over ... > > if (!foo) { > if (!bar) { > ... Done.
The search key is different than the other key modifiers in that it is requires interaction with the EventRewriter, instead of accelerators. When I was implementing sticky keys, the rewriter undergoing some refactoring to handle this. I should have followed up with implementing the search key when this refactoring was completed. Sorry, about that. Can you add a test case to chrome/browser/chromeos/accessibility/sticky_keys_browsertest.cc to exercise a search key rewrite shortcut (eg. search+left -> home) ?
Patchset #4 (id:180001) has been deleted
PTAL; test added. On 2014/09/24 18:07:15, Tim Song wrote: > The search key is different than the other key modifiers in that it is requires > interaction with the EventRewriter, instead of accelerators. When I was > implementing sticky keys, the rewriter undergoing some refactoring to handle > this. > > I should have followed up with implementing the search key when this refactoring > was completed. Sorry, about that. > > Can you add a test case to > chrome/browser/chromeos/accessibility/sticky_keys_browsertest.cc to exercise a > search key rewrite shortcut (eg. search+left -> home) ? Done.
LGTM. Thanks for the test.
The CQ bit was checked by dtseng@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/598733002/200001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
dtseng@chromium.org changed reviewers: + sky@chromium.org
+ sky for ui/ash/
dtseng@chromium.org changed reviewers: + oshima@chromium.org - sky@chromium.org
+ oshima for event_rewriter.cc
sky@chromium.org changed reviewers: + sky@chromium.org
ash (there is no ui/ash) LGTM
The CQ bit was checked by dtseng@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/598733002/200001
Message was sent while issue was closed.
Committed patchset #4 (id:200001) as 7840ab8a5c087a00bf7ea24d01dac6ce8aa27c8a
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/151aef63430e196c0bdbf1ad1e0706a5c8e484a9 Cr-Commit-Position: refs/heads/master@{#297054} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
