Removes ash::mus::RootWindowController

Removes ash::mus::RootWindowController

The functionality is moved into two places:
. Maintaining the display goes to ScreenMus.
. Creating the window moves to top_level_window_factory.

As part of this I'm removing some functionality that staggered top
level windows. I suspect this isn't used much and needs some
changes. Will address this later.

BUG=671246
TEST=none
R=msw@chromium.org

Review-Url: https://codereview.chromium.org/2642003002
Cr-Commit-Position: refs/heads/master@{#444745}
Committed: https://chromium.googlesource.com/chromium/src/+/8a8ffd21bdd7e290cbe4587b64471d13e7bb40c3

[sky, 2017-01-18 21:28:03]

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

[commit-bot: I haz the power, 2017-01-18 21:28:31]

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-01-18 22:31:27]

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

[commit-bot: I haz the power, 2017-01-18 22:31:28]

Dry run: This issue passed the CQ dry run.

[msw, 2017-01-18 23:31:27]

mostly lg, some qs and minor comments.

https://codereview.chromium.org/2642003002/diff/1/ash/mus/bridge/wm_lookup_mu...
File ash/mus/bridge/wm_lookup_mus.cc (right):

https://codereview.chromium.org/2642003002/diff/1/ash/mus/bridge/wm_lookup_mu...
ash/mus/bridge/wm_lookup_mus.cc:23: RootWindowController*
WmLookupMus::GetRootWindowControllerWithDisplayId(
aside: I wonder if this helper is still useful.

https://codereview.chromium.org/2642003002/diff/1/ash/mus/bridge/wm_shell_mus.cc
File ash/mus/bridge/wm_shell_mus.cc (right):

https://codereview.chromium.org/2642003002/diff/1/ash/mus/bridge/wm_shell_mus...
ash/mus/bridge/wm_shell_mus.cc:261:
window_manager_->screen()->SetWorkAreaInsets(window->aura_window(), insets);
q: Should this use Screen::GetScreen(), if a test called SetScreenInstance with
a different value than the window manager's screen? (or DCHECK_EQ?) (also okay
as-is, I see you have a check in ScreenMus::SetWorkAreaInsets)

https://codereview.chromium.org/2642003002/diff/1/ash/mus/root_window_control...
File ash/mus/root_window_controller.h (left):

https://codereview.chromium.org/2642003002/diff/1/ash/mus/root_window_control...
ash/mus/root_window_controller.h:66: void SetWorkAreaInests(const gfx::Insets&
insets);
aside: goodbye, typo!

https://codereview.chromium.org/2642003002/diff/1/ash/mus/screen_mus.h
File ash/mus/screen_mus.h (right):

https://codereview.chromium.org/2642003002/diff/1/ash/mus/screen_mus.h#newcode12
ash/mus/screen_mus.h:12: class Window;
nit: is this needed? (already used for GetDisplayNearestWindow)

https://codereview.chromium.org/2642003002/diff/1/ash/mus/screen_mus.h#newcode34
ash/mus/screen_mus.h:34: // Sets the work area of the display |window| is on.
optional grammar nit: "of the display containing |window|."

https://codereview.chromium.org/2642003002/diff/1/ash/mus/test/wm_test_base.cc
File ash/mus/test/wm_test_base.cc (right):

https://codereview.chromium.org/2642003002/diff/1/ash/mus/test/wm_test_base.c...
ash/mus/test/wm_test_base.cc:96: return
roots[0]->GetWindow()->GetDisplayNearestWindow();
q: does it matter if we call GetWindow vs. GetRootWindow here (and below)?

https://codereview.chromium.org/2642003002/diff/1/ash/mus/top_level_window_fa...
File ash/mus/top_level_window_factory.cc (right):

https://codereview.chromium.org/2642003002/diff/1/ash/mus/top_level_window_fa...
ash/mus/top_level_window_factory.cc:14: #include "ash/mus/screen_mus.h"
nit: needed?

https://codereview.chromium.org/2642003002/diff/1/ash/mus/top_level_window_fa...
ash/mus/top_level_window_factory.cc:48: const int64_t display_id =
GetInitialDisplayId(*properties);
nit: skip the loop for display::kInvalidDisplayId?

