Adding application lifetime tests to athena, allowing to override focus request for applciations

Adding application lifetime tests to athena, allowing to override focus request for applciations

Background: For Athena we have an intelligent applciation management which allows us to load and unload applications / activities at any time. In some cases we "pre-load" applications to short loading time. In that case we do not want that the application can show its window activated (thus bringing it to front). Instead we have to override that request.

At the same time several things have changed in the application window framework which required. The Browsertests are testing the activation override as well as the activity order upon (re-)creation.

I added miket as OWNER for the extensions/* changes.

BUG=419379
TEST=AppActivityBrowserTest.CreatedAppGetsFocus, AppActivityBrowserTest.UnloadReloadApplicationInPlace, AppActivityBrowserTest.ReloadActivatedApplication, AppActivityBrowserTest.ReloadMovedApplication

Committed: https://crrev.com/3498b1da35e90bcd6ee5b85951a7dc9e4e735be4
Cr-Commit-Position: refs/heads/master@{#299003}

[Mr4D (OOO till 08-26), 2014-10-07 21:58:26]

skuhne@chromium.org changed reviewers:
+ miket@chromium.org, oshima@chromium.org

[Mr4D (OOO till 08-26), 2014-10-07 22:02:23]

Please have a look!

[miket_OOO, 2014-10-07 22:58:21]

lgtm

https://codereview.chromium.org/631333003/diff/1/extensions/browser/extension...
File extensions/browser/extension_registry.h (right):

https://codereview.chromium.org/631333003/diff/1/extensions/browser/extension...
extensions/browser/extension_registry.h:48: // NOTE: These sets are *eventually*
mutually exclusive, but an extension can
Thanks!

[oshima, 2014-10-08 19:02:31]

https://codereview.chromium.org/631333003/diff/20001/athena/content/app_activ...
File athena/content/app_activity.cc (right):

https://codereview.chromium.org/631333003/diff/20001/athena/content/app_activ...
athena/content/app_activity.cc:113: // If the proxy was the active window, its
deletion will cause a window
Since the window may not be visible when created, we should do this when the
window becomes visible, shouldn't we?

https://codereview.chromium.org/631333003/diff/20001/athena/content/app_activ...
athena/content/app_activity.cc:123:
window_list_provider->GetWindowList().back());
If the app_proxy's window was active, shouldn't we activate the new window?

https://codereview.chromium.org/631333003/diff/20001/athena/content/app_activ...
athena/content/app_activity.cc:128: Activity::Delete(app_proxy);
how about something like

bool was_active = wm::IsActiveWindow(app_proxy->GetWindow());
Activity::Delegate(app_proxy);
if (was_active)
   wm::ActivateWindow(GetWindow());
else
  StackWindowFrontOF....

https://codereview.chromium.org/631333003/diff/20001/athena/content/app_activ...
File athena/content/app_activity_browsertest.cc (right):

https://codereview.chromium.org/631333003/diff/20001/athena/content/app_activ...
athena/content/app_activity_browsertest.cc:27: 
// <class to override>:

https://codereview.chromium.org/631333003/diff/20001/athena/extensions/athena...
File athena/extensions/athena_app_window_client_base.cc (right):

https://codereview.chromium.org/631333003/diff/20001/athena/extensions/athena...
athena/extensions/athena_app_window_client_base.cc:44: if (iter !=
window_list.end() && ++iter != window_list.end())
This looks fragile. How about adding explicit RESUMED state?

https://codereview.chromium.org/631333003/diff/20001/extensions/browser/app_w...
File extensions/browser/app_window/app_window_client.h (right):

https://codereview.chromium.org/631333003/diff/20001/extensions/browser/app_w...
extensions/browser/app_window/app_window_client.h:36: AppWindow::CreateParams&
params) = 0;
in_out parameter should be * rather than reference.

[Mr4D (OOO till 08-26), 2014-10-08 23:48:17]

Please have another look!

https://codereview.chromium.org/631333003/diff/20001/athena/content/app_activ...
File athena/content/app_activity.cc (right):

https://codereview.chromium.org/631333003/diff/20001/athena/content/app_activ...
athena/content/app_activity.cc:113: // If the proxy was the active window, its
deletion will cause a window
As stated in the next comment - the visibility will be resolved when the app
gets shown. All we do is to position it properly (no activation).

Beside that the app proxy should be deleted as early as possible because
otherwise the proxy activity has influence on the resource manager's decisions
and that should not be (unless we want to add there more logic to avoid this).

I added a comment to revisit this code after discussing how we want to address
temporarily invisibility of applications. (Same applies for the other comments
here)

https://codereview.chromium.org/631333003/diff/20001/athena/content/app_activ...
athena/content/app_activity.cc:123:
window_list_provider->GetWindowList().back());
A hidden window cannot be activated. As you have pointed out, applications can
create their window hidden. The activation will either happen in
BaseWindow::Show() - or - it will remain hidden and the currently activated
window will remain activated (and visible since the window is hidden).

https://codereview.chromium.org/631333003/diff/20001/athena/content/app_activ...
athena/content/app_activity.cc:128: Activity::Delete(app_proxy);
As stated above already - the window might neither be nor become visible - so
ActivateWindow might not work on it.

https://codereview.chromium.org/631333003/diff/20001/athena/content/app_activ...
File athena/content/app_activity_browsertest.cc (right):

https://codereview.chromium.org/631333003/diff/20001/athena/content/app_activ...
athena/content/app_activity_browsertest.cc:27: 
On 2014/10/08 19:02:31, oshima wrote:
> // <class to override>:

Done.

https://codereview.chromium.org/631333003/diff/20001/athena/extensions/athena...
File athena/extensions/athena_app_window_client_base.cc (right):

https://codereview.chromium.org/631333003/diff/20001/athena/extensions/athena...
athena/extensions/athena_app_window_client_base.cc:44: if (iter !=
window_list.end() && ++iter != window_list.end())
As discussed - added comment.

https://codereview.chromium.org/631333003/diff/20001/extensions/browser/app_w...
File extensions/browser/app_window/app_window_client.h (right):

https://codereview.chromium.org/631333003/diff/20001/extensions/browser/app_w...
extensions/browser/app_window/app_window_client.h:36: AppWindow::CreateParams&
params) = 0;
On 2014/10/08 19:02:31, oshima wrote:
> in_out parameter should be * rather than reference.

Done.

[oshima, 2014-10-09 04:53:33]

https://codereview.chromium.org/631333003/diff/20001/athena/content/app_activ...
File athena/content/app_activity.cc (right):

https://codereview.chromium.org/631333003/diff/20001/athena/content/app_activ...
athena/content/app_activity.cc:113: // If the proxy was the active window, its
deletion will cause a window
On 2014/10/08 23:48:16, Mr4D wrote:
> As stated in the next comment - the visibility will be resolved when the app
> gets shown. All we do is to position it properly (no activation).
> 
> Beside that the app proxy should be deleted as early as possible because
> otherwise the proxy activity has influence on the resource manager's decisions
> and that should not be (unless we want to add there more logic to avoid this).
> 
> I added a comment to revisit this code after discussing how we want to address
> temporarily invisibility of applications. (Same applies for the other comments
> here)

Acknowledged.

https://codereview.chromium.org/631333003/diff/20001/athena/content/app_activ...
athena/content/app_activity.cc:123:
window_list_provider->GetWindowList().back());
On 2014/10/08 23:48:16, Mr4D wrote:
> A hidden window cannot be activated. As you have pointed out, applications can
> create their window hidden. The activation will either happen in
> BaseWindow::Show() - or - it will remain hidden and the currently activated
> window will remain activated (and visible since the window is hidden).

Acknowledged.

https://codereview.chromium.org/631333003/diff/20001/athena/extensions/athena...
File athena/extensions/athena_app_window_client_base.cc (right):

https://codereview.chromium.org/631333003/diff/20001/athena/extensions/athena...
athena/extensions/athena_app_window_client_base.cc:32: // not set the focus
since it will change the window order again.
what happens in split view mode?

https://codereview.chromium.org/631333003/diff/20001/athena/extensions/athena...
athena/extensions/athena_app_window_client_base.cc:40: // will not be in our
WindowListProvider list. However - if it was not at
PANEL windows are in the list. see WindowListProviderImpl::IsValidWindow

https://codereview.chromium.org/631333003/diff/20001/athena/extensions/athena...
athena/extensions/athena_app_window_client_base.cc:44: if (iter !=
window_list.end() && ++iter != window_list.end())
On 2014/10/08 23:48:16, Mr4D wrote:
> As discussed - added comment.

I still think this is not right way to do. The code like this is fragile as this
depends on specific implementation details. I'm ok as temporal solution, but
this needs be fixed regardless of visibility issue.

If checking with the top window does the right thing (see my comment about split
view mode),
then you can do:

params.focused = !list.size() || list.back() == native_app_window;

https://codereview.chromium.org/631333003/diff/40001/athena/content/app_activ...
File athena/content/app_activity.cc (right):

https://codereview.chromium.org/631333003/diff/40001/athena/content/app_activ...
athena/content/app_activity.cc:129: Activity::Delete(app_proxy);
Can you move Activity::Delegate(app_proxy) before moving window so that both
path does delete -> restacking

[Mr4D (OOO till 08-26), 2014-10-09 14:46:44]

Have another look!

https://codereview.chromium.org/631333003/diff/20001/athena/extensions/athena...
File athena/extensions/athena_app_window_client_base.cc (right):

https://codereview.chromium.org/631333003/diff/20001/athena/extensions/athena...
athena/extensions/athena_app_window_client_base.cc:32: // not set the focus
since it will change the window order again.
The focused/activated window always being on top it doesn't matter if this is
split mode or not. In the end only one of the two windows can have focus (which
is nothing else than we have in ash).

https://codereview.chromium.org/631333003/diff/20001/athena/extensions/athena...
athena/extensions/athena_app_window_client_base.cc:40: // will not be in our
WindowListProvider list. However - if it was not at
Since it got changed ysterday they were explicitly excluded.

https://codereview.chromium.org/631333003/diff/20001/athena/extensions/athena...
athena/extensions/athena_app_window_client_base.cc:44: if (iter !=
window_list.end() && ++iter != window_list.end())
Both versions do exactly the same - but this one is simpler.

Done.

I have also changed the comment and removed the TODO since I realized that this
has nothing to do with the issue since an application can be preloaded by the
resource manager. See comment.

https://codereview.chromium.org/631333003/diff/40001/athena/content/app_activ...
File athena/content/app_activity.cc (right):

https://codereview.chromium.org/631333003/diff/40001/athena/content/app_activ...
athena/content/app_activity.cc:129: Activity::Delete(app_proxy);
That is what I tried to tell you yesterday - no this order is crucial. By
deleting the activity, in one case the focused window will get deleted causing
the next foucussable window to bubble to the front (since this window is yet
hidden and therefore not activatable). So the move has to be done *after* the
reorder.
In the other case however you have to first reorganize the window relative to
the proxy and then delete it - otherwise the proxy is gone and you cannot move
the window into its position. Note also that in that case the activation is not
on the proxy - which is this difference about.

[oshima, 2014-10-09 16:06:43]

https://codereview.chromium.org/631333003/diff/20001/athena/extensions/athena...
File athena/extensions/athena_app_window_client_base.cc (right):

https://codereview.chromium.org/631333003/diff/20001/athena/extensions/athena...
athena/extensions/athena_app_window_client_base.cc:32: // not set the focus
since it will change the window order again.
On 2014/10/09 14:46:44, Mr4D wrote:
> The focused/activated window always being on top it doesn't matter if this is
> split mode or not. In the end only one of the two windows can have focus
(which
> is nothing else than we have in ash).

Consider following:

1) Got to split view, with two app activities.
2) Switch to another activity. This moves both activities behind the top one.
3) Left activity got evicted.
4) Got to overview mode, and select right activity of the split view one.
5) Since both activities become visible, left activity should be resumed.

In this case, the right activity should be activated, but this code may
re-activate the left activity when resumed?

https://codereview.chromium.org/631333003/diff/40001/athena/content/app_activ...
File athena/content/app_activity.cc (right):

https://codereview.chromium.org/631333003/diff/40001/athena/content/app_activ...
athena/content/app_activity.cc:129: Activity::Delete(app_proxy);
On 2014/10/09 14:46:44, Mr4D wrote:
> That is what I tried to tell you yesterday - no this order is crucial. By
> deleting the activity, in one case the focused window will get deleted causing
> the next foucussable window to bubble to the front (since this window is yet
> hidden and therefore not activatable). So the move has to be done *after* the
> reorder.
> In the other case however you have to first reorganize the window relative to
> the proxy and then delete it - otherwise the proxy is gone and you cannot move
> the window into its position. Note also that in that case the activation is
not
> on the proxy - which is this difference about.

I see, in that case, can you update the comment why it's doing this way
rather than what it is doing?

[oshima, 2014-10-09 17:19:20]

lgtm

https://codereview.chromium.org/631333003/diff/20001/athena/extensions/athena...
File athena/extensions/athena_app_window_client_base.cc (right):

https://codereview.chromium.org/631333003/diff/20001/athena/extensions/athena...
athena/extensions/athena_app_window_client_base.cc:32: // not set the focus
since it will change the window order again.
On 2014/10/09 16:06:43, oshima wrote:
> On 2014/10/09 14:46:44, Mr4D wrote:
> > The focused/activated window always being on top it doesn't matter if this
is
> > split mode or not. In the end only one of the two windows can have focus
> (which
> > is nothing else than we have in ash).
> 
> Consider following:
> 
> 1) Got to split view, with two app activities.
> 2) Switch to another activity. This moves both activities behind the top one.
> 3) Left activity got evicted.
> 4) Got to overview mode, and select right activity of the split view one.
> 5) Since both activities become visible, left activity should be resumed.
> 
> In this case, the right activity should be activated, but this code may
> re-activate the left activity when resumed?

Discussed offline. This never happens during overview mode transition, so this
is fine.

[Mr4D (OOO till 08-26), 2014-10-09 17:28:03]

Added comment and created issue to create a browser test for the split view case
in a separate issue.

Thanks for the review!

https://codereview.chromium.org/631333003/diff/20001/athena/extensions/athena...
File athena/extensions/athena_app_window_client_base.cc (right):

https://codereview.chromium.org/631333003/diff/20001/athena/extensions/athena...
athena/extensions/athena_app_window_client_base.cc:32: // not set the focus
since it will change the window order again.
It's fine. It's exactly covered by this case. (Well at least until something
changes this again - and yes I agree that there are far too many things which
can change activation in this event chain(!!).)

I create however a browser test which verifies that this is correct. => issue
421947.

https://codereview.chromium.org/631333003/diff/40001/athena/content/app_activ...
File athena/content/app_activity.cc (right):

https://codereview.chromium.org/631333003/diff/40001/athena/content/app_activ...
athena/content/app_activity.cc:129: Activity::Delete(app_proxy);
On 2014/10/09 16:06:43, oshima wrote:
> On 2014/10/09 14:46:44, Mr4D wrote:
> > That is what I tried to tell you yesterday - no this order is crucial. By
> > deleting the activity, in one case the focused window will get deleted
causing
> > the next foucussable window to bubble to the front (since this window is yet
> > hidden and therefore not activatable). So the move has to be done *after*
the
> > reorder.
> > In the other case however you have to first reorganize the window relative
to
> > the proxy and then delete it - otherwise the proxy is gone and you cannot
move
> > the window into its position. Note also that in that case the activation is
> not
> > on the proxy - which is this difference about.
> 
> I see, in that case, can you update the comment why it's doing this way
> rather than what it is doing?

Done.

[Mr4D (OOO till 08-26), 2014-10-09 17:28:10]

The CQ bit was checked by skuhne@chromium.org

[commit-bot: I haz the power, 2014-10-09 17:29:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/631333003/90001

[commit-bot: I haz the power, 2014-10-09 18:37:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-09 18:37:17]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Mr4D (OOO till 08-26), 2014-10-09 19:31:37]

The CQ bit was checked by skuhne@chromium.org

[commit-bot: I haz the power, 2014-10-09 19:32:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/631333003/280001

[commit-bot: I haz the power, 2014-10-09 20:21:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-09 20:21:27]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Mr4D (OOO till 08-26), 2014-10-09 20:58:18]

The CQ bit was checked by skuhne@chromium.org

[commit-bot: I haz the power, 2014-10-09 20:59:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/631333003/440001

[commit-bot: I haz the power, 2014-10-09 23:00:09]

Committed patchset #7 (id:440001)

[commit-bot: I haz the power, 2014-10-09 23:01:07]

Patchset 7 (id:??) landed as
https://crrev.com/3498b1da35e90bcd6ee5b85951a7dc9e4e735be4
Cr-Commit-Position: refs/heads/master@{#299003}