chromeos: Fix shelf appearing at login screen under mash

chromeos: Fix shelf appearing at login screen under mash

Previously chrome --mash would always create the shelf on startup, even at
the login screen. Now it waits until login is complete, like classic ash.

Ash watches for SessionState::ACTIVE via a SessionStateObserver to create the
shelf, rather than NOTIFICATION_LOGIN_USER_PROFILE_PREPARED. For most login
flows this doesn't matter, but for supervised user creation it means that the
shelf is not created until the flow is completed. This is an improvement
because in the old code the shelf would be created too early and had to be
explicitly hidden.

Always create a SessionControllerClient in chrome, even in classic ash. This
allows classic ash to use the mojo pathway via ash::SessionController.

Remove WmShell::ShowShelf(), which was introduced in crrev.com/10693003 to
delay showing the shelf until post-login OOBE (like avatar picture select)
was complete. It does not seem to be needed anymore, either in production or
in tests.

Change AshTestImplMus to simulate a user logging in which is required to
create the shelf on the primary display and also to have a non-zero user
count to create the shelf on additional displays.

Remove some unnecessary OS_CHROMEOS ifdefs in //c/b/ui/ash.

BUG=666021
TEST=ash_unittests and chrome browser_tests

Committed: https://crrev.com/7f99e933e595824281102ff6737075dccbbc4d5c
Cr-Commit-Position: refs/heads/master@{#439845}

[James Cook, 2016-12-14 02:47:25]

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

[commit-bot: I haz the power, 2016-12-14 02:47: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-12-14 03:30:13]

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

[commit-bot: I haz the power, 2016-12-14 03:30:13]

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

[James Cook, 2016-12-14 22:19:49]

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

[commit-bot: I haz the power, 2016-12-14 22:20:24]

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-12-14 22:30:19]

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

[commit-bot: I haz the power, 2016-12-14 22:30:21]

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

[James Cook, 2016-12-14 23:59:54]

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

[commit-bot: I haz the power, 2016-12-15 00:00:26]

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-12-15 00:19:23]

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

[commit-bot: I haz the power, 2016-12-15 00:19:24]

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

[James Cook, 2016-12-15 00:59:04]

Description was changed from

==========
chromeos: Fix shelf appearing at login screen under mash

Previously chrome --mash would always create the shelf on startup, even at
the login screen. Now it waits until login is complete, like classic ash.

* Always create a SessionControllerClient in chrome, even in classic ash.
* Use a SessionStateObserver in ash to create the shelf

Also remove WmShell::ShowShelf(), which was introduced in crrev.com/10693003
to delay showing the shelf until post-login OOBE (like avatar picture select)
was complete. It does not seem to be needed anymore, either in production or
in tests.

BUG=666021
TEST=ash_unittests and chrome browser_tests
==========

to

==========
chromeos: Fix shelf appearing at login screen under mash

Previously chrome --mash would always create the shelf on startup, even at
the login screen. Now it waits until login is complete, like classic ash.

Ash watches for SessionState::ACTIVE via a SessionStateObserver to create the
shelf, rather than NOTIFICATION_LOGIN_USER_PROFILE_PREPARED. For most login
flows this doesn't matter, but for supervised user creation it means that the
shelf is not created until the flow is completed. This is an improvement
because in the old code the shelf would be created too early and had to be
explicitly hidden.

Always create a SessionControllerClient in chrome, even in classic ash. This
allows classic ash to use the mojo pathway via ash::SessionController.

Also remove WmShell::ShowShelf(), which was introduced in crrev.com/10693003
to delay showing the shelf until post-login OOBE (like avatar picture select)
was complete. It does not seem to be needed anymore, either in production or
in tests.

BUG=666021
TEST=ash_unittests and chrome browser_tests
==========

[James Cook, 2016-12-16 01:12:57]

Description was changed from

==========
chromeos: Fix shelf appearing at login screen under mash

Previously chrome --mash would always create the shelf on startup, even at
the login screen. Now it waits until login is complete, like classic ash.

Ash watches for SessionState::ACTIVE via a SessionStateObserver to create the
shelf, rather than NOTIFICATION_LOGIN_USER_PROFILE_PREPARED. For most login
flows this doesn't matter, but for supervised user creation it means that the
shelf is not created until the flow is completed. This is an improvement
because in the old code the shelf would be created too early and had to be
explicitly hidden.