https://codereview.chromium.org/2642003002/diff/1/ash/mus/top_level_window_fa...
ash/mus/top_level_window_factory.cc:77: gfx::Rect bounds(0, 0,
root_size.width(), root_size.height());
nit: use Rect(const Size& size) ctor

https://codereview.chromium.org/2642003002/diff/1/ash/mus/top_level_window_fa...
ash/mus/top_level_window_factory.cc:80:
root_window_controller->GetWindow()->GetDisplayNearestWindow();
ditto q: GetWindow/GetRootWindow, optionally cache root used above.

https://codereview.chromium.org/2642003002/diff/1/ash/mus/top_level_window_fa...
ash/mus/top_level_window_factory.cc:81:
bounds.Offset(display.bounds().origin().x(),
nit: bounds.Offset(display.bounds()OffsetFromOrigin());

https://codereview.chromium.org/2642003002/diff/1/ash/mus/top_level_window_fa...
ash/mus/top_level_window_factory.cc:88: int width, height;
nit: explicit 0 init

https://codereview.chromium.org/2642003002/diff/1/ash/mus/top_level_window_fa...
ash/mus/top_level_window_factory.cc:93: width = std::max(0,
std::min(max_size.width(), pref.width()));
optional nit: re-use |pref| with Size::SetToMax and SetToMin

https://codereview.chromium.org/2642003002/diff/1/ash/mus/top_level_window_fa...
ash/mus/top_level_window_factory.cc:96: width = root_size.width() - 240;
optional nit: use a const for this magic number, ditto below

https://codereview.chromium.org/2642003002/diff/1/ash/mus/top_level_window_fa...
ash/mus/top_level_window_factory.cc:101: const int window_count = 0;
q: I realize this has a TODO, but did you mean to make this static, or really
always just use 0 for now?

https://codereview.chromium.org/2642003002/diff/1/ash/mus/top_level_window_fa...
ash/mus/top_level_window_factory.cc:102: return gfx::Rect(40 + (window_count %
4) * 40, 40 + (window_count % 4) * 40,
optional nit: cache |40 + (window_count % 4) * 40|

https://codereview.chromium.org/2642003002/diff/1/ash/mus/top_level_window_fa...
ash/mus/top_level_window_factory.cc:119: container_window =
root_window_controller->GetWindow()
aside: GetWindow==GetRootWindow is bugging me, try to avoid GetWindow per your
function comment

https://codereview.chromium.org/2642003002/diff/1/ash/mus/top_level_window_fa...
ash/mus/top_level_window_factory.cc:123: context =
root_window_controller->GetRootWindow();
aside: it might be nice to avoid context and pick the appropriate/default
container here as needed, but that might change NonClientFrameController's
behavior

https://codereview.chromium.org/2642003002/diff/1/ash/mus/top_level_window_fa...
ash/mus/top_level_window_factory.cc:129: const bool provide_non_client_frame =
optional nit: inline

https://codereview.chromium.org/2642003002/diff/1/ash/mus/top_level_window_fa...
ash/mus/top_level_window_factory.cc:184:
ui::mojom::WindowManager::kWindowIgnoredByShelf_Property)) {
aside: itd be nice if this were an aura window prop

https://codereview.chromium.org/2642003002/diff/1/ash/mus/top_level_window_fa...
File ash/mus/top_level_window_factory.h (right):

https://codereview.chromium.org/2642003002/diff/1/ash/mus/top_level_window_fa...
ash/mus/top_level_window_factory.h:29: // Creates and parent a new top-level
window and returns it. The returned
nit: parents

https://codereview.chromium.org/2642003002/diff/1/ash/mus/top_level_window_fa...
ash/mus/top_level_window_factory.h:30: // aura::Window is owned by it's parent.
nit: "its"

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

https://codereview.chromium.org/2642003002/diff/1/ash/mus/window_manager.cc#n...
ash/mus/window_manager.cc:204: new RootWindowController(nullptr,
window_tree_host.release()));
nit: MakeUnique

[sky, 2017-01-19 01:02:20]

I think I got them all. Thanks!

https://codereview.chromium.org/2642003002/diff/1/ash/mus/bridge/wm_lookup_mu...
File ash/mus/bridge/wm_lookup_mus.cc (right):

