Cleanup WorkspaceController and WorkspaceLayoutManager.

ABANDONED AS PER https://codereview.chromium.org/1888153002

Cleanup WorkspaceController and WorkspaceLayoutManager.

Avoid direct calls to ShelfLayoutManager; misc cleanup.

Bail from UpdateFullscreenState if not the default container...
(this was toggling off of shelf_'s validity, but that seems odd)

BUG=NONE
TEST=No behavior changes; no regressions.
R=oshima@chromium.org

[msw, 2016-03-29 20:54:18]

Description was changed from

==========
Cleanup WorkspaceController and WorkspaceLayoutManager.

BUG=NONE
TEST=No behavior changes; no regressions.
R=oshima@chromium.org
==========

to

==========
Cleanup WorkspaceController and WorkspaceLayoutManager.

Avoid direct calls to ShelfLayoutManager; misc cleanup.

BUG=NONE
TEST=No behavior changes; no regressions.
R=oshima@chromium.org
==========

[msw, 2016-03-29 20:54:31]

Hey Oshima, please take a look; thanks!

[msw, 2016-03-29 20:55:01]

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

[commit-bot: I haz the power, 2016-03-29 20:55:48]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1841803002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1841803002/1

[commit-bot: I haz the power, 2016-03-29 21:45:52]

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

[commit-bot: I haz the power, 2016-03-29 21:45:53]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[msw, 2016-03-29 22:05:15]

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

[commit-bot: I haz the power, 2016-03-29 22:05:46]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1841803002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1841803002/20001

[commit-bot: I haz the power, 2016-03-29 22:33:49]

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

[commit-bot: I haz the power, 2016-03-29 22:33:52]

Dry run: Try jobs failed on following builders:
  linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[oshima, 2016-03-29 22:42:26]

lgtm with nits

https://codereview.chromium.org/1841803002/diff/20001/ash/wm/workspace/worksp...
File ash/wm/workspace/workspace_layout_manager.cc (right):

https://codereview.chromium.org/1841803002/diff/20001/ash/wm/workspace/worksp...
ash/wm/workspace/workspace_layout_manager.cc:54: for (auto window : windows_)
nit: auto*

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

https://codereview.chromium.org/1841803002/diff/20001/ash/wm/workspace_contro...
ash/wm/workspace_controller.cc:81: for (auto window : windows) {
nit: auto*

[msw, 2016-03-29 22:48:00]

Thanks!

https://codereview.chromium.org/1841803002/diff/20001/ash/wm/workspace/worksp...
File ash/wm/workspace/workspace_layout_manager.cc (right):

https://codereview.chromium.org/1841803002/diff/20001/ash/wm/workspace/worksp...
ash/wm/workspace/workspace_layout_manager.cc:54: for (auto window : windows_)
On 2016/03/29 22:42:25, oshima wrote:
> nit: auto*

Done here and in this file's other similar loops.

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

https://codereview.chromium.org/1841803002/diff/20001/ash/wm/workspace_contro...
ash/wm/workspace_controller.cc:81: for (auto window : windows) {
On 2016/03/29 22:42:25, oshima wrote:
> nit: auto*

Done.

[msw, 2016-03-29 22:48:04]

The CQ bit was checked by msw@chromium.org

[msw, 2016-03-29 22:48:04]

The patchset sent to the CQ was uploaded after l-g-t-m from oshima@chromium.org
Link to the patchset: https://codereview.chromium.org/1841803002/#ps40001
(title: "Address comments.")

[commit-bot: I haz the power, 2016-03-29 22:48:43]

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

[commit-bot: I haz the power, 2016-03-29 23:19:33]

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

[commit-bot: I haz the power, 2016-03-29 23:19:34]

Try jobs failed on following builders:
  linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[msw, 2016-04-06 00:12:32]

Description was changed from

==========
Cleanup WorkspaceController and WorkspaceLayoutManager.

Avoid direct calls to ShelfLayoutManager; misc cleanup.

BUG=NONE
TEST=No behavior changes; no regressions.
R=oshima@chromium.org
==========

to

==========
Cleanup WorkspaceController and WorkspaceLayoutManager.

Avoid direct calls to ShelfLayoutManager; misc cleanup.

Bail from UpdateFullscreenState if for_always_on_top_container...
(this was toggling off of shelf_'s validity, but that seems odd)

BUG=NONE
TEST=No behavior changes; no regressions.
R=oshima@chromium.org
==========

[msw, 2016-04-06 00:12:39]

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

[commit-bot: I haz the power, 2016-04-06 00:12:55]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1841803002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1841803002/60001

[msw, 2016-04-06 00:17:15]

Hey Oshima, please take another look; thanks.

I had to update the code to bail from UpdateFullscreenState if the instance was
created for the AlwaysOnTopController, in order to prevent crashes in
interactive_ui_test's AppWindowTest.* (see Patch Set 3 try runs). This function
previously bailed if |shelf_| was null, and that was never set for
AlwaysOnTopController's instance... I don't pretend to understand the actual
intended logic here, so I'm happy to take any advice or abandon this CL.

[commit-bot: I haz the power, 2016-04-06 00:27:35]

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

[commit-bot: I haz the power, 2016-04-06 00:27:36]

Dry run: Try jobs failed on following builders:
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[msw, 2016-04-06 00:39:10]

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

[commit-bot: I haz the power, 2016-04-06 00:39:43]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1841803002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1841803002/80001

[commit-bot: I haz the power, 2016-04-06 01:18:59]

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

[commit-bot: I haz the power, 2016-04-06 01:19:00]

Dry run: This issue passed the CQ dry run.

[msw, 2016-04-14 19:23:39]

Oshima, friendly ping; please take a look; thanks!
(just lmk if you want me to abandon this patch)

[msw, 2016-04-14 20:09:09]

FYI: If I omit the early return from
WorkspaceLayoutManager::UpdateFullscreenState, then we get the following crash
(partial callstack), when running a few tests, like interactive_ui_tests
--gtest_filter=AppWindowTest.InitAlwaysOnTopToFullscreen

[1597:1597:0414/130329:FATAL:window.cc(834)] Check failed: child != target
(0x2278cba4d60 vs. 0x2278cba4d60)
#0 0x7f2e8f14ea5e base::debug::StackTrace::StackTrace()
#1 0x7f2e8f1a468c logging::LogMessage::~LogMessage()
#2 0x7f2e869a09c1 aura::Window::StackChildRelativeTo()
#3 0x7f2e869a0907 aura::Window::StackChildAbove()
#4 0x7f2e86362e0e ash::wm::WindowState::DisableAlwaysOnTop()
#5 0x7f2e8637785a ash::WorkspaceLayoutManager::OnFullscreenStateChanged()
#6 0x7f2e861bd5d5 ash::Shell::NotifyFullscreenStateChange()
#7 0x7f2e86376db2 ash::WorkspaceLayoutManager::UpdateFullscreenState()
#8 0x7f2e86377e3f ash::WorkspaceLayoutManager::OnPostWindowStateTypeChange()
#9 0x7f2e86363b96 ash::wm::WindowState::NotifyPostStateTypeChange()
#10 0x7f2e862dbe76 ash::wm::DefaultState::EnterToNextState()
#11 0x7f2e862db114 ash::wm::DefaultState::OnWMEvent()
#12 0x7f2e86362c76 ash::wm::WindowState::OnWMEvent()
#13 0x7f2e86363654 ash::wm::WindowState::OnWindowPropertyChanged()
#14 0x7f2e869a31e4 aura::Window::SetPropertyInternal()
#15 0x7f2e86993fb2 aura::subtle::PropertyHelper::Set<>()
#16 0x7f2e86993693 aura::Window::SetProperty<>()
#17 0x7f2e8ffb6965 views::NativeWidgetAura::SetFullscreen()
#18 0x7f2e8ff7c7be views::Widget::SetFullscreen()
#19 0x000003a0e38d ChromeNativeAppWindowViews::SetFullscreen()
#20 0x000003964ffb ChromeNativeAppWindowViewsAuraAsh::SetFullscreen()
#21 0x000002a62956 extensions::AppWindow::SetNativeWindowFullscreen()
#22 0x000002a628fc extensions::AppWindow::SetFullscreen()
#23 0x000002a60bfe extensions::AppWindow::Fullscreen()
#24 0x0000017aa38a extensions::(anonymous
namespace)::AppWindowTest::CheckAlwaysOnTopToFullscreen()

[msw, 2016-04-14 20:55:33]

Description was changed from

==========
Cleanup WorkspaceController and WorkspaceLayoutManager.

Avoid direct calls to ShelfLayoutManager; misc cleanup.

Bail from UpdateFullscreenState if for_always_on_top_container...
(this was toggling off of shelf_'s validity, but that seems odd)

BUG=NONE
TEST=No behavior changes; no regressions.
R=oshima@chromium.org
==========

to

==========
Cleanup WorkspaceController and WorkspaceLayoutManager.

Avoid direct calls to ShelfLayoutManager; misc cleanup.

Bail from UpdateFullscreenState if not the default container...
(this was toggling off of shelf_'s validity, but that seems odd)

BUG=NONE
TEST=No behavior changes; no regressions.
R=oshima@chromium.org
==========

[msw, 2016-04-14 20:55:53]

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

[msw, 2016-04-14 20:56:20]

Please take another look; thanks!

[commit-bot: I haz the power, 2016-04-14 20:56:41]

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

[oshima, 2016-04-14 20:58:26]

lgtm

[commit-bot: I haz the power, 2016-04-14 21:26:12]

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

[commit-bot: I haz the power, 2016-04-14 21:26:13]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[msw, 2016-04-14 21:35:14]

The CQ bit was checked by msw@chromium.org

[commit-bot: I haz the power, 2016-04-14 21:35:57]

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

[commit-bot: I haz the power, 2016-04-14 21:45:29]

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

[commit-bot: I haz the power, 2016-04-14 21:45:30]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[msw, 2016-04-14 22:01:05]

Description was changed from

==========
Cleanup WorkspaceController and WorkspaceLayoutManager.

Avoid direct calls to ShelfLayoutManager; misc cleanup.

Bail from UpdateFullscreenState if not the default container...
(this was toggling off of shelf_'s validity, but that seems odd)

BUG=NONE
TEST=No behavior changes; no regressions.
R=oshima@chromium.org
==========

to

==========
ABANDONED AS PER https://codereview.chromium.org/1888153002

Cleanup WorkspaceController and WorkspaceLayoutManager.

Avoid direct calls to ShelfLayoutManager; misc cleanup.

Bail from UpdateFullscreenState if not the default container...
(this was toggling off of shelf_'s validity, but that seems odd)

BUG=NONE
TEST=No behavior changes; no regressions.
R=oshima@chromium.org
==========