Add power-user keyboard mode for ChromeOS with Search key acting as a typical Fn key.

Add power-user keyboard mode for ChromeOS with Search key acting as a typical Fn key.

All new behaviour in this CL is hidden behind the command line flag
--enable-chromebook-function-key.

Currently the keyboard shortcuts for PageUp, PageDown, Delete, etc are all
accessed by holding Alt or Alt+Control. This prevents you from using Alt or
Control as a modifier to the desired keyboard shortcut, which prevents access
to advanced keyboard shortcuts in applications, and even normal shortcuts such
as Control-End to move to the end of the document.

This adds a command line flag --enable-chromebook-function-key that adds a
checkbox to the Keyboard settings UI. When the checkbox is enabled, the
Search key replaces Alt/Control as the modifier used to access advanced
keyboard shortcuts such as Home, Delete, etc. The following shortcuts
become accessible with the Search key:

Search+Up => Page Up
Search+Down => Page Down
Search+Left => Home
Search+Right => End
Search+Backspace => Delete

R=yusukes@chromium.org
BUG=162268
TEST=unit_tests:EventRewriter.TestRewriteBackspaceAndArrowKeys
TEST=ash_unittests:AcceleratorControllerTest.GlobalAcceleratorsToggleAppListWithoutSearchAsModifier
TEST=ash_unittests:AcceleratorControllerTest.GlobalAcceleratorsToggleAppListWithSearchAsModifier

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=169741

[Yusuke Sato, 2012-11-26 06:09:37]

Thanks a lot for working on this! Did initial review.

nit: TEST= line is missing.

https://codereview.chromium.org/11421055/diff/3019/ash/accelerators/accelerat...
File ash/accelerators/accelerator_table.cc (right):

https://codereview.chromium.org/11421055/diff/3019/ash/accelerators/accelerat...
ash/accelerators/accelerator_table.cc:114: { false, ui::VKEY_LWIN, ui::EF_NONE,
TOGGLE_APP_LIST },
Could you keep the accelerator to be triggered on press? (e.g. by rewriting
ui::VKEY_LWIN to ui::VKEY_UNKNOWN or something when the new feature is enabled.)

I think release accelerators have been a source of anxiety for us and are
something we really want to avoid whenever possible. For example, please take a
look at comments near 'case NEXT_IME' in AcceleratorController::PerformAction().

Another issue for this change is latency. The use who is used to the current
Search behavior might feel laggy if we do this since the app menu is not shown
until the user releases Search.

https://codereview.chromium.org/11421055/diff/3019/ash/accelerators/accelerat...
ash/accelerators/accelerator_table.cc:116: { false, ui::VKEY_LWIN,
ui::EF_ALT_DOWN, TOGGLE_CAPS_LOCK },
I guess TOGGLE_CAPS_LOCK is not fired if the user presses/releases keys in this
order: press Alt, press Search, release Alt, release Search.

I think the easiest way to avoid this kind of problem would be to avoid release
accelerators.

https://codereview.chromium.org/11421055/diff/3019/chrome/browser/chromeos/pr...
File chrome/browser/chromeos/preferences.cc (right):

https://codereview.chromium.org/11421055/diff/3019/chrome/browser/chromeos/pr...
chrome/browser/chromeos/preferences.cc:527: PrefService* prefs =
g_browser_process->local_state();
For consistency, I'd prefer not to reflect the owner pref to local_state because
Alt/Ctrl/Search remapping is always disabled on login screen.

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

