Properly set ShelfAlignment during login

Properly set ShelfAlignment during login

ShelfLayoutManager has three methods from which the ShelfAlignment can be
applied. For supressed user sessions the shelf is forced to the bottom, however
this was only addressing locked screens, and adding new users.

When the first login occurs a different state transition takes place, where the
SessionState becomes active while the user session is still supressed.
ShelfLayoutManager was not applying alignment changes made during this time.

Update ShelfLayoutManager to unify the logic for overriding the user set
ShelfAlignment. Update this logic to account for having to apply alignment
during the initial login.

Manual testing was also done for: logging in; locking screen; unlocking screen;
adding a new user; switching users.

TEST=ShelfLayoutManagerTest.SideAlignmentInteractionWithLockScreen, ShelfLayoutManagerTest.SideAlignmentInteractionWithAddUserScreen, ShelfLayoutManagerTest.SideAlignmentInteractionWithLoginScreen
BUG=456199
Committed: https://crrev.com/0a30be1d88f00f510af35144e708bbe797e706d8
Cr-Commit-Position: refs/heads/master@{#316673}

[jonross, 2015-02-10 21:47:18]

jonross@chromium.org changed reviewers:
+ tdanderson@chromium.org

[jonross, 2015-02-10 21:47:19]

Hi,

Could you take a first look at this review?

Thanks,
Jon

[tdanderson, 2015-02-11 20:32:36]

Hey Jon, the logic looks good but I have made a few suggestions below to improve
readability.

https://codereview.chromium.org/909293002/diff/1/ash/shelf/shelf_layout_manag...
File ash/shelf/shelf_layout_manager.cc (right):

https://codereview.chromium.org/909293002/diff/1/ash/shelf/shelf_layout_manag...
ash/shelf/shelf_layout_manager.cc:244: bool
ShelfLayoutManager::SetAlignment(ShelfAlignment alignment) {
Does the documentation for SetAlignment() in the header file require any
updating?

https://codereview.chromium.org/909293002/diff/1/ash/shelf/shelf_layout_manag...
ash/shelf/shelf_layout_manager.cc:1162: bool
ShelfLayoutManager::ShouldOverrideAlignment() const {
I suggest re-naming this method to something of the form Is...() to make it
clear exactly what its return value means just from the method name itself.

https://codereview.chromium.org/909293002/diff/1/ash/shelf/shelf_layout_manag...
ash/shelf/shelf_layout_manager.cc:1168: // overriding alignment during the
transition.
I love that you're adding documentation to the code, but I don't think there
should be any inside of this method. Instead I'd move this elsewhere:

* Move the documentation for what the method does to the header.
* Move the documentation for why you're calling the method to the individual
call sites.

https://codereview.chromium.org/909293002/diff/1/ash/shelf/shelf_layout_manag...
ash/shelf/shelf_layout_manager.cc:1169: return
((Shell::GetInstance()->session_state_delegate()->GetSessionState() !=
I find this compound expression really difficult to read. I suggest doing one of
two things:

(1) Break up into two or more early returns.
(2) Keep it as is but use local variables to specify the clauses.

My preference is for (1).

https://codereview.chromium.org/909293002/diff/1/ash/shelf/shelf_layout_manag...
File ash/shelf/shelf_layout_manager.h (right):

https://codereview.chromium.org/909293002/diff/1/ash/shelf/shelf_layout_manag...
ash/shelf/shelf_layout_manager.h:1: // Copyright 2012 The Chromium Authors. All
rights reserved.
Nit: wrap the lines of your CL description to 80 characters. This makes it
easier for others to read when looking at logs.

https://codereview.chromium.org/909293002/diff/1/ash/test/test_session_state_...
File ash/test/test_session_state_delegate.h (right):

https://codereview.chromium.org/909293002/diff/1/ash/test/test_session_state_...
ash/test/test_session_state_delegate.h:86: void SetSessionState(SessionState
session_state);
The preference for a simple mutator such as this would be to name as void
set_session_state(...) and put the definition in the header file.

https://codereview.chromium.org/909293002/diff/1/ash/test/test_session_state_...
ash/test/test_session_state_delegate.h:118: // The current state of the login
screen. |session_state_| becomes true before
Specify what you mean by "becomes true" since |session_state_| is an enum.

[jonross, 2015-02-11 22:17:45]

https://codereview.chromium.org/909293002/diff/1/ash/shelf/shelf_layout_manag...
File ash/shelf/shelf_layout_manager.cc (right):

https://codereview.chromium.org/909293002/diff/1/ash/shelf/shelf_layout_manag...
ash/shelf/shelf_layout_manager.cc:244: bool
ShelfLayoutManager::SetAlignment(ShelfAlignment alignment) {
On 2015/02/11 20:32:35, tdanderson wrote:
> Does the documentation for SetAlignment() in the header file require any
> updating?

Nope, the docs already say that the setting can be delayed for locked
alignments.

https://codereview.chromium.org/909293002/diff/1/ash/shelf/shelf_layout_manag...
ash/shelf/shelf_layout_manager.cc:1162: bool
ShelfLayoutManager::ShouldOverrideAlignment() const {
On 2015/02/11 20:32:35, tdanderson wrote:
> I suggest re-naming this method to something of the form Is...() to make it
> clear exactly what its return value means just from the method name itself.

Done.

https://codereview.chromium.org/909293002/diff/1/ash/shelf/shelf_layout_manag...
ash/shelf/shelf_layout_manager.cc:1168: // overriding alignment during the
transition.
On 2015/02/11 20:32:35, tdanderson wrote:
> I love that you're adding documentation to the code, but I don't think there
> should be any inside of this method. Instead I'd move this elsewhere:
> 
> * Move the documentation for what the method does to the header.
> * Move the documentation for why you're calling the method to the individual
> call sites.

Done.

https://codereview.chromium.org/909293002/diff/1/ash/shelf/shelf_layout_manag...
ash/shelf/shelf_layout_manager.cc:1169: return
((Shell::GetInstance()->session_state_delegate()->GetSessionState() !=
On 2015/02/11 20:32:35, tdanderson wrote:
> I find this compound expression really difficult to read. I suggest doing one
of
> two things:
> 
> (1) Break up into two or more early returns.
> (2) Keep it as is but use local variables to specify the clauses.
> 
> My preference is for (1).

Done.

https://codereview.chromium.org/909293002/diff/1/ash/shelf/shelf_layout_manag...
File ash/shelf/shelf_layout_manager.h (right):

https://codereview.chromium.org/909293002/diff/1/ash/shelf/shelf_layout_manag...
ash/shelf/shelf_layout_manager.h:1: // Copyright 2012 The Chromium Authors. All
rights reserved.
On 2015/02/11 20:32:35, tdanderson wrote:
> Nit: wrap the lines of your CL description to 80 characters. This makes it
> easier for others to read when looking at logs.

Done.

https://codereview.chromium.org/909293002/diff/1/ash/test/test_session_state_...
File ash/test/test_session_state_delegate.h (right):

https://codereview.chromium.org/909293002/diff/1/ash/test/test_session_state_...
ash/test/test_session_state_delegate.h:86: void SetSessionState(SessionState
session_state);
On 2015/02/11 20:32:35, tdanderson wrote:
> The preference for a simple mutator such as this would be to name as void
> set_session_state(...) and put the definition in the header file.

Done.

https://codereview.chromium.org/909293002/diff/1/ash/test/test_session_state_...
ash/test/test_session_state_delegate.h:118: // The current state of the login
screen. |session_state_| becomes true before
On 2015/02/11 20:32:35, tdanderson wrote:
> Specify what you mean by "becomes true" since |session_state_| is an enum.

Done.

[tdanderson, 2015-02-11 22:59:53]

lgtm with two more nits

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

https://codereview.chromium.org/909293002/diff/20001/ash/shelf/shelf_layout_m...
ash/shelf/shelf_layout_manager.cc:1174: return true;
nit: seems reasonable to combine these last two using ||.

https://codereview.chromium.org/909293002/diff/20001/ash/test/test_session_st...
File ash/test/test_session_state_delegate.h (right):

https://codereview.chromium.org/909293002/diff/20001/ash/test/test_session_st...
ash/test/test_session_state_delegate.h:84: // Updates the internal state that
indicates the state of the session. Does
nit: comment unnecessary here, and this can move to the block at line 25.

[jonross, 2015-02-12 15:32:58]

New patchsets have been uploaded after l-g-t-m from tdanderson@chromium.org

[jonross, 2015-02-12 15:36:58]

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

https://codereview.chromium.org/909293002/diff/20001/ash/shelf/shelf_layout_m...
ash/shelf/shelf_layout_manager.cc:1174: return true;
On 2015/02/11 22:59:53, tdanderson wrote:
> nit: seems reasonable to combine these last two using ||.

Done.

https://codereview.chromium.org/909293002/diff/20001/ash/test/test_session_st...
File ash/test/test_session_state_delegate.h (right):

https://codereview.chromium.org/909293002/diff/20001/ash/test/test_session_st...
ash/test/test_session_state_delegate.h:84: // Updates the internal state that
indicates the state of the session. Does
On 2015/02/11 22:59:53, tdanderson wrote:
> nit: comment unnecessary here, and this can move to the block at line 25.

Done.

[jonross, 2015-02-12 15:37:26]

jonross@chromium.org changed reviewers:
+ skuhne@chromium.org

[jonross, 2015-02-12 15:37:26]

skuhne@chromium.org: Please review changes in 

ash/shelf/

This change fixes a layout issue for the shelf that occurs during login.

[jonross, 2015-02-12 15:38:28]

jonross@chromium.org changed reviewers:
+ oshima@chromium.org

[jonross, 2015-02-12 15:38:29]

oshima@chromium.org: Please review changes in 

ash/test/

I've updated the TestSessionStateDelegate to track the session state, so that I
can right a unit test that depends on the state change.

[oshima, 2015-02-12 19:07:58]

https://codereview.chromium.org/909293002/diff/60001/ash/test/test_session_st...
File ash/test/test_session_state_delegate.cc (right):

https://codereview.chromium.org/909293002/diff/60001/ash/test/test_session_st...
ash/test/test_session_state_delegate.cc:67: active_user_session_started_(false),
Is it possible to eliminate this flag now?

[Mr4D (OOO till 08-26), 2015-02-12 22:04:01]

lgtm

https://codereview.chromium.org/909293002/diff/60001/ash/shelf/shelf_layout_m...
File ash/shelf/shelf_layout_manager.cc (right):

https://codereview.chromium.org/909293002/diff/60001/ash/shelf/shelf_layout_m...
ash/shelf/shelf_layout_manager.cc:1167: if
(session_state_delegate->GetSessionState() ==
Could you add a comment towards why _ACTIVE needs to be pre-filtered?

It should not be _ACTIVE with "is_Adding_user_screen" being true. And if I
remember correctly, IsUserSessionBlocked should be false in that case.

[jonross, 2015-02-12 22:21:06]

https://codereview.chromium.org/909293002/diff/60001/ash/shelf/shelf_layout_m...
File ash/shelf/shelf_layout_manager.cc (right):

https://codereview.chromium.org/909293002/diff/60001/ash/shelf/shelf_layout_m...
ash/shelf/shelf_layout_manager.cc:1167: if
(session_state_delegate->GetSessionState() ==
On 2015/02/12 22:04:01, Mr4D wrote:
> Could you add a comment towards why _ACTIVE needs to be pre-filtered?
> 
> It should not be _ACTIVE with "is_Adding_user_screen" being true. And if I
> remember correctly, IsUserSessionBlocked should be false in that case.

I will update this with comments. Though I don't believe that _ACTIVE &&
is_adding_user_screen is possible. As the state is tied directly to
_LOGIN_SECONDARY. (see line 1144)

We can have _ACTIVE true, and IsUserSessionBlocked() true. This occurs when a
user first logs in, as it considers a user session blocked until ui/browser has
completed its setup.

[jonross, 2015-02-13 15:15:47]

New patchsets have been uploaded after l-g-t-m from skuhne@chromium.org

[jonross, 2015-02-13 15:29:08]

https://codereview.chromium.org/909293002/diff/60001/ash/shelf/shelf_layout_m...
File ash/shelf/shelf_layout_manager.cc (right):

https://codereview.chromium.org/909293002/diff/60001/ash/shelf/shelf_layout_m...
ash/shelf/shelf_layout_manager.cc:1167: if
(session_state_delegate->GetSessionState() ==
On 2015/02/12 22:21:06, jonross wrote:
> On 2015/02/12 22:04:01, Mr4D wrote:
> > Could you add a comment towards why _ACTIVE needs to be pre-filtered?
> > 
> > It should not be _ACTIVE with "is_Adding_user_screen" being true. And if I
> > remember correctly, IsUserSessionBlocked should be false in that case.
> 
> I will update this with comments. Though I don't believe that _ACTIVE &&
> is_adding_user_screen is possible. As the state is tied directly to
> _LOGIN_SECONDARY. (see line 1144)
> 
> We can have _ACTIVE true, and IsUserSessionBlocked() true. This occurs when a
> user first logs in, as it considers a user session blocked until ui/browser
has
> completed its setup.

Done.

https://codereview.chromium.org/909293002/diff/60001/ash/test/test_session_st...
File ash/test/test_session_state_delegate.cc (right):

https://codereview.chromium.org/909293002/diff/60001/ash/test/test_session_st...
ash/test/test_session_state_delegate.cc:67: active_user_session_started_(false),
On 2015/02/12 19:07:58, oshima wrote:
> Is it possible to eliminate this flag now?

It is not. The SessionStateDelegate can be in a state where SESSION_STATE_ACTIVE
is set, and active_user_session_started_ is false.

In the actual system this represents the transition from no one logged in, to
user logged in. Where SessionState represents the state of the login screen, and
IsActiveUserSessionStarted() is waiting for the entire UI to be ready to
display.

[oshima, 2015-02-13 21:45:44]

https://codereview.chromium.org/909293002/diff/60001/ash/test/test_session_st...
File ash/test/test_session_state_delegate.cc (right):

https://codereview.chromium.org/909293002/diff/60001/ash/test/test_session_st...
ash/test/test_session_state_delegate.cc:67: active_user_session_started_(false),
On 2015/02/13 15:29:08, jonross wrote:
> On 2015/02/12 19:07:58, oshima wrote:
> > Is it possible to eliminate this flag now?
> 
> It is not. The SessionStateDelegate can be in a state where
SESSION_STATE_ACTIVE
> is set, and active_user_session_started_ is false.

Can you point me the code path? I couldn't find such case here.

> 
> In the actual system this represents the transition from no one logged in, to
> user logged in. Where SessionState represents the state of the login screen,
and
> IsActiveUserSessionStarted() is waiting for the entire UI to be ready to
> display.

Hmm, this sounds design flaw because SessionState should represent the session's
state,
not UI state.

If the duration of such state is 0ms in the test, I'd like to eliminate dup
flag.

[jonross, 2015-02-17 14:11:29]

On 2015/02/13 21:45:44, oshima wrote:
>
https://codereview.chromium.org/909293002/diff/60001/ash/test/test_session_st...
> File ash/test/test_session_state_delegate.cc (right):
> 
>
https://codereview.chromium.org/909293002/diff/60001/ash/test/test_session_st...
> ash/test/test_session_state_delegate.cc:67:
active_user_session_started_(false),
> On 2015/02/13 15:29:08, jonross wrote:
> > On 2015/02/12 19:07:58, oshima wrote:
> > > Is it possible to eliminate this flag now?
> > 
> > It is not. The SessionStateDelegate can be in a state where
> SESSION_STATE_ACTIVE
> > is set, and active_user_session_started_ is false.
> 
> Can you point me the code path? I couldn't find such case here.
> 
> > 
> > In the actual system this represents the transition from no one logged in,
to
> > user logged in. Where SessionState represents the state of the login screen,
> and
> > IsActiveUserSessionStarted() is waiting for the entire UI to be ready to
> > display.
> 
> Hmm, this sounds design flaw because SessionState should represent the
session's
> state,
> not UI state.
> 
> If the duration of such state is 0ms in the test, I'd like to eliminate dup
> flag.

Should we address this in SessionStateDelegateChromeOS? As this state lasts more
than 0ms during runtime.

For the unittest ShelfLayoutManager is executing during this state.

[jonross, 2015-02-17 19:17:12]

On 2015/02/17 14:11:29, jonross wrote:
> On 2015/02/13 21:45:44, oshima wrote:
> >
>
https://codereview.chromium.org/909293002/diff/60001/ash/test/test_session_st...
> > File ash/test/test_session_state_delegate.cc (right):
> > 
> >
>
https://codereview.chromium.org/909293002/diff/60001/ash/test/test_session_st...
> > ash/test/test_session_state_delegate.cc:67:
> active_user_session_started_(false),
> > On 2015/02/13 15:29:08, jonross wrote:
> > > On 2015/02/12 19:07:58, oshima wrote:
> > > > Is it possible to eliminate this flag now?
> > > 
> > > It is not. The SessionStateDelegate can be in a state where
> > SESSION_STATE_ACTIVE
> > > is set, and active_user_session_started_ is false.
> > 
> > Can you point me the code path? I couldn't find such case here.
> > 
> > > 
> > > In the actual system this represents the transition from no one logged in,
> to
> > > user logged in. Where SessionState represents the state of the login
screen,
> > and
> > > IsActiveUserSessionStarted() is waiting for the entire UI to be ready to
> > > display.
> > 
> > Hmm, this sounds design flaw because SessionState should represent the
> session's
> > state,
> > not UI state.
> > 
> > If the duration of such state is 0ms in the test, I'd like to eliminate dup
> > flag.
> 
> Should we address this in SessionStateDelegateChromeOS? As this state lasts
more
> than 0ms during runtime.
> 
> For the unittest ShelfLayoutManager is executing during this state.

crbug.com/459287 filed to record the issue with active_user_session_started_

[jonross, 2015-02-17 19:19:03]

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

[jonross, 2015-02-17 19:19:03]

Hi James,

Would you be able to provide an owners review for ash/test/* while Oshime is
OOO?

I have filed a bug to track his concern with TestSessionStateDelegate, and plan
to follow this change with a rewrite of TestSessionStateDelegate to properly
track states.

[James Cook, 2015-02-17 20:03:36]

LGTM but please CC me on the bug to clean up TestSessionStateDelegate.

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

https://codereview.chromium.org/909293002/diff/100001/ash/shelf/shelf_layout_...
ash/shelf/shelf_layout_manager.cc:1163: SessionStateDelegate*
session_state_delegate =
nit: move this lower down, closer to where it is used

https://codereview.chromium.org/909293002/diff/100001/ash/test/ash_test_base.h
File ash/test/ash_test_base.h (right):

https://codereview.chromium.org/909293002/diff/100001/ash/test/ash_test_base....
ash/test/ash_test_base.h:120: void SetSessionStarting();
Please document how this relates to SetSessionStarted().

[jonross, 2015-02-17 21:18:00]

The CQ bit was checked by jonross@chromium.org

[commit-bot: I haz the power, 2015-02-17 21:19:15]

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

[commit-bot: I haz the power, 2015-02-17 22:25:32]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-02-17 22:26:24]

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