Always create a SessionControllerClient in chrome, even in classic ash. This
allows classic ash to use the mojo pathway via ash::SessionController.

Also remove WmShell::ShowShelf(), which was introduced in crrev.com/10693003
to delay showing the shelf until post-login OOBE (like avatar picture select)
was complete. It does not seem to be needed anymore, either in production or
in tests.

BUG=666021
TEST=ash_unittests and chrome browser_tests
==========

to

==========
chromeos: Fix shelf appearing at login screen under mash

Previously chrome --mash would always create the shelf on startup, even at
the login screen. Now it waits until login is complete, like classic ash.

Ash watches for SessionState::ACTIVE via a SessionStateObserver to create the
shelf, rather than NOTIFICATION_LOGIN_USER_PROFILE_PREPARED. For most login
flows this doesn't matter, but for supervised user creation it means that the
shelf is not created until the flow is completed. This is an improvement
because in the old code the shelf would be created too early and had to be
explicitly hidden.

Always create a SessionControllerClient in chrome, even in classic ash. This
allows classic ash to use the mojo pathway via ash::SessionController.

Remove WmShell::ShowShelf(), which was introduced in crrev.com/10693003 to
delay showing the shelf until post-login OOBE (like avatar picture select)
was complete. It does not seem to be needed anymore, either in production or
in tests.

Remove some unnecessary OS_CHROMEOS ifdefs in //c/b/ui/ash.

BUG=666021
TEST=ash_unittests and chrome browser_tests
==========

[James Cook, 2016-12-16 01:14:17]

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

[commit-bot: I haz the power, 2016-12-16 01:15:05]

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-12-16 01:28:26]

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

[James Cook, 2016-12-16 01:28:27]

