Notify about major session changes events.

Notify about major session changes events.

This makes sure that multi-profile user adding UI gets 100% of working area and
also works fine with onscreen keyboard.

BUG=361157
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=263611

[Mr4D (OOO till 08-26), 2014-04-10 01:33:08]

Only some nits. lgtm.

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

https://codereview.chromium.org/231123002/diff/10001/ash/shelf/shelf_layout_m...
ash/shelf/shelf_layout_manager.cc:745: gfx::Insets insets;
By creating the insets they should be initialized to 0. As such line 749-756
could be simplified to:

if (!session_state_delegate->IsUserSessionBlocked() ||
    !keyboard_bounds_.IsEmpty()) {
  insets = target_bounds.work_area_insets;
}

https://codereview.chromium.org/231123002/diff/10001/chrome/browser/ui/ash/se...
File chrome/browser/ui/ash/session_state_delegate_chromeos.cc (right):

https://codereview.chromium.org/231123002/diff/10001/chrome/browser/ui/ash/se...
chrome/browser/ui/ash/session_state_delegate_chromeos.cc:235: void
SessionStateDelegateChromeos::OnUserAddingFinished() {
Do we also get here if the user cancels out of the "add user to session" screen?

[Nikita (slow), 2014-04-10 09:52:21]

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

https://codereview.chromium.org/231123002/diff/10001/ash/shelf/shelf_layout_m...
ash/shelf/shelf_layout_manager.cc:745: gfx::Insets insets;
On 2014/04/10 01:33:08, Mr4D wrote:
> By creating the insets they should be initialized to 0. As such line 749-756
> could be simplified to:
> 
> if (!session_state_delegate->IsUserSessionBlocked() ||
>     !keyboard_bounds_.IsEmpty()) {
>   insets = target_bounds.work_area_insets;
> }

Done.

https://codereview.chromium.org/231123002/diff/10001/chrome/browser/ui/ash/se...
File chrome/browser/ui/ash/session_state_delegate_chromeos.cc (right):

https://codereview.chromium.org/231123002/diff/10001/chrome/browser/ui/ash/se...
chrome/browser/ui/ash/session_state_delegate_chromeos.cc:235: void
SessionStateDelegateChromeos::OnUserAddingFinished() {
On 2014/04/10 01:33:08, Mr4D wrote:
> Do we also get here if the user cancels out of the "add user to session"
screen?

Yes.

[Nikita (slow), 2014-04-10 15:15:27]

The CQ bit was checked by nkostylev@chromium.org

[commit-bot: I haz the power, 2014-04-10 15:15:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nkostylev@chromium.org/231123002/70001

[Nikita (slow), 2014-04-10 15:33:32]

+Dan for OWNERS review of ash/* and chrome/browser/ui/ash/*

[commit-bot: I haz the power, 2014-04-10 15:52:09]

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

[commit-bot: I haz the power, 2014-04-10 15:52:09]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[Daniel Erat, 2014-04-10 16:02:09]

i am concerned that there is too much duplication going on in this domain. we
already have chromeos::LoginState (chromeos/login/login_state.h),
ash::user::LoginStatus (ash/system/user/login_status.h), chromeos::UserType
(chrome/browser/chromeos/login/user.h), and possibly other similar enums that
i'm not aware of. we should be moving toward storing login/session state in
fewer places rather than more.

[Nikita (slow), 2014-04-10 16:10:54]

On 2014/04/10 16:02:09, Daniel Erat wrote:
> i am concerned that there is too much duplication going on in this domain. we
> already have chromeos::LoginState (chromeos/login/login_state.h),
> ash::user::LoginStatus (ash/system/user/login_status.h), chromeos::UserType
> (chrome/browser/chromeos/login/user.h), and possibly other similar enums that
> i'm not aware of. we should be moving toward storing login/session state in
> fewer places rather than more.

I agree with your concerns yet I see SessionStateDelegate the right place for
this information in Ash.

This means that I would like to move
ShellObserver::OnLoginStateChanged(user::LoginStatus status) to
SessionStateDelegate::OnLoginStateChanged().

This means that LoginState and SessionStateDelegate, SessionStateObserver would
also be moved from

ash/system/user/login_status.h
ash/session_state_delegate.h
ash/session_state_observer.h

to something like ash/session/*

wdyt?

I would prefer to this move not in this CL if possible.

[Nikita (slow), 2014-04-10 16:11:51]

On 2014/04/10 16:10:54, Nikita Kostylev wrote:
> This means that I would like to move
> ShellObserver::OnLoginStateChanged(user::LoginStatus status) to
> SessionStateDelegate::OnLoginStateChanged().

To SessionStateObserver::OnLoginStateChanged() actually.

[Daniel Erat, 2014-04-10 16:30:32]

i'm okay with this approach if you follow it up with another change that
simplifies things. :-)

https://codereview.chromium.org/231123002/diff/70001/ash/session_state_delega...
File ash/session_state_delegate.h (right):

https://codereview.chromium.org/231123002/diff/70001/ash/session_state_delega...
ash/session_state_delegate.h:52: STATE_UNKNOWN = 0,
please don't add an "unknown" state; this means that every class using this enum
will need to handle it. more about this in the delegate

https://codereview.chromium.org/231123002/diff/70001/ash/session_state_delega...
File ash/session_state_delegate_stub.cc (right):

https://codereview.chromium.org/231123002/diff/70001/ash/session_state_delega...
ash/session_state_delegate_stub.cc:77: // Assuming that if session is not active
we're at login.
nit: s/Assuming/Assume/

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

https://codereview.chromium.org/231123002/diff/70001/ash/shelf/shelf_layout_m...
ash/shelf/shelf_layout_manager.cc:216:
Shell::GetInstance()->session_state_delegate()->AddSessionStateObserver(this);
you're missing the call to remove this in the d'tor

https://codereview.chromium.org/231123002/diff/70001/chrome/browser/ui/ash/se...
File chrome/browser/ui/ash/session_state_delegate_chromeos.cc (right):

https://codereview.chromium.org/231123002/diff/70001/chrome/browser/ui/ash/se...
chrome/browser/ui/ash/session_state_delegate_chromeos.cc:31: :
session_state_(STATE_UNKNOWN) {
it'd be better to have a separate "initialized" member, add a "force" parameter
to SetSessionState(), or something similar. otherwise all of the observers are
forced to deal with an implementation detail of this class.

https://codereview.chromium.org/231123002/diff/70001/chrome/browser/ui/ash/se...
chrome/browser/ui/ash/session_state_delegate_chromeos.cc:237:
SetSessionState(STATE_SESSION);
nit:

  SetSessionState(chromeos::LoginState::Get()->IsUserLoggedIn() ?
      STATE_SESSION : STATE_LOGIN_PRIMARY);

https://codereview.chromium.org/231123002/diff/70001/chrome/browser/ui/ash/se...
chrome/browser/ui/ash/session_state_delegate_chromeos.cc:252:
SetSessionState(STATE_SESSION);
why is this necessary? shouldn't LoggedInStateChanged() also fire?

https://codereview.chromium.org/231123002/diff/70001/chrome/browser/ui/ash/se...
chrome/browser/ui/ash/session_state_delegate_chromeos.cc:260:
SetSessionState(STATE_SESSION);
why is this necessary? shouldn't OnUserAddingFinished() also fire?

https://codereview.chromium.org/231123002/diff/70001/chrome/browser/ui/ash/se...
chrome/browser/ui/ash/session_state_delegate_chromeos.cc:279: case
chrome::NOTIFICATION_SCREEN_LOCK_STATE_CHANGED: {
this seems ugly. do we really need to watch for both
ScreenIsLocked/ScreenIsUnlocked and this notification? if so, why are there
cases where one happens but not the other?

[stevenjb, 2014-04-10 17:00:05]

https://codereview.chromium.org/231123002/diff/70001/ash/session_state_delega...
File ash/session_state_delegate.h (right):

https://codereview.chromium.org/231123002/diff/70001/ash/session_state_delega...
ash/session_state_delegate.h:62: STATE_LOCK,
I'm not sure I understand this state. If this is synonymous with screen lock I
might suggest removing this state and checking screen lock directly where
necessary. Otherwise please elaborate in the comment.

https://codereview.chromium.org/231123002/diff/70001/ash/session_state_delega...
ash/session_state_delegate.h:66: STATE_LOGIN_SECONDARY,
While I hate to second guess a change in an area I'm only peripherally familiar
with, I'm also uncomfortable with adding yet another state, even in the "short"
term.

I assume there are future plans for GetSessionState(), since this CL doesn't
appear to actually use it outside of tests, but it looks to me like
LOGIN_SECONDARY is the only new "state" and we might be better off with adding
something like LOGIN_UI_STATE_{NONE|PRIMARY|SECONDARY}, rather than combining
states from different sources here? The sheer number of observers added to
SessionStateDelegateChromeos suggests to me that we are trying to do too much
with a single state; I think it will be OK to push some of that logic to the UI
code.

If we go this route then we should name these SESSION_STATE_ since, as mentioned
we already have so many login / session related states we want to reduce
confusion as much as possible.

[Nikita (slow), 2014-04-10 17:22:50]

https://codereview.chromium.org/231123002/diff/70001/ash/session_state_delega...
File ash/session_state_delegate.h (right):

https://codereview.chromium.org/231123002/diff/70001/ash/session_state_delega...
ash/session_state_delegate.h:52: STATE_UNKNOWN = 0,
On 2014/04/10 16:30:32, Daniel Erat wrote:
> please don't add an "unknown" state; this means that every class using this
enum
> will need to handle it. more about this in the delegate

Done.

https://codereview.chromium.org/231123002/diff/70001/ash/session_state_delega...
ash/session_state_delegate.h:62: STATE_LOCK,
On 2014/04/10 17:00:06, stevenjb wrote:
> I'm not sure I understand this state. If this is synonymous with screen lock I
> might suggest removing this state and checking screen lock directly where
> necessary. Otherwise please elaborate in the comment.

Done.

https://codereview.chromium.org/231123002/diff/70001/ash/session_state_delega...
ash/session_state_delegate.h:66: STATE_LOGIN_SECONDARY,
On 2014/04/10 17:00:06, stevenjb wrote:
> While I hate to second guess a change in an area I'm only peripherally
familiar
> with, I'm also uncomfortable with adding yet another state, even in the
"short"
> term.
> 
> I assume there are future plans for GetSessionState(), since this CL doesn't
> appear to actually use it outside of tests, but it looks to me like
> LOGIN_SECONDARY is the only new "state" and we might be better off with adding
> something like LOGIN_UI_STATE_{NONE|PRIMARY|SECONDARY}, rather than combining
> states from different sources here? The sheer number of observers added to
> SessionStateDelegateChromeos suggests to me that we are trying to do too much
> with a single state; I think it will be OK to push some of that logic to the
UI
> code.
> 
> If we go this route then we should name these SESSION_STATE_ since, as
mentioned
> we already have so many login / session related states we want to reduce
> confusion as much as possible.

I've dropped STATE_LOCK for now.

I would prefer to merge LOGIN_STATE into SessionStateDelegate.

https://codereview.chromium.org/231123002/diff/70001/ash/session_state_delega...
File ash/session_state_delegate_stub.cc (right):

https://codereview.chromium.org/231123002/diff/70001/ash/session_state_delega...
ash/session_state_delegate_stub.cc:77: // Assuming that if session is not active
we're at login.
On 2014/04/10 16:30:32, Daniel Erat wrote:
> nit: s/Assuming/Assume/

Done.

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

https://codereview.chromium.org/231123002/diff/70001/ash/shelf/shelf_layout_m...
ash/shelf/shelf_layout_manager.cc:216:
Shell::GetInstance()->session_state_delegate()->AddSessionStateObserver(this);
On 2014/04/10 16:30:32, Daniel Erat wrote:
> you're missing the call to remove this in the d'tor

Done.

https://codereview.chromium.org/231123002/diff/70001/chrome/browser/ui/ash/se...
File chrome/browser/ui/ash/session_state_delegate_chromeos.cc (right):

https://codereview.chromium.org/231123002/diff/70001/chrome/browser/ui/ash/se...
chrome/browser/ui/ash/session_state_delegate_chromeos.cc:31: :
session_state_(STATE_UNKNOWN) {
On 2014/04/10 16:30:32, Daniel Erat wrote:
> it'd be better to have a separate "initialized" member, add a "force"
parameter
> to SetSessionState(), or something similar. otherwise all of the observers are
> forced to deal with an implementation detail of this class.

Done.

https://codereview.chromium.org/231123002/diff/70001/chrome/browser/ui/ash/se...
chrome/browser/ui/ash/session_state_delegate_chromeos.cc:237:
SetSessionState(STATE_SESSION);
On 2014/04/10 16:30:32, Daniel Erat wrote:
> nit:
> 
>   SetSessionState(chromeos::LoginState::Get()->IsUserLoggedIn() ?
>       STATE_SESSION : STATE_LOGIN_PRIMARY);

Done.

https://codereview.chromium.org/231123002/diff/70001/chrome/browser/ui/ash/se...
chrome/browser/ui/ash/session_state_delegate_chromeos.cc:252:
SetSessionState(STATE_SESSION);
On 2014/04/10 16:30:32, Daniel Erat wrote:
> why is this necessary? shouldn't LoggedInStateChanged() also fire?

Done.

https://codereview.chromium.org/231123002/diff/70001/chrome/browser/ui/ash/se...
chrome/browser/ui/ash/session_state_delegate_chromeos.cc:252:
SetSessionState(STATE_SESSION);
On 2014/04/10 16:30:32, Daniel Erat wrote:
> why is this necessary? shouldn't LoggedInStateChanged() also fire?

Done.

https://codereview.chromium.org/231123002/diff/70001/chrome/browser/ui/ash/se...
chrome/browser/ui/ash/session_state_delegate_chromeos.cc:260:
SetSessionState(STATE_SESSION);
On 2014/04/10 16:30:32, Daniel Erat wrote:
> why is this necessary? shouldn't OnUserAddingFinished() also fire?

Done.

https://codereview.chromium.org/231123002/diff/70001/chrome/browser/ui/ash/se...
chrome/browser/ui/ash/session_state_delegate_chromeos.cc:279: case
chrome::NOTIFICATION_SCREEN_LOCK_STATE_CHANGED: {
On 2014/04/10 16:30:32, Daniel Erat wrote:
> this seems ugly. do we really need to watch for both
> ScreenIsLocked/ScreenIsUnlocked and this notification? if so, why are there
> cases where one happens but not the other?

Turns out ScreenLockerTest only creates ScreenLocker which in turn just fire
notification.

But I've removed STATE_LOCK completely per Steven's comment.

[Nikita (slow), 2014-04-10 17:29:27]

https://codereview.chromium.org/231123002/diff/70001/ash/session_state_delega...
File ash/session_state_delegate.h (right):

https://codereview.chromium.org/231123002/diff/70001/ash/session_state_delega...
ash/session_state_delegate.h:66: STATE_LOGIN_SECONDARY,
On 2014/04/10 17:22:51, Nikita Kostylev wrote:
> On 2014/04/10 17:00:06, stevenjb wrote:
> > While I hate to second guess a change in an area I'm only peripherally
> familiar
> > with, I'm also uncomfortable with adding yet another state, even in the
> "short"
> > term.
> > 
> > I assume there are future plans for GetSessionState(), since this CL doesn't
> > appear to actually use it outside of tests, but it looks to me like
> > LOGIN_SECONDARY is the only new "state" and we might be better off with
adding
> > something like LOGIN_UI_STATE_{NONE|PRIMARY|SECONDARY}, rather than
combining
> > states from different sources here? The sheer number of observers added to
> > SessionStateDelegateChromeos suggests to me that we are trying to do too
much
> > with a single state; I think it will be OK to push some of that logic to the
> UI
> > code.
> > 
> > If we go this route then we should name these SESSION_STATE_ since, as
> mentioned
> > we already have so many login / session related states we want to reduce
> > confusion as much as possible.
> 
> I've dropped STATE_LOCK for now.
> 
> I would prefer to merge LOGIN_STATE into SessionStateDelegate.

Since adding this logic to ShellObserver::OnLoginStateChanged() doesn't seem
like a right place.

Next I would really like move OnLoginStateChanged() from there.

[stevenjb, 2014-04-10 17:47:54]

lgtm w/ naming suggestion

https://codereview.chromium.org/231123002/diff/70001/ash/session_state_delega...
File ash/session_state_delegate.h (right):

https://codereview.chromium.org/231123002/diff/70001/ash/session_state_delega...
ash/session_state_delegate.h:66: STATE_LOGIN_SECONDARY,
On 2014/04/10 17:29:27, Nikita Kostylev wrote:
> On 2014/04/10 17:22:51, Nikita Kostylev wrote:
> > On 2014/04/10 17:00:06, stevenjb wrote:
> > > While I hate to second guess a change in an area I'm only peripherally
> > familiar
> > > with, I'm also uncomfortable with adding yet another state, even in the
> > "short"
> > > term.
> > > 
> > > I assume there are future plans for GetSessionState(), since this CL
doesn't
> > > appear to actually use it outside of tests, but it looks to me like
> > > LOGIN_SECONDARY is the only new "state" and we might be better off with
> adding
> > > something like LOGIN_UI_STATE_{NONE|PRIMARY|SECONDARY}, rather than
> combining
> > > states from different sources here? The sheer number of observers added to
> > > SessionStateDelegateChromeos suggests to me that we are trying to do too
> much
> > > with a single state; I think it will be OK to push some of that logic to
the
> > UI
> > > code.
> > > 
> > > If we go this route then we should name these SESSION_STATE_ since, as
> > mentioned
> > > we already have so many login / session related states we want to reduce
> > > confusion as much as possible.
> > 
> > I've dropped STATE_LOCK for now.
> > 
> > I would prefer to merge LOGIN_STATE into SessionStateDelegate.
> 
> Since adding this logic to ShellObserver::OnLoginStateChanged() doesn't seem
> like a right place.
> 
> Next I would really like move OnLoginStateChanged() from there.

I agree. That's another one where we conflate "LoginState" and screen lock
state. It would be nice to eliminate ash::user::LoginStatus and use LoginState +
a separate screen lock state in the UI directly.

https://codereview.chromium.org/231123002/diff/90001/ash/session_state_delega...
File ash/session_state_delegate.h (right):

https://codereview.chromium.org/231123002/diff/90001/ash/session_state_delega...
ash/session_state_delegate.h:53: STATE_LOGIN_PRIMARY = 0,
I'd still like to name these SESSION_STATE_*

https://codereview.chromium.org/231123002/diff/90001/ash/session_state_delega...
ash/session_state_delegate.h:56: STATE_SESSION,
Maybe SESSION_STATE_ACTIVE?

[Nikita (slow), 2014-04-11 08:58:41]

https://codereview.chromium.org/231123002/diff/90001/ash/session_state_delega...
File ash/session_state_delegate.h (right):

https://codereview.chromium.org/231123002/diff/90001/ash/session_state_delega...
ash/session_state_delegate.h:53: STATE_LOGIN_PRIMARY = 0,
On 2014/04/10 17:47:55, stevenjb wrote:
> I'd still like to name these SESSION_STATE_*

Done.

https://codereview.chromium.org/231123002/diff/90001/ash/session_state_delega...
ash/session_state_delegate.h:56: STATE_SESSION,
On 2014/04/10 17:47:55, stevenjb wrote:
> Maybe SESSION_STATE_ACTIVE?

Done.

[Nikita (slow), 2014-04-11 08:58:53]

Dan, please take a look.

[Daniel Erat, 2014-04-11 14:43:40]

lgtm w/comments

https://codereview.chromium.org/231123002/diff/110001/ash/session_state_deleg...
File ash/session_state_delegate.h (right):

https://codereview.chromium.org/231123002/diff/110001/ash/session_state_deleg...
ash/session_state_delegate.h:55: // Inside user session, no blocking UI
(login/lock) is shown.
update this comment since this now includes the lock screen

https://codereview.chromium.org/231123002/diff/110001/ash/shelf/shelf_layout_...
File ash/shelf/shelf_layout_manager.h (right):

https://codereview.chromium.org/231123002/diff/110001/ash/shelf/shelf_layout_...
ash/shelf/shelf_layout_manager.h:196: // Overridden from
ash::SessionStateChanged:
s/SessionStateChanged/SessionStateObserver/

https://codereview.chromium.org/231123002/diff/110001/chrome/browser/ui/ash/s...
File chrome/browser/ui/ash/session_state_delegate_chromeos.h (right):

https://codereview.chromium.org/231123002/diff/110001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/session_state_delegate_chromeos.h:83: // Session state
like at login or lock screen or inside session.
this doesn't include the lock screen anymore, so i'd just say something like
"Session state (e.g. login screen vs. user session)".

https://codereview.chromium.org/231123002/diff/110001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/session_state_delegate_chromeos.h:86: // True if session
state has been explicitly set.
i don't think you need this now that SetSessionState() has a 'force' argument;
|session_state_| is initialized in the c'tor

[Nikita (slow), 2014-04-14 08:53:15]

https://codereview.chromium.org/231123002/diff/110001/ash/session_state_deleg...
File ash/session_state_delegate.h (right):

https://codereview.chromium.org/231123002/diff/110001/ash/session_state_deleg...
ash/session_state_delegate.h:55: // Inside user session, no blocking UI
(login/lock) is shown.
On 2014/04/11 14:43:41, Daniel Erat wrote:
> update this comment since this now includes the lock screen

Done.

https://codereview.chromium.org/231123002/diff/110001/ash/shelf/shelf_layout_...
File ash/shelf/shelf_layout_manager.h (right):

https://codereview.chromium.org/231123002/diff/110001/ash/shelf/shelf_layout_...
ash/shelf/shelf_layout_manager.h:196: // Overridden from
ash::SessionStateChanged:
On 2014/04/11 14:43:41, Daniel Erat wrote:
> s/SessionStateChanged/SessionStateObserver/

Done.

https://codereview.chromium.org/231123002/diff/110001/chrome/browser/ui/ash/s...
File chrome/browser/ui/ash/session_state_delegate_chromeos.h (right):

https://codereview.chromium.org/231123002/diff/110001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/session_state_delegate_chromeos.h:83: // Session state
like at login or lock screen or inside session.
On 2014/04/11 14:43:41, Daniel Erat wrote:
> this doesn't include the lock screen anymore, so i'd just say something like
> "Session state (e.g. login screen vs. user session)".

Done.

https://codereview.chromium.org/231123002/diff/110001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/session_state_delegate_chromeos.h:86: // True if session
state has been explicitly set.
On 2014/04/11 14:43:41, Daniel Erat wrote:
> i don't think you need this now that SetSessionState() has a 'force' argument;
> |session_state_| is initialized in the c'tor

Done.

[Nikita (slow), 2014-04-14 09:18:10]

The CQ bit was checked by nkostylev@chromium.org

[commit-bot: I haz the power, 2014-04-14 09:18:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nkostylev@chromium.org/231123002/190001

[commit-bot: I haz the power, 2014-04-14 12:23:54]

Change committed as 263611