Initial cut of ash/wm/common classes for mus

Initial cut of ash/wm/common classes for mus

Tons of NOTIMPLEMENTS, but the basics are there. This is not wired to
anything yet.

BUG=603369
TEST=none
R=jamescook@chromium.org

Committed: https://crrev.com/99526c41079e7de62a26a9c2cb9619a78f4df60a
Cr-Commit-Position: refs/heads/master@{#392210}

[James Cook, 2016-05-06 19:46:47]

Questions and nits.

https://codereview.chromium.org/1954933002/diff/60001/mash/wm/bridge/mus_layo...
File mash/wm/bridge/mus_layout_manager_adapter.cc (right):

https://codereview.chromium.org/1954933002/diff/60001/mash/wm/bridge/mus_layo...
mash/wm/bridge/mus_layout_manager_adapter.cc:64:
layout_manager_->OnWindowResized();
Is this the place where the SetChildBounds() call mentioned in the class comment
happens? If so, maybe worth mentioning that?

https://codereview.chromium.org/1954933002/diff/60001/mash/wm/bridge/mus_layo...
File mash/wm/bridge/mus_layout_manager_adapter.h (right):

https://codereview.chromium.org/1954933002/diff/60001/mash/wm/bridge/mus_layo...
mash/wm/bridge/mus_layout_manager_adapter.h:34: explicit
MusLayoutManagerAdapter(
nit: no explicit

https://codereview.chromium.org/1954933002/diff/60001/mash/wm/bridge/wm_globa...
File mash/wm/bridge/wm_globals_mus.cc (right):

https://codereview.chromium.org/1954933002/diff/60001/mash/wm/bridge/wm_globa...
mash/wm/bridge/wm_globals_mus.cc:44:
root_window_controllers_.erase(std::find(root_window_controllers_.begin(),
optional: Maybe DCHECK that controller is in the container?

https://codereview.chromium.org/1954933002/diff/60001/mash/wm/bridge/wm_globa...
mash/wm/bridge/wm_globals_mus.cc:53: return nullptr;
Does this match the function comment? Or is this handling the case where
|window| is itself a container?

https://codereview.chromium.org/1954933002/diff/60001/mash/wm/bridge/wm_globa...
File mash/wm/bridge/wm_globals_mus.h (right):

https://codereview.chromium.org/1954933002/diff/60001/mash/wm/bridge/wm_globa...
mash/wm/bridge/wm_globals_mus.h:40: // toplevel. |window| may be null.
What if window itself is toplevel? Returns null?

https://codereview.chromium.org/1954933002/diff/60001/mash/wm/bridge/wm_root_...
File mash/wm/bridge/wm_root_window_controller_mus.cc (right):

https://codereview.chromium.org/1954933002/diff/60001/mash/wm/bridge/wm_root_...
mash/wm/bridge/wm_root_window_controller_mus.cc:28:
WmRootWindowControllerMus::WmRootWindowControllerMus(
JAMES - ownership, who creates?

https://codereview.chromium.org/1954933002/diff/60001/mash/wm/bridge/wm_root_...
mash/wm/bridge/wm_root_window_controller_mus.cc:109:
root_window_controller_->root()->connection()->NewWindow(nullptr);
optional: Maybe just NewWindow() so the reader doesn't worry about what the
nullptr is?

Also, just to double-check, does the window need its visibility set / to be
shown?

https://codereview.chromium.org/1954933002/diff/60001/mash/wm/bridge/wm_windo...
File mash/wm/bridge/wm_window_mus.cc (right):

https://codereview.chromium.org/1954933002/diff/60001/mash/wm/bridge/wm_windo...
mash/wm/bridge/wm_window_mus.cc:211: gfx::Point result(point);
nit: Either result = point here, or result(point) above

https://codereview.chromium.org/1954933002/diff/60001/mash/wm/bridge/wm_windo...
mash/wm/bridge/wm_window_mus.cc:459: // TODO(sky): this isn't quite right.
Should key off CanFocus(), which is not
Can this use Widget::CanActivate()?  Or do you need it for windows that aren't
widgets?

https://codereview.chromium.org/1954933002/diff/60001/mash/wm/window_layout.cc
File mash/wm/window_layout.cc (right):

https://codereview.chromium.org/1954933002/diff/60001/mash/wm/window_layout.c...
mash/wm/window_layout.cc:28: // Maximized/fullscreen/presentation windows should
be sized to the bounds
nit: presentation doesn't exist now

https://codereview.chromium.org/1954933002/diff/60001/mash/wm/window_layout.c...
mash/wm/window_layout.cc:45: case mus::mojom::ShowState::NORMAL: {
nit: order these either alphabetically, or in the order in the enum definition

https://codereview.chromium.org/1954933002/diff/60001/mash/wm/window_layout.c...
mash/wm/window_layout.cc:50: CenterWindow(window, preferred_size);
Do we want to center docked windows? Or will they always have user set bounds?
Maybe fit to container?

[James Cook, 2016-05-06 19:47:46]

https://codereview.chromium.org/1954933002/diff/60001/mash/wm/bridge/wm_root_...
File mash/wm/bridge/wm_root_window_controller_mus.cc (right):

https://codereview.chromium.org/1954933002/diff/60001/mash/wm/bridge/wm_root_...
mash/wm/bridge/wm_root_window_controller_mus.cc:28:
WmRootWindowControllerMus::WmRootWindowControllerMus(
On 2016/05/06 19:46:46, James Cook wrote:
> JAMES - ownership, who creates?

Whoops, didn't mean to leave this in. Strike it from the record!

[sky, 2016-05-06 22:12:57]

https://codereview.chromium.org/1954933002/diff/60001/mash/wm/bridge/mus_layo...
File mash/wm/bridge/mus_layout_manager_adapter.cc (right):

https://codereview.chromium.org/1954933002/diff/60001/mash/wm/bridge/mus_layo...
mash/wm/bridge/mus_layout_manager_adapter.cc:64:
layout_manager_->OnWindowResized();
On 2016/05/06 19:46:46, James Cook wrote:
> Is this the place where the SetChildBounds() call mentioned in the class
comment
> happens? If so, maybe worth mentioning that?

This call is different. This call is to tell the layoutmanager the window the
layoutmanager is associated with has been resized. In other words, |window|
isn't a child.

https://codereview.chromium.org/1954933002/diff/60001/mash/wm/bridge/mus_layo...
File mash/wm/bridge/mus_layout_manager_adapter.h (right):

https://codereview.chromium.org/1954933002/diff/60001/mash/wm/bridge/mus_layo...
mash/wm/bridge/mus_layout_manager_adapter.h:34: explicit
MusLayoutManagerAdapter(
On 2016/05/06 19:46:46, James Cook wrote:
> nit: no explicit

Done.

https://codereview.chromium.org/1954933002/diff/60001/mash/wm/bridge/wm_globa...
File mash/wm/bridge/wm_globals_mus.cc (right):

https://codereview.chromium.org/1954933002/diff/60001/mash/wm/bridge/wm_globa...
mash/wm/bridge/wm_globals_mus.cc:44:
root_window_controllers_.erase(std::find(root_window_controllers_.begin(),
On 2016/05/06 19:46:46, James Cook wrote:
> optional: Maybe DCHECK that controller is in the container?

Done.

https://codereview.chromium.org/1954933002/diff/60001/mash/wm/bridge/wm_globa...
mash/wm/bridge/wm_globals_mus.cc:53: return nullptr;
On 2016/05/06 19:46:46, James Cook wrote:
> Does this match the function comment? Or is this handling the case where
> |window| is itself a container?

Good catch, updated.

https://codereview.chromium.org/1954933002/diff/60001/mash/wm/bridge/wm_globa...
File mash/wm/bridge/wm_globals_mus.h (right):

https://codereview.chromium.org/1954933002/diff/60001/mash/wm/bridge/wm_globa...
mash/wm/bridge/wm_globals_mus.h:40: // toplevel. |window| may be null.
On 2016/05/06 19:46:46, James Cook wrote:
> What if window itself is toplevel? Returns null?

Does the '(including window)' clarify that?

https://codereview.chromium.org/1954933002/diff/60001/mash/wm/bridge/wm_root_...
File mash/wm/bridge/wm_root_window_controller_mus.cc (right):

https://codereview.chromium.org/1954933002/diff/60001/mash/wm/bridge/wm_root_...
mash/wm/bridge/wm_root_window_controller_mus.cc:109:
root_window_controller_->root()->connection()->NewWindow(nullptr);
On 2016/05/06 19:46:46, James Cook wrote:
> optional: Maybe just NewWindow() so the reader doesn't worry about what the
> nullptr is?

Done.

> Also, just to double-check, does the window need its visibility set / to be
> shown?

No. Widgets should be hidden initially until shown.

https://codereview.chromium.org/1954933002/diff/60001/mash/wm/bridge/wm_windo...
File mash/wm/bridge/wm_window_mus.cc (right):

https://codereview.chromium.org/1954933002/diff/60001/mash/wm/bridge/wm_windo...
mash/wm/bridge/wm_window_mus.cc:211: gfx::Point result(point);
On 2016/05/06 19:46:47, James Cook wrote:
> nit: Either result = point here, or result(point) above

Done.

https://codereview.chromium.org/1954933002/diff/60001/mash/wm/bridge/wm_windo...
mash/wm/bridge/wm_window_mus.cc:459: // TODO(sky): this isn't quite right.
Should key off CanFocus(), which is not
On 2016/05/06 19:46:47, James Cook wrote:
> Can this use Widget::CanActivate()?  Or do you need it for windows that aren't
> widgets?

I think we only need it for widgets, but I don't think it's plumbed through
correctly for NonClientFrameController.

https://codereview.chromium.org/1954933002/diff/60001/mash/wm/window_layout.cc
File mash/wm/window_layout.cc (right):

https://codereview.chromium.org/1954933002/diff/60001/mash/wm/window_layout.c...
mash/wm/window_layout.cc:28: // Maximized/fullscreen/presentation windows should
be sized to the bounds
On 2016/05/06 19:46:47, James Cook wrote:
> nit: presentation doesn't exist now

Done.

https://codereview.chromium.org/1954933002/diff/60001/mash/wm/window_layout.c...
mash/wm/window_layout.cc:45: case mus::mojom::ShowState::NORMAL: {
On 2016/05/06 19:46:47, James Cook wrote:
> nit: order these either alphabetically, or in the order in the enum definition

Done.

https://codereview.chromium.org/1954933002/diff/60001/mash/wm/window_layout.c...
mash/wm/window_layout.cc:50: CenterWindow(window, preferred_size);
On 2016/05/06 19:46:47, James Cook wrote:
> Do we want to center docked windows? Or will they always have user set bounds?
> Maybe fit to container?

This code won't end up being used for docked windows, or at least it shouldn't.

I'll likely keep this class around in the short term, but I'm a bit troubled by
having the two layout types. I may nuke this.

[James Cook, 2016-05-06 22:52:06]

LGTM

[sky, 2016-05-06 22:59:24]

The CQ bit was checked by sky@chromium.org

[sky, 2016-05-06 22:59:24]

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/1954933002/#ps100001
(title: "merge to trunk")

[commit-bot: I haz the power, 2016-05-06 22:59:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1954933002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1954933002/100001

[commit-bot: I haz the power, 2016-05-07 00:03:43]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-05-07 00:04:49]

Description was changed from

==========
Initial cut of ash/wm/common classes for mus

Tons of NOTIMPLEMENTS, but the basics are there. This is not wired to
anything yet.

BUG=603369
TEST=none
R=jamescook@chromium.org
==========

to

==========
Initial cut of ash/wm/common classes for mus

Tons of NOTIMPLEMENTS, but the basics are there. This is not wired to
anything yet.

BUG=603369
TEST=none
R=jamescook@chromium.org

Committed: https://crrev.com/99526c41079e7de62a26a9c2cb9619a78f4df60a
Cr-Commit-Position: refs/heads/master@{#392210}
==========

[commit-bot: I haz the power, 2016-05-07 00:04:51]

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