msw, please take a look at ash/*

xiyuan, please take a look at chrome/*

https://codereview.chromium.org/2573703003/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/session/chrome_session_manager.cc (right):

https://codereview.chromium.org/2573703003/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/login/session/chrome_session_manager.cc:141:
SessionControllerClient::FlushForTesting();
xiyuan, this seemed better than just pumping the message loop. I added a
g_instance to SessionControllerClient because there's no simple way to access it
via ChromeBrowserMainExtraPartsAsh.

Eventually ChromeBrowserMainExtraPartsAsh should become part of
ChromeBrowserMainPartsChromeOS, but that's a much bigger change than I can do in
this CL.

https://codereview.chromium.org/2573703003/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/supervised/supervised_user_creation_screen.cc
(left):

https://codereview.chromium.org/2573703003/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/login/supervised/supervised_user_creation_screen.cc:376:
->SetAlignment(ash::ShelfAlignment::SHELF_ALIGNMENT_BOTTOM_LOCKED);
This isn't needed anymore. It was added because if the parent user had their
shelf aligned to the left and they did the partial login to create a supervised
user then the system tray would move to the lower-left corner of the screen. We
don't create the shelf anymore so the system tray doesn't move.

https://codereview.chromium.org/2573703003/diff/80001/chrome/browser/ui/ash/c...
File chrome/browser/ui/ash/chrome_shell_delegate.cc (left):

https://codereview.chromium.org/2573703003/diff/80001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/chrome_shell_delegate.cc:566:
ash::Shell::GetInstance()->OnLoginUserProfilePrepared();
This is the core of the change. The shelf is created a little later, in response
to the session starting, using code that is the same in classic ash and mash.

[commit-bot: I haz the power, 2016-12-16 03:50:10]

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

[commit-bot: I haz the power, 2016-12-16 03:50:11]

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_...)

[msw, 2016-12-16 16:26:20]

https://codereview.chromium.org/2573703003/diff/80001/ash/aura/wm_shell_aura.cc
File ash/aura/wm_shell_aura.cc (right):

https://codereview.chromium.org/2573703003/diff/80001/ash/aura/wm_shell_aura....
ash/aura/wm_shell_aura.cc:250: CreateShelf();
It's a bit odd that we'll potentially call CreateShelf multiple times.

https://codereview.chromium.org/2573703003/diff/80001/ash/aura/wm_shell_aura....
ash/aura/wm_shell_aura.cc:251: Shell::GetInstance()->CreateKeyboard();
Is this necessary here? Shouldn't it be triggered when the keyboard is needed?

https://codereview.chromium.org/2573703003/diff/80001/ash/common/shelf/shelf_...
File ash/common/shelf/shelf_widget.cc (right):

https://codereview.chromium.org/2573703003/diff/80001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_widget.cc:337: // visibility code could be simplified.
http://crbug.com/674773
What happens for the lock screen? Isn't the shelf hidden then?

https://codereview.chromium.org/2573703003/diff/80001/ash/common/test/test_se...
File ash/common/test/test_session_state_delegate.cc (right):

https://codereview.chromium.org/2573703003/diff/80001/ash/common/test/test_se...
ash/common/test/test_session_state_delegate.cc:164: if (!has_active_user) {
optional nit: remove curlies or change this to a ternary statement.

https://codereview.chromium.org/2573703003/diff/80001/ash/common/wm_shell.h
File ash/common/wm_shell.h (right):

https://codereview.chromium.org/2573703003/diff/80001/ash/common/wm_shell.h#n...
ash/common/wm_shell.h:104: class ASH_EXPORT WmShell : public
SessionStateObserver {
Hmm, this is a little unfortunate; how tough would it be to compose an object
which does this? (could ShelfController itself do this?)

https://codereview.chromium.org/2573703003/diff/80001/ash/common/wm_shell.h#n...
ash/common/wm_shell.h:365: virtual void OnUserSessionStateActive() = 0;
This is a bit odd; if we do indeed keep WmShell as a SessionStateObserver, I
would prefer that the separate WmShell subclasses simply override
SessionStateChanged  and just check for ACTIVE (each subclass could inherit
SessionStateObserver, or the  override here could be protected instead of
private). Otherwise, this just seems like a less useful duplicate of the
observer function, with a slightly awkward name.

https://codereview.chromium.org/2573703003/diff/80001/ash/mus/bridge/wm_shell...
File ash/mus/bridge/wm_shell_mus.cc (right):

https://codereview.chromium.org/2573703003/diff/80001/ash/mus/bridge/wm_shell...
ash/mus/bridge/wm_shell_mus.cc:397: // Mash has a different codepath for virtual
keyboard initialization.
This comment is a bit odd, since nothing keyboard related is happening here.
Remove the comment or provide some context, like "WmShelAura initializes the
Keyboard at this stage, but Mash does so elsewhere."

https://codereview.chromium.org/2573703003/diff/80001/ash/mus/window_manager.cc
File ash/mus/window_manager.cc (left):

https://codereview.chromium.org/2573703003/diff/80001/ash/mus/window_manager....
ash/mus/window_manager.cc:141: // TODO: this should be called when logged in.
See http://crbug.com/654606.
This is a good step for now, but someday I hope that we'll merge the fake and
shelf with the actual shelf, create it earlier, show it all the time, and just
swap state/content as needed.

https://codereview.chromium.org/2573703003/diff/80001/ash/root_window_control...
File ash/root_window_controller_unittest.cc (right):

https://codereview.chromium.org/2573703003/diff/80001/ash/root_window_control...
ash/root_window_controller_unittest.cc:979:
WmShell::Get()->OnUserSessionStateActive();
This is okay, but it'd be even better if we could trigger the keyboard code
directly, unless we're testing its relation to the SessionState.

[James Cook, 2016-12-16 22:31:27]

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

[commit-bot: I haz the power, 2016-12-16 22:32: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, 2016-12-16 22:36:51]

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

[commit-bot: I haz the power, 2016-12-16 22:36:52]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[James Cook, 2016-12-19 17:43:04]

xiyuan / msw - please hold off for now. I'm dealing with some test failures due
to changes in the timing of shelf creation, and they're also revealing problems
with always creating a SessionController. I'll ping again when I'm ready.

[James Cook, 2016-12-19 17:53:42]

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

[commit-bot: I haz the power, 2016-12-19 17:54:01]

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-12-20 01:10:24]

Description was changed from

==========
chromeos: Fix shelf appearing at login screen under mash

Previously chrome --mash would always create the shelf on startup, even at
the login screen. Now it waits until login is complete, like classic ash.

Ash watches for SessionState::ACTIVE via a SessionStateObserver to create the
shelf, rather than NOTIFICATION_LOGIN_USER_PROFILE_PREPARED. For most login
flows this doesn't matter, but for supervised user creation it means that the
shelf is not created until the flow is completed. This is an improvement
because in the old code the shelf would be created too early and had to be
explicitly hidden.

Always create a SessionControllerClient in chrome, even in classic ash. This
allows classic ash to use the mojo pathway via ash::SessionController.

Remove WmShell::ShowShelf(), which was introduced in crrev.com/10693003 to
delay showing the shelf until post-login OOBE (like avatar picture select)
was complete. It does not seem to be needed anymore, either in production or
in tests.

Remove some unnecessary OS_CHROMEOS ifdefs in //c/b/ui/ash.

BUG=666021
TEST=ash_unittests and chrome browser_tests
==========

to

==========
chromeos: Fix shelf appearing at login screen under mash

Previously chrome --mash would always create the shelf on startup, even at
the login screen. Now it waits until login is complete, like classic ash.

Ash watches for SessionState::ACTIVE via a SessionStateObserver to create the
shelf, rather than NOTIFICATION_LOGIN_USER_PROFILE_PREPARED. For most login
flows this doesn't matter, but for supervised user creation it means that the
shelf is not created until the flow is completed. This is an improvement
because in the old code the shelf would be created too early and had to be
explicitly hidden.

Always create a SessionControllerClient in chrome, even in classic ash. This
allows classic ash to use the mojo pathway via ash::SessionController.

Remove WmShell::ShowShelf(), which was introduced in crrev.com/10693003 to
delay showing the shelf until post-login OOBE (like avatar picture select)
was complete. It does not seem to be needed anymore, either in production or
in tests.

Change AshTestImplMus to simulate a user logging in which is required to
create the shelf on the primary display and also to have a non-zero user
count to create the shelf on additional displays.

Remove some unnecessary OS_CHROMEOS ifdefs in //c/b/ui/ash.

BUG=666021
TEST=ash_unittests and chrome browser_tests
==========

[James Cook, 2016-12-20 01:16:04]

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

[commit-bot: I haz the power, 2016-12-20 01:16: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, 2016-12-20 02:05:21]

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

[commit-bot: I haz the power, 2016-12-20 02:05:22]

Dry run: This issue passed the CQ dry run.

[James Cook, 2016-12-20 03:06:51]

Patchset #8 (id:140001) has been deleted

[James Cook, 2016-12-20 03:07:05]

Patchset #9 (id:180001) has been deleted

[James Cook, 2016-12-20 03:44:49]

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

[commit-bot: I haz the power, 2016-12-20 03:45:07]

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-12-20 04:02:43]

msw and xiyuan, please take another look.

(And thanks, xiyuan, for all your help with test failures!)

https://codereview.chromium.org/2573703003/diff/80001/ash/aura/wm_shell_aura.cc
File ash/aura/wm_shell_aura.cc (right):

https://codereview.chromium.org/2573703003/diff/80001/ash/aura/wm_shell_aura....
ash/aura/wm_shell_aura.cc:250: CreateShelf();
On 2016/12/16 16:26:20, msw wrote:
> It's a bit odd that we'll potentially call CreateShelf multiple times.

OK, fixed that.

(I could also just rename this function to CreateShelfIfNeeded().)

https://codereview.chromium.org/2573703003/diff/80001/ash/aura/wm_shell_aura....
ash/aura/wm_shell_aura.cc:251: Shell::GetInstance()->CreateKeyboard();
On 2016/12/16 16:26:20, msw wrote:
> Is this necessary here? Shouldn't it be triggered when the keyboard is needed?

The virtual keyboard can be shown at the login screen before the shelf is shown,
but seems to destroy itself during login and needs to be recreated. It adds a
bunch of shelf-observers, so needs to be done after shelf creation. It also gets
torn down and recreated after multiprofile switch. It might be due to accessing
the active user's profile in the webui code.

Regardless, this is what the old code did and I'm afraid of breaking it.

https://codereview.chromium.org/2573703003/diff/80001/ash/common/shelf/shelf_...
File ash/common/shelf/shelf_widget.cc (right):

https://codereview.chromium.org/2573703003/diff/80001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_widget.cc:337: // visibility code could be simplified.
http://crbug.com/674773
On 2016/12/16 16:26:20, msw wrote:
> What happens for the lock screen? Isn't the shelf hidden then?

Updated comment.

The shelf is hidden at the lock screen because it's in a container that is
hidden.

When auto-hidden it is transparent.

https://codereview.chromium.org/2573703003/diff/80001/ash/common/test/test_se...
File ash/common/test/test_session_state_delegate.cc (right):

https://codereview.chromium.org/2573703003/diff/80001/ash/common/test/test_se...
ash/common/test/test_session_state_delegate.cc:164: if (!has_active_user) {
On 2016/12/16 16:26:20, msw wrote:
> optional nit: remove curlies or change this to a ternary statement.

Done.

https://codereview.chromium.org/2573703003/diff/80001/ash/common/wm_shell.h
File ash/common/wm_shell.h (right):

https://codereview.chromium.org/2573703003/diff/80001/ash/common/wm_shell.h#n...
ash/common/wm_shell.h:104: class ASH_EXPORT WmShell : public
SessionStateObserver {
On 2016/12/16 16:26:20, msw wrote:
> Hmm, this is a little unfortunate; how tough would it be to compose an object
> which does this? (could ShelfController itself do this?)

I don't have an elegant way to get rid of this.

I tried making ShelfController be a SessionStateObserver, but I don't like it.
(See next patchset for how I did it.) I have to inject the session_controller_
because WmShell isn't done initializing yet, and there's a not-very-obvious
observer order dependency between ShelfController (which must run first to
create the shelf) and WmShellAura (which has to create the keyboard).

I'm going to upload another patchset that keeps this a SessionStateObserver but
makes OnSessionStateChange() protected, like you mention below. Let me know
which approach you prefer.

https://codereview.chromium.org/2573703003/diff/80001/ash/common/wm_shell.h#n...
ash/common/wm_shell.h:365: virtual void OnUserSessionStateActive() = 0;
On 2016/12/16 16:26:20, msw wrote:
> This is a bit odd; if we do indeed keep WmShell as a SessionStateObserver, I
> would prefer that the separate WmShell subclasses simply override
> SessionStateChanged  and just check for ACTIVE (each subclass could inherit
> SessionStateObserver, or the  override here could be protected instead of
> private). Otherwise, this just seems like a less useful duplicate of the
> observer function, with a slightly awkward name.

Removed.

https://codereview.chromium.org/2573703003/diff/80001/ash/mus/bridge/wm_shell...
File ash/mus/bridge/wm_shell_mus.cc (right):

https://codereview.chromium.org/2573703003/diff/80001/ash/mus/bridge/wm_shell...
ash/mus/bridge/wm_shell_mus.cc:397: // Mash has a different codepath for virtual
keyboard initialization.
On 2016/12/16 16:26:20, msw wrote:
> This comment is a bit odd, since nothing keyboard related is happening here.
> Remove the comment or provide some context, like "WmShelAura initializes the
> Keyboard at this stage, but Mash does so elsewhere."

This codepath doesn't exist anymore.

https://codereview.chromium.org/2573703003/diff/80001/ash/mus/window_manager.cc
File ash/mus/window_manager.cc (left):

https://codereview.chromium.org/2573703003/diff/80001/ash/mus/window_manager....
ash/mus/window_manager.cc:141: // TODO: this should be called when logged in.
See http://crbug.com/654606.
On 2016/12/16 16:26:20, msw wrote:
> This is a good step for now, but someday I hope that we'll merge the fake and
> shelf with the actual shelf, create it earlier, show it all the time, and just
> swap state/content as needed.

Acknowledged.

https://codereview.chromium.org/2573703003/diff/80001/ash/root_window_control...
File ash/root_window_controller_unittest.cc (right):

https://codereview.chromium.org/2573703003/diff/80001/ash/root_window_control...
ash/root_window_controller_unittest.cc:979:
WmShell::Get()->OnUserSessionStateActive();
On 2016/12/16 16:26:20, msw wrote:
> This is okay, but it'd be even better if we could trigger the keyboard code
> directly, unless we're testing its relation to the SessionState.

Done.

[commit-bot: I haz the power, 2016-12-20 04:14:28]

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

[commit-bot: I haz the power, 2016-12-20 04:14:29]

Dry run: This issue passed the CQ dry run.

[msw, 2016-12-20 15:37:30]

One comment, otherwise, lg

https://codereview.chromium.org/2573703003/diff/80001/ash/common/wm_shell.h
File ash/common/wm_shell.h (right):

https://codereview.chromium.org/2573703003/diff/80001/ash/common/wm_shell.h#n...
ash/common/wm_shell.h:104: class ASH_EXPORT WmShell : public
SessionStateObserver {
On 2016/12/20 04:02:42, James Cook wrote:
> On 2016/12/16 16:26:20, msw wrote:
> > Hmm, this is a little unfortunate; how tough would it be to compose an
object
> > which does this? (could ShelfController itself do this?)
> 
> I don't have an elegant way to get rid of this.
> 
> I tried making ShelfController be a SessionStateObserver, but I don't like it.
> (See next patchset for how I did it.) I have to inject the session_controller_
> because WmShell isn't done initializing yet, and there's a not-very-obvious
> observer order dependency between ShelfController (which must run first to
> create the shelf) and WmShellAura (which has to create the keyboard).
> 
> I'm going to upload another patchset that keeps this a SessionStateObserver
but
> makes OnSessionStateChange() protected, like you mention below. Let me know
> which approach you prefer.

I guess it's okay for the shell to observe session state to initialize the
shelf. I'm just a little concerned about inviting the shelf to inherit other
base classes and take on responsibilities that should be encapsulated in
per-component objects, like ShelfController.

https://codereview.chromium.org/2573703003/diff/220001/ash/common/wm_shell.cc
File ash/common/wm_shell.cc (right):

https://codereview.chromium.org/2573703003/diff/220001/ash/common/wm_shell.cc...
ash/common/wm_shell.cc:422: WmShell::Get()->CreateShelf();
Isn't WmShell::Get() equivalent to |this|, and not needed?

https://codereview.chromium.org/2573703003/diff/220001/ash/common/wm_shell.cc...
ash/common/wm_shell.cc:423: first_session_active_ = true;
Should this ever be reset to false, eg. when the user logs out, or is the
WmShell object destroyed then? Otherwise, it seems like the second user to log
in won't get a shelf, right? You can go back to calling CreateShelf multiple
times if that's safe (with a comment to that effect).

[James Cook, 2016-12-20 17:27:06]

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

[James Cook, 2016-12-20 17:27:19]

msw / xiyuan, please take another look.

https://codereview.chromium.org/2573703003/diff/80001/ash/common/wm_shell.h
File ash/common/wm_shell.h (right):

https://codereview.chromium.org/2573703003/diff/80001/ash/common/wm_shell.h#n...
ash/common/wm_shell.h:104: class ASH_EXPORT WmShell : public
SessionStateObserver {
On 2016/12/20 15:37:30, msw wrote:
> On 2016/12/20 04:02:42, James Cook wrote:
> > On 2016/12/16 16:26:20, msw wrote:
> > > Hmm, this is a little unfortunate; how tough would it be to compose an
> object
> > > which does this? (could ShelfController itself do this?)
> > 
> > I don't have an elegant way to get rid of this.
> > 
> > I tried making ShelfController be a SessionStateObserver, but I don't like
it.
> > (See next patchset for how I did it.) I have to inject the
session_controller_
> > because WmShell isn't done initializing yet, and there's a not-very-obvious
> > observer order dependency between ShelfController (which must run first to
> > create the shelf) and WmShellAura (which has to create the keyboard).
> > 
> > I'm going to upload another patchset that keeps this a SessionStateObserver
> but
> > makes OnSessionStateChange() protected, like you mention below. Let me know
> > which approach you prefer.
> 
> I guess it's okay for the shell to observe session state to initialize the
> shelf. I'm just a little concerned about inviting the shelf to inherit other
> base classes and take on responsibilities that should be encapsulated in
> per-component objects, like ShelfController.

Per offline discussion, keeping as-is.

We also might need to do other things in WmShell related to session state in the
future, like replace UpdateAfterLoginStatusChange().

https://codereview.chromium.org/2573703003/diff/220001/ash/common/wm_shell.cc
File ash/common/wm_shell.cc (right):

https://codereview.chromium.org/2573703003/diff/220001/ash/common/wm_shell.cc...
ash/common/wm_shell.cc:422: WmShell::Get()->CreateShelf();
On 2016/12/20 15:37:30, msw wrote:
> Isn't WmShell::Get() equivalent to |this|, and not needed?

Whoops, done.

https://codereview.chromium.org/2573703003/diff/220001/ash/common/wm_shell.cc...
ash/common/wm_shell.cc:423: first_session_active_ = true;
On 2016/12/20 15:37:30, msw wrote:
> Should this ever be reset to false, eg. when the user logs out, or is the
> WmShell object destroyed then? Otherwise, it seems like the second user to log
> in won't get a shelf, right? You can go back to calling CreateShelf multiple
> times if that's safe (with a comment to that effect).

Changed to always attempt to create the shelf (which is safe) and added a
comment. I'm pretty sure that logout restarts the browser, but there's no harm
in calling it twice. This will also make it easier to switch to "PopulateShelf"
in the future when we always create the ShelfView.

[commit-bot: I haz the power, 2016-12-20 17:27:37]

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

[xiyuan, 2016-12-20 17:28:36]

lgtm

[msw, 2016-12-20 17:43:22]

lgtm

[James Cook, 2016-12-20 17:56:00]

The CQ bit was unchecked by jamescook@chromium.org

[James Cook, 2016-12-20 17:56:06]

The CQ bit was checked by jamescook@chromium.org

[commit-bot: I haz the power, 2016-12-20 17:56:23]

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-12-20 18:10:38]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1482256566477930,
"parent_rev": "29a9bdf07e0c104f93a7e67bc5eba7704cb971e7", "commit_rev":
"19def6560960093319a9925be48fc5be62bdbda7"}

[commit-bot: I haz the power, 2016-12-20 18:11:08]

Description was changed from

==========
chromeos: Fix shelf appearing at login screen under mash

Previously chrome --mash would always create the shelf on startup, even at
the login screen. Now it waits until login is complete, like classic ash.

Ash watches for SessionState::ACTIVE via a SessionStateObserver to create the
shelf, rather than NOTIFICATION_LOGIN_USER_PROFILE_PREPARED. For most login
flows this doesn't matter, but for supervised user creation it means that the
shelf is not created until the flow is completed. This is an improvement
because in the old code the shelf would be created too early and had to be
explicitly hidden.

Always create a SessionControllerClient in chrome, even in classic ash. This
allows classic ash to use the mojo pathway via ash::SessionController.

Remove WmShell::ShowShelf(), which was introduced in crrev.com/10693003 to
delay showing the shelf until post-login OOBE (like avatar picture select)
was complete. It does not seem to be needed anymore, either in production or
in tests.

Change AshTestImplMus to simulate a user logging in which is required to
create the shelf on the primary display and also to have a non-zero user
count to create the shelf on additional displays.

Remove some unnecessary OS_CHROMEOS ifdefs in //c/b/ui/ash.

BUG=666021
TEST=ash_unittests and chrome browser_tests
==========

to

==========
chromeos: Fix shelf appearing at login screen under mash

Previously chrome --mash would always create the shelf on startup, even at
the login screen. Now it waits until login is complete, like classic ash.

Ash watches for SessionState::ACTIVE via a SessionStateObserver to create the
shelf, rather than NOTIFICATION_LOGIN_USER_PROFILE_PREPARED. For most login
flows this doesn't matter, but for supervised user creation it means that the
shelf is not created until the flow is completed. This is an improvement
because in the old code the shelf would be created too early and had to be
explicitly hidden.

Always create a SessionControllerClient in chrome, even in classic ash. This
allows classic ash to use the mojo pathway via ash::SessionController.

Remove WmShell::ShowShelf(), which was introduced in crrev.com/10693003 to
delay showing the shelf until post-login OOBE (like avatar picture select)
was complete. It does not seem to be needed anymore, either in production or
in tests.

Change AshTestImplMus to simulate a user logging in which is required to
create the shelf on the primary display and also to have a non-zero user
count to create the shelf on additional displays.

Remove some unnecessary OS_CHROMEOS ifdefs in //c/b/ui/ash.

BUG=666021
TEST=ash_unittests and chrome browser_tests

Review-Url: https://codereview.chromium.org/2573703003
==========

[commit-bot: I haz the power, 2016-12-20 18:11:12]

Committed patchset #11 (id:240001)

[commit-bot: I haz the power, 2016-12-20 18:15:14]

Description was changed from

==========
chromeos: Fix shelf appearing at login screen under mash

Previously chrome --mash would always create the shelf on startup, even at
the login screen. Now it waits until login is complete, like classic ash.

Ash watches for SessionState::ACTIVE via a SessionStateObserver to create the
shelf, rather than NOTIFICATION_LOGIN_USER_PROFILE_PREPARED. For most login
flows this doesn't matter, but for supervised user creation it means that the
shelf is not created until the flow is completed. This is an improvement
because in the old code the shelf would be created too early and had to be
explicitly hidden.

Always create a SessionControllerClient in chrome, even in classic ash. This
allows classic ash to use the mojo pathway via ash::SessionController.

Remove WmShell::ShowShelf(), which was introduced in crrev.com/10693003 to
delay showing the shelf until post-login OOBE (like avatar picture select)
was complete. It does not seem to be needed anymore, either in production or
in tests.

Change AshTestImplMus to simulate a user logging in which is required to
create the shelf on the primary display and also to have a non-zero user
count to create the shelf on additional displays.

Remove some unnecessary OS_CHROMEOS ifdefs in //c/b/ui/ash.

BUG=666021
TEST=ash_unittests and chrome browser_tests

Review-Url: https://codereview.chromium.org/2573703003
==========

to

==========
chromeos: Fix shelf appearing at login screen under mash

Previously chrome --mash would always create the shelf on startup, even at
the login screen. Now it waits until login is complete, like classic ash.

Ash watches for SessionState::ACTIVE via a SessionStateObserver to create the
shelf, rather than NOTIFICATION_LOGIN_USER_PROFILE_PREPARED. For most login
flows this doesn't matter, but for supervised user creation it means that the
shelf is not created until the flow is completed. This is an improvement
because in the old code the shelf would be created too early and had to be
explicitly hidden.

Always create a SessionControllerClient in chrome, even in classic ash. This
allows classic ash to use the mojo pathway via ash::SessionController.

Remove WmShell::ShowShelf(), which was introduced in crrev.com/10693003 to
delay showing the shelf until post-login OOBE (like avatar picture select)
was complete. It does not seem to be needed anymore, either in production or
in tests.

Change AshTestImplMus to simulate a user logging in which is required to
create the shelf on the primary display and also to have a non-zero user
count to create the shelf on additional displays.

Remove some unnecessary OS_CHROMEOS ifdefs in //c/b/ui/ash.

BUG=666021
TEST=ash_unittests and chrome browser_tests

Committed: https://crrev.com/7f99e933e595824281102ff6737075dccbbc4d5c
Cr-Commit-Position: refs/heads/master@{#439845}
==========

[commit-bot: I haz the power, 2016-12-20 18:15:16]

Patchset 11 (id:??) landed as
https://crrev.com/7f99e933e595824281102ff6737075dccbbc4d5c
Cr-Commit-Position: refs/heads/master@{#439845}

[James Cook, 2017-01-10 00:55:08]

On 2016/12/20 18:15:16, commit-bot: I haz the power wrote:
> Patchset 11 (id:??) landed as
> https://crrev.com/7f99e933e595824281102ff6737075dccbbc4d5c
> Cr-Commit-Position: refs/heads/master@{#439845}

This CL causes crbug.com/679513 Chrome_ChromeOS: Crash Report -
ash::ShelfView::GetIdealBoundsOfItemIcon, which is the top crash in M57 dev.
It's caused by having a minimized browser window during session restore.

I'm reverting in https://codereview.chromium.org/2619943002 (there were trivial
conflicts I had to resolve).