https://codereview.chromium.org/11421055/diff/3019/chrome/browser/ui/ash/even...
chrome/browser/ui/ash/event_rewriter.cc:741: bool
EventRewriter::DropSearchKey(ui::KeyEvent* event) {
I guess we can remove this function if we stop using release accelerators for
VKEY_LWIN.

https://codereview.chromium.org/11421055/diff/3019/chrome/browser/ui/ash/even...
chrome/browser/ui/ash/event_rewriter.cc:759: // If we the Search key acts like a
modifier for another key, then drop the
nit: s/we//

https://codereview.chromium.org/11421055/diff/3019/chrome/browser/ui/ash/even...
File chrome/browser/ui/ash/event_rewriter.h (right):

https://codereview.chromium.org/11421055/diff/3019/chrome/browser/ui/ash/even...
chrome/browser/ui/ash/event_rewriter.h:126: // Returns true whn the |event| is
rewritten.
nit: when

https://codereview.chromium.org/11421055/diff/3019/chrome/browser/ui/ash/even...
chrome/browser/ui/ash/event_rewriter.h:163: bool drop_search_key_release_;
nit: you might want to remove line 164-166.

https://codereview.chromium.org/11421055/diff/3019/chrome/browser/ui/ash/even...
chrome/browser/ui/ash/event_rewriter.h:193: unsigned int f1_xkeycode_;
Looks like this part is for the next CL.

https://codereview.chromium.org/11421055/diff/3019/chrome/browser/ui/ash/even...
File chrome/browser/ui/ash/event_rewriter_unittest.cc (right):

https://codereview.chromium.org/11421055/diff/3019/chrome/browser/ui/ash/even...
chrome/browser/ui/ash/event_rewriter_unittest.cc:253: TEST_F(EventRewriterTest,
TestRewriteCommandToControl) {
This test seems red:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

Please fix.

https://codereview.chromium.org/11421055/diff/3019/chrome/common/chrome_switc...
File chrome/common/chrome_switches.h (right):

https://codereview.chromium.org/11421055/diff/3019/chrome/common/chrome_switc...
chrome/common/chrome_switches.h:391: extern const char
kEnableChromeOSFunctionKey[];
nit: alphabetical order.

[Yusuke Sato, 2012-11-26 07:58:11]

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

https://codereview.chromium.org/11421055/diff/3019/chrome/browser/ui/ash/even...
chrome/browser/ui/ash/event_rewriter.cc:660: const bool search_as_function_key =
pref_service &&
please do either of the following: 1) check the command line flag here or 2)
make the new user pref UNSYNCABLE.

[danakj, 2012-11-26 17:20:15]

Thanks for the review. Some thoughts/questions about Search as a modifier and
apps menu interactions.

https://codereview.chromium.org/11421055/diff/3019/ash/accelerators/accelerat...
File ash/accelerators/accelerator_table.cc (right):

https://codereview.chromium.org/11421055/diff/3019/ash/accelerators/accelerat...
ash/accelerators/accelerator_table.cc:114: { false, ui::VKEY_LWIN, ui::EF_NONE,
TOGGLE_APP_LIST },
On 2012/11/26 06:09:38, Yusuke Sato wrote:
> Could you keep the accelerator to be triggered on press? (e.g. by rewriting
> ui::VKEY_LWIN to ui::VKEY_UNKNOWN or something when the new feature is
enabled.)

If we keep it on press, then when the feature is enable, the Search key can't do
anything except be a modifier. This allows it to still pop up the Apps menu. I
think losing access to the apps menu would be a bad thing.. It functions similar
to other desktop OSes that provide a modifer key for running various things, but
also provide access to the apps menu when not used as a modifier.

> I think release accelerators have been a source of anxiety for us and are
> something we really want to avoid whenever possible. For example, please take
a
> look at comments near 'case NEXT_IME' in
AcceleratorController::PerformAction().
> 
> Another issue for this change is latency. The use who is used to the current
> Search behavior might feel laggy if we do this since the app menu is not shown
> until the user releases Search.

Making this a bit laggy seems better than making it stop working altogether.

I thought at first we could drop the press, and then when the release happened,
generate a press and release together. But the system seems designed to edit
events, not to insert new ones. And turning the release into a press seems bad,
because then someone watching would see the Search key as being held down.

Do you have some idea of how to let us use Search as a modifer, and still use it
to access the apps menu, without binding it to release?

