Allow focus to be sent between browser window and launcher/status window

Allow focus to be sent between browser window and launcher/status window

R=sky@chromium.org
BUG=104192
TEST=Use ctrl+forward/back to move between browser window and status area/launcher


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

[Zachary Kuznia, 2012-01-30 12:21:02]

Hello Sky,

Could you have a look at this CL?  It still has one TODO, but otherwise it's
ready.  In particular, I want to make sure I'm using a good method to move focus
between areas.  Also, I'm not sure the best way to activate the browser again
once the status area or launcher has been activated.  Do you have any suggestion
for this?

Thanks!
-Zach

[sky, 2012-01-30 15:52:45]

I believe all this logic can live in the ash side. Use
FocusManager::RegisterAccelerator to move the code out of status area/launcher
and into shell.cc. I'm not saying the logic need live in shell.cc, but shell
should create the appropriate classes that register the accelerators. To focus
the first browser window you should be able to use GetCycleWindowList.

https://chromiumcodereview.appspot.com/9295049/diff/1/ash/launcher/launcher.cc
File ash/launcher/launcher.cc (right):

https://chromiumcodereview.appspot.com/9295049/diff/1/ash/launcher/launcher.c...
ash/launcher/launcher.cc:138: if (focus) {
nit: no {}

https://chromiumcodereview.appspot.com/9295049/diff/1/ash/launcher/launcher.h
File ash/launcher/launcher.h (right):

https://chromiumcodereview.appspot.com/9295049/diff/1/ash/launcher/launcher.h...
ash/launcher/launcher.h:36: bool Activate();
Newline between 35/36 and add a description.

[Zachary Kuznia, 2012-01-31 11:00:50]

Pretty much all of the logic is encompassed in FocusCycler now.  It made the
rest of the code much prettier.

https://chromiumcodereview.appspot.com/9295049/diff/1/ash/launcher/launcher.cc
File ash/launcher/launcher.cc (right):

https://chromiumcodereview.appspot.com/9295049/diff/1/ash/launcher/launcher.c...
ash/launcher/launcher.cc:138: if (focus) {
On 2012/01/30 15:52:46, sky wrote:
> nit: no {}

Done.

https://chromiumcodereview.appspot.com/9295049/diff/1/ash/launcher/launcher.h
File ash/launcher/launcher.h (right):

https://chromiumcodereview.appspot.com/9295049/diff/1/ash/launcher/launcher.h...
ash/launcher/launcher.h:36: bool Activate();
On 2012/01/30 15:52:46, sky wrote:
> Newline between 35/36 and add a description.

Done.

[sky, 2012-01-31 16:38:14]

One additional thing that needs to be handled. In compact mode the launcher is
not visible. In full screen mode the launcher and status are is not visible. The
rotate code will need to handle this.

https://chromiumcodereview.appspot.com/9295049/diff/7001/ash/focus_cycler.cc
File ash/focus_cycler.cc (right):

https://chromiumcodereview.appspot.com/9295049/diff/7001/ash/focus_cycler.cc#...
ash/focus_cycler.cc:47: bool FocusCycler::CanHandleAccelerators() const {
Make order match that of header.

https://chromiumcodereview.appspot.com/9295049/diff/7001/ash/focus_cycler.cc#...
ash/focus_cycler.cc:75: return;
remove the early return here and 85.

https://chromiumcodereview.appspot.com/9295049/diff/7001/ash/focus_cycler.h
File ash/focus_cycler.h (right):

https://chromiumcodereview.appspot.com/9295049/diff/7001/ash/focus_cycler.h#n...
ash/focus_cycler.h:9: #include "ash/accelerators/accelerator_controller.h"
Include ui/base/accelerators/accelerator instead.

https://chromiumcodereview.appspot.com/9295049/diff/7001/ash/focus_cycler.h#n...
ash/focus_cycler.h:11: #include <vector>
vector should be the first include.

https://chromiumcodereview.appspot.com/9295049/diff/7001/ash/focus_cycler.h#n...
ash/focus_cycler.h:18: 
If this class isn't needed outside of ash, put it in the internal namespace.

https://chromiumcodereview.appspot.com/9295049/diff/7001/ash/focus_cycler.h#n...
ash/focus_cycler.h:24: ~FocusCycler();
virtual

https://chromiumcodereview.appspot.com/9295049/diff/7001/ash/focus_cycler.h#n...
ash/focus_cycler.h:31: void RotateFocus(bool forwards);
Use an enum to make it clear what true/value mean.

https://chromiumcodereview.appspot.com/9295049/diff/7001/ash/focus_cycler.h#n...
ash/focus_cycler.h:38: ui::Accelerator ctrl_forward_key_;
Do these really need to exist as members? Can AddWidget create them on the stack
to pass to Register?

[sky, 2012-01-31 16:38:31]

Is it possible to write a test case for FocusCycler?

[Zachary Kuznia, 2012-01-31 17:57:42]

I'm looking into making the test for the focus cycler in a separate CL.  I can
file a bug and add a TODO if needed.

To handle hidden Launcher and Status window, I put the focus in a loop, similar
to the BrowserView implementation.

https://chromiumcodereview.appspot.com/9295049/diff/7001/ash/focus_cycler.cc
File ash/focus_cycler.cc (right):

https://chromiumcodereview.appspot.com/9295049/diff/7001/ash/focus_cycler.cc#...
ash/focus_cycler.cc:47: bool FocusCycler::CanHandleAccelerators() const {
On 2012/01/31 16:38:15, sky wrote:
> Make order match that of header.

Done.

https://chromiumcodereview.appspot.com/9295049/diff/7001/ash/focus_cycler.cc#...
ash/focus_cycler.cc:75: return;
On 2012/01/31 16:38:15, sky wrote:
> remove the early return here and 85.

Changed to break to exit the loop.  The loop is added to skip over invisible
elements.

https://chromiumcodereview.appspot.com/9295049/diff/7001/ash/focus_cycler.h
File ash/focus_cycler.h (right):

https://chromiumcodereview.appspot.com/9295049/diff/7001/ash/focus_cycler.h#n...
ash/focus_cycler.h:9: #include "ash/accelerators/accelerator_controller.h"
On 2012/01/31 16:38:15, sky wrote:
> Include ui/base/accelerators/accelerator instead.

Done.

https://chromiumcodereview.appspot.com/9295049/diff/7001/ash/focus_cycler.h#n...
ash/focus_cycler.h:11: #include <vector>
On 2012/01/31 16:38:15, sky wrote:
> vector should be the first include.

Done.

https://chromiumcodereview.appspot.com/9295049/diff/7001/ash/focus_cycler.h#n...
ash/focus_cycler.h:18: 
On 2012/01/31 16:38:15, sky wrote:
> If this class isn't needed outside of ash, put it in the internal namespace.

Done.

https://chromiumcodereview.appspot.com/9295049/diff/7001/ash/focus_cycler.h#n...
ash/focus_cycler.h:24: ~FocusCycler();
On 2012/01/31 16:38:15, sky wrote:
> virtual

Done.

https://chromiumcodereview.appspot.com/9295049/diff/7001/ash/focus_cycler.h#n...
ash/focus_cycler.h:31: void RotateFocus(bool forwards);
On 2012/01/31 16:38:15, sky wrote:
> Use an enum to make it clear what true/value mean.

Done.

https://chromiumcodereview.appspot.com/9295049/diff/7001/ash/focus_cycler.h#n...
ash/focus_cycler.h:38: ui::Accelerator ctrl_forward_key_;
On 2012/01/31 16:38:15, sky wrote:
> Do these really need to exist as members? Can AddWidget create them on the
stack
> to pass to Register?

Done.

[sky, 2012-01-31 18:06:16]

Did you make sure we don't end up giving focus to the launcher when in compact
mode?

[Zachary Kuznia, 2012-01-31 18:13:10]

In compact mode, the Launcher's widget isn't added to the FocusCycler in
shell.cc.  Also, if it isn't visible, it shouldn't be able to set focus,
and would continue to the next widget.

On Wed, Feb 1, 2012 at 3:06 AM, <sky@chromium.org> wrote:

> Did you make sure we don't end up giving focus to the launcher when in
> compact
> mode?
>
>
https://chromiumcodereview.**appspot.com/9295049/<https://chromiumcodereview....
>

[sky, 2012-01-31 18:56:08]

Ah, ok. That covers compact model. My worry with fullscreen mode is
that I don't know if we actually make the launcher invisible. Did you
verify that too?

  -Scott

On Tue, Jan 31, 2012 at 10:13 AM, Zach Kuznia <zork@chromium.org> wrote:
> In compact mode, the Launcher's widget isn't added to the FocusCycler in
> shell.cc.  Also, if it isn't visible, it shouldn't be able to set focus, and
> would continue to the next widget.
>
>
> On Wed, Feb 1, 2012 at 3:06 AM, <sky@chromium.org> wrote:
>>
>> Did you make sure we don't end up giving focus to the launcher when in
>> compact
>> mode?
>>
>> https://chromiumcodereview.appspot.com/9295049/
>
>

[Zachary Kuznia, 2012-02-01 08:00:43]

I have verified that in fullscreen mode focus does not move to the status area
or launcher, as expected.

[sky, 2012-02-03 15:53:21]

https://chromiumcodereview.appspot.com/9295049/diff/17001/ash/shell.cc
File ash/shell.cc (right):

https://chromiumcodereview.appspot.com/9295049/diff/17001/ash/shell.cc#newcod...
ash/shell.cc:303: if (!IsWindowModeCompact())
The window mode can be changed at runtime. See ChangeWindowMOde.

[Zachary Kuznia, 2012-02-06 05:48:08]

https://chromiumcodereview.appspot.com/9295049/diff/17001/ash/shell.cc
File ash/shell.cc (right):

https://chromiumcodereview.appspot.com/9295049/diff/17001/ash/shell.cc#newcod...
ash/shell.cc:303: if (!IsWindowModeCompact())
On 2012/02/03 15:53:22, sky wrote:
> The window mode can be changed at runtime. See ChangeWindowMOde.

Fixed to always add the launcher, and skip over it when it's hidden.

[sky, 2012-02-06 16:29:31]

LGTM

https://chromiumcodereview.appspot.com/9295049/diff/14006/ash/focus_cycler.cc
File ash/focus_cycler.cc (right):

https://chromiumcodereview.appspot.com/9295049/diff/14006/ash/focus_cycler.cc...
ash/focus_cycler.cc:75: if (widget->IsActive()) {
nit: add a comment here, and remove {}

[sky, 2012-02-06 16:30:54]

Just make sure we don't end up with focus on the launcher when in fullscreen and
the launcher isn't on screen.

[sky, 2012-02-06 16:49:16]

Actually, could you make a test for this case so I don't have to worry about it.

Thanks!

  -Scott

On Mon, Feb 6, 2012 at 8:30 AM,  <sky@chromium.org> wrote:
> Just make sure we don't end up with focus on the launcher when in fullscreen
> and
> the launcher isn't on screen.
>
> http://codereview.chromium.org/9295049/

[Zachary Kuznia, 2012-02-06 23:33:26]

On 2012/02/06 16:49:16, sky wrote:
> Actually, could you make a test for this case so I don't have to worry about
it.
> 
> Thanks!
> 
>   -Scott
> 
> On Mon, Feb 6, 2012 at 8:30 AM,  <mailto:sky@chromium.org> wrote:
> > Just make sure we don't end up with focus on the launcher when in fullscreen
> > and
> > the launcher isn't on screen.
> >
> > http://codereview.chromium.org/9295049/

Certainly.  I'll add a test once the unittest CL lands.

[commit-bot: I haz the power, 2012-02-06 23:37:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zork@chromium.org/9295049/26001

[commit-bot: I haz the power, 2012-02-07 03:04:33]

Try job failure for 9295049-26001 (retry) on mac_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-02-07 03:42:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zork@chromium.org/9295049/26001

[commit-bot: I haz the power, 2012-02-07 05:44:12]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is mac_rel, revision is 120703, job name
was 9295049-26001 (previous was lost) (previous was lost) (previous was lost)
(previous was lost).

[commit-bot: I haz the power, 2012-02-07 06:07:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zork@chromium.org/9295049/26001

[commit-bot: I haz the power, 2012-02-07 07:41:52]

Change committed as 120738