Fix for Ash shortcuts unexpectedly working in system model dialog

See also:
http://code.google.com/p/chromium/issues/detail?id=134093
https://chromiumcodereview.appspot.com/10993067/

This is a better fix than 10993067. This CL undoes that change and moves the fix to the AcceleratorController. This is a more general fix which employs a table of actions that are allowed when in a modal dialog. All other actions are suppressed.

BUG=153077
TEST=System Tray -> Ehternet -> Join Other; while the modal dialog is up, try various ash shortcuts. Observe that the ones are working (like volume up) make sense, but things like new window or moving around windows are inactive.

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

[sschmitz, 2012-09-28 22:57:08]

Thanks for all your feedback (on the original bug). This seems a much better
solution. Yusukesan, would you please see whether I put too much or too little
in the table of "allowed actions when in a modal dialog". THX.

[Yusuke Sato, 2012-09-29 00:54:33]

Thanks. Checked the table:

https://codereview.chromium.org/10977088/diff/1/ash/accelerators/accelerator_...
File ash/accelerators/accelerator_table.cc (right):

https://codereview.chromium.org/10977088/diff/1/ash/accelerators/accelerator_...
ash/accelerators/accelerator_table.cc:231: const AcceleratorAction
kActionsAllowedAtModalWindow[] = {
I'm wondering if it's okay to omit OPEN_FEEDBACK_PAGE. What should the user do
if he/she finds a problem while a modal dialog is up?

https://codereview.chromium.org/10977088/diff/1/ash/accelerators/accelerator_...
ash/accelerators/accelerator_table.cc:248: SHOW_KEYBOARD_OVERLAY,
does this work fine when a modal window is up? please check if you haven't done
it.

https://codereview.chromium.org/10977088/diff/1/ash/accelerators/accelerator_...
ash/accelerators/accelerator_table.cc:249: SWAP_PRIMARY_DISPLAY,
please also check if this works fine (you might need to connect a secondary
monitor to you chromebook to check it).

https://codereview.chromium.org/10977088/diff/1/ash/accelerators/accelerator_...
ash/accelerators/accelerator_table.cc:260: CYCLE_DISPLAY_MODE,
the same.

[Daniel Erat, 2012-09-30 01:24:47]

Mind adding a test for this?

https://codereview.chromium.org/10977088/diff/1/ash/accelerators/accelerator_...
File ash/accelerators/accelerator_controller.cc (right):

https://codereview.chromium.org/10977088/diff/1/ash/accelerators/accelerator_...
ash/accelerators/accelerator_controller.cc:433: // for things Alt+Tab that would
cause an undesired effect in
nit: s/things/things like/

[sschmitz, 2012-10-15 23:50:33]

Rebased; add one action to allowed list: OPEN_FEEDBACK_PAGE; added two unittests

https://codereview.chromium.org/10977088/diff/1/ash/accelerators/accelerator_...
File ash/accelerators/accelerator_controller.cc (right):

https://codereview.chromium.org/10977088/diff/1/ash/accelerators/accelerator_...
ash/accelerators/accelerator_controller.cc:433: // for things Alt+Tab that would
cause an undesired effect in
On 2012/09/30 01:24:47, Daniel Erat wrote:
> nit: s/things/things like/

Done.

https://codereview.chromium.org/10977088/diff/1/ash/accelerators/accelerator_...
File ash/accelerators/accelerator_table.cc (right):

https://codereview.chromium.org/10977088/diff/1/ash/accelerators/accelerator_...
ash/accelerators/accelerator_table.cc:231: const AcceleratorAction
kActionsAllowedAtModalWindow[] = {
On 2012/09/29 00:54:33, Yusuke Sato wrote:
> I'm wondering if it's okay to omit OPEN_FEEDBACK_PAGE. What should the user do
> if he/she finds a problem while a modal dialog is up?
I experimented with this and talked to kuscher@. I will add OPEN_FEEDBACK_PAGE
to the "allow" list. We probably want to take the modal popup down after the
screenshot has been taken in this case.

https://codereview.chromium.org/10977088/diff/1/ash/accelerators/accelerator_...
ash/accelerators/accelerator_table.cc:248: SHOW_KEYBOARD_OVERLAY,
On 2012/09/29 00:54:33, Yusuke Sato wrote:
> does this work fine when a modal window is up? please check if you haven't
done
> it.
Yes, it works. 
Done.

https://codereview.chromium.org/10977088/diff/1/ash/accelerators/accelerator_...
ash/accelerators/accelerator_table.cc:249: SWAP_PRIMARY_DISPLAY,
On 2012/09/29 00:54:33, Yusuke Sato wrote:
> please also check if this works fine (you might need to connect a secondary
> monitor to you chromebook to check it).
Yes, it works.
Done.

https://codereview.chromium.org/10977088/diff/1/ash/accelerators/accelerator_...
ash/accelerators/accelerator_table.cc:260: CYCLE_DISPLAY_MODE,
On 2012/09/29 00:54:33, Yusuke Sato wrote:
> the same.
Yes, this works. 
Done.

[Yusuke Sato, 2012-10-16 01:03:18]

https://codereview.chromium.org/10977088/diff/7001/ash/accelerators/accelerat...
File ash/accelerators/accelerator_controller_unittest.cc (right):

https://codereview.chromium.org/10977088/diff/7001/ash/accelerators/accelerat...
ash/accelerators/accelerator_controller_unittest.cc:1048:
EXPECT_TRUE(GetController()->PerformAction(*it, dummy))
This test itself looks good to me, but I think it would be nicer if we could
also have tests like this (to make sure the top-row keys etc work even when a
modal window is up):

1. Add DummyXXXControllerDelegate objects to the controller
2. Call PerformAction() with F6, F7, F8, F9, F10, etc.
3. Confirm call counts of the delegate objects are >0 (see line 648-)

[Daniel Erat, 2012-10-16 03:03:50]

LGTM after addressing a few small comments

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

https://codereview.chromium.org/10977088/diff/7001/ash/accelerators/accelerat...
ash/accelerators/accelerator_controller.cc:365: for (size_t i = 0; i <
kActionsAllowedAtModalWindowLength; ++i) {
nit: not your fault, but i won't object if you want to clean it up here: chrome
style is typically to omit curly brackets for single-line if statements

https://codereview.chromium.org/10977088/diff/7001/ash/accelerators/accelerat...
ash/accelerators/accelerator_controller.cc:428: bool at_modal_window =
shell->IsModalWindowOpen();
nit: i don't think that there's any reason to create temporary variables for
this and at_lock_screen; you can just move the shell->Is...() calls into the if
conditions that are currently using the variables.  at_login_screen is probably
just a variable because it has the #ifdef-ed chrome os code.

https://codereview.chromium.org/10977088/diff/7001/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/10977088/diff/7001/ash/shell.cc#newcode532
ash/shell.cc:532: if (simulate_modal_window_open_for_testing_) return true;
move "return true" to the next line, indented two spaces beyond this one

https://codereview.chromium.org/10977088/diff/7001/ash/shell.h
File ash/shell.h (right):

https://codereview.chromium.org/10977088/diff/7001/ash/shell.h#newcode218
ash/shell.h:218: void SimulateModalWindowOpenForTesting(bool modalWindowOpen) {
s/modalWindowOpen/modal_window_open/

[sschmitz, 2012-10-16 18:18:23]

Thanks for your comments... I implemented them.

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

https://codereview.chromium.org/10977088/diff/7001/ash/accelerators/accelerat...
ash/accelerators/accelerator_controller.cc:365: for (size_t i = 0; i <
kActionsAllowedAtModalWindowLength; ++i) {
On 2012/10/16 03:03:50, Daniel Erat wrote:
> nit: not your fault, but i won't object if you want to clean it up here:
chrome
> style is typically to omit curly brackets for single-line if statements

Done.

https://codereview.chromium.org/10977088/diff/7001/ash/accelerators/accelerat...
ash/accelerators/accelerator_controller.cc:428: bool at_modal_window =
shell->IsModalWindowOpen();
On 2012/10/16 03:03:50, Daniel Erat wrote:
> nit: i don't think that there's any reason to create temporary variables for
> this and at_lock_screen; you can just move the shell->Is...() calls into the
if
> conditions that are currently using the variables.  at_login_screen is
probably
> just a variable because it has the #ifdef-ed chrome os code.

Done.

https://codereview.chromium.org/10977088/diff/7001/ash/accelerators/accelerat...
File ash/accelerators/accelerator_controller_unittest.cc (right):

https://codereview.chromium.org/10977088/diff/7001/ash/accelerators/accelerat...
ash/accelerators/accelerator_controller_unittest.cc:1048:
EXPECT_TRUE(GetController()->PerformAction(*it, dummy))
On 2012/10/16 01:03:18, Yusuke Sato wrote:
> This test itself looks good to me, but I think it would be nicer if we could
> also have tests like this (to make sure the top-row keys etc work even when a
> modal window is up):
> 
> 1. Add DummyXXXControllerDelegate objects to the controller
> 2. Call PerformAction() with F6, F7, F8, F9, F10, etc.
> 3. Confirm call counts of the delegate objects are >0 (see line 648-)

Done.

https://codereview.chromium.org/10977088/diff/7001/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/10977088/diff/7001/ash/shell.cc#newcode532
ash/shell.cc:532: if (simulate_modal_window_open_for_testing_) return true;
On 2012/10/16 03:03:50, Daniel Erat wrote:
> move "return true" to the next line, indented two spaces beyond this one

Done.

https://codereview.chromium.org/10977088/diff/7001/ash/shell.h
File ash/shell.h (right):

https://codereview.chromium.org/10977088/diff/7001/ash/shell.h#newcode218
ash/shell.h:218: void SimulateModalWindowOpenForTesting(bool modalWindowOpen) {
On 2012/10/16 03:03:50, Daniel Erat wrote:
> s/modalWindowOpen/modal_window_open/

Done.

[Yusuke Sato, 2012-10-16 18:30:10]

lgtm, thanks.

On 2012/10/16 18:18:23, sschmitz wrote:
> Thanks for your comments... I implemented them.
> 
>
https://codereview.chromium.org/10977088/diff/7001/ash/accelerators/accelerat...
> File ash/accelerators/accelerator_controller.cc (right):
> 
>
https://codereview.chromium.org/10977088/diff/7001/ash/accelerators/accelerat...
> ash/accelerators/accelerator_controller.cc:365: for (size_t i = 0; i <
> kActionsAllowedAtModalWindowLength; ++i) {
> On 2012/10/16 03:03:50, Daniel Erat wrote:
> > nit: not your fault, but i won't object if you want to clean it up here:
> chrome
> > style is typically to omit curly brackets for single-line if statements
> 
> Done.
> 
>
https://codereview.chromium.org/10977088/diff/7001/ash/accelerators/accelerat...
> ash/accelerators/accelerator_controller.cc:428: bool at_modal_window =
> shell->IsModalWindowOpen();
> On 2012/10/16 03:03:50, Daniel Erat wrote:
> > nit: i don't think that there's any reason to create temporary variables for
> > this and at_lock_screen; you can just move the shell->Is...() calls into the
> if
> > conditions that are currently using the variables.  at_login_screen is
> probably
> > just a variable because it has the #ifdef-ed chrome os code.
> 
> Done.
> 
>
https://codereview.chromium.org/10977088/diff/7001/ash/accelerators/accelerat...
> File ash/accelerators/accelerator_controller_unittest.cc (right):
> 
>
https://codereview.chromium.org/10977088/diff/7001/ash/accelerators/accelerat...
> ash/accelerators/accelerator_controller_unittest.cc:1048:
> EXPECT_TRUE(GetController()->PerformAction(*it, dummy))
> On 2012/10/16 01:03:18, Yusuke Sato wrote:
> > This test itself looks good to me, but I think it would be nicer if we could
> > also have tests like this (to make sure the top-row keys etc work even when
a
> > modal window is up):
> > 
> > 1. Add DummyXXXControllerDelegate objects to the controller
> > 2. Call PerformAction() with F6, F7, F8, F9, F10, etc.
> > 3. Confirm call counts of the delegate objects are >0 (see line 648-)
> 
> Done.
> 
> https://codereview.chromium.org/10977088/diff/7001/ash/shell.cc
> File ash/shell.cc (right):
> 
> https://codereview.chromium.org/10977088/diff/7001/ash/shell.cc#newcode532
> ash/shell.cc:532: if (simulate_modal_window_open_for_testing_) return true;
> On 2012/10/16 03:03:50, Daniel Erat wrote:
> > move "return true" to the next line, indented two spaces beyond this one
> 
> Done.
> 
> https://codereview.chromium.org/10977088/diff/7001/ash/shell.h
> File ash/shell.h (right):
> 
> https://codereview.chromium.org/10977088/diff/7001/ash/shell.h#newcode218
> ash/shell.h:218: void SimulateModalWindowOpenForTesting(bool modalWindowOpen)
{
> On 2012/10/16 03:03:50, Daniel Erat wrote:
> > s/modalWindowOpen/modal_window_open/
> 
> Done.

[commit-bot: I haz the power, 2012-10-16 18:40:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sschmitz@chromium.org/10977088/18001

[commit-bot: I haz the power, 2012-10-16 20:48:04]

Change committed as 162240