Add SessionStateObserver with ActiveUserChanged()

Add SessionStateObserver with ActiveUserChanged()

Depends on https://codereview.chromium.org/15974008/ which renames SessionStateObserver to LockStateObserver

* Rename few instances of email to user_id
* Add SessionStateDelegateChromeos implementation

BUG=180903
R=bartfab@chromium.org, skuhne@chromium.org, sky@chromium.org, stevenjb@chromium.org

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

[Nikita (slow), 2013-05-31 15:05:04]

bartfab@ - please review whole CL
skuhne@ - please review *ash/*

[Mr4D (OOO till 08-26), 2013-05-31 22:23:39]

lgtm

Only a few comments.

https://codereview.chromium.org/15718003/diff/13001/ash/system/tray/system_tr...
File ash/system/tray/system_tray.h (right):

https://codereview.chromium.org/15718003/diff/13001/ash/system/tray/system_tr...
ash/system/tray/system_tray.h:127: // Closes system bubble and returns true if
that existed.
.. if it did exist.

https://codereview.chromium.org/15718003/diff/13001/ash/system/user/tray_user.cc
File ash/system/user/tray_user.cc (right):

https://codereview.chromium.org/15718003/diff/13001/ash/system/user/tray_user...
ash/system/user/tray_user.cc:601: owner_(owner),
Instead of passing the owner you might consider to use:

GetRootWindowController(Shell::GetActiveRootWindow())->shelf()->status_area_widget()->system_tray();

But then again - the other tray items are doing it the same way, so this should
be fine.

https://codereview.chromium.org/15718003/diff/13001/ash/system/user/tray_user...
ash/system/user/tray_user.cc:717: // User list will change and it makes sense to
close bubble.
Since the user list is about to change the system menu should get closed.

[bartfab (slow), 2013-06-03 09:00:06]

https://codereview.chromium.org/15718003/diff/13001/ash/session_state_observer.h
File ash/session_state_observer.h (right):

https://codereview.chromium.org/15718003/diff/13001/ash/session_state_observe...
ash/session_state_observer.h:17: virtual void ActiveUserChanged(const
std::string& user_id) {}
Thank you for using |user_id|. Hopefully the nomenclature will stick and all new
code will use it :).

https://codereview.chromium.org/15718003/diff/13001/ash/system/user/tray_user.cc
File ash/system/user/tray_user.cc (right):

https://codereview.chromium.org/15718003/diff/13001/ash/system/user/tray_user...
ash/system/user/tray_user.cc:601: owner_(owner),
On 2013/05/31 22:23:39, Mr4D wrote:
> Instead of passing the owner you might consider to use:
> 
>
GetRootWindowController(Shell::GetActiveRootWindow())->shelf()->status_area_widget()->system_tray();
> 
> But then again - the other tray items are doing it the same way, so this
should
> be fine.

|owner_| will continue to work if/when we have multiple system trays one day,
singleton accesses will not.

https://codereview.chromium.org/15718003/diff/13001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/profiles/profile_helper.cc (right):

https://codereview.chromium.org/15718003/diff/13001/chrome/browser/chromeos/p...
chrome/browser/chromeos/profiles/profile_helper.cc:147: 
Nit: Remove double blank line.

https://codereview.chromium.org/15718003/diff/13001/chrome/browser/ui/ash/ses...
File chrome/browser/ui/ash/session_state_delegate.h (right):

https://codereview.chromium.org/15718003/diff/13001/chrome/browser/ui/ash/ses...
chrome/browser/ui/ash/session_state_delegate.h:17: class User;
When overriding a method, the common pattern I have seen is to not include
additional headers or forward-declare classes needed by that method's arguments
only. Going by that, no need to forward-declare |User| here.

https://codereview.chromium.org/15718003/diff/13001/chrome/browser/ui/ash/ses...
chrome/browser/ui/ash/session_state_delegate.h:58:
ObserverList<ash::SessionStateObserver> session_state_observer_list_;
Nit: The list is used in the Chrome OS implementation only. It should be
#ifdef'd to OS_CHROMEOS.

https://codereview.chromium.org/15718003/diff/13001/chrome/browser/ui/ash/ses...
File chrome/browser/ui/ash/session_state_delegate_chromeos.cc (right):

https://codereview.chromium.org/15718003/diff/13001/chrome/browser/ui/ash/ses...
chrome/browser/ui/ash/session_state_delegate_chromeos.cc:105:
session_state_observer_list_,
Nit: Indent one space less.

[Nikita (slow), 2013-06-03 09:21:28]

Addressed all comments, please take a look.

https://codereview.chromium.org/15718003/diff/13001/ash/session_state_observer.h
File ash/session_state_observer.h (right):

https://codereview.chromium.org/15718003/diff/13001/ash/session_state_observe...
ash/session_state_observer.h:17: virtual void ActiveUserChanged(const
std::string& user_id) {}
On 2013/06/03 09:00:06, bartfab wrote:
> Thank you for using |user_id|. Hopefully the nomenclature will stick and all
new
> code will use it :).

We've discussed with Stefan that I'll send an email to CrOS UI devs to see if
there're any objections.
I want to have few examples in existing code before that.

https://codereview.chromium.org/15718003/diff/13001/ash/system/tray/system_tr...
File ash/system/tray/system_tray.h (right):

https://codereview.chromium.org/15718003/diff/13001/ash/system/tray/system_tr...
ash/system/tray/system_tray.h:127: // Closes system bubble and returns true if
that existed.
On 2013/05/31 22:23:39, Mr4D wrote:
> .. if it did exist.

Done.

https://codereview.chromium.org/15718003/diff/13001/ash/system/user/tray_user.cc
File ash/system/user/tray_user.cc (right):

https://codereview.chromium.org/15718003/diff/13001/ash/system/user/tray_user...
ash/system/user/tray_user.cc:601: owner_(owner),
On 2013/06/03 09:00:06, bartfab wrote:
> On 2013/05/31 22:23:39, Mr4D wrote:
> > Instead of passing the owner you might consider to use:
> > 
> >
>
GetRootWindowController(Shell::GetActiveRootWindow())->shelf()->status_area_widget()->system_tray();
> > 
> > But then again - the other tray items are doing it the same way, so this
> should
> > be fine.
> 
> |owner_| will continue to work if/when we have multiple system trays one day,
> singleton accesses will not.

Leaving as is then to follow existing pattern and to address this concern.

https://codereview.chromium.org/15718003/diff/13001/ash/system/user/tray_user...
ash/system/user/tray_user.cc:717: // User list will change and it makes sense to
close bubble.
On 2013/05/31 22:23:39, Mr4D wrote:
> Since the user list is about to change the system menu should get closed.

Done.

https://codereview.chromium.org/15718003/diff/13001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/profiles/profile_helper.cc (right):

https://codereview.chromium.org/15718003/diff/13001/chrome/browser/chromeos/p...
chrome/browser/chromeos/profiles/profile_helper.cc:147: 
On 2013/06/03 09:00:06, bartfab wrote:
> Nit: Remove double blank line.

Done.

https://codereview.chromium.org/15718003/diff/13001/chrome/browser/ui/ash/ses...
File chrome/browser/ui/ash/session_state_delegate.h (right):

https://codereview.chromium.org/15718003/diff/13001/chrome/browser/ui/ash/ses...
chrome/browser/ui/ash/session_state_delegate.h:17: class User;
On 2013/06/03 09:00:06, bartfab wrote:
> When overriding a method, the common pattern I have seen is to not include
> additional headers or forward-declare classes needed by that method's
arguments
> only. Going by that, no need to forward-declare |User| here.

Done.

https://codereview.chromium.org/15718003/diff/13001/chrome/browser/ui/ash/ses...
chrome/browser/ui/ash/session_state_delegate.h:58:
ObserverList<ash::SessionStateObserver> session_state_observer_list_;
On 2013/06/03 09:00:06, bartfab wrote:
> Nit: The list is used in the Chrome OS implementation only. It should be
> #ifdef'd to OS_CHROMEOS.

Actually SessionStateObserver is defined for everyone.
This might be later used for other platforms I just wanted to leave such option.

WDYT?

https://codereview.chromium.org/15718003/diff/13001/chrome/browser/ui/ash/ses...
File chrome/browser/ui/ash/session_state_delegate_chromeos.cc (right):

https://codereview.chromium.org/15718003/diff/13001/chrome/browser/ui/ash/ses...
chrome/browser/ui/ash/session_state_delegate_chromeos.cc:105:
session_state_observer_list_,
On 2013/06/03 09:00:06, bartfab wrote:
> Nit: Indent one space less.

Done.

[bartfab (slow), 2013-06-03 09:26:52]

lgtm with two more nits.

https://codereview.chromium.org/15718003/diff/13001/chrome/browser/ui/ash/ses...
File chrome/browser/ui/ash/session_state_delegate.h (right):

https://codereview.chromium.org/15718003/diff/13001/chrome/browser/ui/ash/ses...
chrome/browser/ui/ash/session_state_delegate.h:58:
ObserverList<ash::SessionStateObserver> session_state_observer_list_;
On 2013/06/03 09:21:28, Nikita Kostylev wrote:
> On 2013/06/03 09:00:06, bartfab wrote:
> > Nit: The list is used in the Chrome OS implementation only. It should be
> > #ifdef'd to OS_CHROMEOS.
> 
> Actually SessionStateObserver is defined for everyone.
> This might be later used for other platforms I just wanted to leave such
option.
> 
> WDYT?

AddSessionStateObserver and RemoveSessionStateObserver are NOTREACHED() on
non-Chrome-OS platforms. Until we add support for adding and removing observers,
there is no need to allocate memory for an observer list IMHO. If you #ifdef the
observer list here, you can always later remove the #ifdefs when the observer
list becomes supported on other platforms.

https://codereview.chromium.org/15718003/diff/13003/ash/system/user/tray_user.cc
File ash/system/user/tray_user.cc (right):

https://codereview.chromium.org/15718003/diff/13003/ash/system/user/tray_user...
ash/system/user/tray_user.cc:717: // Since the user list is about to change
Nit: Any particular reason for breaking the line here, not after 80 characters?

[Nikita (slow), 2013-06-03 09:46:54]

https://codereview.chromium.org/15718003/diff/13001/chrome/browser/ui/ash/ses...
File chrome/browser/ui/ash/session_state_delegate.h (right):

https://codereview.chromium.org/15718003/diff/13001/chrome/browser/ui/ash/ses...
chrome/browser/ui/ash/session_state_delegate.h:58:
ObserverList<ash::SessionStateObserver> session_state_observer_list_;
On 2013/06/03 09:26:52, bartfab wrote:
> On 2013/06/03 09:21:28, Nikita Kostylev wrote:
> > On 2013/06/03 09:00:06, bartfab wrote:
> > > Nit: The list is used in the Chrome OS implementation only. It should be
> > > #ifdef'd to OS_CHROMEOS.
> > 
> > Actually SessionStateObserver is defined for everyone.
> > This might be later used for other platforms I just wanted to leave such
> option.
> > 
> > WDYT?
> 
> AddSessionStateObserver and RemoveSessionStateObserver are NOTREACHED() on
> non-Chrome-OS platforms. Until we add support for adding and removing
observers,
> there is no need to allocate memory for an observer list IMHO. If you #ifdef
the
> observer list here, you can always later remove the #ifdefs when the observer
> list becomes supported on other platforms.

Done.

https://codereview.chromium.org/15718003/diff/13003/ash/system/user/tray_user.cc
File ash/system/user/tray_user.cc (right):

https://codereview.chromium.org/15718003/diff/13003/ash/system/user/tray_user...
ash/system/user/tray_user.cc:717: // Since the user list is about to change
On 2013/06/03 09:26:52, bartfab wrote:
> Nit: Any particular reason for breaking the line here, not after 80
characters?

Done.

[Nikita (slow), 2013-06-03 12:01:20]

sky@ for owners review
ash/*
ash/shelf/*
ash/test/*
chrome/*

stevenjb@ for owners review
ash/system/*

[Nikita (slow), 2013-06-03 12:01:46]

sky@ for owners review
ash/*
ash/shelf/*
ash/test/*
chrome/*

stevenjb@ for owners review
ash/system/*

[sky, 2013-06-03 15:06:23]

https://codereview.chromium.org/15718003/diff/28002/ash/session_state_delegate.h
File ash/session_state_delegate.h (right):

https://codereview.chromium.org/15718003/diff/28002/ash/session_state_delegat...
ash/session_state_delegate.h:12: #include "ash/session_state_observer.h"
You should be able to forward declare SessionStateObserver.

https://codereview.chromium.org/15718003/diff/28002/ash/session_state_observer.h
File ash/session_state_observer.h (right):

https://codereview.chromium.org/15718003/diff/28002/ash/session_state_observe...
ash/session_state_observer.h:17: virtual void ActiveUserChanged(const
std::string& user_id) {}
How come this is in terms of user_id but the other methods in
SessionStateDelegate are in terms of an index and or an email address? Can we
have some consistency here?

I didn't review the changes to SessionStateDelegate, but looking at them
wouldn't it make more sense to have a User and a method that returns a
vector<User> rather than all the other methods that were exposed? Also,
SessionState seems to have two very distinct sets of methods, one for user state
and the other for lock state. Should they be separated out?

[Nikita (slow), 2013-06-03 15:23:47]

On 2013/06/03 15:06:23, sky wrote:
>
https://codereview.chromium.org/15718003/diff/28002/ash/session_state_delegate.h
> File ash/session_state_delegate.h (right):
> 
>
https://codereview.chromium.org/15718003/diff/28002/ash/session_state_delegat...
> ash/session_state_delegate.h:12: #include "ash/session_state_observer.h"
> You should be able to forward declare SessionStateObserver.
> 
>
https://codereview.chromium.org/15718003/diff/28002/ash/session_state_observer.h
> File ash/session_state_observer.h (right):
> 
>
https://codereview.chromium.org/15718003/diff/28002/ash/session_state_observe...
> ash/session_state_observer.h:17: virtual void ActiveUserChanged(const
> std::string& user_id) {}
> How come this is in terms of user_id but the other methods in
> SessionStateDelegate are in terms of an index and or an email address? Can we
> have some consistency here?

user_id and email are the same. We use user_id in new code.
I'll update all existing references separately.

> 
> I didn't review the changes to SessionStateDelegate, but looking at them
> wouldn't it make more sense to have a User and a method that returns a
> vector<User> rather than all the other methods that were exposed?

We don't have User class in src/ash. If we decide that we want to add such it
makes sense to make so in a separate CL.

> Also, SessionState seems to have two very distinct sets of methods, one for
user state
> and the other for lock state. Should they be separated out?

Yes, this is a known issue too and we should decide on the plan forward
separately (incl. stevenjb@ / bartfab@)

[Nikita (slow), 2013-06-03 15:27:59]

On 2013/06/03 15:23:47, Nikita Kostylev wrote:
> > Also, SessionState seems to have two very distinct sets of methods, one for
> user state
> > and the other for lock state. Should they be separated out?
> 
> Yes, this is a known issue too and we should decide on the plan forward
> separately (incl. stevenjb@ / bartfab@)

See https://codereview.chromium.org/13495003/ for more context.

[Nikita (slow), 2013-06-03 15:31:50]

On 2013/06/03 15:23:47, Nikita Kostylev wrote:
>
https://codereview.chromium.org/15718003/diff/28002/ash/session_state_observe...
> > ash/session_state_observer.h:17: virtual void ActiveUserChanged(const
> > std::string& user_id) {}
> > How come this is in terms of user_id but the other methods in
> > SessionStateDelegate are in terms of an index and or an email address? Can
we
> > have some consistency here?
> 
> user_id and email are the same. We use user_id in new code.
> I'll update all existing references separately.

Renamed to user_id.

[Nikita (slow), 2013-06-03 15:31:57]

https://codereview.chromium.org/15718003/diff/28002/ash/session_state_delegate.h
File ash/session_state_delegate.h (right):

https://codereview.chromium.org/15718003/diff/28002/ash/session_state_delegat...
ash/session_state_delegate.h:12: #include "ash/session_state_observer.h"
On 2013/06/03 15:06:23, sky wrote:
> You should be able to forward declare SessionStateObserver.

Done.

[sky, 2013-06-03 16:09:00]

LGTM

[stevenjb, 2013-06-03 16:51:59]

https://codereview.chromium.org/15718003/diff/36025/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/user_manager.h (right):

https://codereview.chromium.org/15718003/diff/36025/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/user_manager.h:53: virtual void
ActiveUserChanged(const User* active_user) {}
Virtual function implementations shouldn't be inline.

https://codereview.chromium.org/15718003/diff/36025/chrome/browser/chromeos/s...
File chrome/browser/chromeos/system/ash_system_tray_delegate.cc (right):

https://codereview.chromium.org/15718003/diff/36025/chrome/browser/chromeos/s...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:317:
AddSessionStateObserver(this);
We should remove this observer in the destructor.

https://codereview.chromium.org/15718003/diff/36025/chrome/browser/ui/ash/ses...
File chrome/browser/ui/ash/session_state_delegate.h (right):

https://codereview.chromium.org/15718003/diff/36025/chrome/browser/ui/ash/ses...
chrome/browser/ui/ash/session_state_delegate.h:24: public
chromeos::UserManager::UserSessionStateObserver,
Can we create a SessionStateDelegateChromeos implementaiton class in the
chromeos .cc file instead of exposing these details here?

https://codereview.chromium.org/15718003/diff/36025/chrome/browser/ui/ash/ses...
chrome/browser/ui/ash/session_state_delegate.h:60:
ObserverList<ash::SessionStateObserver> session_state_observer_list_;
This also?

[stevenjb, 2013-06-03 17:06:23]

On 2013/06/03 15:23:47, Nikita Kostylev wrote:
> On 2013/06/03 15:06:23, sky wrote:
> >
>
https://codereview.chromium.org/15718003/diff/28002/ash/session_state_delegate.h
> > File ash/session_state_delegate.h (right):
> > 
> >
>
https://codereview.chromium.org/15718003/diff/28002/ash/session_state_delegat...
> > ash/session_state_delegate.h:12: #include "ash/session_state_observer.h"
> > You should be able to forward declare SessionStateObserver.
> > 
> >
>
https://codereview.chromium.org/15718003/diff/28002/ash/session_state_observer.h
> > File ash/session_state_observer.h (right):
> > 
> >
>
https://codereview.chromium.org/15718003/diff/28002/ash/session_state_observe...
> > ash/session_state_observer.h:17: virtual void ActiveUserChanged(const
> > std::string& user_id) {}
> > How come this is in terms of user_id but the other methods in
> > SessionStateDelegate are in terms of an index and or an email address? Can
we
> > have some consistency here?
> 
> user_id and email are the same. We use user_id in new code.
> I'll update all existing references separately.
> 
> > 
> > I didn't review the changes to SessionStateDelegate, but looking at them
> > wouldn't it make more sense to have a User and a method that returns a
> > vector<User> rather than all the other methods that were exposed?
> 
> We don't have User class in src/ash. If we decide that we want to add such it
> makes sense to make so in a separate CL.
> 
> > Also, SessionState seems to have two very distinct sets of methods, one for
> user state
> > and the other for lock state. Should they be separated out?
> 
> Yes, this is a known issue too and we should decide on the plan forward
> separately (incl. stevenjb@ / bartfab@)

I created src/chromeos/login/login_state.h so that we could have a common
ChromeOS class for Ash and Chrome. In that class we removed / separated any lock
screen state / references.

Ideally I would like to see SessionStateDelegate go away and move enough login /
session / display state code migrated to src/chromeos that Ash can use that
directly.

I would be more than happy to meet and discuss any such plans.

[Nikita (slow), 2013-06-04 12:41:32]

Steven, please take a look.
Added SessionStateDelegateChromeos

https://codereview.chromium.org/15718003/diff/36025/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/user_manager.h (right):

https://codereview.chromium.org/15718003/diff/36025/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/user_manager.h:53: virtual void
ActiveUserChanged(const User* active_user) {}
On 2013/06/03 16:52:00, stevenjb (chromium) wrote:
> Virtual function implementations shouldn't be inline.

Done.

https://codereview.chromium.org/15718003/diff/36025/chrome/browser/chromeos/s...
File chrome/browser/chromeos/system/ash_system_tray_delegate.cc (right):

https://codereview.chromium.org/15718003/diff/36025/chrome/browser/chromeos/s...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:317:
AddSessionStateObserver(this);
On 2013/06/03 16:52:00, stevenjb (chromium) wrote:
> We should remove this observer in the destructor.

Done.

https://codereview.chromium.org/15718003/diff/36025/chrome/browser/ui/ash/ses...
File chrome/browser/ui/ash/session_state_delegate.h (right):

https://codereview.chromium.org/15718003/diff/36025/chrome/browser/ui/ash/ses...
chrome/browser/ui/ash/session_state_delegate.h:24: public
chromeos::UserManager::UserSessionStateObserver,
On 2013/06/03 16:52:00, stevenjb (chromium) wrote:
> Can we create a SessionStateDelegateChromeos implementaiton class in the
> chromeos .cc file instead of exposing these details here?

Done.

https://codereview.chromium.org/15718003/diff/36025/chrome/browser/ui/ash/ses...
chrome/browser/ui/ash/session_state_delegate.h:60:
ObserverList<ash::SessionStateObserver> session_state_observer_list_;
On 2013/06/03 16:52:00, stevenjb (chromium) wrote:
> This also?

Done.

[stevenjb, 2013-06-04 16:32:42]

lgtm

[Nikita (slow), 2013-06-04 16:59:24]

Committed patchset #11 manually as r203985 (presubmit successful).