What if it was bound to release only when the feature was enabled?

https://codereview.chromium.org/11421055/diff/3019/ash/accelerators/accelerat...
ash/accelerators/accelerator_table.cc:116: { false, ui::VKEY_LWIN,
ui::EF_ALT_DOWN, TOGGLE_CAPS_LOCK },
On 2012/11/26 06:09:38, Yusuke Sato wrote:
> I guess TOGGLE_CAPS_LOCK is not fired if the user presses/releases keys in
this
> order: press Alt, press Search, release Alt, release Search.
> 
> I think the easiest way to avoid this kind of problem would be to avoid
release
> accelerators.

I can make it not rewrite the Search press when another modifier is being used.
I did that at first, but the apps menu didn't work like that, so I made it a
release modifier, and just made this match.

https://codereview.chromium.org/11421055/diff/3019/chrome/browser/chromeos/pr...
File chrome/browser/chromeos/preferences.cc (right):

https://codereview.chromium.org/11421055/diff/3019/chrome/browser/chromeos/pr...
chrome/browser/chromeos/preferences.cc:527: PrefService* prefs =
g_browser_process->local_state();
On 2012/11/26 06:09:38, Yusuke Sato wrote:
> For consistency, I'd prefer not to reflect the owner pref to local_state
because
> Alt/Ctrl/Search remapping is always disabled on login screen.

Oh I had trouble with the setting being loaded correctly, and was just
copy/pasting other code. I'll try just removing this piece and see what happens
again.

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

https://codereview.chromium.org/11421055/diff/3019/chrome/browser/ui/ash/even...
chrome/browser/ui/ash/event_rewriter.cc:660: const bool search_as_function_key =
pref_service &&
On 2012/11/26 07:58:12, Yusuke Sato wrote:
> please do either of the following: 1) check the command line flag here or 2)
> make the new user pref UNSYNCABLE.

I'll check for ChromeOS keyboard flag here, thanks. Then in future, it could
instead check the event source is a ChromeOS keyboard.

https://codereview.chromium.org/11421055/diff/3019/chrome/browser/ui/ash/even...
chrome/browser/ui/ash/event_rewriter.cc:741: bool
EventRewriter::DropSearchKey(ui::KeyEvent* event) {
On 2012/11/26 06:09:38, Yusuke Sato wrote:
> I guess we can remove this function if we stop using release accelerators for
> VKEY_LWIN.

We still have to drop the press or you get an apps menu when you want to press
Search-Backspace.

[danakj, 2012-11-26 19:36:26]

The only release-accelerator is now Search-key by itself to toggle the Apps
menu.

I think we either need:

1) Keep is as release accelerator. Since it is a single key - no modifiers - do
we avoid the bad side effects of a release accelerator?

2) Add Control-Search as a way to toggle the Apps menu.

https://codereview.chromium.org/11421055/diff/3019/ash/accelerators/accelerat...
File ash/accelerators/accelerator_table.cc (right):

https://codereview.chromium.org/11421055/diff/3019/ash/accelerators/accelerat...
ash/accelerators/accelerator_table.cc:116: { false, ui::VKEY_LWIN,
ui::EF_ALT_DOWN, TOGGLE_CAPS_LOCK },
On 2012/11/26 17:20:16, danakj wrote:
> On 2012/11/26 06:09:38, Yusuke Sato wrote:
> > I guess TOGGLE_CAPS_LOCK is not fired if the user presses/releases keys in
> this
> > order: press Alt, press Search, release Alt, release Search.
> > 
> > I think the easiest way to avoid this kind of problem would be to avoid
> release
> > accelerators.
> 
> I can make it not rewrite the Search press when another modifier is being
used.
> I did that at first, but the apps menu didn't work like that, so I made it a
> release modifier, and just made this match.

Done.

https://codereview.chromium.org/11421055/diff/3019/chrome/browser/chromeos/pr...
File chrome/browser/chromeos/preferences.cc (right):

