Converts mash to use Shell

Converts mash to use Shell

Shell has a bunch of stuff that is cash specific and or needs to be
made to work in mash. In hopes of not making a ginormous patch I'll do
cleanup later on.

BUG=671246
TEST=none
R=jamescook@chromium.org

Review-Url: https://codereview.chromium.org/2633293005
Cr-Commit-Position: refs/heads/master@{#444401}
Committed: https://chromium.googlesource.com/chromium/src/+/abcae81155dabe9d4c2a8cb7e39fc3c93157b703

[sky, 2017-01-18 00:14:48]

The CQ bit was checked by sky@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-18 00:15:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[James Cook, 2017-01-18 01:00:25]

It's nice to see things moving back toward ash::Shell.

https://codereview.chromium.org/2633293005/diff/20001/ash/mus/bridge/wm_shell...
File ash/mus/bridge/wm_shell_mus.cc (right):

https://codereview.chromium.org/2633293005/diff/20001/ash/mus/bridge/wm_shell...
ash/mus/bridge/wm_shell_mus.cc:118: primary_window_(primary_window),
nit: DCHECK primary_window_ or document the parameter can't be null

https://codereview.chromium.org/2633293005/diff/20001/ash/mus/bridge/wm_shell...
File ash/mus/bridge/wm_shell_mus.h (right):

https://codereview.chromium.org/2633293005/diff/20001/ash/mus/bridge/wm_shell...
ash/mus/bridge/wm_shell_mus.h:120: WmWindow* primary_window_;
nit: maybe primary_root_window_ ?

https://codereview.chromium.org/2633293005/diff/20001/ash/mus/keyboard_ui_mus.cc
File ash/mus/keyboard_ui_mus.cc (right):

https://codereview.chromium.org/2633293005/diff/20001/ash/mus/keyboard_ui_mus...
ash/mus/keyboard_ui_mus.cc:32: if (keyboard_)
Are these checks for tests, or is this something that happens in production?
Might be worth a comment, either here or in header.

https://codereview.chromium.org/2633293005/diff/20001/ash/mus/root_window_con...
File ash/mus/root_window_controller.cc (right):

https://codereview.chromium.org/2633293005/diff/20001/ash/mus/root_window_con...
ash/mus/root_window_controller.cc:79:
ash::InitRootWindowSettings(window_tree_host->window())->display_id =
nit: This would be clearer if the result of InitRootWindowSettings() went into a
local variable.

https://codereview.chromium.org/2633293005/diff/20001/ash/mus/shell_delegate_...
File ash/mus/shell_delegate_mus.h (right):

https://codereview.chromium.org/2633293005/diff/20001/ash/mus/shell_delegate_...
ash/mus/shell_delegate_mus.h:56: std::unique_ptr<SystemTrayDelegate>
system_tray_delegate_;
Is this only for testing? system_tray_delegate_for_testing_ ?

https://codereview.chromium.org/2633293005/diff/20001/ash/mus/window_manager.cc
File ash/mus/window_manager.cc (right):

https://codereview.chromium.org/2633293005/diff/20001/ash/mus/window_manager....
ash/mus/window_manager.cc:124: if (root_window_controller_ptr.get() !=
primary_root_window_controller)
nit: {}

https://codereview.chromium.org/2633293005/diff/20001/ash/mus/window_manager....
ash/mus/window_manager.cc:188: void
WindowManager::CreatePrimaryRootWindowController(
per the comment below it sounds like CreateShell() must be called before this
method. If that's true, DCHECK(created_shell_).

https://codereview.chromium.org/2633293005/diff/20001/ash/mus/window_manager....
ash/mus/window_manager.cc:207: init_params.primary_window_tree_host =
window_tree_host.release();
Could init_params.primary_window_tree_host become a unique_ptr<> so you don't
need to use release() here?

https://codereview.chromium.org/2633293005/diff/20001/ash/mus/window_manager.h
File ash/mus/window_manager.h (right):

https://codereview.chromium.org/2633293005/diff/20001/ash/mus/window_manager....
ash/mus/window_manager.h:211: // create the various test delegates rather than a
mem
mem?

https://codereview.chromium.org/2633293005/diff/20001/ash/mus/window_manager....
ash/mus/window_manager.h:212: std::unique_ptr<SystemTrayDelegate>
system_tray_delegate_;
system_tray_delegate_for_testing_? Then you could delete part of the comment.

https://codereview.chromium.org/2633293005/diff/20001/ash/root_window_control...
File ash/root_window_controller.h (right):

https://codereview.chromium.org/2633293005/diff/20001/ash/root_window_control...
ash/root_window_controller.h:106: : std::vector<RootWindowController*>();
nit: #include <vector>

https://codereview.chromium.org/2633293005/diff/20001/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/2633293005/diff/20001/ash/shell.cc#newcode185
ash/shell.cc:185: WmShell* wm_shell = init_params.wm_shell;
This would be clearer if init_params.wm_shell was required to be non-null.

https://codereview.chromium.org/2633293005/diff/20001/ash/shell.cc#newcode436
ash/shell.cc:436: RemovePreTargetHandler(overlay_filter_.get());
I'm guessing none of these pre-target handlers work in mash. Is cleaning this up
(like not adding them) part of the next-steps you mentioned in the CL
description?

https://codereview.chromium.org/2633293005/diff/20001/ash/shell_init_params.h
File ash/shell_init_params.h (right):

https://codereview.chromium.org/2633293005/diff/20001/ash/shell_init_params.h...
ash/shell_init_params.h:31: aura::WindowTreeHostMus* primary_window_tree_host =
nullptr;
Maybe mention in the comment that this is not used on classic ash.

[commit-bot: I haz the power, 2017-01-18 01:28:22]

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

[commit-bot: I haz the power, 2017-01-18 01:28:26]

Dry run: This issue passed the CQ dry run.

[sky, 2017-01-18 04:00:08]

https://codereview.chromium.org/2633293005/diff/20001/ash/mus/bridge/wm_shell...
File ash/mus/bridge/wm_shell_mus.cc (right):

https://codereview.chromium.org/2633293005/diff/20001/ash/mus/bridge/wm_shell...
ash/mus/bridge/wm_shell_mus.cc:118: primary_window_(primary_window),
On 2017/01/18 01:00:24, James Cook wrote:
> nit: DCHECK primary_window_ or document the parameter can't be null

Done.

https://codereview.chromium.org/2633293005/diff/20001/ash/mus/bridge/wm_shell...
File ash/mus/bridge/wm_shell_mus.h (right):

https://codereview.chromium.org/2633293005/diff/20001/ash/mus/bridge/wm_shell...
ash/mus/bridge/wm_shell_mus.h:120: WmWindow* primary_window_;
On 2017/01/18 01:00:24, James Cook wrote:
> nit: maybe primary_root_window_ ?

Done.

https://codereview.chromium.org/2633293005/diff/20001/ash/mus/keyboard_ui_mus.cc
File ash/mus/keyboard_ui_mus.cc (right):

https://codereview.chromium.org/2633293005/diff/20001/ash/mus/keyboard_ui_mus...
ash/mus/keyboard_ui_mus.cc:32: if (keyboard_)
On 2017/01/18 01:00:24, James Cook wrote:
> Are these checks for tests, or is this something that happens in production?
> Might be worth a comment, either here or in header.

Only in tests. The constructor may be passed null. Added comment to header.

https://codereview.chromium.org/2633293005/diff/20001/ash/mus/root_window_con...
File ash/mus/root_window_controller.cc (right):

https://codereview.chromium.org/2633293005/diff/20001/ash/mus/root_window_con...
ash/mus/root_window_controller.cc:79:
ash::InitRootWindowSettings(window_tree_host->window())->display_id =
On 2017/01/18 01:00:24, James Cook wrote:
> nit: This would be clearer if the result of InitRootWindowSettings() went into
a
> local variable.

Done.

https://codereview.chromium.org/2633293005/diff/20001/ash/mus/shell_delegate_...
File ash/mus/shell_delegate_mus.h (right):

https://codereview.chromium.org/2633293005/diff/20001/ash/mus/shell_delegate_...
ash/mus/shell_delegate_mus.h:56: std::unique_ptr<SystemTrayDelegate>
system_tray_delegate_;
On 2017/01/18 01:00:24, James Cook wrote:
> Is this only for testing? system_tray_delegate_for_testing_ ?

Yes. Renamed.

https://codereview.chromium.org/2633293005/diff/20001/ash/mus/window_manager.cc
File ash/mus/window_manager.cc (right):

https://codereview.chromium.org/2633293005/diff/20001/ash/mus/window_manager....
ash/mus/window_manager.cc:124: if (root_window_controller_ptr.get() !=
primary_root_window_controller)
On 2017/01/18 01:00:24, James Cook wrote:
> nit: {}

Done.

https://codereview.chromium.org/2633293005/diff/20001/ash/mus/window_manager....
ash/mus/window_manager.cc:188: void
WindowManager::CreatePrimaryRootWindowController(
On 2017/01/18 01:00:24, James Cook wrote:
> per the comment below it sounds like CreateShell() must be called before this
> method. If that's true, DCHECK(created_shell_).

Done.

https://codereview.chromium.org/2633293005/diff/20001/ash/mus/window_manager....
ash/mus/window_manager.cc:207: init_params.primary_window_tree_host =
window_tree_host.release();
On 2017/01/18 01:00:24, James Cook wrote:
> Could init_params.primary_window_tree_host become a unique_ptr<> so you don't
> need to use release() here?

Yes, but it would require other changes. In particular it's common now to pass
in const ShellInitParams&. I don't believe I could take ownership of a
unique_ptr in such a function. So, some call sites would have to take a
std::unique_ptr<ShellInitParams>. Not the end of the world, but I'm hoping most
of the raw pointers in ShellInitParams goes away.

https://codereview.chromium.org/2633293005/diff/20001/ash/mus/window_manager.h
File ash/mus/window_manager.h (right):

https://codereview.chromium.org/2633293005/diff/20001/ash/mus/window_manager....
ash/mus/window_manager.h:211: // create the various test delegates rather than a
mem
On 2017/01/18 01:00:24, James Cook wrote:
> mem?

D'OH! 'member'. Updated.

https://codereview.chromium.org/2633293005/diff/20001/ash/mus/window_manager....
ash/mus/window_manager.h:212: std::unique_ptr<SystemTrayDelegate>
system_tray_delegate_;
On 2017/01/18 01:00:24, James Cook wrote:
> system_tray_delegate_for_testing_? Then you could delete part of the comment.

I renamed, but left the comment.

https://codereview.chromium.org/2633293005/diff/20001/ash/root_window_control...
File ash/root_window_controller.h (right):

https://codereview.chromium.org/2633293005/diff/20001/ash/root_window_control...
ash/root_window_controller.h:106: : std::vector<RootWindowController*>();
On 2017/01/18 01:00:24, James Cook wrote:
> nit: #include <vector>

Done.

https://codereview.chromium.org/2633293005/diff/20001/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/2633293005/diff/20001/ash/shell.cc#newcode185
ash/shell.cc:185: WmShell* wm_shell = init_params.wm_shell;
On 2017/01/18 01:00:25, James Cook wrote:
> This would be clearer if init_params.wm_shell was required to be non-null.

I choose this as it requires less changes to call sites. Also, wm_shell as an
init param should really go away.

https://codereview.chromium.org/2633293005/diff/20001/ash/shell.cc#newcode436
ash/shell.cc:436: RemovePreTargetHandler(overlay_filter_.get());
On 2017/01/18 01:00:25, James Cook wrote:
> I'm guessing none of these pre-target handlers work in mash. Is cleaning this
up
> (like not adding them) part of the next-steps you mentioned in the CL
> description?

Yes. I want to make Shell not implement EventHandler, which means none of these
calls would work.

https://codereview.chromium.org/2633293005/diff/20001/ash/shell_init_params.h
File ash/shell_init_params.h (right):

https://codereview.chromium.org/2633293005/diff/20001/ash/shell_init_params.h...
ash/shell_init_params.h:31: aura::WindowTreeHostMus* primary_window_tree_host =
nullptr;
On 2017/01/18 01:00:25, James Cook wrote:
> Maybe mention in the comment that this is not used on classic ash.

Done.

[sky, 2017-01-18 04:00:10]

The CQ bit was checked by sky@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-18 04:00:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-18 04:59:32]

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

[commit-bot: I haz the power, 2017-01-18 04:59:33]

Dry run: This issue passed the CQ dry run.

[James Cook, 2017-01-18 16:20:47]

LGTM

[sky, 2017-01-18 16:55:51]

The CQ bit was checked by sky@chromium.org

[commit-bot: I haz the power, 2017-01-18 16:56:10]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-18 17:01:37]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1484758551956270,
"parent_rev": "dce92226064b936c993447a2eb30a0333b1a7848", "commit_rev":
"abcae81155dabe9d4c2a8cb7e39fc3c93157b703"}

[commit-bot: I haz the power, 2017-01-18 17:03:59]

Description was changed from

==========
Converts mash to use Shell

Shell has a bunch of stuff that is cash specific and or needs to be
made to work in mash. In hopes of not making a ginormous patch I'll do
cleanup later on.

BUG=671246
TEST=none
R=jamescook@chromium.org
==========

to

==========
Converts mash to use Shell

Shell has a bunch of stuff that is cash specific and or needs to be
made to work in mash. In hopes of not making a ginormous patch I'll do
cleanup later on.

BUG=671246
TEST=none
R=jamescook@chromium.org

Review-Url: https://codereview.chromium.org/2633293005
Cr-Commit-Position: refs/heads/master@{#444401}
Committed:
https://chromium.googlesource.com/chromium/src/+/abcae81155dabe9d4c2a8cb7e39f...
==========

[commit-bot: I haz the power, 2017-01-18 17:04:01]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/abcae81155dabe9d4c2a8cb7e39f...