ash: Remove ash::Shelf in favor of ash::WmShelf

ash: Remove ash::Shelf in favor of ash::WmShelf

WmShelf exists for the lifetime of the RootWindowController, which makes it
easier to reason about. This also eliminates a wrapper layer in the shelf code.

* Move creation of ShelfView into WmShelf
* Move OnShelfCreated and OnShelfDestroyed notifications to WmShelf
* Rewrite ShelfWidgetTest.ShelfInitiallySizedAfterLogin to test both displays
* Fix IWYU for ShelfWidget
* Move FocusShelf and LaunchApp from AcceleratorControllerDelegateAura to the
common AcceleratorController, as they only use //ash/common

This does not yet change the lifetime or ownership of ShelfWidget, so there is
still some oddness around shelf "creation", which really means ShelfView
creation.

Pure refactor, no behavior changes.

BUG=615502
TEST=ash_unittests, unit_tests, manual tests of login with and without a
secondary display.

Committed: https://crrev.com/1cad77e9db74d3826578d2f51b7474d86a765f40
Cr-Commit-Position: refs/heads/master@{#415504}

[James Cook, 2016-08-30 20:10:30]

Description was changed from

==========
ash: Remove ash::Shelf in favor of ash::WmShelf

WmShelf exists for the lifetime of the RootWindowController, which makes it
easier to reason about. This also eliminates a wrapper layer in the shelf code.

* Move FocusShelf and LaunchApp to AcceleratorController
* Fix IWYU for ShelfWidget
* Move OnShelfCreated and OnShelfDestroyed notifications to WmShelf
* Move creation of ShelfView into WmShelf

This does not yet change the lifetime or ownership of ShelfWidget, so there is
still some oddness around shelf "creation", which really means ShelfView
creation.

Pure refactor, no behavior changes.

BUG=615502
TEST=ash_unittests, unit_tests, manual tests of login with and without a
secondary display.
==========

to

==========
ash: Remove ash::Shelf in favor of ash::WmShelf

WmShelf exists for the lifetime of the RootWindowController, which makes it
easier to reason about. This also eliminates a wrapper layer in the shelf code.

* Move creation of ShelfView into WmShelf
* Move OnShelfCreated and OnShelfDestroyed notifications to WmShelf
* Fix IWYU for ShelfWidget
* Move FocusShelf and LaunchApp from AcceleratorControllerDelegateAura to the
common AcceleratorController, as they only use //ash/common

This does not yet change the lifetime or ownership of ShelfWidget, so there is
still some oddness around shelf "creation", which really means ShelfView
creation.

Pure refactor, no behavior changes.

BUG=615502
TEST=ash_unittests, unit_tests, manual tests of login with and without a
secondary display.
==========

[James Cook, 2016-08-30 20:13:31]

Description was changed from

==========
ash: Remove ash::Shelf in favor of ash::WmShelf

WmShelf exists for the lifetime of the RootWindowController, which makes it
easier to reason about. This also eliminates a wrapper layer in the shelf code.

* Move creation of ShelfView into WmShelf
* Move OnShelfCreated and OnShelfDestroyed notifications to WmShelf
* Fix IWYU for ShelfWidget
* Move FocusShelf and LaunchApp from AcceleratorControllerDelegateAura to the
common AcceleratorController, as they only use //ash/common

This does not yet change the lifetime or ownership of ShelfWidget, so there is
still some oddness around shelf "creation", which really means ShelfView
creation.

Pure refactor, no behavior changes.

BUG=615502
TEST=ash_unittests, unit_tests, manual tests of login with and without a
secondary display.
==========

to

==========
ash: Remove ash::Shelf in favor of ash::WmShelf

WmShelf exists for the lifetime of the RootWindowController, which makes it
easier to reason about. This also eliminates a wrapper layer in the shelf code.

* Move creation of ShelfView into WmShelf
* Move OnShelfCreated and OnShelfDestroyed notifications to WmShelf
* Rewrite ShelfWidgetTest.ShelfInitiallySizedAfterLogin to test both displays
* Fix IWYU for ShelfWidget
* Move FocusShelf and LaunchApp from AcceleratorControllerDelegateAura to the
common AcceleratorController, as they only use //ash/common

This does not yet change the lifetime or ownership of ShelfWidget, so there is
still some oddness around shelf "creation", which really means ShelfView
creation.

Pure refactor, no behavior changes.

BUG=615502
TEST=ash_unittests, unit_tests, manual tests of login with and without a
secondary display.
==========

[James Cook, 2016-08-30 20:21:15]

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

[James Cook, 2016-08-30 20:21:48]

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

[James Cook, 2016-08-30 20:21:49]

msw, please take a look.

[commit-bot: I haz the power, 2016-08-30 20:22:11]

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-30 20:26:31]

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