https://codereview.chromium.org/11421055/diff/3019/chrome/browser/chromeos/pr...
chrome/browser/chromeos/preferences.cc:527: PrefService* prefs =
g_browser_process->local_state();
On 2012/11/26 17:20:16, danakj wrote:
> On 2012/11/26 06:09:38, Yusuke Sato wrote:
> > For consistency, I'd prefer not to reflect the owner pref to local_state
> because
> > Alt/Ctrl/Search remapping is always disabled on login screen.
> 
> Oh I had trouble with the setting being loaded correctly, and was just
> copy/pasting other code. I'll try just removing this piece and see what
happens
> again.

Done.

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

https://codereview.chromium.org/11421055/diff/3019/chrome/browser/ui/ash/even...
chrome/browser/ui/ash/event_rewriter.cc:660: const bool search_as_function_key =
pref_service &&
On 2012/11/26 17:20:16, danakj wrote:
> On 2012/11/26 07:58:12, Yusuke Sato wrote:
> > please do either of the following: 1) check the command line flag here or 2)
> > make the new user pref UNSYNCABLE.
> 
> I'll check for ChromeOS keyboard flag here, thanks. Then in future, it could
> instead check the event source is a ChromeOS keyboard.

Okay, after the explanation of HasChromeOSKeyboard()..

I will use !EventSourceIsChromebookKeyboard() here for an early-out (a no-op for
now), and check for the command-line flag.

https://codereview.chromium.org/11421055/diff/3019/chrome/browser/ui/ash/even...
chrome/browser/ui/ash/event_rewriter.cc:759: // If we the Search key acts like a
modifier for another key, then drop the
On 2012/11/26 06:09:38, Yusuke Sato wrote:
> nit: s/we//

Done.

https://codereview.chromium.org/11421055/diff/3019/chrome/browser/ui/ash/even...
File chrome/browser/ui/ash/event_rewriter.h (right):

https://codereview.chromium.org/11421055/diff/3019/chrome/browser/ui/ash/even...
chrome/browser/ui/ash/event_rewriter.h:126: // Returns true whn the |event| is
rewritten.
On 2012/11/26 06:09:38, Yusuke Sato wrote:
> nit: when

Done.

https://codereview.chromium.org/11421055/diff/3019/chrome/browser/ui/ash/even...
chrome/browser/ui/ash/event_rewriter.h:163: bool drop_search_key_release_;
On 2012/11/26 06:09:38, Yusuke Sato wrote:
> nit: you might want to remove line 164-166.

Done.

https://codereview.chromium.org/11421055/diff/3019/chrome/browser/ui/ash/even...
chrome/browser/ui/ash/event_rewriter.h:193: unsigned int f1_xkeycode_;
On 2012/11/26 06:09:38, Yusuke Sato wrote:
> Looks like this part is for the next CL.

Done.

[Wez, 2012-11-26 20:50:51]

Why do you want an about:flags setting for this, rather than surfacing it
alongside the Control/Search/Alt button configuration UI?

[danakj, 2012-11-26 21:01:30]

On 2012/11/26 20:50:51, Wez wrote:
> Why do you want an about:flags setting for this, rather than surfacing it
> alongside the Control/Search/Alt button configuration UI?

I did add UI exactly as you say. But that UI is hidden behind this flag until we
have some approval from whoever approves new UI/settings. The about:settings
flag should be temporary.

[Yusuke Sato, 2012-11-26 21:15:04]

https://codereview.chromium.org/11421055/diff/3019/ash/accelerators/accelerat...
File ash/accelerators/accelerator_table.cc (right):

https://codereview.chromium.org/11421055/diff/3019/ash/accelerators/accelerat...
ash/accelerators/accelerator_table.cc:114: { false, ui::VKEY_LWIN, ui::EF_NONE,
TOGGLE_APP_LIST },
Oh now I understood what you wanted to do. Thanks for the explanation.

