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

athena/extensions/athena_app_window_client_base.cc

Index: athena/extensions/athena_app_window_client_base.cc
diff --git a/athena/extensions/athena_app_window_client_base.cc b/athena/extensions/athena_app_window_client_base.cc
index a37d1b2a632c1fa8e5f16531a8321cf97f57716e..60e95e03176d0ecf6e24424a7f3207fb4defd552 100644
--- a/athena/extensions/athena_app_window_client_base.cc
+++ b/athena/extensions/athena_app_window_client_base.cc
@@ -7,6 +7,8 @@
 #include "athena/activity/public/activity_factory.h"
 #include "athena/activity/public/activity_manager.h"
 #include "athena/extensions/athena_native_app_window_views.h"
+#include "athena/wm/public/window_list_provider.h"
+#include "athena/wm/public/window_manager.h"
 #include "extensions/common/extension.h"
 
 namespace athena {
@@ -19,11 +21,29 @@ AthenaAppWindowClientBase::~AthenaAppWindowClientBase() {
 
 extensions::NativeAppWindow* AthenaAppWindowClientBase::CreateNativeAppWindow(
     extensions::AppWindow* app_window,
-    const extensions::AppWindow::CreateParams& params) {
+    extensions::AppWindow::CreateParams& params) {
   AthenaNativeAppWindowViews* native_window = new AthenaNativeAppWindowViews;
   native_window->Init(app_window, params);
   ActivityFactory::Get()->CreateAppActivity(app_window->extension_id(),
                                             native_window->GetWebView());
+  if (params.focused) {
+    // This creation will have created the window at the top. If it has been
+    // placed somewhere else it must have been done intentionally and we should
+    // not set the focus since it will change the window order again.

[oshima, 2014/10/09 04:53:32]

what happens in split view mode?

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

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).

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

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?

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

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

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

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.

+    const aura::Window::Windows& window_list =
+        WindowManager::Get()->GetWindowListProvider()->GetWindowList();
+    aura::Window* native_app_window =
+        native_window->widget()->GetNativeWindow();
+    std::vector<aura::Window*>::const_iterator iter =
+        std::find(window_list.begin(), window_list.end(), native_app_window);
+    // The application window might have been created as a panel - if it was, it
+    // will not be in our WindowListProvider list. However - if it was not at

[oshima, 2014/10/09 04:53:32]

PANEL windows are in the list. see WindowListProviderImpl::IsValidWindow

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

Since it got changed ysterday they were explicitly excluded.

+    // the end of the list (top of the stack), the window was intentionally
+    // moved after creation to a different location and we have to respect this
+    // location and suppress focusing - which would bring it back to the front.
+    if (iter != window_list.end() && ++iter != window_list.end())

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

This looks fragile. How about adding explicit RESUMED state?

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

As discussed - added comment.

[oshima, 2014/10/09 04:53:32]

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;

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

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.

+      params.focused = false;
+  }
   return native_window;
 }