Removes the dependency to WMTestHelper from app shell.

Removes the dependency to WMTestHelper from app shell.

Also ShellDesktopController has its own logic to initialize the
window manager, and its subclass (like Athena) can initialize
its own window management logic.

R=jamescook@chromium.org, oshima@chromium.org
TEST=no logic changes, confirmed athena_main running
BUG=

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

[oshima, 2014-05-29 23:06:59]

I assume you're going to define a interface for desktop controller in next CL?

https://codereview.chromium.org/308783002/diff/20001/apps/shell/browser/shell...
File apps/shell/browser/shell_browser_main_parts.cc (right):

https://codereview.chromium.org/308783002/diff/20001/apps/shell/browser/shell...
apps/shell/browser/shell_browser_main_parts.cc:155:
base::MessageLoop::QuitClosure());
Can't you move this code into DesktopController? Then all this class has to do
is to create and delete the desktop controller, i believe.

[Jun Mukai, 2014-05-29 23:26:43]

I believe that many of DesktopController can be shared between app shell and
athena. I don't think we should make DesktopController a pure interface.

Instead, it's better to create another interface for window management stuffs
like apps::ShellWindowManager and DesktopController owns it.

https://codereview.chromium.org/308783002/diff/20001/apps/shell/browser/shell...
File apps/shell/browser/shell_browser_main_parts.cc (right):

https://codereview.chromium.org/308783002/diff/20001/apps/shell/browser/shell...
apps/shell/browser/shell_browser_main_parts.cc:155:
base::MessageLoop::QuitClosure());
On 2014/05/29 23:06:59, oshima wrote:
> Can't you move this code into DesktopController? Then all this class has to do
> is to create and delete the desktop controller, i believe.

I am not sure I'm following.  Do you mean that handing of MessageLoop into
DesktopController?  I don't think that's the responsibility of controlling
desktop. The management of MessageLoop would be the responsibility of MainParts.

[Jun Mukai, 2014-05-30 00:59:03]

https://codereview.chromium.org/308783002/diff/20001/apps/shell/browser/shell...
File apps/shell/browser/shell_browser_main_parts.cc (right):

https://codereview.chromium.org/308783002/diff/20001/apps/shell/browser/shell...
apps/shell/browser/shell_browser_main_parts.cc:155:
base::MessageLoop::QuitClosure());
On 2014/05/29 23:26:44, Jun Mukai wrote:
> On 2014/05/29 23:06:59, oshima wrote:
> > Can't you move this code into DesktopController? Then all this class has to
do
> > is to create and delete the desktop controller, i believe.
> 
> I am not sure I'm following.  Do you mean that handing of MessageLoop into
> DesktopController?  I don't think that's the responsibility of controlling
> desktop. The management of MessageLoop would be the responsibility of
MainParts.

Updated. I think I've done what you're saing, PTAL.

[James Cook, 2014-05-30 15:43:32]

Overall approach seems fine, I'll take a look in detail after oshima.

[oshima, 2014-05-30 16:22:50]

lgtm with nits

https://codereview.chromium.org/308783002/diff/40001/apps/shell/browser/shell...
File apps/shell/browser/shell_desktop_controller.cc (right):

https://codereview.chromium.org/308783002/diff/40001/apps/shell/browser/shell...
apps/shell/browser/shell_desktop_controller.cc:149: 
virtual dtor

https://codereview.chromium.org/308783002/diff/40001/apps/shell/browser/shell...
apps/shell/browser/shell_desktop_controller.cc:290: return;
when are these null checks necessary?

https://codereview.chromium.org/308783002/diff/40001/apps/shell/browser/shell...
File apps/shell/browser/shell_desktop_controller.h (right):

https://codereview.chromium.org/308783002/diff/40001/apps/shell/browser/shell...
apps/shell/browser/shell_desktop_controller.h:13: #include
"ui/gfx/geometry/size.h"
nit: this can be forward decl, i thin.