On 2012/11/26 17:20:16, danakj wrote:
> On 2012/11/26 06:09:38, Yusuke Sato wrote:
> > Could you keep the accelerator to be triggered on press? (e.g. by rewriting
> > ui::VKEY_LWIN to ui::VKEY_UNKNOWN or something when the new feature is
> enabled.)
> 
> If we keep it on press, then when the feature is enable, the Search key can't
do
> anything except be a modifier. This allows it to still pop up the Apps menu. I
> think losing access to the apps menu would be a bad thing.. It functions
similar
> to other desktop OSes that provide a modifer key for running various things,
but
> also provide access to the apps menu when not used as a modifier.
> 
> > I think release accelerators have been a source of anxiety for us and are
> > something we really want to avoid whenever possible. For example, please
take
> a
> > look at comments near 'case NEXT_IME' in
> AcceleratorController::PerformAction().
> > 
> > Another issue for this change is latency. The use who is used to the current
> > Search behavior might feel laggy if we do this since the app menu is not
shown
> > until the user releases Search.
> 
> Making this a bit laggy seems better than making it stop working altogether.

Fair enough.

> I thought at first we could drop the press, and then when the release
happened,
> generate a press and release together. But the system seems designed to edit
> events, not to insert new ones. And turning the release into a press seems
bad,
> because then someone watching would see the Search key as being held down.
> 
> Do you have some idea of how to let us use Search as a modifer, and still use
it
> to access the apps menu, without binding it to release?
> 
> What if it was bound to release only when the feature was enabled?

Sounds good. Please make sure to add some code to accelerator_controller.cc
which is similar to this:

    case DISABLE_CAPS_LOCK:
      if (previous_event_type == ui::ET_KEY_RELEASED)
        return false;

so TOGGLE_APP_LIST would not fire when you press Search+<something>, release
<something>, then release Search.

Please also check my comment in event_rewriter.cc in Patch Set 6.

https://codereview.chromium.org/11421055/diff/1018/chrome/app/chromeos_string...
File chrome/app/chromeos_strings.grdp (right):

https://codereview.chromium.org/11421055/diff/1018/chrome/app/chromeos_string...
chrome/app/chromeos_strings.grdp:2074: Use the Search key to access extended
keybindings.
please get the new string (and the new UI) reviewed by our UX team.

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

https://codereview.chromium.org/11421055/diff/3053/chrome/browser/ui/ash/even...
chrome/browser/ui/ash/event_rewriter.cc:772: drop_key = true;
I'm wondering if this is still necessary. If it is, could you let me know why?

https://codereview.chromium.org/11421055/diff/3053/chrome/browser/ui/ash/even...
chrome/browser/ui/ash/event_rewriter.cc:779: drop_search_key_release_ = false;
As I wrote above, please do that kind of suppression in
accelerator_controller.cc, rather than here.

[danakj, 2012-11-26 21:17:59]

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

https://codereview.chromium.org/11421055/diff/3053/chrome/browser/ui/ash/even...
chrome/browser/ui/ash/event_rewriter.cc:772: drop_key = true;
On 2012/11/26 21:15:05, Yusuke Sato wrote:
> I'm wondering if this is still necessary. If it is, could you let me know why?
> 

Actually, it was just to make sure future shortcuts/changes wouldn't ever break
this feature, which the dev might not use.

Should I remove then?

https://codereview.chromium.org/11421055/diff/3053/chrome/browser/ui/ash/even...
chrome/browser/ui/ash/event_rewriter.cc:779: drop_search_key_release_ = false;
On 2012/11/26 21:15:05, Yusuke Sato wrote:
> As I wrote above, please do that kind of suppression in
> accelerator_controller.cc, rather than here.

Ah, okay will do!

[Yusuke Sato, 2012-11-26 21:26:33]

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

