chromeos: Makes mushrome use simplified display management

chromeos: Makes mushrome use simplified display management

This also adds a stanalone_ash target that is the equivalent of
ash_shell_with_content but running in mushrome mode and without
content.

BUG=708287, 706589
TEST=covered by tests
R=kylechar@chromium.org

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

[sky, 2017-04-25 20:32:29]

There is one test that fails because I haven't landed
https://codereview.chromium.org/2838613002/ . The reason it fails is because all
the other tests set the command line so that in Shell::Init()
display_manager_->InitFromCommandLine() ends up succeeding and
display_initialized is set to true early on. That isn't the case for
AshTestBase. In order to make that test work I need something like
https://codereview.chromium.org/2838613002/ . Let me know what you think the
best way to proceed on that is.

https://codereview.chromium.org/2840043003/diff/1/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/2840043003/diff/1/ash/shell.cc#newcode829
ash/shell.cc:829: (config == Config::MASH ||
display_manager_->InitFromCommandLine());
I need to call InitFromCommandLine as tests manipulate the command line
(switches::kHostWindowBounds) and expect it to be picked up.

[sky, 2017-04-25 22:20:46]

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

[commit-bot: I haz the power, 2017-04-25 22:21:20]

Dry run: CQ is trying da patch.

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

[kylechar, 2017-04-26 16:41:44]

Here are some initial questions/comments. Moving the ash/mus/standalone changes
to a separate CL would be helpful I think!

https://codereview.chromium.org/2840043003/diff/20001/ash/mus/bridge/shell_po...
File ash/mus/bridge/shell_port_mash.cc (right):

https://codereview.chromium.org/2840043003/diff/20001/ash/mus/bridge/shell_po...
ash/mus/bridge/shell_port_mash.cc:525: CHECK(false);
Unrelated to this CL but should this configuration be compiled even? We could
probably just not compile any mash/mus stuff on X11 CrOS config.

enable_package_mash_services = is_chromeos && use_ozone

https://codereview.chromium.org/2840043003/diff/20001/ash/mus/standalone/stan...
File ash/mus/standalone/standalone_mash_main.cc (right):

https://codereview.chromium.org/2840043003/diff/20001/ash/mus/standalone/stan...
ash/mus/standalone/standalone_mash_main.cc:37: input_device_client_ =
base::MakeUnique<ui::InputDeviceClient>();
nit: move to initializer list.

https://codereview.chromium.org/2840043003/diff/20001/ash/mus/test/ash_test_i...
File ash/mus/test/ash_test_impl_mus.cc (right):

