mash: Partially migrate WorkspaceController to ash common.

mash: Partially migrate WorkspaceController to ash common.

Remove direct ShelfLayoutManager / WorkspaceController interaction.
(alleviate some shelf widget and layout manager dependencies)

Handle null WmWindowAura layout managers.
Null check WmRootWindowControllerAura::GetWorkspaceWindowState.
Move scale animation util to ash/common/wm/wm_window_animations.*
Define local kIntialAnimationDurationMS for WorkspaceController.

BUG=615155
TEST=Automated; no cros shelf or workspace regressions.
R=jamescook@chromium.org

Committed: https://crrev.com/6d9e0fd7c9a567eacc7c85bc0b57378aa237f2d3
Cr-Commit-Position: refs/heads/master@{#411547}

[msw, 2016-08-11 20:49:25]

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

[commit-bot: I haz the power, 2016-08-11 20:50:37]

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-08-11 21:16:36]

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

[commit-bot: I haz the power, 2016-08-11 21:16:39]

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

[msw, 2016-08-11 22:17:23]

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

[commit-bot: I haz the power, 2016-08-11 22:18:31]

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

[msw, 2016-08-11 22:48:28]

Description was changed from

==========
mash: Partially migrate WorkspaceController to ash common.

BUG=615155
==========

to

==========
mash: Partially migrate WorkspaceController to ash common.

Remove direct ShelfLayoutManager / WorkspaceController interaction.
(alleviate some shelf widget and layout manager dependencies)

Handle null WmWindowAura layout managers.
Null check WmRootWindowControllerAura::GetWorkspaceWindowState.
Move scale animation util to ash/common/wm/wm_window_animations.*
Define local kIntialAnimationDurationMS for WorkspaceController.

BUG=615155
TEST=Automated; no cros shelf or workspace regressions.
R=jamescook@chromium.org
==========

[msw, 2016-08-11 22:48:28]

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

[msw, 2016-08-11 22:48:44]

Hey James, please take a look; thanks!

[James Cook, 2016-08-11 23:27:36]

https://codereview.chromium.org/2243513004/diff/20001/ash/common/wm/wm_window...
File ash/common/wm/wm_window_animations.cc (right):

https://codereview.chromium.org/2243513004/diff/20001/ash/common/wm/wm_window...
ash/common/wm/wm_window_animations.cc:12: // Returns scale related to the
specified AshWindowScaleType.
Doesn't seem to apply anymore.

https://codereview.chromium.org/2243513004/diff/20001/ash/common/wm/wm_window...
ash/common/wm/wm_window_animations.cc:15: // Scales for AshWindow above/below
current workspace.
WmWindow? just window?

https://codereview.chromium.org/2243513004/diff/20001/ash/root_window_control...
File ash/root_window_controller.cc (right):

https://codereview.chromium.org/2243513004/diff/20001/ash/root_window_control...
ash/root_window_controller.cc:810: WmWindow* default_container =
Thanks for cleaning this up.

https://codereview.chromium.org/2243513004/diff/20001/ash/shelf/shelf_layout_...
File ash/shelf/shelf_layout_manager.cc (right):

https://codereview.chromium.org/2243513004/diff/20001/ash/shelf/shelf_layout_...
ash/shelf/shelf_layout_manager.cc:468: if (!shelf_window)
nit: Is this check really needed? It seems like shelf_widget_ is non-null
through the lifetime of ShelfLayoutManager. There are a few other places in this
file where we check for shelf_widget_ / shelf_window that might be unnecessary
too. If it's hard to figure out then I'm OK with leaving this here, but it would
be nice to clean up and a try run might quickly tell you that it has to be
there.

Or we could take out all the checks in a follow-up CL and see if that works.

https://codereview.chromium.org/2243513004/diff/20001/ash/shelf/shelf_widget.h
File ash/shelf/shelf_widget.h (right):