https://codereview.chromium.org/11421055/diff/3053/chrome/browser/ui/ash/even...
chrome/browser/ui/ash/event_rewriter.cc:772: drop_key = true;
On 2012/11/26 21:17:59, danakj wrote:
> On 2012/11/26 21:15:05, Yusuke Sato wrote:
> > I'm wondering if this is still necessary. If it is, could you let me know
why?
> > 
> 
> Actually, it was just to make sure future shortcuts/changes wouldn't ever
break
> this feature, which the dev might not use.
> 
> Should I remove then?

I see. Then I'd prefer to add a test to accelerator_table_unittest.cc which is
something like EXPECT_EQ(0U, accelerator_list.count(LWIN+Press)) and remove the
DropSearchKey() function.

[danakj, 2012-11-26 21:47:50]

PTAL.

I spoke to rbyers@ and he said that we should keep this behind the about:flags
for now. kuscher@ will review the strings/ui/feature and when he's happy with it
we can drop the about:flags/command line.

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

https://codereview.chromium.org/11421055/diff/3053/chrome/browser/ui/ash/even...
chrome/browser/ui/ash/event_rewriter.cc:772: drop_key = true;
On 2012/11/26 21:26:34, Yusuke Sato wrote:
> On 2012/11/26 21:17:59, danakj wrote:
> > On 2012/11/26 21:15:05, Yusuke Sato wrote:
> > > I'm wondering if this is still necessary. If it is, could you let me know
> why?
> > > 
> > 
> > Actually, it was just to make sure future shortcuts/changes wouldn't ever
> break
> > this feature, which the dev might not use.
> > 
> > Should I remove then?
> 
> I see. Then I'd prefer to add a test to accelerator_table_unittest.cc which is
> something like EXPECT_EQ(0U, accelerator_list.count(LWIN+Press)) and remove
the
> DropSearchKey() function.

Ah, okay. By solving this in accelerator_controller.cc, the unit test has to
send a press, so it was easy to verify it did nothing at that time.

[Yusuke Sato, 2012-11-26 22:22:57]

final comment..

https://codereview.chromium.org/11421055/diff/4022/ash/accelerators/accelerat...
File ash/accelerators/accelerator_table.cc (right):

https://codereview.chromium.org/11421055/diff/4022/ash/accelerators/accelerat...
ash/accelerators/accelerator_table.cc:114: { false, ui::VKEY_LWIN, ui::EF_NONE,
TOGGLE_APP_LIST },
In patch set 4, you told me that you were going to enable the release
accelerator only when the feature is enabled. Could you do that?

I think we could do that in the following way:

1. have both press and release accelerators in this file.

 { true, ui::VKEY_LWIN, ui::EF_NONE, TOGGLE_APP_LIST },
 { false, ui::VKEY_LWIN, ui::EF_NONE, TOGGLE_APP_LIST },

2. modify accelerator_controller.cc so we can ignore Press+LWIN when the feature
is enabled, and we can also ignore Release+LWIN when the feature is disabled.

case TOGGLE_APP_LIST:
  if (the_feature_is_enabled &&
      accelerator.type() == Press &&
      accelerator.key_code() == LWIN) {
    return false;
  }
  if (!the_feature_is_enabled &&
      accelerator.type() == Release &&
      accelerator.key_code() == LWIN) {
    return false;
  }
  if (the_feature_is_enabled &&
      accelerator.type() == Release &&
      accelerator.key_code() == LWIN &&
      previous_event_type == ui::ET_KEY_RELEASED) {
    // If something else was pressed since the LWIN key, then we should ignore
the release of LWIN.
    return false;
  }

3. modify accelerator_controller_unittests.cc if needed.

[danakj, 2012-11-26 22:53:46]

https://codereview.chromium.org/11421055/diff/4022/ash/accelerators/accelerat...
File ash/accelerators/accelerator_table.cc (right):