https://codereview.chromium.org/2840043003/diff/20001/ash/mus/test/ash_test_i...
ash/mus/test/ash_test_impl_mus.cc:47: if
(!command_line->HasSwitch(::switches::kHostWindowBounds)) {
Do tests need this to pass?

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

https://codereview.chromium.org/2840043003/diff/20001/ash/shell.cc#newcode829
ash/shell.cc:829: (config == Config::MASH ||
display_manager_->InitFromCommandLine());
display_manager_->InitFromCommandLine() is what makes display manager load it's
own DisplayInfo if --ash-host-window-bounds flag exists. This is the fake
display path that classic ash uses that cuts out
DisplayConfigurator/DisplayChangeObserver/NativeDisplayDelegate.

That shouldn't be necessary for Config::MUS because we always use
DisplayConfigurator/DisplayChangeObserver/NativeDisplayDelegate. It's definitely
not necessary when running ./chrome --mus. We can accomplish similar off device
configuration with the --screen-config.

I think if ForwardingDisplayDelegate is connected in tests then it also wouldn't
be necessary for tests?

https://codereview.chromium.org/2840043003/diff/20001/services/ui/ws/window_s...
File services/ui/ws/window_server.cc (right):

https://codereview.chromium.org/2840043003/diff/20001/services/ui/ws/window_s...
services/ui/ws/window_server.cc:831: if (display_root && (window ==
display_root->root() ||
Can you pull some of this conditional out into other variables to simplify it?

[sky, 2017-04-26 19:36:42]

Sorry for the merge and fix in one patchset.

https://codereview.chromium.org/2840043003/diff/20001/ash/mus/bridge/shell_po...
File ash/mus/bridge/shell_port_mash.cc (right):

https://codereview.chromium.org/2840043003/diff/20001/ash/mus/bridge/shell_po...
ash/mus/bridge/shell_port_mash.cc:525: CHECK(false);
On 2017/04/26 16:41:44, kylechar wrote:
> Unrelated to this CL but should this configuration be compiled even? We could
> probably just not compile any mash/mus stuff on X11 CrOS config.
> 
> enable_package_mash_services = is_chromeos && use_ozone

Agreed. I'm not going to try to address that here.

https://codereview.chromium.org/2840043003/diff/20001/ash/mus/standalone/stan...
File ash/mus/standalone/standalone_mash_main.cc (right):

https://codereview.chromium.org/2840043003/diff/20001/ash/mus/standalone/stan...
ash/mus/standalone/standalone_mash_main.cc:37: input_device_client_ =
base::MakeUnique<ui::InputDeviceClient>();
On 2017/04/26 16:41:44, kylechar wrote:
> nit: move to initializer list.

Moved to separate cl.

https://codereview.chromium.org/2840043003/diff/20001/ash/mus/test/ash_test_i...
File ash/mus/test/ash_test_impl_mus.cc (right):

https://codereview.chromium.org/2840043003/diff/20001/ash/mus/test/ash_test_i...
ash/mus/test/ash_test_impl_mus.cc:47: if
(!command_line->HasSwitch(::switches::kHostWindowBounds)) {
On 2017/04/26 16:41:44, kylechar wrote:
> Do tests need this to pass?

Yes as otherwise the root ends up at a different size and some tests expect a
certain size.

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

https://codereview.chromium.org/2840043003/diff/20001/ash/shell.cc#newcode829
ash/shell.cc:829: (config == Config::MASH ||
display_manager_->InitFromCommandLine());
On 2017/04/26 16:41:44, kylechar wrote:
> display_manager_->InitFromCommandLine() is what makes display manager load
it's
> own DisplayInfo if --ash-host-window-bounds flag exists. This is the fake
> display path that classic ash uses that cuts out
> DisplayConfigurator/DisplayChangeObserver/NativeDisplayDelegate.
> 
> That shouldn't be necessary for Config::MUS because we always use
> DisplayConfigurator/DisplayChangeObserver/NativeDisplayDelegate. It's
definitely
> not necessary when running ./chrome --mus. We can accomplish similar off
device
> configuration with the --screen-config.
> 
> I think if ForwardingDisplayDelegate is connected in tests then it also
wouldn't
> be necessary for tests?

I motivated the reason for this over IM.

https://codereview.chromium.org/2840043003/diff/20001/services/ui/ws/window_s...
File services/ui/ws/window_server.cc (right):

https://codereview.chromium.org/2840043003/diff/20001/services/ui/ws/window_s...
services/ui/ws/window_server.cc:831: if (display_root && (window ==
display_root->root() ||
On 2017/04/26 16:41:44, kylechar wrote:
> Can you pull some of this conditional out into other variables to simplify it?

Done.

[sky, 2017-04-26 21:45:07]

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

[commit-bot: I haz the power, 2017-04-26 21:46:14]

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-04-26 23:49:36]

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

[commit-bot: I haz the power, 2017-04-26 23:49:37]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sky, 2017-04-27 14:08:07]

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

[commit-bot: I haz the power, 2017-04-27 14:08:38]

Dry run: CQ is trying da patch.

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

[kylechar, 2017-04-27 14:26:31]

https://codereview.chromium.org/2840043003/diff/100001/ash/aura/shell_port_cl...
File ash/aura/shell_port_classic.cc (right):

https://codereview.chromium.org/2840043003/diff/100001/ash/aura/shell_port_cl...
ash/aura/shell_port_classic.cc:46: #include "ui/gfx/x/x11_types.h"  // nogncheck
Is this include used?

https://codereview.chromium.org/2840043003/diff/100001/services/ui/ws/window_...
File services/ui/ws/window_server.cc (right):

https://codereview.chromium.org/2840043003/diff/100001/services/ui/ws/window_...
services/ui/ws/window_server.cc:37: bool IsWindowConsideredDisplayRoot(const
Display* display,
nit: The term display root is used elsewhere for the root ServerWindow that has
a corresponding FrameGenerator. I think this would be more window manager root?

https://codereview.chromium.org/2840043003/diff/100001/services/ui/ws/window_...
services/ui/ws/window_server.cc:52: return !display_root->window_manager_state()
Can we remove this extra level of window hierarchy at some point?

[sky, 2017-04-27 15:09:53]

https://codereview.chromium.org/2840043003/diff/100001/ash/aura/shell_port_cl...
File ash/aura/shell_port_classic.cc (right):

https://codereview.chromium.org/2840043003/diff/100001/ash/aura/shell_port_cl...
ash/aura/shell_port_classic.cc:46: #include "ui/gfx/x/x11_types.h"  // nogncheck
On 2017/04/27 14:26:30, kylechar wrote:
> Is this include used?

It came from those in shell.cc. I'll see if the bots are ok without it.

https://codereview.chromium.org/2840043003/diff/100001/services/ui/ws/window_...
File services/ui/ws/window_server.cc (right):

https://codereview.chromium.org/2840043003/diff/100001/services/ui/ws/window_...
services/ui/ws/window_server.cc:37: bool IsWindowConsideredDisplayRoot(const
Display* display,
On 2017/04/27 14:26:31, kylechar wrote:
> nit: The term display root is used elsewhere for the root ServerWindow that
has
> a corresponding FrameGenerator. I think this would be more window manager
root?

Done.

https://codereview.chromium.org/2840043003/diff/100001/services/ui/ws/window_...
services/ui/ws/window_server.cc:52: return !display_root->window_manager_state()
On 2017/04/27 14:26:31, kylechar wrote:
> Can we remove this extra level of window hierarchy at some point?

Possibly. If I get rid of the mode where mus automatically creates the roots
than I could.

[sky, 2017-04-27 15:10:12]

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

[commit-bot: I haz the power, 2017-04-27 15:10:39]

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-04-27 16:30:03]

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

[commit-bot: I haz the power, 2017-04-27 16:30:04]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[kylechar, 2017-04-27 18:28:43]

lgtm

[sky, 2017-04-28 02:49:19]

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

[commit-bot: I haz the power, 2017-04-28 02:50:24]

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-04-28 04:53:32]

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

[commit-bot: I haz the power, 2017-04-28 04:53:34]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sky, 2017-04-28 13:34:36]

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

[commit-bot: I haz the power, 2017-04-28 13:35:22]

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-04-28 15:39:08]

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

[commit-bot: I haz the power, 2017-04-28 15:39:09]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sky, 2017-04-28 17:09:17]

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

[commit-bot: I haz the power, 2017-04-28 17:09:43]

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-04-28 17:18:52]

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

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

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sky, 2017-04-28 18:05:21]

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