[commit-bot: I haz the power, 2016-08-30 20:26:32]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[James Cook, 2016-08-30 20:27:40]

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

[commit-bot: I haz the power, 2016-08-30 20:28:15]

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-30 20:42:31]

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

[commit-bot: I haz the power, 2016-08-30 20:42:32]

Dry run: Exceeded global retry quota

[msw, 2016-08-30 22:02:07]

lgtm with nits. Goodbye Shelf; woo hoo!

https://codereview.chromium.org/2293183002/diff/40001/ash/common/shelf/wm_she...
File ash/common/shelf/wm_shelf.h (right):

https://codereview.chromium.org/2293183002/diff/40001/ash/common/shelf/wm_she...
ash/common/shelf/wm_shelf.h:51: // TODO(jamescook): Make this create the shelf
widget instead.
nit: I'm not sure about this TODO; it might make sense to just create the
ShelfWidget and ShelfView in the ctor and keep them invisible as we discussed.
Perhaps you've thought more about this, but I'm not sure delaying widget
creation makes sense.

https://codereview.chromium.org/2293183002/diff/40001/ash/common/shelf/wm_she...
ash/common/shelf/wm_shelf.h:52: void CreateShelf();
nit: "InitializeShelf" or similar?

https://codereview.chromium.org/2293183002/diff/40001/ash/common/shelf/wm_she...
ash/common/shelf/wm_shelf.h:53: void DestroyShelf();
nit: "ShutdownShelf" or similar?

https://codereview.chromium.org/2293183002/diff/40001/ash/common/shelf/wm_she...
ash/common/shelf/wm_shelf.h:56: bool IsShelfCreated() const;
optional nit: "IsShelfInitialized" or similar?

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

https://codereview.chromium.org/2293183002/diff/40001/ash/root_window_control...
ash/root_window_controller.cc:487: // Notify shell observers that the shelf has
been created.
nit: I wonder if we can order this and PostCreateShelf before the layout manager
stuff immediately above; then we could move both of these statements into
WmShelf::CreateShelf (it kinda makes sense to call those there). Okay to leave
for later; but nice if we can do it now.

https://codereview.chromium.org/2293183002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/accessibility/accessibility_manager.cc (right):

https://codereview.chromium.org/2293183002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/accessibility/accessibility_manager.cc:1528: if
(!shelf->IsShelfCreated())
nit: maybe skip this and just let the shelf layout manager check suffice.

https://codereview.chromium.org/2293183002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/accessibility/chromevox_panel.cc (right):

https://codereview.chromium.org/2293183002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/accessibility/chromevox_panel.cc:121: if
(!shelf->IsShelfCreated())
nit: maybe skip this and just let the shelf layout manager check suffice.

https://codereview.chromium.org/2293183002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/first_run/steps/tray_step.cc (right):

https://codereview.chromium.org/2293183002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/first_run/steps/tray_step.cc:32:
ash::WmShelf::ForWindow(ash::WmShell::Get()->GetPrimaryRootWindow())
aside: maybe we should do this elsewhere instead of
ash::WmShell::Get()->GetPrimaryRootWindow()->GetRootWindowController()->GetShelf();
or maybe we *should* add WmShelf::GetPrimaryShelf despite my earlier hesitation.

https://codereview.chromium.org/2293183002/diff/40001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc (right):

https://codereview.chromium.org/2293183002/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:1328: //
TODO(jamescook): This check should not be necessary, but otherwise this
nit: duplicate comment above; or should we no-op in WmShelf instead?

[James Cook, 2016-08-30 23:00:57]

Thanks for the quick review.

https://codereview.chromium.org/2293183002/diff/40001/ash/common/shelf/wm_she...
File ash/common/shelf/wm_shelf.h (right):