https://codereview.chromium.org/11421055/diff/4022/ash/accelerators/accelerat...
ash/accelerators/accelerator_table.cc:114: { false, ui::VKEY_LWIN, ui::EF_NONE,
TOGGLE_APP_LIST },
On 2012/11/26 22:22:57, Yusuke Sato wrote:
> In patch set 4, you told me that you were going to enable the release
> accelerator only when the feature is enabled. Could you do that?

Ah, right. To check if the feature is enabled, this code needs to access the
profile and switches which are in chrome/. And ash/ shouldn't be touching
chrome/ right?

Do you have any suggestion on how to plumb that setting into the controller?

Should I be doing this through ash::ShellDelegate? I see
IsSpokenFeedbackEnabled() there, which is the only thing that looks at all
similar.

[Yusuke Sato, 2012-11-26 23:01:49]

https://codereview.chromium.org/11421055/diff/4022/ash/accelerators/accelerat...
File ash/accelerators/accelerator_table.cc (right):

https://codereview.chromium.org/11421055/diff/4022/ash/accelerators/accelerat...
ash/accelerators/accelerator_table.cc:114: { false, ui::VKEY_LWIN, ui::EF_NONE,
TOGGLE_APP_LIST },
On 2012/11/26 22:53:47, danakj wrote:
> On 2012/11/26 22:22:57, Yusuke Sato wrote:
> > In patch set 4, you told me that you were going to enable the release
> > accelerator only when the feature is enabled. Could you do that?
> 
> Ah, right. To check if the feature is enabled, this code needs to access the
> profile and switches which are in chrome/. And ash/ shouldn't be touching
> chrome/ right?
> 
> Do you have any suggestion on how to plumb that setting into the controller?
> 
> Should I be doing this through ash::ShellDelegate? I see
> IsSpokenFeedbackEnabled() there, which is the only thing that looks at all
> similar.

I'm not an ash owner, but I believe ShellDelegate is the right way to go.

[danakj, 2012-11-27 00:05:37]

On 2012/11/26 23:01:49, Yusuke Sato wrote:
>
https://codereview.chromium.org/11421055/diff/4022/ash/accelerators/accelerat...
> File ash/accelerators/accelerator_table.cc (right):
> 
>
https://codereview.chromium.org/11421055/diff/4022/ash/accelerators/accelerat...
> ash/accelerators/accelerator_table.cc:114: { false, ui::VKEY_LWIN,
ui::EF_NONE,
> TOGGLE_APP_LIST },
> On 2012/11/26 22:53:47, danakj wrote:
> > On 2012/11/26 22:22:57, Yusuke Sato wrote:
> > > In patch set 4, you told me that you were going to enable the release
> > > accelerator only when the feature is enabled. Could you do that?
> > 
> > Ah, right. To check if the feature is enabled, this code needs to access the
> > profile and switches which are in chrome/. And ash/ shouldn't be touching
> > chrome/ right?
> > 
> > Do you have any suggestion on how to plumb that setting into the controller?
> > 
> > Should I be doing this through ash::ShellDelegate? I see
> > IsSpokenFeedbackEnabled() there, which is the only thing that looks at all
> > similar.
> 
> I'm not an ash owner, but I believe ShellDelegate is the right way to go.

Done.

[Yusuke Sato, 2012-11-27 01:04:38]

LGTM

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

https://codereview.chromium.org/11421055/diff/14058/ash/accelerators/accelera...
ash/accelerators/accelerator_controller.cc:452: const ui::EventType
previous_event_type =
nit: could you define previous_key_code here? it's slightly easier to read for
me.

https://codereview.chromium.org/11421055/diff/14058/ash/accelerators/accelera...
ash/accelerators/accelerator_controller.cc:557:
context_.previous_accelerator().key_code() != ui::VKEY_LWIN))) {
nit: previous_key_code != ui::VKEY_LWIN

[danakj, 2012-11-27 01:44:31]

Thanks very much :)

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

https://codereview.chromium.org/11421055/diff/14058/ash/accelerators/accelera...
ash/accelerators/accelerator_controller.cc:452: const ui::EventType
previous_event_type =
On 2012/11/27 01:04:38, Yusuke Sato wrote:
> nit: could you define previous_key_code here? it's slightly easier to read for
> me.

