Separate athena's startup process from AppShell's

Separate athena's startup process from AppShell's

* Introduced DesktopController interface. app_shell's Init process stays in ShellDesktopController
* Removed ShellAppWindowController
* Athena has its own AthenaDesktopController
* Intorduced AthenaEnv. This is now used for both AthenaMain and Unit test.
* Removed ScreenManagerDelegate and integrate it into AthenaEnv.
* Moved FocusController to ScreenManager, and removed CreateFocusRules()

BUG=397167
TBR=sky@chromium.org

Committed: https://crrev.com/1629c184e328e7263e93042de97af58651ebe510
Cr-Commit-Position: refs/heads/master@{#291763}

[oshima, 2014-08-23 00:53:42]

mukai -> everything
jamescook -> extensions/shell

[Jun Mukai, 2014-08-23 01:11:12]

superficial comments...

https://codereview.chromium.org/480353006/diff/320001/athena/env/athena_env_i...
File athena/env/athena_env_impl.cc (right):

https://codereview.chromium.org/480353006/diff/320001/athena/env/athena_env_i...
athena/env/athena_env_impl.cc:222: scoped_ptr< ::wm::InputMethodEventFilter>
input_method_filter_;
Does it have to be in ::wm namespace explicitly?  It seems CompountEventFilter,
CursorManager, and UserActivityDetector is simply in wm namespace.  It's better
to be in the same style, and I'd prefer to omit the first :: if possible.

https://codereview.chromium.org/480353006/diff/320001/athena/main/athena_main.cc
File athena/main/athena_main.cc (right):

https://codereview.chromium.org/480353006/diff/320001/athena/main/athena_main...
athena/main/athena_main.cc:42: // extensions::ShellDesktopController:
// extensions::DesktopController:

https://codereview.chromium.org/480353006/diff/320001/athena/main/athena_main...
athena/main/athena_main.cc:47: // Creates the window that hosts the app.
I don't think you need to comment overriding methods. Instead comment on the
superclass.

https://codereview.chromium.org/480353006/diff/320001/athena/main/athena_main...
athena/main/athena_main.cc:90: // TODO(oshima|mukai):
What is the content of the TODO?

[Jun Mukai, 2014-08-23 01:15:36]

one more nitpick

https://codereview.chromium.org/480353006/diff/320001/athena/env/athena_env_i...
File athena/env/athena_env_impl.cc (right):

https://codereview.chromium.org/480353006/diff/320001/athena/env/athena_env_i...
athena/env/athena_env_impl.cc:113: screen_size.SetSize(1280, 720);
If it falls back to 1280x720 here, why not returning it from
GetPrimaryDisplaySize()?

[oshima, 2014-08-23 01:19:36]

https://codereview.chromium.org/480353006/diff/320001/athena/env/athena_env_i...
File athena/env/athena_env_impl.cc (right):

https://codereview.chromium.org/480353006/diff/320001/athena/env/athena_env_i...
athena/env/athena_env_impl.cc:113: screen_size.SetSize(1280, 720);
On 2014/08/23 01:15:35, Jun Mukai wrote:
> If it falls back to 1280x720 here, why not returning it from
> GetPrimaryDisplaySize()?

This is used in OnDisplayModeChanged, and I don't want to use
default if it ever failed to get the display mode.

https://codereview.chromium.org/480353006/diff/320001/athena/env/athena_env_i...
athena/env/athena_env_impl.cc:222: scoped_ptr< ::wm::InputMethodEventFilter>
input_method_filter_;
On 2014/08/23 01:11:12, Jun Mukai wrote:
> Does it have to be in ::wm namespace explicitly?  It seems
CompountEventFilter,
> CursorManager, and UserActivityDetector is simply in wm namespace.  It's
better
> to be in the same style, and I'd prefer to omit the first :: if possible.

Done.

https://codereview.chromium.org/480353006/diff/320001/athena/main/athena_main.cc
File athena/main/athena_main.cc (right):

https://codereview.chromium.org/480353006/diff/320001/athena/main/athena_main...
athena/main/athena_main.cc:42: // extensions::ShellDesktopController:
On 2014/08/23 01:11:12, Jun Mukai wrote:
> // extensions::DesktopController:

Done.

https://codereview.chromium.org/480353006/diff/320001/athena/main/athena_main...
athena/main/athena_main.cc:47: // Creates the window that hosts the app.
On 2014/08/23 01:11:12, Jun Mukai wrote:
> I don't think you need to comment overriding methods. Instead comment on the
> superclass.

Done.

[Jun Mukai, 2014-08-23 01:24:21]

athena lgtm

[James Cook, 2014-08-25 16:56:15]

I only looked at extensions/shell/*. I like it. Just a few nits/questions.

https://codereview.chromium.org/480353006/diff/400001/extensions/shell/app_sh...
File extensions/shell/app_shell.gyp (left):

https://codereview.chromium.org/480353006/diff/400001/extensions/shell/app_sh...
extensions/shell/app_shell.gyp:54:
'browser/default_shell_app_window_controller.cc',
Can this file be deleted?

https://codereview.chromium.org/480353006/diff/400001/extensions/shell/browse...
File extensions/shell/browser/api/shell/shell_api.cc (right):

https://codereview.chromium.org/480353006/diff/400001/extensions/shell/browse...
extensions/shell/browser/api/shell/shell_api.cc:11: #include
"extensions/shell/browser/shell_desktop_controller.h"
nit: just desktop_controller.h?

https://codereview.chromium.org/480353006/diff/400001/extensions/shell/browse...
File extensions/shell/browser/desktop_controller.h (right):

https://codereview.chromium.org/480353006/diff/400001/extensions/shell/browse...
extensions/shell/browser/desktop_controller.h:20: class DesktopController {
nit: Please comment on the purpose of this class.

https://codereview.chromium.org/480353006/diff/400001/extensions/shell/browse...
extensions/shell/browser/desktop_controller.h:30: virtual aura::WindowTreeHost*
GetHost() = 0;
nit: function comment

(I know it seems stupid, but these sorts of comments make a big difference for
me when I'm exploring new regions of the code.)

https://codereview.chromium.org/480353006/diff/400001/extensions/shell/browse...
extensions/shell/browser/desktop_controller.h:44: #endif  // 
EXTENSIONS_SHELL_BROWSER_DESKTOP_CONTROLLER_H_
nit: one space after //

https://codereview.chromium.org/480353006/diff/400001/extensions/shell/browse...
File extensions/shell/browser/shell_desktop_controller.cc (right):

https://codereview.chromium.org/480353006/diff/400001/extensions/shell/browse...
extensions/shell/browser/shell_desktop_controller.cc:170: DestroyRootWindow();
I think you should either app_window_.reset() before you destroy the root window
or DCHECK(!app_window_.get()) to ensure CloseAppWindows() was called.

https://codereview.chromium.org/480353006/diff/400001/extensions/shell/browse...
File extensions/shell/browser/shell_desktop_controller.h (right):

https://codereview.chromium.org/480353006/diff/400001/extensions/shell/browse...
extensions/shell/browser/shell_desktop_controller.h:69: // DeskopController:
nit: Desk*t*opController

[oshima, 2014-08-25 17:29:33]

https://codereview.chromium.org/480353006/diff/400001/extensions/shell/app_sh...
File extensions/shell/app_shell.gyp (left):

https://codereview.chromium.org/480353006/diff/400001/extensions/shell/app_sh...
extensions/shell/app_shell.gyp:54:
'browser/default_shell_app_window_controller.cc',
On 2014/08/25 16:56:14, James Cook wrote:
> Can this file be deleted?

Done.

https://codereview.chromium.org/480353006/diff/400001/extensions/shell/browse...
File extensions/shell/browser/api/shell/shell_api.cc (right):

https://codereview.chromium.org/480353006/diff/400001/extensions/shell/browse...
extensions/shell/browser/api/shell/shell_api.cc:11: #include
"extensions/shell/browser/shell_desktop_controller.h"
On 2014/08/25 16:56:14, James Cook wrote:
> nit: just desktop_controller.h?

Done.

https://codereview.chromium.org/480353006/diff/400001/extensions/shell/browse...
File extensions/shell/browser/desktop_controller.h (right):

https://codereview.chromium.org/480353006/diff/400001/extensions/shell/browse...
extensions/shell/browser/desktop_controller.h:20: class DesktopController {
On 2014/08/25 16:56:14, James Cook wrote:
> nit: Please comment on the purpose of this class.

Done.

https://codereview.chromium.org/480353006/diff/400001/extensions/shell/browse...
extensions/shell/browser/desktop_controller.h:30: virtual aura::WindowTreeHost*
GetHost() = 0;
On 2014/08/25 16:56:14, James Cook wrote:
> nit: function comment
> 
> (I know it seems stupid, but these sorts of comments make a big difference for
> me when I'm exploring new regions of the code.)

I'm fine. Different people have different preference :)

https://codereview.chromium.org/480353006/diff/400001/extensions/shell/browse...
extensions/shell/browser/desktop_controller.h:44: #endif  // 
EXTENSIONS_SHELL_BROWSER_DESKTOP_CONTROLLER_H_
On 2014/08/25 16:56:14, James Cook wrote:
> nit: one space after //

Done.

https://codereview.chromium.org/480353006/diff/400001/extensions/shell/browse...
File extensions/shell/browser/shell_desktop_controller.cc (right):

https://codereview.chromium.org/480353006/diff/400001/extensions/shell/browse...
extensions/shell/browser/shell_desktop_controller.cc:170: DestroyRootWindow();
On 2014/08/25 16:56:14, James Cook wrote:
> I think you should either app_window_.reset() before you destroy the root
window
> or DCHECK(!app_window_.get()) to ensure CloseAppWindows() was called.

Done.

https://codereview.chromium.org/480353006/diff/400001/extensions/shell/browse...
File extensions/shell/browser/shell_desktop_controller.h (right):

https://codereview.chromium.org/480353006/diff/400001/extensions/shell/browse...
extensions/shell/browser/shell_desktop_controller.h:69: // DeskopController:
On 2014/08/25 16:56:15, James Cook wrote:
> nit: Desk*t*opController

Done.

[James Cook, 2014-08-25 17:35:45]

LGTM!

https://codereview.chromium.org/480353006/diff/420001/extensions/shell/browse...
File extensions/shell/browser/desktop_controller.h (right):

https://codereview.chromium.org/480353006/diff/420001/extensions/shell/browse...
extensions/shell/browser/desktop_controller.h:21: // extensions shell.
ShellDesktopController provides a default implementation for
nit: line length? codereview is wrapping the line

[oshima, 2014-08-25 19:06:54]

https://codereview.chromium.org/480353006/diff/420001/extensions/shell/browse...
File extensions/shell/browser/desktop_controller.h (right):

https://codereview.chromium.org/480353006/diff/420001/extensions/shell/browse...
extensions/shell/browser/desktop_controller.h:21: // extensions shell.
ShellDesktopController provides a default implementation for
On 2014/08/25 17:35:45, James Cook wrote:
> nit: line length? codereview is wrapping the line

Done.

[oshima, 2014-08-25 19:21:47]

oshima@chromium.org changed reviewers:
+ sky@chromium.org

[oshima, 2014-08-25 19:21:48]

tbr'ing sky for following deps in athena/env/DEPS, which other athena modules
already have.

ui/gfx
ui/wm/
ui/aura
ui/base

[oshima, 2014-08-25 19:21:54]

The CQ bit was checked by oshima@chromium.org

[commit-bot: I haz the power, 2014-08-25 19:22:35]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/oshima@chromium.org/480353006/480001

[commit-bot: I haz the power, 2014-08-25 20:35:10]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[commit-bot: I haz the power, 2014-08-25 21:33:11]

Committed patchset #9 (480001) as 8a02bf3fcefb691dbbfb673e274bb8297f82cf66

[commit-bot: I haz the power, 2014-09-10 02:37:39]

Patchset 9 (id:??) landed as
https://crrev.com/1629c184e328e7263e93042de97af58651ebe510
Cr-Commit-Position: refs/heads/master@{#291763}