Makes ash/mus use RootWindowControllerCommon

Makes ash/mus use RootWindowControllerCommon

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

Committed: https://crrev.com/8208371635b02313480d59191e7d6c4a35f69e64
Cr-Commit-Position: refs/heads/master@{#398066}

[James Cook, 2016-06-03 00:24:58]

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

https://codereview.chromium.org/2033843003/diff/20001/ash/mus/container_ids.c...
ash/mus/container_ids.cc:31: kShellWindowId_OverlayContainer,
Where does this list come from? Does the order matter?  (I'm guessing it's the
items in shell_window_ids.h, in order, but I'm not sure.)

Is it related to the list in
https://cs-staging.chromium.org/chromium/src/ash/wm/ash_focus_rules.cc?q=ash_...
?

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

https://codereview.chromium.org/2033843003/diff/20001/ash/mus/root_window_con...
ash/mus/root_window_controller.cc:251: const bool is_test =
base::CommandLine::ForCurrentProcess()->HasSwitch(
views_mus_unittests are OK after removing this? I'm not sure what classic ash
does with the non_lock_screen_related_containers during tests.

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

https://codereview.chromium.org/2033843003/diff/20001/ash/mus/root_window_con...
ash/mus/root_window_controller.cc:120:
WmWindowMus::Get(window)->GetShellWindowId() != kShellWindowId_Invalid;
It's hard to see where kShellWindowId_Invalid is returned. Can the constant be
used in WmWindowAura/Mus instead of literal -1?

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

https://codereview.chromium.org/2033843003/diff/20001/ash/mus/window_manager....
ash/mus/window_manager.cc:61: for (auto child :
container->mus_window()->children()) {
nit: instead of "auto", the actual type (mus::Window*?)

https://codereview.chromium.org/2033843003/diff/20001/ash/mus/window_manager....
ash/mus/window_manager.cc:215:
non_lock_screen_containers_container->mus_window()->SetVisible(!locked);
I think this is OK, but I'm not 100% sure. In classic ash the windows are
occluded by moving an opaque background container around. I think this is
equivalent/better, but we should watch for problems.

[sky, 2016-06-03 03:07:03]

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

https://codereview.chromium.org/2033843003/diff/20001/ash/mus/container_ids.c...
ash/mus/container_ids.cc:31: kShellWindowId_OverlayContainer,
On 2016/06/03 00:24:58, James Cook wrote:
> Where does this list come from? Does the order matter?  (I'm guessing it's the
> items in shell_window_ids.h, in order, but I'm not sure.)
> 
> Is it related to the list in
>
https://cs-staging.chromium.org/chromium/src/ash/wm/ash_focus_rules.cc?q=ash_...
> ?

Good call. They should be the same. updated.

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

https://codereview.chromium.org/2033843003/diff/20001/ash/mus/root_window_con...
ash/mus/root_window_controller.cc:251: const bool is_test =
base::CommandLine::ForCurrentProcess()->HasSwitch(
On 2016/06/03 00:24:58, James Cook wrote:
> views_mus_unittests are OK after removing this? I'm not sure what classic ash
> does with the non_lock_screen_related_containers during tests.

They passed locally. I'll find out shortly if the bots like it.

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

https://codereview.chromium.org/2033843003/diff/20001/ash/mus/root_window_con...
ash/mus/root_window_controller.cc:120:
WmWindowMus::Get(window)->GetShellWindowId() != kShellWindowId_Invalid;
On 2016/06/03 00:24:58, James Cook wrote:
> It's hard to see where kShellWindowId_Invalid is returned. Can the constant be
> used in WmWindowAura/Mus instead of literal -1?

It's actually set in aura::Window and mus::Window.

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

https://codereview.chromium.org/2033843003/diff/20001/ash/mus/window_manager....
ash/mus/window_manager.cc:61: for (auto child :
container->mus_window()->children()) {
On 2016/06/03 00:24:58, James Cook wrote:
> nit: instead of "auto", the actual type (mus::Window*?)

Done.

https://codereview.chromium.org/2033843003/diff/20001/ash/mus/window_manager....
ash/mus/window_manager.cc:215:
non_lock_screen_containers_container->mus_window()->SetVisible(!locked);
On 2016/06/03 00:24:58, James Cook wrote:
> I think this is OK, but I'm not 100% sure. In classic ash the windows are
> occluded by moving an opaque background container around. I think this is
> equivalent/better, but we should watch for problems.

Agreed. It's hard to test this given we don't have the rest of the system
working. I'm hoping as we get more working we'll be able to tell what doesn't
work.

[James Cook, 2016-06-03 16:13:16]

LGTM with nits

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

https://codereview.chromium.org/2033843003/diff/20001/ash/mus/root_window_con...
ash/mus/root_window_controller.cc:251: const bool is_test =
base::CommandLine::ForCurrentProcess()->HasSwitch(
On 2016/06/03 03:07:03, sky wrote:
> On 2016/06/03 00:24:58, James Cook wrote:
> > views_mus_unittests are OK after removing this? I'm not sure what classic
ash
> > does with the non_lock_screen_related_containers during tests.
> 
> They passed locally. I'll find out shortly if the bots like it.

Oh, I think it's actually views_mus_interactive_ui_tests that needed this. Maybe
try that one locally -- I'm pretty sure I added it to the bots, but not 100%.

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

https://codereview.chromium.org/2033843003/diff/20001/ash/mus/root_window_con...
ash/mus/root_window_controller.cc:120:
WmWindowMus::Get(window)->GetShellWindowId() != kShellWindowId_Invalid;
On 2016/06/03 03:07:03, sky wrote:
> On 2016/06/03 00:24:58, James Cook wrote:
> > It's hard to see where kShellWindowId_Invalid is returned. Can the constant
be
> > used in WmWindowAura/Mus instead of literal -1?
> 
> It's actually set in aura::Window and mus::Window.

Yeah, I found them, but it's a magic number that has to match. I know those
classes are at a lower level, but can you share a constant somehow (like in
ui/base)? Or comment next to
kShellWindowId_Invalid/mus::Window::local_id/aura::Window::id that the "invalid"
number has to match the ash constant for everything to work?

https://codereview.chromium.org/2033843003/diff/80001/ash/common/shell_window...
File ash/common/shell_window_ids.cc (right):

https://codereview.chromium.org/2033843003/diff/80001/ash/common/shell_window...
ash/common/shell_window_ids.cc:11: const int kActivatableShellWindowIds[] = {
nit: I would put the comment that the order matters here instead of in the
header file, or replicate it. Someone adding a new one might not see the header.

[sky, 2016-06-03 17:38:42]

The CQ bit was checked by sky@chromium.org

[sky, 2016-06-03 17:38:43]

The patchset sent to the CQ was uploaded after l-g-t-m from
jamescook@chromium.org
Link to the patchset: https://codereview.chromium.org/2033843003/#ps120001
(title: "better comment")

[sky, 2016-06-03 17:39:04]

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

https://codereview.chromium.org/2033843003/diff/20001/ash/mus/root_window_con...
ash/mus/root_window_controller.cc:251: const bool is_test =
base::CommandLine::ForCurrentProcess()->HasSwitch(
On 2016/06/03 16:13:16, James Cook wrote:
> On 2016/06/03 03:07:03, sky wrote:
> > On 2016/06/03 00:24:58, James Cook wrote:
> > > views_mus_unittests are OK after removing this? I'm not sure what classic
> ash
> > > does with the non_lock_screen_related_containers during tests.
> > 
> > They passed locally. I'll find out shortly if the bots like it.
> 
> Oh, I think it's actually views_mus_interactive_ui_tests that needed this.
Maybe
> try that one locally -- I'm pretty sure I added it to the bots, but not 100%.

They pass locally. Fingers crossed.

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

https://codereview.chromium.org/2033843003/diff/20001/ash/mus/root_window_con...
ash/mus/root_window_controller.cc:120:
WmWindowMus::Get(window)->GetShellWindowId() != kShellWindowId_Invalid;
On 2016/06/03 16:13:16, James Cook wrote:
> On 2016/06/03 03:07:03, sky wrote:
> > On 2016/06/03 00:24:58, James Cook wrote:
> > > It's hard to see where kShellWindowId_Invalid is returned. Can the
constant
> be
> > > used in WmWindowAura/Mus instead of literal -1?
> > 
> > It's actually set in aura::Window and mus::Window.
> 
> Yeah, I found them, but it's a magic number that has to match. I know those
> classes are at a lower level, but can you share a constant somehow (like in
> ui/base)? Or comment next to
> kShellWindowId_Invalid/mus::Window::local_id/aura::Window::id that the
"invalid"
> number has to match the ash constant for everything to work?

I ended up going with a static_assert for aura and using kShellWindowId_Invalid
for WmWindowMus.

https://codereview.chromium.org/2033843003/diff/80001/ash/common/shell_window...
File ash/common/shell_window_ids.cc (right):

https://codereview.chromium.org/2033843003/diff/80001/ash/common/shell_window...
ash/common/shell_window_ids.cc:11: const int kActivatableShellWindowIds[] = {
On 2016/06/03 16:13:16, James Cook wrote:
> nit: I would put the comment that the order matters here instead of in the
> header file, or replicate it. Someone adding a new one might not see the
header.

Done.

[commit-bot: I haz the power, 2016-06-03 17:39:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2033843003/120001

[commit-bot: I haz the power, 2016-06-03 17:42:30]

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

[commit-bot: I haz the power, 2016-06-03 17:42:31]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  ios-device on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[sky, 2016-06-03 19:20:24]

The CQ bit was checked by sky@chromium.org

[sky, 2016-06-03 19:20:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
jamescook@chromium.org
Link to the patchset: https://codereview.chromium.org/2033843003/#ps140001
(title: "merge")

[commit-bot: I haz the power, 2016-06-03 19:20:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2033843003/140001

[sky, 2016-06-03 20:13:34]

The CQ bit was unchecked by sky@chromium.org

[sky, 2016-06-04 16:04:39]

The CQ bit was checked by sky@chromium.org

[sky, 2016-06-04 16:04:39]

The patchset sent to the CQ was uploaded after l-g-t-m from
jamescook@chromium.org
Link to the patchset: https://codereview.chromium.org/2033843003/#ps160001
(title: "merge")

[commit-bot: I haz the power, 2016-06-04 16:04:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2033843003/160001

[commit-bot: I haz the power, 2016-06-04 16:09:42]

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

[commit-bot: I haz the power, 2016-06-04 16:09:43]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[sky, 2016-06-06 13:12:37]

The CQ bit was checked by sky@chromium.org

[sky, 2016-06-06 13:12:40]

The patchset sent to the CQ was uploaded after l-g-t-m from
jamescook@chromium.org
Link to the patchset: https://codereview.chromium.org/2033843003/#ps180001
(title: "merge")

[commit-bot: I haz the power, 2016-06-06 13:12:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2033843003/180001

[commit-bot: I haz the power, 2016-06-06 13:27:24]

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

[commit-bot: I haz the power, 2016-06-06 13:27:25]

Try jobs failed on following builders:
  chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)

[sky, 2016-06-06 16:49:49]

The CQ bit was checked by sky@chromium.org

[sky, 2016-06-06 16:49:49]

The patchset sent to the CQ was uploaded after l-g-t-m from
jamescook@chromium.org
Link to the patchset: https://codereview.chromium.org/2033843003/#ps200001
(title: "merge fail")

[commit-bot: I haz the power, 2016-06-06 16:50:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2033843003/200001

[commit-bot: I haz the power, 2016-06-06 18:17:44]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2016-06-06 18:20:02]

Description was changed from

==========
Makes ash/mus use RootWindowControllerCommon

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

to

==========
Makes ash/mus use RootWindowControllerCommon

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

Committed: https://crrev.com/8208371635b02313480d59191e7d6c4a35f69e64
Cr-Commit-Position: refs/heads/master@{#398066}
==========

[commit-bot: I haz the power, 2016-06-06 18:20:03]

Patchset 11 (id:??) landed as
https://crrev.com/8208371635b02313480d59191e7d6c4a35f69e64
Cr-Commit-Position: refs/heads/master@{#398066}