https://codereview.chromium.org/308783002/diff/40001/apps/shell/browser/shell...
apps/shell/browser/shell_desktop_controller.h:55: public
aura::WindowTreeHostObserver {
nit: move WTHO before ui::DisplayConfigurator::Observer

[James Cook, 2014-05-30 17:30:18]

LGTM with nits

https://codereview.chromium.org/308783002/diff/40001/apps/shell/browser/shell...
File apps/shell/browser/shell_browser_main_delegate.h (right):

https://codereview.chromium.org/308783002/diff/40001/apps/shell/browser/shell...
apps/shell/browser/shell_browser_main_delegate.h:30: // windo wmanager.
nit: "window manager"

https://codereview.chromium.org/308783002/diff/40001/apps/shell/browser/shell...
File apps/shell/browser/shell_desktop_controller.cc (right):

https://codereview.chromium.org/308783002/diff/40001/apps/shell/browser/shell...
apps/shell/browser/shell_desktop_controller.cc:173: }
I like how the constructor does less work now.

https://codereview.chromium.org/308783002/diff/40001/apps/shell/browser/shell...
File apps/shell/browser/shell_desktop_controller.h (right):

https://codereview.chromium.org/308783002/diff/40001/apps/shell/browser/shell...
apps/shell/browser/shell_desktop_controller.h:41: class CompoundEventFilter;
nit: alphabetize