Done.

https://codereview.chromium.org/11421055/diff/14058/ash/accelerators/accelera...
ash/accelerators/accelerator_controller.cc:557:
context_.previous_accelerator().key_code() != ui::VKEY_LWIN))) {
On 2012/11/27 01:04:38, Yusuke Sato wrote:
> nit: previous_key_code != ui::VKEY_LWIN

Done.

[danakj, 2012-11-27 01:45:06]

+sky could you take a look for OWNERS or suggest someone for it?

Thank you!

[sky, 2012-11-27 02:01:53]

https://codereview.chromium.org/11421055/diff/4031/ash/accelerators/accelerat...
File ash/accelerators/accelerator_controller.cc (right):

https://codereview.chromium.org/11421055/diff/4031/ash/accelerators/accelerat...
ash/accelerators/accelerator_controller.cc:454: const ui::KeyboardCode
previous_key_code =
nit: since you only need this in TOGGLE_APP_LIST, move it there.

https://codereview.chromium.org/11421055/diff/4031/ash/accelerators/accelerat...
ash/accelerators/accelerator_controller.cc:544: bool search_as_function_key =
nit: if you're doing to use const like you did on 454 be consistent and use it
here too.

https://codereview.chromium.org/11421055/diff/4031/chrome/browser/ui/ash/chro...
File chrome/browser/ui/ash/chrome_shell_delegate.cc (right):

https://codereview.chromium.org/11421055/diff/4031/chrome/browser/ui/ash/chro...
chrome/browser/ui/ash/chrome_shell_delegate.cc:502:
prefs::kLanguageSearchKeyActsAsFunctionKey);
Is there a reason to have both the flag and the pref? Can't we go with just the
flag for now?

[danakj, 2012-11-27 02:27:18]

https://codereview.chromium.org/11421055/diff/4031/ash/accelerators/accelerat...
File ash/accelerators/accelerator_controller.cc (right):

https://codereview.chromium.org/11421055/diff/4031/ash/accelerators/accelerat...
ash/accelerators/accelerator_controller.cc:454: const ui::KeyboardCode
previous_key_code =
On 2012/11/27 02:01:53, sky wrote:
> nit: since you only need this in TOGGLE_APP_LIST, move it there.

Done.

https://codereview.chromium.org/11421055/diff/4031/ash/accelerators/accelerat...
ash/accelerators/accelerator_controller.cc:544: bool search_as_function_key =
On 2012/11/27 02:01:53, sky wrote:
> nit: if you're doing to use const like you did on 454 be consistent and use it
> here too.

Done.

https://codereview.chromium.org/11421055/diff/4031/chrome/browser/ui/ash/chro...
File chrome/browser/ui/ash/chrome_shell_delegate.cc (right):

https://codereview.chromium.org/11421055/diff/4031/chrome/browser/ui/ash/chro...
chrome/browser/ui/ash/chrome_shell_delegate.cc:502:
prefs::kLanguageSearchKeyActsAsFunctionKey);
On 2012/11/27 02:01:53, sky wrote:
> Is there a reason to have both the flag and the pref? Can't we go with just
the
> flag for now?

Yeh, I was requested to do it this way so that we can have a path to enabling
the feature as:
1. Try out the feature in default-off state.
2. Flip it to default-on and wait until it gets to stable.
3. Remove the flag.

The pref UI is part of the stuff to be "tried out" in step 1.

[sky, 2012-11-27 15:55:25]

Fair enough, LGTM. I'm not too familiar with EventRewriter. Dan, could you take
a look at that?

[Daniel Erat, 2012-11-27 16:16:08]

LGTM for event_rewriter*

[commit-bot: I haz the power, 2012-11-27 18:20:30]

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

[commit-bot: I haz the power, 2012-11-27 20:54:34]

Change committed as 169741