https://codereview.chromium.org/2642003002/diff/1/ash/mus/bridge/wm_lookup_mu...
ash/mus/bridge/wm_lookup_mus.cc:23: RootWindowController*
WmLookupMus::GetRootWindowControllerWithDisplayId(
On 2017/01/18 23:31:26, msw wrote:
> aside: I wonder if this helper is still useful.

It's still used in a couple of places.

https://codereview.chromium.org/2642003002/diff/1/ash/mus/bridge/wm_shell_mus.cc
File ash/mus/bridge/wm_shell_mus.cc (right):

https://codereview.chromium.org/2642003002/diff/1/ash/mus/bridge/wm_shell_mus...
ash/mus/bridge/wm_shell_mus.cc:261:
window_manager_->screen()->SetWorkAreaInsets(window->aura_window(), insets);
On 2017/01/18 23:31:26, msw wrote:
> q: Should this use Screen::GetScreen(), if a test called SetScreenInstance
with
> a different value than the window manager's screen? (or DCHECK_EQ?) (also okay
> as-is, I see you have a check in ScreenMus::SetWorkAreaInsets)

During shutdown we swap out the screen, so I can't DCHECK.

https://codereview.chromium.org/2642003002/diff/1/ash/mus/screen_mus.h
File ash/mus/screen_mus.h (right):

https://codereview.chromium.org/2642003002/diff/1/ash/mus/screen_mus.h#newcode12
ash/mus/screen_mus.h:12: class Window;
On 2017/01/18 23:31:27, msw wrote:
> nit: is this needed? (already used for GetDisplayNearestWindow)

Done.

https://codereview.chromium.org/2642003002/diff/1/ash/mus/screen_mus.h#newcode34
ash/mus/screen_mus.h:34: // Sets the work area of the display |window| is on.
On 2017/01/18 23:31:27, msw wrote:
> optional grammar nit: "of the display containing |window|."

Done.

https://codereview.chromium.org/2642003002/diff/1/ash/mus/test/wm_test_base.cc
File ash/mus/test/wm_test_base.cc (right):

https://codereview.chromium.org/2642003002/diff/1/ash/mus/test/wm_test_base.c...
ash/mus/test/wm_test_base.cc:96: return
roots[0]->GetWindow()->GetDisplayNearestWindow();
On 2017/01/18 23:31:27, msw wrote:
> q: does it matter if we call GetWindow vs. GetRootWindow here (and below)?

They are the same thing, with different result values:
GetWindow() returns WmWindow
GetRootWindow() returns aura::Window
I have a TODO in RootWindowController to clean that up. As long as we have
WmWindow, the two return values are useful.

https://codereview.chromium.org/2642003002/diff/1/ash/mus/top_level_window_fa...
File ash/mus/top_level_window_factory.cc (right):

https://codereview.chromium.org/2642003002/diff/1/ash/mus/top_level_window_fa...
ash/mus/top_level_window_factory.cc:14: #include "ash/mus/screen_mus.h"
On 2017/01/18 23:31:27, msw wrote:
> nit: needed?

Indeed it isn't. Removed.

https://codereview.chromium.org/2642003002/diff/1/ash/mus/top_level_window_fa...
ash/mus/top_level_window_factory.cc:48: const int64_t display_id =
GetInitialDisplayId(*properties);
On 2017/01/18 23:31:27, msw wrote:
> nit: skip the loop for display::kInvalidDisplayId?

Done.

https://codereview.chromium.org/2642003002/diff/1/ash/mus/top_level_window_fa...
ash/mus/top_level_window_factory.cc:77: gfx::Rect bounds(0, 0,
root_size.width(), root_size.height());
On 2017/01/18 23:31:27, msw wrote:
> nit: use Rect(const Size& size) ctor

Done.

https://codereview.chromium.org/2642003002/diff/1/ash/mus/top_level_window_fa...
ash/mus/top_level_window_factory.cc:80:
root_window_controller->GetWindow()->GetDisplayNearestWindow();
On 2017/01/18 23:31:27, msw wrote:
> ditto q: GetWindow/GetRootWindow, optionally cache root used above.

See earlier comment.