https://codereview.chromium.org/2243513004/diff/20001/ash/shelf/shelf_widget....
ash/shelf/shelf_widget.h:34: WmShelfAura* wm_shelf_aura);
so nice to have fewer dependencies

https://codereview.chromium.org/2243513004/diff/20001/ash/wm/workspace_contro...
File ash/wm/workspace_controller.cc (right):

https://codereview.chromium.org/2243513004/diff/20001/ash/wm/workspace_contro...
ash/wm/workspace_controller.cc:38: const int kIntialAnimationDurationMS = 200;
nit: Intial -> Initial

https://codereview.chromium.org/2243513004/diff/20001/ash/wm/workspace_contro...
ash/wm/workspace_controller.cc:61: if
(!viewport_->GetRootWindowController()->HasShelf())
Q: Does this one have to go second? The previous code returned DEFAULT if there
was no shelf but was a fullscreen window. (Not sure if that can happen in
practice, maybe in OOBE/setup.)

https://codereview.chromium.org/2243513004/diff/20001/ash/wm/workspace_contro...
ash/wm/workspace_controller.cc:77: if ((window_state &&
window_state->ignored_by_shelf()) ||
Kinda weird that window_state can be null. The aura version of GetWindowState()
says it always creates one, seems odd we don't do that on mus.

https://codereview.chromium.org/2243513004/diff/20001/ash/wm/workspace_contro...
ash/wm/workspace_controller.cc:98: viewport_->GetLayer()->SetOpacity(0.0f);
For 7 calls, I would create a local for the Layer*.

[msw, 2016-08-12 00:17:15]

Please take another look; thanks!

https://codereview.chromium.org/2243513004/diff/20001/ash/common/wm/wm_window...
File ash/common/wm/wm_window_animations.cc (right):

https://codereview.chromium.org/2243513004/diff/20001/ash/common/wm/wm_window...
ash/common/wm/wm_window_animations.cc:12: // Returns scale related to the
specified AshWindowScaleType.
On 2016/08/11 23:27:35, James Cook wrote:
> Doesn't seem to apply anymore.

Done.

https://codereview.chromium.org/2243513004/diff/20001/ash/common/wm/wm_window...
ash/common/wm/wm_window_animations.cc:15: // Scales for AshWindow above/below
current workspace.
On 2016/08/11 23:27:35, James Cook wrote:
> WmWindow? just window?

Done.

https://codereview.chromium.org/2243513004/diff/20001/ash/root_window_control...
File ash/root_window_controller.cc (right):

https://codereview.chromium.org/2243513004/diff/20001/ash/root_window_control...
ash/root_window_controller.cc:810: WmWindow* default_container =
On 2016/08/11 23:27:35, James Cook wrote:
> Thanks for cleaning this up.

Acknowledged.

https://codereview.chromium.org/2243513004/diff/20001/ash/shelf/shelf_layout_...
File ash/shelf/shelf_layout_manager.cc (right):

https://codereview.chromium.org/2243513004/diff/20001/ash/shelf/shelf_layout_...
ash/shelf/shelf_layout_manager.cc:468: if (!shelf_window)
On 2016/08/11 23:27:35, James Cook wrote:
> nit: Is this check really needed? It seems like shelf_widget_ is non-null
> through the lifetime of ShelfLayoutManager. There are a few other places in
this
> file where we check for shelf_widget_ / shelf_window that might be unnecessary
> too. If it's hard to figure out then I'm OK with leaving this here, but it
would
> be nice to clean up and a try run might quickly tell you that it has to be
> there.
> 
> Or we could take out all the checks in a follow-up CL and see if that works.

Not needed here or above for ash_unittests. A dry-run will check more.

https://codereview.chromium.org/2243513004/diff/20001/ash/shelf/shelf_widget.h
File ash/shelf/shelf_widget.h (right):

https://codereview.chromium.org/2243513004/diff/20001/ash/shelf/shelf_widget....
ash/shelf/shelf_widget.h:34: WmShelfAura* wm_shelf_aura);
On 2016/08/11 23:27:35, James Cook wrote:
> so nice to have fewer dependencies

