chromeos: wireup more display management for mushrome

chromeos: wireup more display management for mushrome

In addition to making ShellPortMash use more code that
ShellPortClassic uses this also enables more test code for
mushrome. As part of 718860  I plan to remove the functions from
ShellPort that end up being identical.

BUG=706589
TEST=covered by tests
R=msw@chromium.org

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

[sky, 2017-05-11 00:54:10]

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

[commit-bot: I haz the power, 2017-05-11 00:55:34]

Dry run: CQ is trying da patch.

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

[msw, 2017-05-11 01:07:17]

https://codereview.chromium.org/2874963002/diff/1/ash/mus/bridge/shell_port_m...
File ash/mus/bridge/shell_port_mash.cc (right):

https://codereview.chromium.org/2874963002/diff/1/ash/mus/bridge/shell_port_m...
ash/mus/bridge/shell_port_mash.cc:420: void
ShellPortMash::AddDisplayObserver(WmDisplayObserver* observer) {
Why shouldn't callers use
|Shell::Get()->window_tree_host_manager()->AddObserver(this);| directly? It
seems odd to replicate the functionality with ShellPort as an intermediary.

https://codereview.chromium.org/2874963002/diff/1/ash/mus/bridge/shell_port_m...
ash/mus/bridge/shell_port_mash.cc:421: // TODO: need WmDisplayObserver support
for mus. http://crbug.com/705831.
nit: is this obsolete? (or merge this with the mash todo?) (ditto below)

https://codereview.chromium.org/2874963002/diff/1/ash/mus/bridge/shell_port_m...
ash/mus/bridge/shell_port_mash.cc:427: if (!added_display_observer_) {
would it be reasonable to always add this instance as an observer (in mus
config), presuming you keep the intermediary pattern?

https://codereview.chromium.org/2874963002/diff/1/ash/test/ash_test_helper.cc
File ash/test/ash_test_helper.cc (right):

https://codereview.chromium.org/2874963002/diff/1/ash/test/ash_test_helper.cc...
ash/test/ash_test_helper.cc:183: // ScreenLayoutObserver is specific to
classic-ash.
nit: update comment, ditto below (line 189)

https://codereview.chromium.org/2874963002/diff/1/testing/buildbot/filters/as...
File testing/buildbot/filters/ash_mus_unittests.filter (right):

https://codereview.chromium.org/2874963002/diff/1/testing/buildbot/filters/as...
testing/buildbot/filters/ash_mus_unittests.filter:83:
-ToastManagerTest.NullMessageHasNoDismissButton
aside: odd that this change revealed another broken test.

[commit-bot: I haz the power, 2017-05-11 02:27:51]

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

[commit-bot: I haz the power, 2017-05-11 02:27:51]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[sky, 2017-05-11 14:06:35]

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

[sky, 2017-05-11 14:06:53]

https://codereview.chromium.org/2874963002/diff/1/ash/mus/bridge/shell_port_m...
File ash/mus/bridge/shell_port_mash.cc (right):

https://codereview.chromium.org/2874963002/diff/1/ash/mus/bridge/shell_port_m...
ash/mus/bridge/shell_port_mash.cc:420: void
ShellPortMash::AddDisplayObserver(WmDisplayObserver* observer) {
On 2017/05/11 01:07:17, msw wrote:
> Why shouldn't callers use
> |Shell::Get()->window_tree_host_manager()->AddObserver(this);| directly? It
> seems odd to replicate the functionality with ShellPort as an intermediary.

Initially we weren't going to use WindowTreeHostManager in mash at all. This
lead to the creation of WmDisplayObserver. Simplified display management means
we can use WindowTreeHostManager, and that WmDisplayObserver is no longer
necessary.

So, you're definitely right that this code can be nuked. I can't do that now
though as mash isn't using simplified display management yet. Or rather I could,
but I would be shifting around the conditionals, and the change would be much
bigger. So, for the time being I've gone this route. Effectively duplicate the
code from classic ash to here, and once I get simplified display management
working for mash I'll nuke this code. Does that make sense?

https://codereview.chromium.org/2874963002/diff/1/ash/mus/bridge/shell_port_m...
ash/mus/bridge/shell_port_mash.cc:421: // TODO: need WmDisplayObserver support
for mus. http://crbug.com/705831.
On 2017/05/11 01:07:16, msw wrote:
> nit: is this obsolete? (or merge this with the mash todo?) (ditto below)

I'm keeping TODO to remind me to update the bug.

https://codereview.chromium.org/2874963002/diff/1/ash/mus/bridge/shell_port_m...
ash/mus/bridge/shell_port_mash.cc:427: if (!added_display_observer_) {
On 2017/05/11 01:07:16, msw wrote:
> would it be reasonable to always add this instance as an observer (in mus
> config), presuming you keep the intermediary pattern?

I believe there are timing issues doing that. ShellPortClassic has the same
code.

https://codereview.chromium.org/2874963002/diff/1/ash/test/ash_test_helper.cc
File ash/test/ash_test_helper.cc (right):

https://codereview.chromium.org/2874963002/diff/1/ash/test/ash_test_helper.cc...
ash/test/ash_test_helper.cc:183: // ScreenLayoutObserver is specific to
classic-ash.
On 2017/05/11 01:07:17, msw wrote:
> nit: update comment, ditto below (line 189)

Done.

https://codereview.chromium.org/2874963002/diff/1/testing/buildbot/filters/as...
File testing/buildbot/filters/ash_mus_unittests.filter (right):

https://codereview.chromium.org/2874963002/diff/1/testing/buildbot/filters/as...
testing/buildbot/filters/ash_mus_unittests.filter:83:
-ToastManagerTest.NullMessageHasNoDismissButton
On 2017/05/11 01:07:17, msw wrote:
> aside: odd that this change revealed another broken test.

I think all the ToastManagerTests tests are flaky. So, this test (and the next
two) may very well pass on a retry. None-the-less I noticed some weird hangs, so
I'm adding this test to the disabled list.

Once I'm done with simplified display management I plan to put TODOs in here for
each group of failing tests.

[commit-bot: I haz the power, 2017-05-11 14:07:23]

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-05-11 15:03:43]

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

[commit-bot: I haz the power, 2017-05-11 15:03:44]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[msw, 2017-05-11 16:48:06]

lgtm with a q about todos/plans

https://codereview.chromium.org/2874963002/diff/1/ash/mus/bridge/shell_port_m...
File ash/mus/bridge/shell_port_mash.cc (right):

https://codereview.chromium.org/2874963002/diff/1/ash/mus/bridge/shell_port_m...
ash/mus/bridge/shell_port_mash.cc:420: void
ShellPortMash::AddDisplayObserver(WmDisplayObserver* observer) {
On 2017/05/11 14:06:52, sky wrote:
> On 2017/05/11 01:07:17, msw wrote:
> > Why shouldn't callers use
> > |Shell::Get()->window_tree_host_manager()->AddObserver(this);| directly? It
> > seems odd to replicate the functionality with ShellPort as an intermediary.
> 
> Initially we weren't going to use WindowTreeHostManager in mash at all. This
> lead to the creation of WmDisplayObserver. Simplified display management means
> we can use WindowTreeHostManager, and that WmDisplayObserver is no longer
> necessary.
> 
> So, you're definitely right that this code can be nuked. I can't do that now
> though as mash isn't using simplified display management yet. Or rather I
could,
> but I would be shifting around the conditionals, and the change would be much
> bigger. So, for the time being I've gone this route. Effectively duplicate the
> code from classic ash to here, and once I get simplified display management
> working for mash I'll nuke this code. Does that make sense?

Fair enough, but shouldn't the TODOs then say "TODO: Remove WmDisplayObserver
and use WindowTreeHostManager::Observer instead, once mash supports simplified
display management." or similar?

[sky, 2017-05-12 01:49:02]

The CQ bit was checked by sky@chromium.org

[sky, 2017-05-12 01:49:02]

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

[sky, 2017-05-12 01:49:36]

https://codereview.chromium.org/2874963002/diff/1/ash/mus/bridge/shell_port_m...
File ash/mus/bridge/shell_port_mash.cc (right):

https://codereview.chromium.org/2874963002/diff/1/ash/mus/bridge/shell_port_m...
ash/mus/bridge/shell_port_mash.cc:420: void
ShellPortMash::AddDisplayObserver(WmDisplayObserver* observer) {
On 2017/05/11 16:48:06, msw wrote:
> On 2017/05/11 14:06:52, sky wrote:
> > On 2017/05/11 01:07:17, msw wrote:
> > > Why shouldn't callers use
> > > |Shell::Get()->window_tree_host_manager()->AddObserver(this);| directly?
It
> > > seems odd to replicate the functionality with ShellPort as an
intermediary.
> > 
> > Initially we weren't going to use WindowTreeHostManager in mash at all. This
> > lead to the creation of WmDisplayObserver. Simplified display management
means
> > we can use WindowTreeHostManager, and that WmDisplayObserver is no longer
> > necessary.
> > 
> > So, you're definitely right that this code can be nuked. I can't do that now
> > though as mash isn't using simplified display management yet. Or rather I
> could,
> > but I would be shifting around the conditionals, and the change would be
much
> > bigger. So, for the time being I've gone this route. Effectively duplicate
the
> > code from classic ash to here, and once I get simplified display management
> > working for mash I'll nuke this code. Does that make sense?
> 
> Fair enough, but shouldn't the TODOs then say "TODO: Remove WmDisplayObserver
> and use WindowTreeHostManager::Observer instead, once mash supports simplified
> display management." or similar?

Indeed you are right. Updated.

[commit-bot: I haz the power, 2017-05-12 01:49:59]

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-05-12 03:47:28]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1494553742174940,
"parent_rev": "ca4d3f297d0b03d83711a9d17f808e299aa6ff19", "commit_rev":
"edef2a4e69984700e09359b2f3e7234363a16d2c"}

[commit-bot: I haz the power, 2017-05-12 03:47:40]

Description was changed from

==========
chromeos: wireup more display management for mushrome

In addition to making ShellPortMash use more code that
ShellPortClassic uses this also enables more test code for
mushrome. As part of 718860  I plan to remove the functions from
ShellPort that end up being identical.

BUG=706589
TEST=covered by tests
R=msw@chromium.org
==========

to

==========
chromeos: wireup more display management for mushrome

In addition to making ShellPortMash use more code that
ShellPortClassic uses this also enables more test code for
mushrome. As part of 718860  I plan to remove the functions from
ShellPort that end up being identical.

BUG=706589
TEST=covered by tests
R=msw@chromium.org

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

[commit-bot: I haz the power, 2017-05-12 03:47:41]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/edef2a4e69984700e09359b2f3e7...