Centralizes more shared code between ash and mash

Centralizes more shared code between ash and mash

This also makes shutdown of mash better match that of ash.

BUG=none
TEST=covered by tests
R=jamescook@chromium.org

Committed: https://crrev.com/8d971a0a97f570b8a4d89e1e819fe08f062a9f49
Cr-Commit-Position: refs/heads/master@{#420199}

[James Cook, 2016-09-21 22:03:19]

Nice.

https://codereview.chromium.org/2350953009/diff/80001/ash/common/wm_root_wind...
File ash/common/wm_root_window_controller.cc (right):

https://codereview.chromium.org/2350953009/diff/80001/ash/common/wm_root_wind...
ash/common/wm_root_window_controller.cc:490: // NOTE: this may be called
multiple times.
Aside: It's such a bummer this is true.

https://codereview.chromium.org/2350953009/diff/80001/ash/common/wm_root_wind...
ash/common/wm_root_window_controller.cc:511: // destruction such windows may
query the RootWindow for state.
nit: because such windows...

https://codereview.chromium.org/2350953009/diff/80001/ash/common/wm_root_wind...
File ash/common/wm_root_window_controller.h (right):

https://codereview.chromium.org/2350953009/diff/80001/ash/common/wm_root_wind...
ash/common/wm_root_window_controller.h:91: AlwaysOnTopController*
always_on_top_controller() {
nit: move this up with other inline getters

https://codereview.chromium.org/2350953009/diff/80001/ash/common/wm_root_wind...
ash/common/wm_root_window_controller.h:159: // LayoutManagers are owned by the
window they are installed on.
+1 for helpful ownership comment

https://codereview.chromium.org/2350953009/diff/80001/ash/mus/bridge/wm_root_...
File ash/mus/bridge/wm_root_window_controller_mus.cc (right):

https://codereview.chromium.org/2350953009/diff/80001/ash/mus/bridge/wm_root_...
ash/mus/bridge/wm_root_window_controller_mus.cc:134:
ui_window->window_tree()->GetRoots().count(ui_window);
Question for my knowledge: So we don't have a concept of not-owned-by-parent for
ui::Window?  (And that's what makes unit tests hard?)

https://codereview.chromium.org/2350953009/diff/80001/ash/mus/bridge/wm_shelf...
File ash/mus/bridge/wm_shelf_mus.cc (right):

https://codereview.chromium.org/2350953009/diff/80001/ash/mus/bridge/wm_shelf...
ash/mus/bridge/wm_shelf_mus.cc:24: WmShelfMus::~WmShelfMus() {}
Hooray!

https://codereview.chromium.org/2350953009/diff/80001/ash/mus/bridge/wm_windo...
File ash/mus/bridge/wm_window_mus.cc (right):

https://codereview.chromium.org/2350953009/diff/80001/ash/mus/bridge/wm_windo...
ash/mus/bridge/wm_window_mus.cc:164: // TODO(sky): to match aura behavior this
should delete children.
Are we going to leak windows at shutdown until this is fixed? If so this
probably should have a tracking bug.

https://codereview.chromium.org/2350953009/diff/80001/ash/mus/test/wm_test_he...
File ash/mus/test/wm_test_helper.cc (right):

https://codereview.chromium.org/2350953009/diff/80001/ash/mus/test/wm_test_he...
ash/mus/test/wm_test_helper.cc:162: bounds.set_y(*next_x);
set_y ?

https://codereview.chromium.org/2350953009/diff/80001/ash/mus/test/wm_test_he...
File ash/mus/test/wm_test_helper.h (right):

https://codereview.chromium.org/2350953009/diff/80001/ash/mus/test/wm_test_he...
ash/mus/test/wm_test_helper.h:54: // configuration of the display.
nit: Document |next_x| here and below.

Or consider making it a member.

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

https://codereview.chromium.org/2350953009/diff/80001/ash/mus/window_manager....
ash/mus/window_manager.cc:175:
WmWindowMus::Get(GetPrimaryRootWindowController()->root()));
DCHECK that the root_window_controller being destroyed is not the primary?

(I'm not sure when the primary window switch happens. However, it is definitely
possible to disconnect the primary display.)

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

https://codereview.chromium.org/2350953009/diff/80001/ash/root_window_control...
ash/root_window_controller.h:224: DockedWindowLayoutManager*
docked_window_layout_manager();
GetDockedWindowLayoutManager() and GetPanelLayoutManager() please

[sky, 2016-09-21 22:47:49]

https://codereview.chromium.org/2350953009/diff/80001/ash/common/wm_root_wind...
File ash/common/wm_root_window_controller.cc (right):

https://codereview.chromium.org/2350953009/diff/80001/ash/common/wm_root_wind...
ash/common/wm_root_window_controller.cc:511: // destruction such windows may
query the RootWindow for state.
On 2016/09/21 22:03:18, James Cook wrote:
> nit: because such windows...

Done.

https://codereview.chromium.org/2350953009/diff/80001/ash/common/wm_root_wind...
File ash/common/wm_root_window_controller.h (right):

https://codereview.chromium.org/2350953009/diff/80001/ash/common/wm_root_wind...
ash/common/wm_root_window_controller.h:91: AlwaysOnTopController*
always_on_top_controller() {
On 2016/09/21 22:03:18, James Cook wrote:
> nit: move this up with other inline getters

Done.

https://codereview.chromium.org/2350953009/diff/80001/ash/mus/bridge/wm_root_...
File ash/mus/bridge/wm_root_window_controller_mus.cc (right):

https://codereview.chromium.org/2350953009/diff/80001/ash/mus/bridge/wm_root_...
ash/mus/bridge/wm_root_window_controller_mus.cc:134:
ui_window->window_tree()->GetRoots().count(ui_window);
On 2016/09/21 22:03:18, James Cook wrote:
> Question for my knowledge: So we don't have a concept of not-owned-by-parent
for
> ui::Window?  (And that's what makes unit tests hard?)

Currently ui::Window doesn't own it's children, so not-owned-by-parent doesn't
apply to ui::Window. I filed 647513 for this. I see two possibilities:
. We change ownership of ui::Window to match aura::Window.
. We make WmWindowMus own it's children (to mirror aura).

But I may be misunderstanding your question. If you're asking why we can't make
tests have code like:

std::unique_ptr<WmWindow> window;

It's because WmWindow is owned by the corresponding ui::Window or aura::Window.
We could introduce a different ownership model e.g. something like
WINDOWS_OWNS_NATIVE_WINDOW and NATIVE_WINDOW_OWNS_WINDOW, but it would only be
for tests and result in a much different shutdown flow than is normally seen.

Does that answer your question?

https://codereview.chromium.org/2350953009/diff/80001/ash/mus/bridge/wm_windo...
File ash/mus/bridge/wm_window_mus.cc (right):

https://codereview.chromium.org/2350953009/diff/80001/ash/mus/bridge/wm_windo...
ash/mus/bridge/wm_window_mus.cc:164: // TODO(sky): to match aura behavior this
should delete children.
On 2016/09/21 22:03:19, James Cook wrote:
> Are we going to leak windows at shutdown until this is fixed? If so this
> probably should have a tracking bug.

Indeed. Added pointer to bug.

https://codereview.chromium.org/2350953009/diff/80001/ash/mus/test/wm_test_he...
File ash/mus/test/wm_test_helper.cc (right):

https://codereview.chromium.org/2350953009/diff/80001/ash/mus/test/wm_test_he...
ash/mus/test/wm_test_helper.cc:162: bounds.set_y(*next_x);
On 2016/09/21 22:03:19, James Cook wrote:
> set_y ?

Done.

https://codereview.chromium.org/2350953009/diff/80001/ash/mus/test/wm_test_he...
File ash/mus/test/wm_test_helper.h (right):

https://codereview.chromium.org/2350953009/diff/80001/ash/mus/test/wm_test_he...
ash/mus/test/wm_test_helper.h:54: // configuration of the display.
On 2016/09/21 22:03:19, James Cook wrote:
> nit: Document |next_x| here and below.
> 
> Or consider making it a member.

Done.

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

https://codereview.chromium.org/2350953009/diff/80001/ash/mus/window_manager....
ash/mus/window_manager.cc:175:
WmWindowMus::Get(GetPrimaryRootWindowController()->root()));
On 2016/09/21 22:03:19, James Cook wrote:
> DCHECK that the root_window_controller being destroyed is not the primary?
> 
> (I'm not sure when the primary window switch happens. However, it is
definitely
> possible to disconnect the primary display.)

Done.

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

https://codereview.chromium.org/2350953009/diff/80001/ash/root_window_control...
ash/root_window_controller.h:224: DockedWindowLayoutManager*
docked_window_layout_manager();
On 2016/09/21 22:03:19, James Cook wrote:
> GetDockedWindowLayoutManager() and GetPanelLayoutManager() please

Can you motivate that? The style guide says "Functions that are very cheap to
call may instead follow the style for variable names (all lower-case, with
underscores between words)." Ideally I would inline the implementation, but that
would mean having to include wm_root_window_controller.h here, which we try to
avoid in chromium style.

[sky, 2016-09-21 22:48:03]

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

[commit-bot: I haz the power, 2016-09-21 22:48:21]

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

[James Cook, 2016-09-21 22:56:11]

LGTM

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

https://codereview.chromium.org/2350953009/diff/80001/ash/root_window_control...
ash/root_window_controller.h:224: DockedWindowLayoutManager*
docked_window_layout_manager();
On 2016/09/21 22:47:49, sky wrote:
> On 2016/09/21 22:03:19, James Cook wrote:
> > GetDockedWindowLayoutManager() and GetPanelLayoutManager() please
> 
> Can you motivate that? The style guide says "Functions that are very cheap to
> call may instead follow the style for variable names (all lower-case, with
> underscores between words)." Ideally I would inline the implementation, but
that
> would mean having to include wm_root_window_controller.h here, which we try to
> avoid in chromium style.

I associate them with inline functions, but I agree that not including
wm_root_window_controller.h is better and the style guide does not mandate that
hacker_style() means inline, so keep them as-is.

[sky, 2016-09-21 23:00:31]

The CQ bit was unchecked by sky@chromium.org

[sky, 2016-09-21 23:00:35]

The CQ bit was checked by sky@chromium.org

[commit-bot: I haz the power, 2016-09-21 23:01: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, 2016-09-21 23:38:21]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-09-22 00:31:15]

Description was changed from

==========
Centralizes more shared code between ash and mash

This also makes shutdown of mash better match that of ash.

BUG=none
TEST=covered by tests
R=jamescook@chromium.org
==========

to

==========
Centralizes more shared code between ash and mash

This also makes shutdown of mash better match that of ash.

BUG=none
TEST=covered by tests
R=jamescook@chromium.org

Committed: https://crrev.com/8d971a0a97f570b8a4d89e1e819fe08f062a9f49
Cr-Commit-Position: refs/heads/master@{#420199}
==========

[commit-bot: I haz the power, 2016-09-22 00:31:16]

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