Activate windows in topmost containers when a window is hidden.

Activate windows in higher containers if they exist.

BUG=116239
TEST=Open a system modal dialog, lock and unlock the screen. The system modal dialog should get focus back.

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

[flackr, 2012-03-01 23:51:55]

Hi Scott,

Can you take a look at this. This is making me wonder if perhaps we should have
a separate ordering for focus than we do for stacking as with the always on top
container case. Also I should add a test but the test_container idea will have
to be reworked.

Anyways, I just wanted to get early feedback. PTAL, thanks!

https://chromiumcodereview.appspot.com/9568045/diff/1/ash/wm/activation_contr...
File ash/wm/activation_controller.cc (right):

https://chromiumcodereview.appspot.com/9568045/diff/1/ash/wm/activation_contr...
ash/wm/activation_controller.cc:31: kShellWindowId_AlwaysOnTopContainer,
Not sure if this is right. This might be annoying if your always on top windows
are always the next to get focus.

https://chromiumcodereview.appspot.com/9568045/diff/1/ash/wm/activation_contr...
ash/wm/activation_controller.cc:222: if (default_container_for_test_) {
Currently this prevents me from testing this properly. Do you have any ideas?

[sky, 2012-03-02 00:20:25]

https://chromiumcodereview.appspot.com/9568045/diff/1/ash/wm/activation_contr...
File ash/wm/activation_controller.cc (right):

https://chromiumcodereview.appspot.com/9568045/diff/1/ash/wm/activation_contr...
ash/wm/activation_controller.cc:26: const int kWindowContainersCount = 5;
Can't you use arraysize?

https://chromiumcodereview.appspot.com/9568045/diff/1/ash/wm/activation_contr...
ash/wm/activation_controller.cc:31: kShellWindowId_AlwaysOnTopContainer,
On 2012/03/01 23:51:55, flackr wrote:
> Not sure if this is right. This might be annoying if your always on top
windows
> are always the next to get focus.

Maybe we should start in the container you're in. Also, shouldn't this be the
same list as that in SupportsChildActivation?

https://chromiumcodereview.appspot.com/9568045/diff/1/ash/wm/activation_contr...
ash/wm/activation_controller.cc:222: if (default_container_for_test_) {
On 2012/03/01 23:51:55, flackr wrote:
> Currently this prevents me from testing this properly. Do you have any ideas?

Set it to NULL for your test?

[flackr, 2012-03-02 18:09:28]

I've added some tests, and addressed your comments. Please take another look.

https://chromiumcodereview.appspot.com/9568045/diff/1/ash/wm/activation_contr...
File ash/wm/activation_controller.cc (right):

https://chromiumcodereview.appspot.com/9568045/diff/1/ash/wm/activation_contr...
ash/wm/activation_controller.cc:26: const int kWindowContainersCount = 5;
On 2012/03/02 00:20:25, sky wrote:
> Can't you use arraysize?

Cool, thanks! Done.

https://chromiumcodereview.appspot.com/9568045/diff/1/ash/wm/activation_contr...
ash/wm/activation_controller.cc:31: kShellWindowId_AlwaysOnTopContainer,
On 2012/03/02 00:20:25, sky wrote:
> On 2012/03/01 23:51:55, flackr wrote:
> > Not sure if this is right. This might be annoying if your always on top
> windows
> > are always the next to get focus.
> 
> Maybe we should start in the container you're in. Also, shouldn't this be the
> same list as that in SupportsChildActivation?

I like that idea, starting from the container you're in. I don't think it should
be possible for a window under the lock screen to get focus and disappear
causing it to ignore the lock screen. Done.

https://chromiumcodereview.appspot.com/9568045/diff/1/ash/wm/activation_contr...
ash/wm/activation_controller.cc:222: if (default_container_for_test_) {
On 2012/03/02 00:20:25, sky wrote:
> On 2012/03/01 23:51:55, flackr wrote:
> > Currently this prevents me from testing this properly. Do you have any
ideas?
> 
> Set it to NULL for your test?

Right, I'll do that and use all the standard containers, thanks.

[flackr, 2012-03-02 18:12:22]

https://chromiumcodereview.appspot.com/9568045/diff/1/ash/wm/activation_contr...
File ash/wm/activation_controller.cc (right):

https://chromiumcodereview.appspot.com/9568045/diff/1/ash/wm/activation_contr...
ash/wm/activation_controller.cc:222: if (default_container_for_test_) {
On 2012/03/02 18:09:28, flackr wrote:
> On 2012/03/02 00:20:25, sky wrote:
> > On 2012/03/01 23:51:55, flackr wrote:
> > > Currently this prevents me from testing this properly. Do you have any
> ideas?
> > 
> > Set it to NULL for your test?
> 
> Right, I'll do that and use all the standard containers, thanks.

This didn't actually end up being an issue.

[oshima, 2012-03-02 18:25:38]

https://chromiumcodereview.appspot.com/9568045/diff/8001/ash/wm/activation_co...
File ash/wm/activation_controller.cc (right):

https://chromiumcodereview.appspot.com/9568045/diff/8001/ash/wm/activation_co...
ash/wm/activation_controller.cc:225: if (default_container_for_test_) {
I know this isn't the scope of your CL, but asking anyway..

Since all ash tests should run with shell instance, can we remove this path? I'm
a bit uncomfortable to have different execution path for test because we're
losing coverage.

https://chromiumcodereview.appspot.com/9568045/diff/8001/ash/wm/activation_co...
ash/wm/activation_controller.cc:230: size_t i = 0;
current_container_index / active_container_index ?

[flackr, 2012-03-02 18:58:40]

https://chromiumcodereview.appspot.com/9568045/diff/8001/ash/wm/activation_co...
File ash/wm/activation_controller.cc (right):

https://chromiumcodereview.appspot.com/9568045/diff/8001/ash/wm/activation_co...
ash/wm/activation_controller.cc:225: if (default_container_for_test_) {
On 2012/03/02 18:25:38, oshima wrote:
> I know this isn't the scope of your CL, but asking anyway..
> 
> Since all ash tests should run with shell instance, can we remove this path?
I'm
> a bit uncomfortable to have different execution path for test because we're
> losing coverage.

Done.

https://chromiumcodereview.appspot.com/9568045/diff/8001/ash/wm/activation_co...
ash/wm/activation_controller.cc:230: size_t i = 0;
On 2012/03/02 18:25:38, oshima wrote:
> current_container_index / active_container_index ?

Done.

[oshima, 2012-03-02 19:08:40]

https://chromiumcodereview.appspot.com/9568045/diff/6005/ash/wm/activation_co...
File ash/wm/activation_controller.cc (right):

https://chromiumcodereview.appspot.com/9568045/diff/6005/ash/wm/activation_co...
ash/wm/activation_controller.cc:51: }
I think you can use  std::find, although it may not looks much better, so i'll
leave it to you.

https://chromiumcodereview.appspot.com/9568045/diff/6005/ash/wm/activation_co...
ash/wm/activation_controller.cc:227: for (size_t i = 0; i <
arraysize(kWindowContainerIds); i++) {
can ignore be null? if so, ignore && i < ...

https://chromiumcodereview.appspot.com/9568045/diff/6005/ash/wm/activation_co...
ash/wm/activation_controller.cc:238: current_container_index++) {
indent

https://chromiumcodereview.appspot.com/9568045/diff/6005/ash/wm/activation_co...
File ash/wm/activation_controller.h (left):

https://chromiumcodereview.appspot.com/9568045/diff/6005/ash/wm/activation_co...
ash/wm/activation_controller.h:54: }
oh, it wasn't used? glad I asked :) and thank you for taking care of this.

[flackr, 2012-03-02 19:28:37]

https://chromiumcodereview.appspot.com/9568045/diff/6005/ash/wm/activation_co...
File ash/wm/activation_controller.cc (right):

https://chromiumcodereview.appspot.com/9568045/diff/6005/ash/wm/activation_co...
ash/wm/activation_controller.cc:51: }
On 2012/03/02 19:08:40, oshima wrote:
> I think you can use  std::find, although it may not looks much better, so i'll
> leave it to you.

I'll leave it as it is, std::find looks pretty messy.

https://chromiumcodereview.appspot.com/9568045/diff/6005/ash/wm/activation_co...
ash/wm/activation_controller.cc:227: for (size_t i = 0; i <
arraysize(kWindowContainerIds); i++) {
On 2012/03/02 19:08:40, oshima wrote:
> can ignore be null? if so, ignore && i < ...

Done.

https://chromiumcodereview.appspot.com/9568045/diff/6005/ash/wm/activation_co...
ash/wm/activation_controller.cc:238: current_container_index++) {
On 2012/03/02 19:08:40, oshima wrote:
> indent

Done.

[sky, 2012-03-02 20:45:06]

LGTM

[oshima, 2012-03-02 20:46:40]

lgtm