https://codereview.chromium.org/308783002/diff/40001/apps/shell/browser/shell...
apps/shell/browser/shell_desktop_controller.h:89: aura::WindowTreeHost* host() {
optional nit: move up near instance() because it is a simple getter

https://codereview.chromium.org/308783002/diff/40001/apps/shell/browser/shell...
apps/shell/browser/shell_desktop_controller.h:94: // Creates and sets the aura
clients and window manager stuffs. Subclass may
nit: stuffs -> stuff

[Jun Mukai, 2014-05-30 19:57:46]

https://codereview.chromium.org/308783002/diff/40001/apps/shell/browser/shell...
File apps/shell/browser/shell_browser_main_delegate.h (right):

https://codereview.chromium.org/308783002/diff/40001/apps/shell/browser/shell...
apps/shell/browser/shell_browser_main_delegate.h:30: // windo wmanager.
On 2014/05/30 17:30:18, James Cook wrote:
> nit: "window manager"

Done.

https://codereview.chromium.org/308783002/diff/40001/apps/shell/browser/shell...
File apps/shell/browser/shell_desktop_controller.cc (right):

https://codereview.chromium.org/308783002/diff/40001/apps/shell/browser/shell...
apps/shell/browser/shell_desktop_controller.cc:149: 
On 2014/05/30 16:22:51, oshima wrote:
> virtual dtor

Done.

https://codereview.chromium.org/308783002/diff/40001/apps/shell/browser/shell...
apps/shell/browser/shell_desktop_controller.cc:290: return;
On 2014/05/30 16:22:51, oshima wrote:
> when are these null checks necessary?

removed.
|host_| is NULL in ctor and this method is called in dtor, so it can be null
theoretically (not happening actually), that was my intention.

https://codereview.chromium.org/308783002/diff/40001/apps/shell/browser/shell...
File apps/shell/browser/shell_desktop_controller.h (right):

https://codereview.chromium.org/308783002/diff/40001/apps/shell/browser/shell...
apps/shell/browser/shell_desktop_controller.h:13: #include
"ui/gfx/geometry/size.h"
On 2014/05/30 16:22:51, oshima wrote:
> nit: this can be forward decl, i thin.

Done.

https://codereview.chromium.org/308783002/diff/40001/apps/shell/browser/shell...
apps/shell/browser/shell_desktop_controller.h:41: class CompoundEventFilter;
On 2014/05/30 17:30:18, James Cook wrote:
> nit: alphabetize

Done.

https://codereview.chromium.org/308783002/diff/40001/apps/shell/browser/shell...
apps/shell/browser/shell_desktop_controller.h:55: public
aura::WindowTreeHostObserver {
On 2014/05/30 16:22:51, oshima wrote:
> nit: move WTHO before ui::DisplayConfigurator::Observer

Done.

https://codereview.chromium.org/308783002/diff/40001/apps/shell/browser/shell...
apps/shell/browser/shell_desktop_controller.h:89: aura::WindowTreeHost* host() {
On 2014/05/30 17:30:18, James Cook wrote:
> optional nit: move up near instance() because it is a simple getter

Done.

https://codereview.chromium.org/308783002/diff/40001/apps/shell/browser/shell...
apps/shell/browser/shell_desktop_controller.h:94: // Creates and sets the aura
clients and window manager stuffs. Subclass may
On 2014/05/30 17:30:18, James Cook wrote:
> nit: stuffs -> stuff

Done.

[Jun Mukai, 2014-05-30 19:57:54]

The CQ bit was checked by mukai@chromium.org

[commit-bot: I haz the power, 2014-05-30 20:02:07]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mukai@chromium.org/308783002/60001

[tfarina, 2014-05-30 20:05:51]

https://codereview.chromium.org/308783002/diff/60001/apps/shell/browser/shell...
File apps/shell/browser/shell_desktop_controller.h (right):

https://codereview.chromium.org/308783002/diff/60001/apps/shell/browser/shell...
apps/shell/browser/shell_desktop_controller.h:72: };
ouch. no, no need of ; here. No presubmit lint cautch this? :/

also I bet you can do it in one line.

[tfarina, 2014-05-30 20:08:20]

https://codereview.chromium.org/308783002/diff/60001/apps/shell/browser/shell...
File apps/shell/browser/shell_desktop_controller.cc (left):

https://codereview.chromium.org/308783002/diff/60001/apps/shell/browser/shell...
apps/shell/browser/shell_desktop_controller.cc:25: #include
"ui/wm/test/wm_test_helper.h"
if you are removing this, then can you also remove the dependency on
wm_test_support target?

[tfarina, 2014-05-30 20:08:27]

The CQ bit was unchecked by tfarina@chromium.org

[Jun Mukai, 2014-05-30 20:13:32]

nice catches!

https://codereview.chromium.org/308783002/diff/60001/apps/shell/browser/shell...
File apps/shell/browser/shell_desktop_controller.cc (left):

https://codereview.chromium.org/308783002/diff/60001/apps/shell/browser/shell...
apps/shell/browser/shell_desktop_controller.cc:25: #include
"ui/wm/test/wm_test_helper.h"
On 2014/05/30 20:08:20, tfarina wrote:
> if you are removing this, then can you also remove the dependency on
> wm_test_support target?

Done.

https://codereview.chromium.org/308783002/diff/60001/apps/shell/browser/shell...
File apps/shell/browser/shell_desktop_controller.h (right):

https://codereview.chromium.org/308783002/diff/60001/apps/shell/browser/shell...
apps/shell/browser/shell_desktop_controller.h:72: };
On 2014/05/30 20:05:51, tfarina wrote:
> ouch. no, no need of ; here. No presubmit lint cautch this? :/
> 
> also I bet you can do it in one line.

Done.

[Jun Mukai, 2014-05-30 20:13:36]

The CQ bit was checked by mukai@chromium.org

[commit-bot: I haz the power, 2014-05-30 20:16:58]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mukai@chromium.org/308783002/80001

[commit-bot: I haz the power, 2014-05-31 01:24:53]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[commit-bot: I haz the power, 2014-05-31 02:25:38]

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

[commit-bot: I haz the power, 2014-05-31 02:25:39]

Try jobs failed on following builders:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[Jun Mukai, 2014-06-02 01:20:35]

The CQ bit was checked by mukai@chromium.org

[commit-bot: I haz the power, 2014-06-02 01:20:43]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mukai@chromium.org/308783002/80001

[commit-bot: I haz the power, 2014-06-02 05:33:48]

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

[commit-bot: I haz the power, 2014-06-02 05:33:49]

Failed to apply patch for athena/main/athena_main.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file athena/main/athena_main.cc
  Hunk #1 FAILED at 18.
  1 out of 1 hunk FAILED -- saving rejects to file
athena/main/athena_main.cc.rej

Patch:       athena/main/athena_main.cc
Index: athena/main/athena_main.cc
diff --git a/athena/main/athena_main.cc b/athena/main/athena_main.cc
index
6ee8ee4a8c748ebbd75089187239ee6da3f413a1..000a3432b1a6e16b1539f98d60aa43291a83f426
100644
--- a/athena/main/athena_main.cc
+++ b/athena/main/athena_main.cc
@@ -18,14 +18,19 @@ class AthenaBrowserMainDelegate : public
apps::ShellBrowserMainDelegate {
 
   // apps::ShellBrowserMainDelegate:
   virtual void Start(content::BrowserContext* context) OVERRIDE {
-    athena::StartAthena(apps::ShellDesktopController::instance()
-                            ->GetWindowTreeHost()
-                            ->window());
+    athena::StartAthena(
+        apps::ShellDesktopController::instance()->host()->window());
     CreateTestWindows();
   }
 
   virtual void Shutdown() OVERRIDE { athena::ShutdownAthena(); }
 
+  virtual apps::ShellDesktopController* CreateDesktopController() OVERRIDE {
+    // TODO(mukai): create Athena's own ShellDesktopController subclass so that
+    // it can initialize its own window manager logic.
+    return new apps::ShellDesktopController();
+  }
+
  private:
   DISALLOW_COPY_AND_ASSIGN(AthenaBrowserMainDelegate);
 };

[Jun Mukai, 2014-06-02 16:51:19]

The CQ bit was checked by mukai@chromium.org

[commit-bot: I haz the power, 2014-06-02 16:52:22]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mukai@chromium.org/308783002/80001

[commit-bot: I haz the power, 2014-06-02 16:53:34]

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

[commit-bot: I haz the power, 2014-06-02 16:53:35]

Failed to apply patch for athena/main/athena_main.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file athena/main/athena_main.cc
  Hunk #1 FAILED at 18.
  1 out of 1 hunk FAILED -- saving rejects to file
athena/main/athena_main.cc.rej

Patch:       athena/main/athena_main.cc
Index: athena/main/athena_main.cc
diff --git a/athena/main/athena_main.cc b/athena/main/athena_main.cc
index
6ee8ee4a8c748ebbd75089187239ee6da3f413a1..000a3432b1a6e16b1539f98d60aa43291a83f426
100644
--- a/athena/main/athena_main.cc
+++ b/athena/main/athena_main.cc
@@ -18,14 +18,19 @@ class AthenaBrowserMainDelegate : public
apps::ShellBrowserMainDelegate {
 
   // apps::ShellBrowserMainDelegate:
   virtual void Start(content::BrowserContext* context) OVERRIDE {
-    athena::StartAthena(apps::ShellDesktopController::instance()
-                            ->GetWindowTreeHost()
-                            ->window());
+    athena::StartAthena(
+        apps::ShellDesktopController::instance()->host()->window());
     CreateTestWindows();
   }
 
   virtual void Shutdown() OVERRIDE { athena::ShutdownAthena(); }
 
+  virtual apps::ShellDesktopController* CreateDesktopController() OVERRIDE {
+    // TODO(mukai): create Athena's own ShellDesktopController subclass so that
+    // it can initialize its own window manager logic.
+    return new apps::ShellDesktopController();
+  }
+
  private:
   DISALLOW_COPY_AND_ASSIGN(AthenaBrowserMainDelegate);
 };

[Jun Mukai, 2014-06-02 16:55:34]

The CQ bit was checked by mukai@chromium.org

[Jun Mukai, 2014-06-02 16:55:39]

The CQ bit was unchecked by mukai@chromium.org

[Jun Mukai, 2014-06-02 16:56:06]

Committed patchset #6 manually as r274263 (presubmit successful).