Enable Search-key modifiers for extended key shortcuts.

Enable Search-key modifiers for extended key shortcuts.

We use the Search-key as a modifier to disable existing shortcuts that
do not use the Search key. So Alt+Backspace still generates a Delete, but
Search+Alt+Backspace generates Alt+Backspace. And, Search+Backspace will
also generate Delete.

Search acts as a modifier for accessing all of the extended keys: Home,
End, Delete, Page up, Page down, Insert.

This removes the command line flag entirely, making everything work
in a way that does not need a flag or a preference option.

Tests:
unit_tests:EventRewriterTest.TestRewriteExtendedKeys
unit_tests:EventRewriterTest.TestRewriteFunctionKeys
unit_tests:EventRewriterTest.TestRewriteExtendedKeysWithSearchRemapped

BUG=162268
NOTRY=true

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=173607

[danakj, 2012-12-15 02:46:45]

This moves the non-controvertial parts of the power user keyboard functionality
out from behind the command line flag.

The flag is now used to disable existing shortcuts such as alt-backspace, as
that isn't desirable default behaviour right now.

[Yusuke Sato, 2012-12-17 00:26:17]

https://codereview.chromium.org/11578044/diff/6026/chrome/browser/ui/ash/even...
File chrome/browser/ui/ash/event_rewriter.cc (right):