[commit-bot: I haz the power, 2017-04-28 18:06:07]

Dry run: CQ is trying da patch.

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

[sky, 2017-04-28 19:02:35]

The CQ bit was unchecked by sky@chromium.org

[sky, 2017-04-28 19:02:38]

The CQ bit was checked by sky@chromium.org

[sky, 2017-04-28 19:02:39]

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

[commit-bot: I haz the power, 2017-04-28 19:04:35]

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-04-28 20:53:01]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1493406158617540,
"parent_rev": "2605f2b320fe9d3d9fbe4d00964fa784a1b6ab16", "commit_rev":
"cfaad89ae8da7b8a0d0055db9f5bc5ac76cf6cf5"}

[commit-bot: I haz the power, 2017-04-28 20:53:12]

Description was changed from

==========
chromeos: Makes mushrome use simplified display management

This also adds a stanalone_ash target that is the equivalent of
ash_shell_with_content but running in mushrome mode and without
content.

BUG=708287,706589
TEST=covered by tests
R=kylechar@chromium.org
==========

to

==========
chromeos: Makes mushrome use simplified display management

This also adds a stanalone_ash target that is the equivalent of
ash_shell_with_content but running in mushrome mode and without
content.

BUG=708287,706589
TEST=covered by tests
R=kylechar@chromium.org

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

[commit-bot: I haz the power, 2017-04-28 20:53:13]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/cfaad89ae8da7b8a0d0055db9f5b...

[vabr (Chromium), 2017-05-02 13:38:36]

A revert of this CL (patchset #8 id:140001) has been created in
https://codereview.chromium.org/2858563002/ by vabr@chromium.org.

The reason for reverting is: This appears to have broken
https://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%...,
and that bot has mus_browser_test failing reliably since then.

More info on the associated bug:
BUG=717523.