https://codereview.chromium.org/2642003002/diff/1/ash/mus/top_level_window_fa...
ash/mus/top_level_window_factory.cc:81:
bounds.Offset(display.bounds().origin().x(),
On 2017/01/18 23:31:27, msw wrote:
> nit: bounds.Offset(display.bounds()OffsetFromOrigin());

Done.

https://codereview.chromium.org/2642003002/diff/1/ash/mus/top_level_window_fa...
ash/mus/top_level_window_factory.cc:88: int width, height;
On 2017/01/18 23:31:27, msw wrote:
> nit: explicit 0 init

Done.

https://codereview.chromium.org/2642003002/diff/1/ash/mus/top_level_window_fa...
ash/mus/top_level_window_factory.cc:93: width = std::max(0,
std::min(max_size.width(), pref.width()));
On 2017/01/18 23:31:27, msw wrote:
> optional nit: re-use |pref| with Size::SetToMax and SetToMin

I think you mean SetToMin.

https://codereview.chromium.org/2642003002/diff/1/ash/mus/top_level_window_fa...
ash/mus/top_level_window_factory.cc:96: width = root_size.width() - 240;
On 2017/01/18 23:31:27, msw wrote:
> optional nit: use a const for this magic number, ditto below

Done.

https://codereview.chromium.org/2642003002/diff/1/ash/mus/top_level_window_fa...
ash/mus/top_level_window_factory.cc:101: const int window_count = 0;
On 2017/01/18 23:31:27, msw wrote:
> q: I realize this has a TODO, but did you mean to make this static, or really
> always just use 0 for now?

I changed to static, and I removed a bunch of this with an improved comment.

https://codereview.chromium.org/2642003002/diff/1/ash/mus/top_level_window_fa...
ash/mus/top_level_window_factory.cc:102: return gfx::Rect(40 + (window_count %
4) * 40, 40 + (window_count % 4) * 40,
On 2017/01/18 23:31:27, msw wrote:
> optional nit: cache |40 + (window_count % 4) * 40|

No longer applicable.

https://codereview.chromium.org/2642003002/diff/1/ash/mus/top_level_window_fa...
ash/mus/top_level_window_factory.cc:119: container_window =
root_window_controller->GetWindow()
On 2017/01/18 23:31:27, msw wrote:
> aside: GetWindow==GetRootWindow is bugging me, try to avoid GetWindow per your
> function comment

Sorry. Some operations are easier on WmWindow at the moment, so I use which one
the surrounding code is generally using.

https://codereview.chromium.org/2642003002/diff/1/ash/mus/top_level_window_fa...
ash/mus/top_level_window_factory.cc:123: context =
root_window_controller->GetRootWindow();
On 2017/01/18 23:31:27, msw wrote:
> aside: it might be nice to avoid context and pick the appropriate/default
> container here as needed, but that might change NonClientFrameController's
> behavior

We should probably try to make ash::wm::GetDefaultParent work in this case.
That's trickier though.

https://codereview.chromium.org/2642003002/diff/1/ash/mus/top_level_window_fa...
ash/mus/top_level_window_factory.cc:129: const bool provide_non_client_frame =
On 2017/01/18 23:31:27, msw wrote:
> optional nit: inline

I went with this as it better documents the conditional (IMO).

https://codereview.chromium.org/2642003002/diff/1/ash/mus/top_level_window_fa...
ash/mus/top_level_window_factory.cc:184:
ui::mojom::WindowManager::kWindowIgnoredByShelf_Property)) {
On 2017/01/18 23:31:27, msw wrote:
> aside: itd be nice if this were an aura window prop

Agreed. I filed 671729 a while back on it.

https://codereview.chromium.org/2642003002/diff/1/ash/mus/top_level_window_fa...
File ash/mus/top_level_window_factory.h (right):

https://codereview.chromium.org/2642003002/diff/1/ash/mus/top_level_window_fa...
ash/mus/top_level_window_factory.h:29: // Creates and parent a new top-level
window and returns it. The returned
On 2017/01/18 23:31:27, msw wrote:
> nit: parents

Done.

https://codereview.chromium.org/2642003002/diff/1/ash/mus/top_level_window_fa...
ash/mus/top_level_window_factory.h:30: // aura::Window is owned by it's parent.
On 2017/01/18 23:31:27, msw wrote:
> nit: "its"

Done.

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

https://codereview.chromium.org/2642003002/diff/1/ash/mus/window_manager.cc#n...
ash/mus/window_manager.cc:204: new RootWindowController(nullptr,
window_tree_host.release()));
On 2017/01/18 23:31:27, msw wrote:
> nit: MakeUnique