Acknowledged.

https://codereview.chromium.org/2243513004/diff/20001/ash/wm/workspace_contro...
File ash/wm/workspace_controller.cc (right):

https://codereview.chromium.org/2243513004/diff/20001/ash/wm/workspace_contro...
ash/wm/workspace_controller.cc:38: const int kIntialAnimationDurationMS = 200;
On 2016/08/11 23:27:36, James Cook wrote:
> nit: Intial -> Initial

Done.

https://codereview.chromium.org/2243513004/diff/20001/ash/wm/workspace_contro...
ash/wm/workspace_controller.cc:61: if
(!viewport_->GetRootWindowController()->HasShelf())
On 2016/08/11 23:27:36, James Cook wrote:
> Q: Does this one have to go second? The previous code returned DEFAULT if
there
> was no shelf but was a fullscreen window. (Not sure if that can happen in
> practice, maybe in OOBE/setup.)

Restored order.

https://codereview.chromium.org/2243513004/diff/20001/ash/wm/workspace_contro...
ash/wm/workspace_controller.cc:77: if ((window_state &&
window_state->ignored_by_shelf()) ||
On 2016/08/11 23:27:36, James Cook wrote:
> Kinda weird that window_state can be null. The aura version of
GetWindowState()
> says it always creates one, seems odd we don't do that on mus.

This check isn't needed in the latest patch set; removed.

https://codereview.chromium.org/2243513004/diff/20001/ash/wm/workspace_contro...
ash/wm/workspace_controller.cc:98: viewport_->GetLayer()->SetOpacity(0.0f);
On 2016/08/11 23:27:36, James Cook wrote:
> For 7 calls, I would create a local for the Layer*.

Done.

[msw, 2016-08-12 00:17:24]

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

[commit-bot: I haz the power, 2016-08-12 00:18:56]

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-08-12 01:05:08]

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

[commit-bot: I haz the power, 2016-08-12 01:05:09]

Dry run: This issue passed the CQ dry run.

[James Cook, 2016-08-12 01:52:50]

LGTM

https://codereview.chromium.org/2243513004/diff/40001/ash/shelf/shelf_layout_...
File ash/shelf/shelf_layout_manager.cc (right):

https://codereview.chromium.org/2243513004/diff/40001/ash/shelf/shelf_layout_...
ash/shelf/shelf_layout_manager.cc:229: WmRootWindowController* controller =
shelf_window->GetRootWindowController();
optional: I liked the old name better, |controller| is a bit generic. Up to you.
Maybe this makes the line wrapping better.

