Adds support for multiple displays to WmTestBase

Adds support for multiple displays to WmTestBase

This gives the basics. The one test I hoped to use against it fails
for other reasons (WmWindowMus::SetBoundsInScreen()) isn't implemented
yet.

BUG=612627
TEST=none

Committed: https://crrev.com/20686a4c5e578609252f1ba564010db7b5799338
Cr-Commit-Position: refs/heads/master@{#418133}

[sky, 2016-09-12 20:53:02]

Description was changed from

==========
WIP: multiple displays
==========

to

==========
Adds support for multiple displays to WmTestBase

This gives the basics. The one test I hoped to use against it fails
for other reasons (WmWindowMus::SetBoundsInScreen()) isn't implemented
yet.

BUG=612627
TEST=none
==========

[sky, 2016-09-12 20:53:03]

sky@chromium.org changed reviewers:
+ msw@chromium.org

[sky, 2016-09-12 20:59:09]

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

[commit-bot: I haz the power, 2016-09-12 20:59: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, 2016-09-12 21:43:55]

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

[commit-bot: I haz the power, 2016-09-12 21:43:56]

Dry run: This issue passed the CQ dry run.

[msw, 2016-09-12 22:01:51]

lgtm with nits/qs, I'll look again after any updates.

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

https://codereview.chromium.org/2322613002/diff/40001/ash/mus/test/wm_test_he...
ash/mus/test/wm_test_helper.cc:58: int w, h;
nit: = 0 ?

https://codereview.chromium.org/2322613002/diff/40001/ash/mus/test/wm_test_he...
ash/mus/test/wm_test_helper.cc:98: // Need an id other than kInvalidDisplayID so
the Display is considered valid.
nit: could this call CreateRootWindowController with minor refactoring?

https://codereview.chromium.org/2322613002/diff/40001/ash/mus/test/wm_test_he...
ash/mus/test/wm_test_helper.cc:157: // Offset the height slightly to give a
different work area. -2 is arbitrary,
q: Why -2 here and below, but -20 above?

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

https://codereview.chromium.org/2322613002/diff/40001/ash/mus/window_manager....
ash/mus/window_manager.cc:184: void WindowManager::RemoveDisplay(ui::Window*
window) {
q: just window->Destroy and let OnWindowDestroy[ing|ed] do the rest?
(I see you want to move away from that pattern, so maybe it's okay to support
both and leave this as-is for now... just more code!)

https://codereview.chromium.org/2322613002/diff/40001/ash/mus/window_manager....
ash/mus/window_manager.cc:185: // TODO(sky): WindowManager shouldn't need to
observer the windows. Instead
nit: "observer"

https://codereview.chromium.org/2322613002/diff/40001/ash/mus/window_manager....
ash/mus/window_manager.cc:226: for (auto it = root_window_controllers_.begin();
optional nit: for (auto it : root_window_controllers_) {

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

https://codereview.chromium.org/2322613002/diff/40001/ash/mus/window_manager....
ash/mus/window_manager.h:103: // Removes a Display by window.
nit: "Removes a Display with the given [root] window." or similar?
(optionally handle non-root windows?)

https://codereview.chromium.org/2322613002/diff/40001/services/ui/public/cpp/...
File services/ui/public/cpp/tests/window_tree_client_private.h (right):

https://codereview.chromium.org/2322613002/diff/40001/services/ui/public/cpp/...
services/ui/public/cpp/tests/window_tree_client_private.h:55: WindowTreeClient*
tree_client_impl_;
nit: indentation here and below

[sky, 2016-09-12 23:11:11]

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

[sky, 2016-09-12 23:11:28]

A lot changed, please take another look.

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

https://codereview.chromium.org/2322613002/diff/40001/ash/mus/test/wm_test_he...
ash/mus/test/wm_test_helper.cc:58: int w, h;
On 2016/09/12 22:01:51, msw wrote:
> nit: = 0 ?

Done.

https://codereview.chromium.org/2322613002/diff/40001/ash/mus/test/wm_test_he...
ash/mus/test/wm_test_helper.cc:98: // Need an id other than kInvalidDisplayID so
the Display is considered valid.
On 2016/09/12 22:01:51, msw wrote:
> nit: could this call CreateRootWindowController with minor refactoring?

This resulted in a bit more changes, but it's where I wanted to go anyway.

https://codereview.chromium.org/2322613002/diff/40001/ash/mus/test/wm_test_he...
ash/mus/test/wm_test_helper.cc:157: // Offset the height slightly to give a
different work area. -2 is arbitrary,
On 2016/09/12 22:01:51, msw wrote:
> q: Why -2 here and below, but -20 above?

Fixed.

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

https://codereview.chromium.org/2322613002/diff/40001/ash/mus/window_manager....
ash/mus/window_manager.cc:184: void WindowManager::RemoveDisplay(ui::Window*
window) {
On 2016/09/12 22:01:51, msw wrote:
> q: just window->Destroy and let OnWindowDestroy[ing|ed] do the rest?
> (I see you want to move away from that pattern, so maybe it's okay to support
> both and leave this as-is for now... just more code!)

Indeed. I want to move away from the observer and prefer this approach.

https://codereview.chromium.org/2322613002/diff/40001/ash/mus/window_manager....
ash/mus/window_manager.cc:185: // TODO(sky): WindowManager shouldn't need to
observer the windows. Instead
On 2016/09/12 22:01:51, msw wrote:
> nit: "observer"

Done.

https://codereview.chromium.org/2322613002/diff/40001/ash/mus/window_manager....
ash/mus/window_manager.cc:226: for (auto it = root_window_controllers_.begin();
On 2016/09/12 22:01:51, msw wrote:
> optional nit: for (auto it : root_window_controllers_) {

That'll give me a std::unique_ptr<RootWindowController*>&. I want the iterator.

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

https://codereview.chromium.org/2322613002/diff/40001/ash/mus/window_manager....
ash/mus/window_manager.h:103: // Removes a Display by window.
On 2016/09/12 22:01:51, msw wrote:
> nit: "Removes a Display with the given [root] window." or similar?
> (optionally handle non-root windows?)

I ended up renaming this to DestroyRootWindowController.

https://codereview.chromium.org/2322613002/diff/40001/services/ui/public/cpp/...
File services/ui/public/cpp/tests/window_tree_client_private.h (right):

https://codereview.chromium.org/2322613002/diff/40001/services/ui/public/cpp/...
services/ui/public/cpp/tests/window_tree_client_private.h:55: WindowTreeClient*
tree_client_impl_;
On 2016/09/12 22:01:51, msw wrote:
> nit: indentation here and below

Done.

[commit-bot: I haz the power, 2016-09-12 23:11:43]

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

[msw, 2016-09-12 23:19:48]

nice improvements; lgtm with nits.

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

https://codereview.chromium.org/2322613002/diff/60001/ash/mus/test/wm_test_he...
ash/mus/test/wm_test_helper.cc:156: // Offset the height slightly to give a
different work area. -2 is arbitrary,
nit: -2 versus -20 above.

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

https://codereview.chromium.org/2322613002/diff/60001/ash/mus/window_manager....
ash/mus/window_manager.cc:187: if (!was_displays_empty) {
nit: unconditionally add the display above?

[sky, 2016-09-13 00:09:54]

The CQ bit was checked by sky@chromium.org

[sky, 2016-09-13 00:09:55]

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/2322613002/#ps100001
(title: "feedback")

[sky, 2016-09-13 00:10:26]

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

https://codereview.chromium.org/2322613002/diff/60001/ash/mus/test/wm_test_he...
ash/mus/test/wm_test_helper.cc:156: // Offset the height slightly to give a
different work area. -2 is arbitrary,
On 2016/09/12 23:19:48, msw wrote:
> nit: -2 versus -20 above.

Done.

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

https://codereview.chromium.org/2322613002/diff/60001/ash/mus/window_manager....
ash/mus/window_manager.cc:187: if (!was_displays_empty) {
On 2016/09/12 23:19:48, msw wrote:
> nit: unconditionally add the display above?

Sadly it has to be like this, and I suspect even this is fragile. I've added a
comment as to why we have to add after the fact:

    // If this isn't the initial display then add the display to Screen after
    // creating the RootWindowController. We need to do this after creating the
    // RootWindowController as adding the display triggers OnDisplayAdded(),
    // which triggers some overrides asking for the RootWindowController for the
    // new display.

[commit-bot: I haz the power, 2016-09-13 00:10:30]

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-13 01:27:13]

Description was changed from

==========
Adds support for multiple displays to WmTestBase

This gives the basics. The one test I hoped to use against it fails
for other reasons (WmWindowMus::SetBoundsInScreen()) isn't implemented
yet.

BUG=612627
TEST=none
==========

to

==========
Adds support for multiple displays to WmTestBase

This gives the basics. The one test I hoped to use against it fails
for other reasons (WmWindowMus::SetBoundsInScreen()) isn't implemented
yet.

BUG=612627
TEST=none
==========

[commit-bot: I haz the power, 2016-09-13 01:27:15]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-09-13 01:29:32]

Description was changed from

==========
Adds support for multiple displays to WmTestBase

This gives the basics. The one test I hoped to use against it fails
for other reasons (WmWindowMus::SetBoundsInScreen()) isn't implemented
yet.

BUG=612627
TEST=none
==========

to

==========
Adds support for multiple displays to WmTestBase

This gives the basics. The one test I hoped to use against it fails
for other reasons (WmWindowMus::SetBoundsInScreen()) isn't implemented
yet.

BUG=612627
TEST=none

Committed: https://crrev.com/20686a4c5e578609252f1ba564010db7b5799338
Cr-Commit-Position: refs/heads/master@{#418133}
==========

[commit-bot: I haz the power, 2016-09-13 01:29:33]

Patchset 6 (id:??) landed as
https://crrev.com/20686a4c5e578609252f1ba564010db7b5799338
Cr-Commit-Position: refs/heads/master@{#418133}