https://codereview.chromium.org/11578044/diff/6026/chrome/browser/ui/ash/even...
chrome/browser/ui/ash/event_rewriter.cc:693: if (keep_non_search_shortcuts &&
remapped_keycode == ui::VKEY_UNKNOWN) {
Do we really need the new chrome://flag? Our UX team explicitly told us that
it's not possible to remove the old shortcuts such as Alt+BS.

Since the old one (Alt+BS etc) and the new one (Search+BS etc) can coexist
without any problem, I'd prefer to change the line to 'if (remapped_keycode ==
ui::VKEY_UNKNOWN)' and remove the new flag.

https://codereview.chromium.org/11578044/diff/6026/chrome/common/chrome_switc...
File chrome/common/chrome_switches.cc (right):

https://codereview.chromium.org/11578044/diff/6026/chrome/common/chrome_switc...
chrome/common/chrome_switches.cc:1441: // If true, extended keys such as Delete
or Home will only be accessible
If you agree with my comment in event_rewriter.cc, please remove
kDisableNonSearchExtendedKeyShortcuts.

[Yusuke Sato, 2012-12-17 01:01:34]

LGTM if the new flag is removed.

On 2012/12/17 00:26:17, Yusuke Sato wrote:
>
https://codereview.chromium.org/11578044/diff/6026/chrome/browser/ui/ash/even...
> File chrome/browser/ui/ash/event_rewriter.cc (right):
> 
>
https://codereview.chromium.org/11578044/diff/6026/chrome/browser/ui/ash/even...
> chrome/browser/ui/ash/event_rewriter.cc:693: if (keep_non_search_shortcuts &&
> remapped_keycode == ui::VKEY_UNKNOWN) {
> Do we really need the new chrome://flag? Our UX team explicitly told us that
> it's not possible to remove the old shortcuts such as Alt+BS.
> 
> Since the old one (Alt+BS etc) and the new one (Search+BS etc) can coexist
> without any problem, I'd prefer to change the line to 'if (remapped_keycode ==
> ui::VKEY_UNKNOWN)' and remove the new flag.
> 
>
https://codereview.chromium.org/11578044/diff/6026/chrome/common/chrome_switc...
> File chrome/common/chrome_switches.cc (right):
> 
>
https://codereview.chromium.org/11578044/diff/6026/chrome/common/chrome_switc...
> chrome/common/chrome_switches.cc:1441: // If true, extended keys such as
Delete
> or Home will only be accessible
> If you agree with my comment in event_rewriter.cc, please remove
> kDisableNonSearchExtendedKeyShortcuts.

[danakj, 2012-12-17 02:05:14]

mazda@ and derat@ can you please take a look? thanks.

https://codereview.chromium.org/11578044/diff/6026/chrome/browser/ui/ash/even...
File chrome/browser/ui/ash/event_rewriter.cc (right):

https://codereview.chromium.org/11578044/diff/6026/chrome/browser/ui/ash/even...
chrome/browser/ui/ash/event_rewriter.cc:693: if (keep_non_search_shortcuts &&
remapped_keycode == ui::VKEY_UNKNOWN) {
On 2012/12/17 00:26:17, Yusuke Sato wrote:
> Do we really need the new chrome://flag? Our UX team explicitly told us that
> it's not possible to remove the old shortcuts such as Alt+BS.
> 
> Since the old one (Alt+BS etc) and the new one (Search+BS etc) can coexist
> without any problem, I'd prefer to change the line to 'if (remapped_keycode ==
> ui::VKEY_UNKNOWN)' and remove the new flag.

Ok. The one downside is you have to choose between Search+Alt+Backspace as
becoming Alt+Backspace or Alt+Delete, but you can't access both keys. Or same
for Ctrl+Alt+PageDown. If that becomes an issue we can revisit it. I've removed
the flag entirely for now.

https://codereview.chromium.org/11578044/diff/6026/chrome/common/chrome_switc...
File chrome/common/chrome_switches.cc (right):

https://codereview.chromium.org/11578044/diff/6026/chrome/common/chrome_switc...
chrome/common/chrome_switches.cc:1441: // If true, extended keys such as Delete
or Home will only be accessible
On 2012/12/17 00:26:17, Yusuke Sato wrote:
> If you agree with my comment in event_rewriter.cc, please remove
> kDisableNonSearchExtendedKeyShortcuts.

Done.

[mazda, 2012-12-17 18:27:51]

https://codereview.chromium.org/11578044/diff/9019/chrome/browser/resources/c...
File chrome/browser/resources/chromeos/keyboard_overlay.js (left):

https://codereview.chromium.org/11578044/diff/9019/chrome/browser/resources/c...
chrome/browser/resources/chromeos/keyboard_overlay.js:145: var
searchModifierAddShortcuts = {
Could you keep these shortcuts data separated from other shrotcuts for now?
I'll enable these shortcuts to be generated by a script as other shortcuts.

[danakj, 2012-12-17 18:30:48]

https://codereview.chromium.org/11578044/diff/9019/chrome/browser/resources/c...
File chrome/browser/resources/chromeos/keyboard_overlay.js (left):

https://codereview.chromium.org/11578044/diff/9019/chrome/browser/resources/c...
chrome/browser/resources/chromeos/keyboard_overlay.js:145: var
searchModifierAddShortcuts = {
On 2012/12/17 18:27:51, mazda wrote:
> Could you keep these shortcuts data separated from other shrotcuts for now?
> I'll enable these shortcuts to be generated by a script as other shortcuts.

Ah, right ok! I'll move them back here and just have them always appended.

[danakj, 2012-12-17 20:01:29]

PTAL

https://codereview.chromium.org/11578044/diff/9019/chrome/browser/resources/c...
File chrome/browser/resources/chromeos/keyboard_overlay.js (left):

https://codereview.chromium.org/11578044/diff/9019/chrome/browser/resources/c...
chrome/browser/resources/chromeos/keyboard_overlay.js:145: var
searchModifierAddShortcuts = {
On 2012/12/17 18:27:51, mazda wrote:
> Could you keep these shortcuts data separated from other shrotcuts for now?
> I'll enable these shortcuts to be generated by a script as other shortcuts.

Done.

[mazda, 2012-12-17 20:26:35]

Thanks! LGTM

[Daniel Erat, 2012-12-17 20:32:01]

lgtm

https://codereview.chromium.org/11578044/diff/23001/chrome/browser/ui/ash/eve...
File chrome/browser/ui/ash/event_rewriter.cc (right):

https://codereview.chromium.org/11578044/diff/23001/chrome/browser/ui/ash/eve...
chrome/browser/ui/ash/event_rewriter.cc:660: Mod1Mask | ControlMask | Mod4Mask,
It doesn't need to happen in this change, but it would be nice if this file used
more-descriptively-named constants in place of Mod1Mask and Mod4Mask.

[danakj, 2012-12-17 20:33:47]

Thanks!

+sky for OWNERS, please. Removing the command line flag so mostly this is
deleting code.

https://codereview.chromium.org/11578044/diff/23001/chrome/browser/ui/ash/eve...
File chrome/browser/ui/ash/event_rewriter.cc (right):

https://codereview.chromium.org/11578044/diff/23001/chrome/browser/ui/ash/eve...
chrome/browser/ui/ash/event_rewriter.cc:660: Mod1Mask | ControlMask | Mod4Mask,
On 2012/12/17 20:32:02, Daniel Erat wrote:
> It doesn't need to happen in this change, but it would be nice if this file
used
> more-descriptively-named constants in place of Mod1Mask and Mod4Mask.

That could be useful, yeah. I'll keep it out of this one to increase
mergability.

[mazda, 2012-12-17 21:18:39]

https://codereview.chromium.org/11578044/diff/23001/ash/accelerators/accelera...
File ash/accelerators/accelerator_controller.cc (right):

https://codereview.chromium.org/11578044/diff/23001/ash/accelerators/accelera...
ash/accelerators/accelerator_controller.cc:545: previous_key_code !=
ui::VKEY_LWIN)
Does this work when Alt key is released first like the following?

Alt pressed
LWIN pressed
Alt released
LWIN released

[danakj, 2012-12-17 21:29:24]

https://codereview.chromium.org/11578044/diff/23001/ash/accelerators/accelera...
File ash/accelerators/accelerator_controller.cc (right):

https://codereview.chromium.org/11578044/diff/23001/ash/accelerators/accelera...
ash/accelerators/accelerator_controller.cc:545: previous_key_code !=
ui::VKEY_LWIN)
On 2012/12/17 21:18:40, mazda wrote:
> Does this work when Alt key is released first like the following?
> 
> Alt pressed
> LWIN pressed
> Alt released
> LWIN released

No, the binding does not fire then, as Alt-release is not bound to do anything.
I thought about adding a Search+Alt binding (ie. search as modifier) but we have
no method to designate search as a modifier in the accelerator table.

Can we punt on this for now and address it in a separate CL?

[danakj, 2012-12-17 21:34:56]

https://codereview.chromium.org/11578044/diff/23001/ash/accelerators/accelera...
File ash/accelerators/accelerator_controller.cc (right):

https://codereview.chromium.org/11578044/diff/23001/ash/accelerators/accelera...
ash/accelerators/accelerator_controller.cc:545: previous_key_code !=
ui::VKEY_LWIN)
On 2012/12/17 21:29:24, danakj wrote:
> On 2012/12/17 21:18:40, mazda wrote:
> > Does this work when Alt key is released first like the following?
> > 
> > Alt pressed
> > LWIN pressed
> > Alt released
> > LWIN released
> 
> No, the binding does not fire then, as Alt-release is not bound to do
anything.
> I thought about adding a Search+Alt binding (ie. search as modifier) but we
have
> no method to designate search as a modifier in the accelerator table.
> 
> Can we punt on this for now and address it in a separate CL?

Oh, this is meant for https://codereview.chromium.org/11580005/ right?

There's no Alt involved in this CL.

[mazda, 2012-12-17 21:39:15]

https://codereview.chromium.org/11578044/diff/23001/ash/accelerators/accelera...
File ash/accelerators/accelerator_controller.cc (right):

https://codereview.chromium.org/11578044/diff/23001/ash/accelerators/accelera...
ash/accelerators/accelerator_controller.cc:545: previous_key_code !=
ui::VKEY_LWIN)
On 2012/12/17 21:34:56, danakj wrote:
> On 2012/12/17 21:29:24, danakj wrote:
> > On 2012/12/17 21:18:40, mazda wrote:
> > > Does this work when Alt key is released first like the following?
> > > 
> > > Alt pressed
> > > LWIN pressed
> > > Alt released
> > > LWIN released
> > 
> > No, the binding does not fire then, as Alt-release is not bound to do
> anything.
> > I thought about adding a Search+Alt binding (ie. search as modifier) but we
> have
> > no method to designate search as a modifier in the accelerator table.
> > 
> > Can we punt on this for now and address it in a separate CL?
> 
> Oh, this is meant for https://codereview.chromium.org/11580005/ right?

Ah, yes. I mistakenly commented on a different tab, sorry.
This CL lgtm.

[sky, 2012-12-17 22:28:59]

LGTM

https://codereview.chromium.org/11578044/diff/22003/ash/accelerators/accelera...
File ash/accelerators/accelerator_controller.cc (right):

https://codereview.chromium.org/11578044/diff/22003/ash/accelerators/accelera...
ash/accelerators/accelerator_controller.cc:544: if (previous_event_type ==
ui::ET_KEY_RELEASED ||
nit: combine ifs.

[danakj, 2012-12-17 22:32:12]

Thanks!

https://codereview.chromium.org/11578044/diff/22003/ash/accelerators/accelera...
File ash/accelerators/accelerator_controller.cc (right):

https://codereview.chromium.org/11578044/diff/22003/ash/accelerators/accelera...
ash/accelerators/accelerator_controller.cc:544: if (previous_event_type ==
ui::ET_KEY_RELEASED ||
On 2012/12/17 22:29:00, sky wrote:
> nit: combine ifs.

Done.

[commit-bot: I haz the power, 2012-12-17 22:55:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/11578044/36001