https://codereview.chromium.org/2293183002/diff/40001/ash/common/shelf/wm_she...
ash/common/shelf/wm_shelf.h:51: // TODO(jamescook): Make this create the shelf
widget instead.
On 2016/08/30 22:02:07, msw wrote:
> nit: I'm not sure about this TODO; it might make sense to just create the
> ShelfWidget and ShelfView in the ctor and keep them invisible as we discussed.
> Perhaps you've thought more about this, but I'm not sure delaying widget
> creation makes sense.

Removed the comment. I'm still not sure about the right thing to do here. If we
create the ShelfView with WmShelf we'll be creating it at the login screen
before the user profile is loaded, and we'll be running code in
ChromeLauncherControllerImpl that expects the user to be logged in. We could
work around that, but it might be easier to go the other way and defer creation
of the ShelfWidget. I'm not sure yet, so I removed the comment.

https://codereview.chromium.org/2293183002/diff/40001/ash/common/shelf/wm_she...
ash/common/shelf/wm_shelf.h:52: void CreateShelf();
On 2016/08/30 22:02:07, msw wrote:
> nit: "InitializeShelf" or similar?

Done.

https://codereview.chromium.org/2293183002/diff/40001/ash/common/shelf/wm_she...
ash/common/shelf/wm_shelf.h:53: void DestroyShelf();
On 2016/08/30 22:02:07, msw wrote:
> nit: "ShutdownShelf" or similar?

Done.

https://codereview.chromium.org/2293183002/diff/40001/ash/common/shelf/wm_she...
ash/common/shelf/wm_shelf.h:56: bool IsShelfCreated() const;
On 2016/08/30 22:02:07, msw wrote:
> optional nit: "IsShelfInitialized" or similar?

Done.

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

https://codereview.chromium.org/2293183002/diff/40001/ash/root_window_control...
ash/root_window_controller.cc:487: // Notify shell observers that the shelf has
been created.
On 2016/08/30 22:02:07, msw wrote:
> nit: I wonder if we can order this and PostCreateShelf before the layout
manager
> stuff immediately above; then we could move both of these statements into
> WmShelf::CreateShelf (it kinda makes sense to call those there). Okay to leave
> for later; but nice if we can do it now.

I'm going to do this in a follow-up CL. It turns out that
NotifyShelfCreatedForRootWindow() calls into AttachedPanelWindowTargeter which
assumes that PanelLayoutManager has a WmShelf set. This is fixable, but it's a
bit more involved than I want to include in this CL. Added TODO.

https://codereview.chromium.org/2293183002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/accessibility/accessibility_manager.cc (right):

https://codereview.chromium.org/2293183002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/accessibility/accessibility_manager.cc:1528: if
(!shelf->IsShelfCreated())
On 2016/08/30 22:02:07, msw wrote:
> nit: maybe skip this and just let the shelf layout manager check suffice.

I'd prefer to keep this as-is. Otherwise there are additional calls into
SetChromevoxPanelHeight() when you turn Chromevox on and off at the login
screen, since the ShelfLayoutManager exists at that point. While it seems to
work in my manual testing, it's hard for me to be certain that it's safe to
change.

https://codereview.chromium.org/2293183002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/accessibility/chromevox_panel.cc (right):

https://codereview.chromium.org/2293183002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/accessibility/chromevox_panel.cc:121: if
(!shelf->IsShelfCreated())
On 2016/08/30 22:02:07, msw wrote:
> nit: maybe skip this and just let the shelf layout manager check suffice.

ditto

https://codereview.chromium.org/2293183002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/first_run/steps/tray_step.cc (right):

https://codereview.chromium.org/2293183002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/first_run/steps/tray_step.cc:32:
ash::WmShelf::ForWindow(ash::WmShell::Get()->GetPrimaryRootWindow())
On 2016/08/30 22:02:07, msw wrote:
> aside: maybe we should do this elsewhere instead of
>
ash::WmShell::Get()->GetPrimaryRootWindow()->GetRootWindowController()->GetShelf();
> or maybe we *should* add WmShelf::GetPrimaryShelf despite my earlier
hesitation.

I cleaned up the other ones to call WmShelf::ForWindow(). I don't want to add
WmShelf::GetPrimaryShelf() -- people should have to work at least a little bit
harder before they do something evil. :-)

https://codereview.chromium.org/2293183002/diff/40001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc (right):

https://codereview.chromium.org/2293183002/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:1328: //
TODO(jamescook): This check should not be necessary, but otherwise this
On 2016/08/30 22:02:07, msw wrote:
> nit: duplicate comment above; or should we no-op in WmShelf instead?