The constructor is private and WindowManager is a friend, so MakeUnique won't
work in this case.

[sky, 2017-01-19 01:02:35]

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

[commit-bot: I haz the power, 2017-01-19 01:03:16]

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

[msw, 2017-01-19 01:20:33]

lgtm with a nit

https://codereview.chromium.org/2642003002/diff/1/ash/mus/test/wm_test_base.cc
File ash/mus/test/wm_test_base.cc (right):

https://codereview.chromium.org/2642003002/diff/1/ash/mus/test/wm_test_base.c...
ash/mus/test/wm_test_base.cc:96: return
roots[0]->GetWindow()->GetDisplayNearestWindow();
On 2017/01/19 01:02:19, sky wrote:
> On 2017/01/18 23:31:27, msw wrote:
> > q: does it matter if we call GetWindow vs. GetRootWindow here (and below)?
> 
> They are the same thing, with different result values:
> GetWindow() returns WmWindow
> GetRootWindow() returns aura::Window
> I have a TODO in RootWindowController to clean that up. As long as we have
> WmWindow, the two return values are useful.

Ah, I didn't notice the WmWindow/aura::Window return types. Gotcha

https://codereview.chromium.org/2642003002/diff/20001/ash/mus/top_level_windo...
File ash/mus/top_level_window_factory.cc (right):

https://codereview.chromium.org/2642003002/diff/20001/ash/mus/top_level_windo...
ash/mus/top_level_window_factory.cc:92: window_size.SetToMin(root_size);
nit: also do SetToMax(gfx::Size()) to also ensure dimensions are >=0?
(maybe do that after the conditionals to handle the else case too?)

[commit-bot: I haz the power, 2017-01-19 03:34:50]

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

[commit-bot: I haz the power, 2017-01-19 03:34:51]

Dry run: This issue passed the CQ dry run.

[sky, 2017-01-19 15:50:45]

https://codereview.chromium.org/2642003002/diff/20001/ash/mus/top_level_windo...
File ash/mus/top_level_window_factory.cc (right):

https://codereview.chromium.org/2642003002/diff/20001/ash/mus/top_level_windo...
ash/mus/top_level_window_factory.cc:92: window_size.SetToMin(root_size);
On 2017/01/19 01:20:33, msw (out 1-19) wrote:
> nit: also do SetToMax(gfx::Size()) to also ensure dimensions are >=0?
> (maybe do that after the conditionals to handle the else case too?)

gfx::Size forces width/height to be >= 0.

[sky, 2017-01-19 15:50:54]

The CQ bit was checked by sky@chromium.org

[commit-bot: I haz the power, 2017-01-19 15:51:08]

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-01-19 15:55:39]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1484841054616690,
"parent_rev": "a1193bdc94a5163af5cd0a71bf438c44fa08fd16", "commit_rev":
"8a8ffd21bdd7e290cbe4587b64471d13e7bb40c3"}

[commit-bot: I haz the power, 2017-01-19 15:56:08]

Description was changed from

==========
Removes ash::mus::RootWindowController

The functionality is moved into two places:
. Maintaining the display goes to ScreenMus.
. Creating the window moves to top_level_window_factory.

As part of this I'm removing some functionality that staggered top
level windows. I suspect this isn't used much and needs some
changes. Will address this later.

BUG=671246
TEST=none
R=msw@chromium.org
==========

to

==========
Removes ash::mus::RootWindowController

The functionality is moved into two places:
. Maintaining the display goes to ScreenMus.
. Creating the window moves to top_level_window_factory.

As part of this I'm removing some functionality that staggered top
level windows. I suspect this isn't used much and needs some
changes. Will address this later.

BUG=671246
TEST=none
R=msw@chromium.org

Review-Url: https://codereview.chromium.org/2642003002
Cr-Commit-Position: refs/heads/master@{#444745}
Committed:
https://chromium.googlesource.com/chromium/src/+/8a8ffd21bdd7e290cbe4587b6447...
==========

[commit-bot: I haz the power, 2017-01-19 15:56:09]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/8a8ffd21bdd7e290cbe4587b6447...