https://codereview.chromium.org/2243513004/diff/40001/ash/shelf/shelf_layout_...
ash/shelf/shelf_layout_manager.cc:859: if (visibility_state != SHELF_AUTO_HIDE
|| !shelf_widget_ ||
optional: Maybe not needed?

https://codereview.chromium.org/2243513004/diff/40001/ash/shelf/shelf_layout_...
ash/shelf/shelf_layout_manager.cc:962: if (!window || !shelf_widget_)
optional: I also wondered if this one was needed.

https://codereview.chromium.org/2243513004/diff/40001/ash/shelf/shelf_layout_...
ash/shelf/shelf_layout_manager.cc:1110: if (shelf_widget_) {
optional: This one too.

[msw, 2016-08-12 04:01:57]

https://codereview.chromium.org/2243513004/diff/40001/ash/shelf/shelf_layout_...
File ash/shelf/shelf_layout_manager.cc (right):

https://codereview.chromium.org/2243513004/diff/40001/ash/shelf/shelf_layout_...
ash/shelf/shelf_layout_manager.cc:229: WmRootWindowController* controller =
shelf_window->GetRootWindowController();
On 2016/08/12 01:52:50, James Cook wrote:
> optional: I liked the old name better, |controller| is a bit generic. Up to
you.
> Maybe this makes the line wrapping better.

Acknowledged; I'll leave it as-is.

https://codereview.chromium.org/2243513004/diff/40001/ash/shelf/shelf_layout_...
ash/shelf/shelf_layout_manager.cc:859: if (visibility_state != SHELF_AUTO_HIDE
|| !shelf_widget_ ||
On 2016/08/12 01:52:49, James Cook wrote:
> optional: Maybe not needed?

We can remove these separately.

https://codereview.chromium.org/2243513004/diff/40001/ash/shelf/shelf_layout_...
ash/shelf/shelf_layout_manager.cc:962: if (!window || !shelf_widget_)
On 2016/08/12 01:52:49, James Cook wrote:
> optional: I also wondered if this one was needed.

We can remove these separately.

https://codereview.chromium.org/2243513004/diff/40001/ash/shelf/shelf_layout_...
ash/shelf/shelf_layout_manager.cc:1110: if (shelf_widget_) {
On 2016/08/12 01:52:50, James Cook wrote:
> optional: This one too.

We can remove these separately.

[msw, 2016-08-12 04:02:06]

The CQ bit was checked by msw@chromium.org

[commit-bot: I haz the power, 2016-08-12 04:02:28]

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-08-12 04:06:20]

Description was changed from

==========
mash: Partially migrate WorkspaceController to ash common.

Remove direct ShelfLayoutManager / WorkspaceController interaction.
(alleviate some shelf widget and layout manager dependencies)

Handle null WmWindowAura layout managers.
Null check WmRootWindowControllerAura::GetWorkspaceWindowState.
Move scale animation util to ash/common/wm/wm_window_animations.*
Define local kIntialAnimationDurationMS for WorkspaceController.

BUG=615155
TEST=Automated; no cros shelf or workspace regressions.
R=jamescook@chromium.org
==========

to

==========
mash: Partially migrate WorkspaceController to ash common.

Remove direct ShelfLayoutManager / WorkspaceController interaction.
(alleviate some shelf widget and layout manager dependencies)

Handle null WmWindowAura layout managers.
Null check WmRootWindowControllerAura::GetWorkspaceWindowState.
Move scale animation util to ash/common/wm/wm_window_animations.*
Define local kIntialAnimationDurationMS for WorkspaceController.

BUG=615155
TEST=Automated; no cros shelf or workspace regressions.
R=jamescook@chromium.org
==========

[commit-bot: I haz the power, 2016-08-12 04:06:23]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-08-12 04:08:36]

Description was changed from

==========
mash: Partially migrate WorkspaceController to ash common.

Remove direct ShelfLayoutManager / WorkspaceController interaction.
(alleviate some shelf widget and layout manager dependencies)

Handle null WmWindowAura layout managers.
Null check WmRootWindowControllerAura::GetWorkspaceWindowState.
Move scale animation util to ash/common/wm/wm_window_animations.*
Define local kIntialAnimationDurationMS for WorkspaceController.

BUG=615155
TEST=Automated; no cros shelf or workspace regressions.
R=jamescook@chromium.org
==========

to

==========
mash: Partially migrate WorkspaceController to ash common.

Remove direct ShelfLayoutManager / WorkspaceController interaction.
(alleviate some shelf widget and layout manager dependencies)

Handle null WmWindowAura layout managers.
Null check WmRootWindowControllerAura::GetWorkspaceWindowState.
Move scale animation util to ash/common/wm/wm_window_animations.*
Define local kIntialAnimationDurationMS for WorkspaceController.

BUG=615155
TEST=Automated; no cros shelf or workspace regressions.
R=jamescook@chromium.org

Committed: https://crrev.com/6d9e0fd7c9a567eacc7c85bc0b57378aa237f2d3
Cr-Commit-Position: refs/heads/master@{#411547}
==========

[commit-bot: I haz the power, 2016-08-12 04:08:37]

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