I duplicated the comment. (I think this code is subtly wrong, and I don't want
other callers to try to set alignment and have it be an unexpected no-op.)

[James Cook, 2016-08-30 23:01:10]

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

[commit-bot: I haz the power, 2016-08-30 23:01:53]

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

[James Cook, 2016-08-30 23:02:27]

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

[James Cook, 2016-08-30 23:02:28]

xiyuan, can I get OWNERS for chrome/browser/chromeos/* ?

[commit-bot: I haz the power, 2016-08-30 23:06:02]

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

[commit-bot: I haz the power, 2016-08-30 23:06:03]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[James Cook, 2016-08-30 23:12:26]

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

[commit-bot: I haz the power, 2016-08-30 23:13:04]

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

[xiyuan, 2016-08-30 23:14:31]

chrome/browser/chromeos LGTM

https://codereview.chromium.org/2293183002/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/accessibility/accessibility_manager.cc (right):

https://codereview.chromium.org/2293183002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/accessibility/accessibility_manager.cc:1528: if
(!shelf->IsShelfInitialized())
I assume that shelf would never bu nullptr.

[James Cook, 2016-08-30 23:18:33]

Thanks for the review.

https://codereview.chromium.org/2293183002/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/accessibility/accessibility_manager.cc (right):

https://codereview.chromium.org/2293183002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/accessibility/accessibility_manager.cc:1528: if
(!shelf->IsShelfInitialized())
On 2016/08/30 23:14:31, xiyuan wrote:
> I assume that shelf would never bu nullptr.

Yes, WmShelf* always exists as long as there is a RootWindowController. (In that
way it's better than Shelf*, which used to be null sometimes.)

[James Cook, 2016-08-30 23:18:39]

The CQ bit was unchecked by jamescook@chromium.org

[James Cook, 2016-08-30 23:18:45]

The CQ bit was checked by jamescook@chromium.org

[James Cook, 2016-08-30 23:18:45]

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/2293183002/#ps80001
(title: "rebase again")

[commit-bot: I haz the power, 2016-08-30 23:19:19]

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-31 00:03:00]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-08-31 00:05:06]

Description was changed from

==========
ash: Remove ash::Shelf in favor of ash::WmShelf

WmShelf exists for the lifetime of the RootWindowController, which makes it
easier to reason about. This also eliminates a wrapper layer in the shelf code.

* Move creation of ShelfView into WmShelf
* Move OnShelfCreated and OnShelfDestroyed notifications to WmShelf
* Rewrite ShelfWidgetTest.ShelfInitiallySizedAfterLogin to test both displays
* Fix IWYU for ShelfWidget
* Move FocusShelf and LaunchApp from AcceleratorControllerDelegateAura to the
common AcceleratorController, as they only use //ash/common

This does not yet change the lifetime or ownership of ShelfWidget, so there is
still some oddness around shelf "creation", which really means ShelfView
creation.

Pure refactor, no behavior changes.

BUG=615502
TEST=ash_unittests, unit_tests, manual tests of login with and without a
secondary display.
==========

to

==========
ash: Remove ash::Shelf in favor of ash::WmShelf

WmShelf exists for the lifetime of the RootWindowController, which makes it
easier to reason about. This also eliminates a wrapper layer in the shelf code.

* Move creation of ShelfView into WmShelf
* Move OnShelfCreated and OnShelfDestroyed notifications to WmShelf
* Rewrite ShelfWidgetTest.ShelfInitiallySizedAfterLogin to test both displays
* Fix IWYU for ShelfWidget
* Move FocusShelf and LaunchApp from AcceleratorControllerDelegateAura to the
common AcceleratorController, as they only use //ash/common

This does not yet change the lifetime or ownership of ShelfWidget, so there is
still some oddness around shelf "creation", which really means ShelfView
creation.

Pure refactor, no behavior changes.

BUG=615502
TEST=ash_unittests, unit_tests, manual tests of login with and without a
secondary display.

Committed: https://crrev.com/1cad77e9db74d3826578d2f51b7474d86a765f40
Cr-Commit-Position: refs/heads/master@{#415504}
==========

[commit-bot: I haz the power, 2016-08-31 00:05:08]

Patchset 5 (id:??) landed as
https://crrev.com/1cad77e9db74d3826578d2f51b7474d86a765f40
Cr-Commit-Position: refs/heads/master@{#415504}