Implement cycle window forward/backward by keyboard shortcuts.

Implement cycle window forward/backward by keyboard shortcuts.


BUG=105204
TEST=Manual


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

[Ben Goodger (Google), 2011-12-07 17:13:00]

1. unit tests for this, please!
2. can you verify that this does the right thing (i.e. nothing) when a modal
dialog is showing? e.g. show a js alert from a page then try hitting Alt+Tab.
3. can you verify that this works in the non-chrome aura_shell_exe, and that
when it does, it does NOT trigger when the lock screen view is visible?

http://codereview.chromium.org/8817018/diff/2001/ui/aura_shell/shell_accelera...
File ui/aura_shell/shell_accelerator_controller.cc (right):

http://codereview.chromium.org/8817018/diff/2001/ui/aura_shell/shell_accelera...
ui/aura_shell/shell_accelerator_controller.cc:59: LOG(ERROR) << "Active window
not found";
does this ever happen?

[mazda, 2011-12-09 15:09:46]

Please take another look.

> 1. unit tests for this, please!
Done.
Since Launcher didn't work without ShellDelegate attched to Shell,
I added TestShellDelegate and attached it to Shell in the unittest.

> 2. can you verify that this does the right thing (i.e. nothing) when a modal
> dialog is showing? e.g. show a js alert from a page then try hitting Alt+Tab.
I verified that alt-tab and alt-shift-tab don't work when a modal dialog is
showing.
I added code to check this in the unittest.

> 3. can you verify that this works in the non-chrome aura_shell_exe, and that
> when it does, it does NOT trigger when the lock screen view is visible?
It worked in aura_shell_exe but it triggered even when the lock screen view is
visible,
so I added a check for the visibility of the screen view.
I also added a test to check if cycling window does not take effect when any
screen lock view is visible.

http://codereview.chromium.org/8817018/diff/2001/ui/aura_shell/shell_accelera...
File ui/aura_shell/shell_accelerator_controller.cc (right):

http://codereview.chromium.org/8817018/diff/2001/ui/aura_shell/shell_accelera...
ui/aura_shell/shell_accelerator_controller.cc:59: LOG(ERROR) << "Active window
not found";
On 2011/12/07 17:13:00, Ben Goodger (Google) wrote:
> does this ever happen?

All the windows in the launcher model are contained in the default container, so
this happens when the other kind of window is active (i.e. modal window).
I changed LOG(ERROR) to VLOG(2) because I rethought these are not error cases.

[Ben Goodger (Google), 2011-12-09 22:53:19]

http://codereview.chromium.org/8817018/diff/8001/ui/aura_shell/shell_accelera...
File ui/aura_shell/shell_accelerator_controller.cc (right):

http://codereview.chromium.org/8817018/diff/8001/ui/aura_shell/shell_accelera...
ui/aura_shell/shell_accelerator_controller.cc:56: bool IsLocked() {
I wonder if we might want to make this a function on Shell... I can imagine
other pieces of code wanting to query this state.

[mazda, 2011-12-10 04:44:53]

On 2011/12/09 22:53:19, Ben Goodger (Google) wrote:
>
http://codereview.chromium.org/8817018/diff/8001/ui/aura_shell/shell_accelera...
> File ui/aura_shell/shell_accelerator_controller.cc (right):
> 
>
http://codereview.chromium.org/8817018/diff/8001/ui/aura_shell/shell_accelera...
> ui/aura_shell/shell_accelerator_controller.cc:56: bool IsLocked() {
> I wonder if we might want to make this a function on Shell... I can imagine
> other pieces of code wanting to query this state.

I moved IsLocked to Shell (and renamed it to a clearer name, IsScreenLocked). I
also added a test for Shell::IsScreenLocked.
Please take another look.

[Ben Goodger (Google), 2011-12-11 21:54:05]

http://codereview.chromium.org/8817018/diff/11001/ui/aura_shell/shell.cc
File ui/aura_shell/shell.cc (right):

http://codereview.chromium.org/8817018/diff/11001/ui/aura_shell/shell.cc#newc...
ui/aura_shell/shell.cc:298: const aura::Window* lock_modal_container =
GetContainer(
I don't believe it's possible to return true from this check but not the
previous check.

[mazda, 2011-12-12 04:53:32]

http://codereview.chromium.org/8817018/diff/11001/ui/aura_shell/shell.cc
File ui/aura_shell/shell.cc (right):

http://codereview.chromium.org/8817018/diff/11001/ui/aura_shell/shell.cc#newc...
ui/aura_shell/shell.cc:298: const aura::Window* lock_modal_container =
GetContainer(
On 2011/12/11 21:54:05, Ben Goodger (Google) wrote:
> I don't believe it's possible to return true from this check but not the
> previous check.

This returns true in the line 240 of shell_unittest.cc in this CL.
Is that an unintended use of the lock modal window?

[Ben Goodger (Google), 2011-12-12 16:25:34]

We shouldn't have anything in the lock modal container unless the screen is
locked (i.e. there is content in the lock container)... so it seems checking for
content in the lock container is sufficient.

[mazda, 2011-12-12 18:53:28]

On 2011/12/12 16:25:34, Ben Goodger (Google) wrote:
> We shouldn't have anything in the lock modal container unless the screen is
> locked (i.e. there is content in the lock container)... so it seems checking
for
> content in the lock container is sufficient.

Thanks.
I removed the check of the lock modal container in Shell::IsScreenLocked and
modified the corresponding tests.

[Ben Goodger (Google), 2011-12-12 20:45:27]

lgtm

[commit-bot: I haz the power, 2011-12-13 04:28:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mazda@chromium.org/8817018/24001

[commit-bot: I haz the power, 2011-12-13 04:28:12]

Can't apply patch for file ui/aura_shell/aura_shell.gyp.
While running patch -p1 --forward --force;
patching file ui/aura_shell/aura_shell.gyp
Hunk #1 FAILED at 160.
1 out of 1 hunk FAILED -- saving rejects to file
ui/aura_shell/aura_shell.gyp.rej

[commit-bot: I haz the power, 2011-12-13 07:57:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mazda@chromium.org/8817018/26002

[commit-bot: I haz the power, 2011-12-13 09:48:55]

Change committed as 114198