Add LoginState class to src/chromeos/login

Add LoginState class to src/chromeos/login


BUG=226495
For Misc. chromeos-specific chrome/browser login related changes:
TBR=sky@chromium.org


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

[stevenjb, 2013-04-03 02:23:08]

Eventually it would be great to get all of UserManager into src/chromeos, but in
the meanwhile there are a couple of classes (CryptohomeLibrary, CertLibrary)
that only need to know about the logged in status, which will benefit from
having a login state available outside of src/chrome.

[bartfab (slow), 2013-04-03 08:21:35]

We will be running a hackathon very soon to refactor and update the UserManager
and related code. Maybe it is better to hold off on this stop-gap measure and
move the UserManager to src/chromeos during the hackathon instead? Otherwise, we
get yet another layer of abstraction that needs maintaining.

https://codereview.chromium.org/13495003/diff/7/chrome/browser/chromeos/login...
File chrome/browser/chromeos/login/user_manager_impl.cc (right):

https://codereview.chromium.org/13495003/diff/7/chrome/browser/chromeos/login...
chrome/browser/chromeos/login/user_manager_impl.cc:1316: login_state =
LoginState::LOGGED_IN_KIOSK_APP;
Ugh. Does single-app mode really reuse the retail mode login path?

https://codereview.chromium.org/13495003/diff/7/chromeos/chromeos.gyp
File chromeos/chromeos.gyp (right):

https://codereview.chromium.org/13495003/diff/7/chromeos/chromeos.gyp#newcode424
chromeos/chromeos.gyp:424: 'login/login_state_unittest.cc',
This belongs after the ime/ entries (not in alphabetic order as it is right now)

Also, where is that unit test file? It seems to be missing from the CL.

https://codereview.chromium.org/13495003/diff/7/chromeos/login/login_state.cc
File chromeos/login/login_state.cc (right):

https://codereview.chromium.org/13495003/diff/7/chromeos/login/login_state.cc...
chromeos/login/login_state.cc:15: CHECK(!g_login_state);
#include "base/logging.h"

[Nikita (slow), 2013-04-03 10:23:24]

This state has to be multi-profile aware from the start as we add that support
now.

[Nikita (slow), 2013-04-03 10:26:30]

On 2013/04/03 10:23:24, Nikita Kostylev wrote:
> This state has to be multi-profile aware from the start as we add that support
> now.

But at the same time in case of several users being logged in state will be
either LOGGED_IN_USER or LOGGED_IN_OWNER as we only plan to support regular user
being signed in simultaneously.

So there's a corner case when you're signed in as an owner user and as another
user. So depending on which user is currently active state will be changed (I
guess). Or as an alternative we should switch to user that is an owner (if need
that for some specific operation).

[Nikita (slow), 2013-04-03 10:29:17]

I think that if we want to introduce something like this state it has to be
integrated with ChromeShellDelegate from the start.

Which already has methods like

 // Returns true if user has logged in.
  virtual bool IsUserLoggedIn() const = 0;

  // Returns true if we're logged in and browser has been started
  virtual bool IsSessionStarted() const = 0;

  // Returns true if we're logged in as guest.
  virtual bool IsGuestSession() const = 0;

  // Returns true if we're running in forced app mode.
  virtual bool IsRunningInForcedAppMode() const = 0;

  // Returns true if the screen is currently locked.
  virtual bool IsScreenLocked() const = 0;

[stevenjb, 2013-04-03 17:21:42]

https://chromiumcodereview.appspot.com/13495003/diff/7/chrome/browser/chromeo...
File chrome/browser/chromeos/login/user_manager_impl.cc (right):

https://chromiumcodereview.appspot.com/13495003/diff/7/chrome/browser/chromeo...
chrome/browser/chromeos/login/user_manager_impl.cc:1316: login_state =
LoginState::LOGGED_IN_KIOSK_APP;
On 2013/04/03 08:21:36, bartfab wrote:
> Ugh. Does single-app mode really reuse the retail mode login path?
I'm not sure I understand the question, but I copied the logic from
SystemTrayDelegate.

https://chromiumcodereview.appspot.com/13495003/diff/7/chromeos/chromeos.gyp
File chromeos/chromeos.gyp (right):

https://chromiumcodereview.appspot.com/13495003/diff/7/chromeos/chromeos.gyp#...
chromeos/chromeos.gyp:424: 'login/login_state_unittest.cc',
On 2013/04/03 08:21:36, bartfab wrote:
> This belongs after the ime/ entries (not in alphabetic order as it is right
now)
> 
> Also, where is that unit test file? It seems to be missing from the CL.

Done and added.

https://chromiumcodereview.appspot.com/13495003/diff/7/chromeos/login/login_s...
File chromeos/login/login_state.cc (right):

https://chromiumcodereview.appspot.com/13495003/diff/7/chromeos/login/login_s...
chromeos/login/login_state.cc:15: CHECK(!g_login_state);
On 2013/04/03 08:21:36, bartfab wrote:
> #include "base/logging.h"

Done.

[stevenjb, 2013-04-03 17:23:58]

On 2013/04/03 10:29:17, Nikita Kostylev wrote:
> I think that if we want to introduce something like this state it has to be
> integrated with ChromeShellDelegate from the start.
> 
> Which already has methods like
> 
>  // Returns true if user has logged in.
>   virtual bool IsUserLoggedIn() const = 0;
> 
>   // Returns true if we're logged in and browser has been started
>   virtual bool IsSessionStarted() const = 0;
> 
>   // Returns true if we're logged in as guest.
>   virtual bool IsGuestSession() const = 0;
> 
>   // Returns true if we're running in forced app mode.
>   virtual bool IsRunningInForcedAppMode() const = 0;
> 
>   // Returns true if the screen is currently locked.
>   virtual bool IsScreenLocked() const = 0;

OK, you asked for it... :)

I was able to convert IsUserLoggedIn(), IsGuestSession(), and IsSessionStarted()
by adding a session_started_ to LoginState.

Again, eventually I would like to see all, or at least most of UserManager
migrated. This is a small step in that direction.

I had to move the login switches to src/chromeos/chromeos_switches.h (well, I
had to move one, so figured I should move them all), thus the much larger CL.

PTAL

[Nikita (slow), 2013-04-03 17:48:13]

https://chromiumcodereview.appspot.com/13495003/diff/7/chrome/browser/chromeo...
File chrome/browser/chromeos/login/user_manager_impl.cc (right):

https://chromiumcodereview.appspot.com/13495003/diff/7/chrome/browser/chromeo...
chrome/browser/chromeos/login/user_manager_impl.cc:1316: login_state =
LoginState::LOGGED_IN_KIOSK_APP;
On 2013/04/03 17:21:42, stevenjb (chromium) wrote:
> On 2013/04/03 08:21:36, bartfab wrote:
> > Ugh. Does single-app mode really reuse the retail mode login path?
> I'm not sure I understand the question, but I copied the logic from
> SystemTrayDelegate.

I think these two should be separated.

DemoUser = retail mode (previously known as kiosk mode).

KIOSK_APP = single app mode (new definition for kiosk).

[Nikita (slow), 2013-04-03 17:52:46]

So LoginState would be a temporary implementation till UserManager is migrated
to src/chromeos ?

[stevenjb, 2013-04-03 18:16:38]

On 2013/04/03 17:48:13, Nikita Kostylev wrote:
>
https://chromiumcodereview.appspot.com/13495003/diff/7/chrome/browser/chromeo...
> File chrome/browser/chromeos/login/user_manager_impl.cc (right):
> 
>
https://chromiumcodereview.appspot.com/13495003/diff/7/chrome/browser/chromeo...
> chrome/browser/chromeos/login/user_manager_impl.cc:1316: login_state =
> LoginState::LOGGED_IN_KIOSK_APP;
> On 2013/04/03 17:21:42, stevenjb (chromium) wrote:
> > On 2013/04/03 08:21:36, bartfab wrote:
> > > Ugh. Does single-app mode really reuse the retail mode login path?
> > I'm not sure I understand the question, but I copied the logic from
> > SystemTrayDelegate.
> 
> I think these two should be separated.
> 
> DemoUser = retail mode (previously known as kiosk mode).
> 
> KIOSK_APP = single app mode (new definition for kiosk).

Could you provide the correct logic for that? I don't know anything about
either.

[stevenjb, 2013-04-03 18:18:40]

On 2013/04/03 17:52:46, Nikita Kostylev wrote:
> So LoginState would be a temporary implementation till UserManager is migrated
> to src/chromeos ?

Yes, and/or a separate smaller class that we would maintain alongside
UserManager. I don't have a preference, but there is a fair bit of code that
still exists in src/chrome only because it needs to be able to know the current
login state, and I would like to start migrating it.

[Nikita (slow), 2013-04-04 08:26:10]

There's also  ash::user::LoginStatus enum.

[stevenjb, 2013-04-04 19:57:31]

OK, I:
* extracted the switches changes
* eliminated currently unused states (LOCALLY_MANAGED, KIOSK)
* updated code that only uses UserManager for IsUserLoggedIn() to use LoginState

[stevenjb, 2013-04-04 20:11:54]

On 2013/04/04 08:26:10, Nikita Kostylev wrote:
> There's also  ash::user::LoginStatus enum.

Also: I can't eliminate ash::user::LoginStatus, since that is not ChromeOS
specific, but I made ash::user::LoginStatus map directly to LoginState::State,
and only do that in one place now.

[xiyuan, 2013-04-04 20:29:22]

On 2013/04/04 19:57:31, stevenjb (chromium) wrote:
> OK, I:
> * extracted the switches changes
> * eliminated currently unused states (LOCALLY_MANAGED, KIOSK)
> * updated code that only uses UserManager for IsUserLoggedIn() to use
LoginState

I am not sure KIOSK state should be removed. We cannot treat it 100% as a normal
user, e.g. sync should not be running this user.

I added this kiosk user recently but I have not completely go over the code to
update everywhere IsLoggedInAsGuest is called. But most of the places, the call
sites should be updated with an additional check for current user is the kiosk
app robot.

[stevenjb, 2013-04-04 20:50:15]

On 2013/04/04 20:29:22, xiyuan wrote:
> On 2013/04/04 19:57:31, stevenjb (chromium) wrote:
> > OK, I:
> > * extracted the switches changes
> > * eliminated currently unused states (LOCALLY_MANAGED, KIOSK)
> > * updated code that only uses UserManager for IsUserLoggedIn() to use
> LoginState
> 
> I am not sure KIOSK state should be removed. We cannot treat it 100% as a
normal
> user, e.g. sync should not be running this user.
> 
> I added this kiosk user recently but I have not completely go over the code to
> update everywhere IsLoggedInAsGuest is called. But most of the places, the
call
> sites should be updated with an additional check for current user is the kiosk
> app robot.

So, I removed it because we never return it in GetUserLoginStatus(), so the
current behavior is at least partially broken. (We only set it in
PowerButtonObserver()).

I can add it back if we are planning to use it. Will upload a patch shortly...

[stevenjb (google-dont-use), 2013-04-04 20:57:51]

Added back LOGGED_IN_KIOSK_APP



On Thu, Apr 4, 2013 at 1:50 PM, <stevenjb@chromium.org> wrote:

> On 2013/04/04 20:29:22, xiyuan wrote:
>
>> On 2013/04/04 19:57:31, stevenjb (chromium) wrote:
>> > OK, I:
>> > * extracted the switches changes
>> > * eliminated currently unused states (LOCALLY_MANAGED, KIOSK)
>> > * updated code that only uses UserManager for IsUserLoggedIn() to use
>> LoginState
>>
>
>  I am not sure KIOSK state should be removed. We cannot treat it 100% as a
>>
> normal
>
>> user, e.g. sync should not be running this user.
>>
>
>  I added this kiosk user recently but I have not completely go over the
>> code to
>> update everywhere IsLoggedInAsGuest is called. But most of the places, the
>>
> call
>
>> sites should be updated with an additional check for current user is the
>> kiosk
>> app robot.
>>
>
> So, I removed it because we never return it in GetUserLoginStatus(), so the
> current behavior is at least partially broken. (We only set it in
> PowerButtonObserver()).
>
> I can add it back if we are planning to use it. Will upload a patch
> shortly...
>
>
>
>
https://codereview.chromium.**org/13495003/<https://codereview.chromium.org/1...
>

[xiyuan, 2013-04-04 21:24:53]

LGTM but wait for nkostylev and bartfab.

https://codereview.chromium.org/13495003/diff/48001/chrome/browser/chromeos/s...
File chrome/browser/chromeos/status/data_promo_notification.cc (right):

https://codereview.chromium.org/13495003/diff/48001/chrome/browser/chromeos/s...
chrome/browser/chromeos/status/data_promo_notification.cc:144:
LoginState::Get()->GetLoginState() != LoginState::LOGGED_IN_GUEST &&
Out of curiosity and out of the scope of the CL. Should we only offer the data
promo only to a normal user? It does not make much sense for public account,
retail mode and kiosk app robot.

https://codereview.chromium.org/13495003/diff/48001/chromeos/login/login_stat...
File chromeos/login/login_state.cc (right):

https://codereview.chromium.org/13495003/diff/48001/chromeos/login/login_stat...
chromeos/login/login_state.cc:106: LoginState::LoginState() :
state_(LOGGED_IN_OOBE),
nit: should it be initialized to LOGGED_IN_UNKNOWN?

[stevenjb, 2013-04-04 21:35:46]

https://codereview.chromium.org/13495003/diff/48001/chrome/browser/chromeos/s...
File chrome/browser/chromeos/status/data_promo_notification.cc (right):

https://codereview.chromium.org/13495003/diff/48001/chrome/browser/chromeos/s...
chrome/browser/chromeos/status/data_promo_notification.cc:144:
LoginState::Get()->GetLoginState() != LoginState::LOGGED_IN_GUEST &&
On 2013/04/04 21:24:54, xiyuan wrote:
> Out of curiosity and out of the scope of the CL. Should we only offer the data
> promo only to a normal user? It does not make much sense for public account,
> retail mode and kiosk app robot.
Good point, I think it can/should be safely fixed here. Done.

https://codereview.chromium.org/13495003/diff/48001/chromeos/login/login_stat...
File chromeos/login/login_state.cc (right):

https://codereview.chromium.org/13495003/diff/48001/chromeos/login/login_stat...
chromeos/login/login_state.cc:106: LoginState::LoginState() :
state_(LOGGED_IN_OOBE),
On 2013/04/04 21:24:54, xiyuan wrote:
> nit: should it be initialized to LOGGED_IN_UNKNOWN?
I got rid of LOGGED_IN_UNKNOWN, it doesn't really make any sense. If we query
LoginState before the Wizard sets it, we should default to OOBE. Thanks for
pointing that out.

[Nikita (slow), 2013-04-05 04:08:52]

lgtm with nits

Please wait for review from bartafb@

https://codereview.chromium.org/13495003/diff/52001/ash/system/user/login_sta...
File ash/system/user/login_status.h (left):

https://codereview.chromium.org/13495003/diff/52001/ash/system/user/login_sta...
ash/system/user/login_status.h:20: LOGGED_IN_LOCALLY_MANAGED,  // A locally
managed user is logged in.
Please leave LOGGED_IN_LOCALLY_MANAGED status as we most likely to configure
some UI elements based on that later.
If we end up not using it, I'll do a cleanup.

https://codereview.chromium.org/13495003/diff/52001/chromeos/login/login_stat...
File chromeos/login/login_state.cc (right):

https://codereview.chromium.org/13495003/diff/52001/chromeos/login/login_stat...
chromeos/login/login_state.cc:71: if (AlwaysLoggedIn()) {
niut: I don't think that you specifically need this non-Chrome OS device
handling as when you run Chrome w/o parameter you're logged in as stub user and
it should initialize LoginState properly.

And it's better to rely on that functionality because it also configures User
instance. Otherwise just returning LoginState as logged in you may end up in
accessing User instance that has not been initialized.

https://codereview.chromium.org/13495003/diff/52001/chromeos/login/login_state.h
File chromeos/login/login_state.h (right):

https://codereview.chromium.org/13495003/diff/52001/chromeos/login/login_stat...
chromeos/login/login_state.h:21: LOGGED_IN_USER,             // A normal user is
logged in
nit: normal > regular
To match naming that we have in User

https://codereview.chromium.org/13495003/diff/52001/chromeos/login/login_stat...
chromeos/login/login_state.h:58: // Tracks wherther the user is logged in and
the browser process has started.
>>  the browser process has started
Browser instance has been created to be more specific.

[bartfab (slow), 2013-04-05 13:16:33]

I think this CL is taking things in the wrong direction. With
chromeos::LoginState, every consumer needs to know which of the statuses should
cause a particular feature to be on or off. When a new user type is added (e.g.
the new manager users or the new single-app mode), every single consumer of
chromeos::LoginState needs to be checked and potentially updated so that it does
the right thing for the new user type as well. This is the wrong way to do
things.

Instead of an enum that every consumer has to interpret, the status should be
broken down into separate methods that indicate whether a particular
feature/property applies to the currently logged-in user or not. So rather than
e.g. enabling the avatar chooser for owner/regular/managed_user, it should be
enabled iff CanCurrentUserModifyAvatar() is true. This way, we can trivially add
more user types or modify the existing user types later without having to hunt
down all consumers of chromeos::LoginState and updating them all.

I broke down the chromeos::User class into a number of specialized sub-classes
precisely so that there can be e.g. a common bool can_lock() method for which
User object returns |true| or |false|, depending on the properties of the user.
Instead of an enum, we should be adding methods that allow specific
features/properties to be queried.

Think of this as user agent sniffing (boo!) vs. checking whether a particular
feature is present (yay!).

https://codereview.chromium.org/13495003/diff/52001/ash/system/user/login_sta...
File ash/system/user/login_status.h (left):

https://codereview.chromium.org/13495003/diff/52001/ash/system/user/login_sta...
ash/system/user/login_status.h:21: LOGGED_IN_KIOSK_APP,        // Is in kiosk
app mode.
1/ Please leave the LOGGED_IN_KIOSK_APP status. This mode is still in flux,
which is why you do not see the status used anywhere yet.

2/ Nit: I presume that this constant means a kiosk app mode session is actually
running. A better comment would then be "Kiosk app session in progress."

3/ @xiyuan: I strongly object to yet another usage of "kiosk". It is way
overloaded already. Can we please rename this to something like
"LOGGED_IN_SINGLE_APP" for "single-app mode"?

https://codereview.chromium.org/13495003/diff/52001/ash/system/user/tray_user.cc
File ash/system/user/tray_user.cc (left):

https://codereview.chromium.org/13495003/diff/52001/ash/system/user/tray_user...
ash/system/user/tray_user.cc:616: case user::LOGGED_IN_LOCALLY_MANAGED:
Please restore this. Locally managed mode is in flux, this is why you do not see
it being set anywhere right now.

https://codereview.chromium.org/13495003/diff/52001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/user_manager_impl.cc (right):

https://codereview.chromium.org/13495003/diff/52001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/user_manager_impl.cc:1310: return;  // may not be
intialized in tests
Nit: Format comment as a sentence.

https://codereview.chromium.org/13495003/diff/52001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/power/power_button_observer.cc (right):

https://codereview.chromium.org/13495003/diff/52001/chrome/browser/chromeos/p...
chrome/browser/chromeos/power/power_button_observer.cc:24: return 
ash::user::LOGGED_IN_NONE;
Nit: Double space after "return".

https://codereview.chromium.org/13495003/diff/52001/chrome/browser/chromeos/s...
File chrome/browser/chromeos/status/data_promo_notification.cc (right):

https://codereview.chromium.org/13495003/diff/52001/chrome/browser/chromeos/s...
chrome/browser/chromeos/status/data_promo_notification.cc:140: // Display
one-time notification for normal users on first use
Nit: s/normal/regular/

https://codereview.chromium.org/13495003/diff/52001/chrome/browser/chromeos/s...
chrome/browser/chromeos/status/data_promo_notification.cc:144: if ((login_state
== LoginState::LOGGED_IN_OWNER ||
This is a great example why I think checking the value of GetLoginState() in
consumers is a bad idea: This code here wants to offer a promo to all GAIA
users. Instead of checking whether the current user is a GAIA user of any type,
it needs to be aware that both an owner and any other regular user fall into
that category.

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

https://codereview.chromium.org/13495003/diff/52001/chrome/browser/chromeos/s...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:384:
LoginState::State state = LoginState::Get()->GetLoginState();
I really dislike this - two almost-identical enums getting mapped to each other
with some subtle differences (one knows about OOBE, the other does not) that are
very easy to miss and not explained or justified anywhere.

https://codereview.chromium.org/13495003/diff/52001/chrome/browser/extensions...
File chrome/browser/extensions/api/autotest_private/autotest_private_api.cc
(left):

https://codereview.chromium.org/13495003/diff/52001/chrome/browser/extensions...
chrome/browser/extensions/api/autotest_private/autotest_private_api.cc:40: case
ash::user::LOGGED_IN_LOCALLY_MANAGED:
Locally managed users are still in flux. Please leve the code in for now.

https://codereview.chromium.org/13495003/diff/52001/chrome/browser/google_api...
File chrome/browser/google_apis/auth_service.cc (right):

https://codereview.chromium.org/13495003/diff/52001/chrome/browser/google_api...
chrome/browser/google_apis/auth_service.cc:297: if (login_state ==
chromeos::LoginState::LOGGED_IN_NONE ||
Another great example why GetLoginState() is the wrong solution. There should be
a method in UserManager that tells whether authentication is possible. The
consumer should not have to distinguish between the various user types.

https://codereview.chromium.org/13495003/diff/52001/chromeos/login/login_stat...
File chromeos/login/login_state.cc (right):

https://codereview.chromium.org/13495003/diff/52001/chromeos/login/login_stat...
chromeos/login/login_state.cc:50: return !!g_login_state;
Nit: The !! for converting to a bool are useful in JavaScript. In C++, the
conversion will happen implicitly for you anyway.

https://codereview.chromium.org/13495003/diff/52001/chromeos/login/login_stat...
chromeos/login/login_state.cc:80: // |state_| may get set before
|session_started_|.
|state_| actually always gets set before |session_started_| from
UserManagerImpl::UpdateLoginState(). This will cause the observers to be
notified that the state changed to LOGGED_IN_NONE and then to be notified again
a moment later when |session_started_| is set.

I think the SetLoginState() and SetSessionStarted() methods should be joined
into one to avoid this spurious notification.

https://codereview.chromium.org/13495003/diff/52001/chromeos/login/login_stat...
chromeos/login/login_state.cc:90: if (login_state == LOGGED_IN_OOBE ||
login_state == LOGGED_IN_NONE)
Another case where a consumer has to know about the various enum values when
really, it wants a boolean result that could be gotten directly from the
UserManager.

https://codereview.chromium.org/13495003/diff/52001/chromeos/login/login_state.h
File chromeos/login/login_state.h (right):

https://codereview.chromium.org/13495003/diff/52001/chromeos/login/login_stat...
chromeos/login/login_state.h:17: enum State {
1/ This is essentially a redefinition of ash::user::LoginStatus just waiting to
get out of sync. I think that's putting us on a path that will cause more
problems in the long term than it solves in the short term.

2/ The ash::user::LoginStatus enum has the annoying property that it mixes
constants for the various user types (regular user, guest, etc.) with a constant
to indicate that the screen is locked. Locking the screen does not change the
logged-in user. The user type should still be accessible and the information
whether the screen is locked should be passed around separately.

If you want to use an enum that indicates the currently logged in user type,
let's do it right. Let's move the lock status out of this enum. This is not just
an academic nit by the way: We already have code that cashes the user type it
got from the current login status because it knows that if the screen gets
locked, the user type will no longer be accessible.

https://codereview.chromium.org/13495003/diff/52001/chromeos/login/login_stat...
chromeos/login/login_state.h:18: LOGGED_IN_OOBE,             // Out of box
experience not completed
Full stops missing at the ends of all comments.

https://codereview.chromium.org/13495003/diff/52001/chromeos/login/login_stat...
chromeos/login/login_state.h:19: LOGGED_IN_NONE,             // Not logged in
Nit: A better comment would be "No session in progress." (for some types of
sessions you log in, for others you do not - the term "session in progress" is
neutral and covers both).

https://codereview.chromium.org/13495003/diff/52001/chromeos/login/login_stat...
chromeos/login/login_state.h:23: LOGGED_IN_GUEST,            // A guest is
logged in (i.e. incognito)
Nit: A better comment would be "Guest session in progress."

https://codereview.chromium.org/13495003/diff/52001/chromeos/login/login_stat...
chromeos/login/login_state.h:24: LOGGED_IN_RETAIL_MODE,      // Is in retail
mode
Nit: I presume that this constant means a retail mode session is actually
running. A better comment would then be "Retail mode session in progress."

https://codereview.chromium.org/13495003/diff/52001/chromeos/login/login_stat...
chromeos/login/login_state.h:25: LOGGED_IN_PUBLIC,           // A public account
is logged in
Nit: A better comment would be "Public session in progress." (we recently
renamed public accounts to public sessions).

https://codereview.chromium.org/13495003/diff/52001/chromeos/login/login_stat...
chromeos/login/login_state.h:26: LOGGED_IN_KIOSK_APP         // Is in kiosk app
mode
Nit: I presume that this constant means a kiosk app mode session is actually
running. A better comment would then be "Kiosk app session in progress."

@xiyuan: I strongly object to yet another usage of "kiosk". It is way overloaded
already. Can we please rename this to something like "LOGGED_IN_SINGLE_APP" for
"single-app mode"?

https://codereview.chromium.org/13495003/diff/52001/chromeos/login/login_stat...
chromeos/login/login_state.h:32: virtual void
LoginStateChanged(LoginState::State state) = 0;
Nit: No need for the |LoginState::| prefix here.

https://codereview.chromium.org/13495003/diff/52001/chromeos/login/login_stat...
chromeos/login/login_state.h:41: static LoginState* Get();
One of the goals for our upcoming hackathon is to get rid of the copious use of
singletons in the Chrome OS user management code. Adding yet another singleton
is a bad idea IMHO.

https://codereview.chromium.org/13495003/diff/52001/chromeos/login/login_stat...
chromeos/login/login_state.h:66: // SessionManagerClient::Observer
Nit: Full stop or colon at the end of line.

https://codereview.chromium.org/13495003/diff/52001/chromeos/login/login_stat...
chromeos/login/login_state.h:67: virtual void LockScreen() OVERRIDE;
#include "base/compiler_specific.h"

https://codereview.chromium.org/13495003/diff/52001/chromeos/login/login_stat...
chromeos/login/login_state.h:75: ObserverList<LoginState::Observer>
observer_list_;
Nit: No need for the |LoginState::| prefix here.

https://codereview.chromium.org/13495003/diff/52001/chromeos/login/login_stat...
chromeos/login/login_state.h:77: DISALLOW_COPY_AND_ASSIGN(LoginState);
#include "base/basictypes.h"

https://codereview.chromium.org/13495003/diff/52001/chromeos/login/login_stat...
File chromeos/login/login_state_unittest.cc (right):

https://codereview.chromium.org/13495003/diff/52001/chromeos/login/login_stat...
chromeos/login/login_state_unittest.cc:9: #include
"chromeos/dbus/session_manager_client.h"
Nit: Already included by login_state.h.

https://codereview.chromium.org/13495003/diff/52001/chromeos/login/login_stat...
chromeos/login/login_state_unittest.cc:38: // LoginState::Observer
Nit: Colon or full stop at the end of the line.

[stevenjb, 2013-04-05 18:17:15]

https://codereview.chromium.org/13495003/diff/52001/ash/system/user/login_sta...
File ash/system/user/login_status.h (left):

https://codereview.chromium.org/13495003/diff/52001/ash/system/user/login_sta...
ash/system/user/login_status.h:20: LOGGED_IN_LOCALLY_MANAGED,  // A locally
managed user is logged in.
On 2013/04/05 04:08:53, Nikita Kostylev wrote:
> Please leave LOGGED_IN_LOCALLY_MANAGED status as we most likely to configure
> some UI elements based on that later.
> If we end up not using it, I'll do a cleanup.

Ugh, another place where we were setting this in PowerButtonObserver, but not in
AshSystemTrayDelegate. Cleanup FTW. Done.

https://codereview.chromium.org/13495003/diff/52001/ash/system/user/login_sta...
ash/system/user/login_status.h:21: LOGGED_IN_KIOSK_APP,        // Is in kiosk
app mode.
On 2013/04/05 13:16:33, bartfab wrote:
> 1/ Please leave the LOGGED_IN_KIOSK_APP status. This mode is still in flux,
> which is why you do not see the status used anywhere yet.
> 
> 2/ Nit: I presume that this constant means a kiosk app mode session is
actually
> running. A better comment would then be "Kiosk app session in progress."
> 
> 3/ @xiyuan: I strongly object to yet another usage of "kiosk". It is way
> overloaded already. Can we please rename this to something like
> "LOGGED_IN_SINGLE_APP" for "single-app mode"?

I'm going to restore this to how it was, with any future renaming TBD by someone
who is familiar with it :)

https://codereview.chromium.org/13495003/diff/52001/ash/system/user/tray_user.cc
File ash/system/user/tray_user.cc (left):

https://codereview.chromium.org/13495003/diff/52001/ash/system/user/tray_user...
ash/system/user/tray_user.cc:616: case user::LOGGED_IN_LOCALLY_MANAGED:
On 2013/04/05 13:16:33, bartfab wrote:
> Please restore this. Locally managed mode is in flux, this is why you do not
see
> it being set anywhere right now.

Done.

https://codereview.chromium.org/13495003/diff/52001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/user_manager_impl.cc (right):

https://codereview.chromium.org/13495003/diff/52001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/user_manager_impl.cc:1310: return;  // may not be
intialized in tests
On 2013/04/05 13:16:33, bartfab wrote:
> Nit: Format comment as a sentence.
Sure. Done.

https://codereview.chromium.org/13495003/diff/52001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/power/power_button_observer.cc (right):

https://codereview.chromium.org/13495003/diff/52001/chrome/browser/chromeos/p...
chrome/browser/chromeos/power/power_button_observer.cc:24: return 
ash::user::LOGGED_IN_NONE;
On 2013/04/05 13:16:33, bartfab wrote:
> Nit: Double space after "return".

Done.

https://codereview.chromium.org/13495003/diff/52001/chrome/browser/chromeos/s...
File chrome/browser/chromeos/status/data_promo_notification.cc (right):

https://codereview.chromium.org/13495003/diff/52001/chrome/browser/chromeos/s...
chrome/browser/chromeos/status/data_promo_notification.cc:140: // Display
one-time notification for normal users on first use
On 2013/04/05 13:16:33, bartfab wrote:
> Nit: s/normal/regular/

Done.

https://codereview.chromium.org/13495003/diff/52001/chrome/browser/chromeos/s...
chrome/browser/chromeos/status/data_promo_notification.cc:144: if ((login_state
== LoginState::LOGGED_IN_OWNER ||
On 2013/04/05 13:16:33, bartfab wrote:
> This is a great example why I think checking the value of GetLoginState() in
> consumers is a bad idea: This code here wants to offer a promo to all GAIA
> users. Instead of checking whether the current user is a GAIA user of any
type,
> it needs to be aware that both an owner and any other regular user fall into
> that category.

That makes sense to me, but seems beyond the scope of this CL? Or did you have a
suggested change? I believe that logically this is at least slightly more
accurate than before?

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

https://codereview.chromium.org/13495003/diff/52001/chrome/browser/chromeos/s...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:384:
LoginState::State state = LoginState::Get()->GetLoginState();
On 2013/04/05 13:16:33, bartfab wrote:
> I really dislike this - two almost-identical enums getting mapped to each
other
> with some subtle differences (one knows about OOBE, the other does not) that
are
> very easy to miss and not explained or justified anywhere.

One of them is Ash-specific (which may be on Windows), the other is Chrome OS
specific. Ash does not know about OOBE; I will add a comment to that effect.

https://codereview.chromium.org/13495003/diff/52001/chrome/browser/extensions...
File chrome/browser/extensions/api/autotest_private/autotest_private_api.cc
(left):

https://codereview.chromium.org/13495003/diff/52001/chrome/browser/extensions...
chrome/browser/extensions/api/autotest_private/autotest_private_api.cc:40: case
ash::user::LOGGED_IN_LOCALLY_MANAGED:
On 2013/04/05 13:16:33, bartfab wrote:
> Locally managed users are still in flux. Please leve the code in for now.

Done.

https://codereview.chromium.org/13495003/diff/52001/chrome/browser/google_api...
File chrome/browser/google_apis/auth_service.cc (right):

https://codereview.chromium.org/13495003/diff/52001/chrome/browser/google_api...
chrome/browser/google_apis/auth_service.cc:297: if (login_state ==
chromeos::LoginState::LOGGED_IN_NONE ||
On 2013/04/05 13:16:33, bartfab wrote:
> Another great example why GetLoginState() is the wrong solution. There should
be
> a method in UserManager that tells whether authentication is possible. The
> consumer should not have to distinguish between the various user types.

Again, sounds reasonable, but for a future CL, preferably by someone who
understands the login code a bit better.

https://codereview.chromium.org/13495003/diff/52001/chromeos/login/login_stat...
File chromeos/login/login_state.cc (right):

https://codereview.chromium.org/13495003/diff/52001/chromeos/login/login_stat...
chromeos/login/login_state.cc:50: return !!g_login_state;
On 2013/04/05 13:16:33, bartfab wrote:
> Nit: The !! for converting to a bool are useful in JavaScript. In C++, the
> conversion will happen implicitly for you anyway.

We have this a lot in our code; I forget where I ran into this being required,
it might be an older compatibility issue. I spoke with jam@ and confirmed that
the implicit conversion is fine. Done.

https://codereview.chromium.org/13495003/diff/52001/chromeos/login/login_stat...
chromeos/login/login_state.cc:71: if (AlwaysLoggedIn()) {
On 2013/04/05 04:08:53, Nikita Kostylev wrote:
> niut: I don't think that you specifically need this non-Chrome OS device
> handling as when you run Chrome w/o parameter you're logged in as stub user
and
> it should initialize LoginState properly.
> 
> And it's better to rely on that functionality because it also configures User
> instance. Otherwise just returning LoginState as logged in you may end up in
> accessing User instance that has not been initialized.

I don't want to change this logic with this CL. I was careful to preserve the
existing logic, and a lot of us depend on it for testing on Linux. I can talk to
some of the other UI devs about changing this in the future. I think I'd
actually like to expand and improve our ability to fake various fake login
states on Linux.

https://codereview.chromium.org/13495003/diff/52001/chromeos/login/login_stat...
chromeos/login/login_state.cc:80: // |state_| may get set before
|session_started_|.
On 2013/04/05 13:16:33, bartfab wrote:
> |state_| actually always gets set before |session_started_| from
> UserManagerImpl::UpdateLoginState(). This will cause the observers to be
> notified that the state changed to LOGGED_IN_NONE and then to be notified
again
> a moment later when |session_started_| is set.
> 
> I think the SetLoginState() and SetSessionStarted() methods should be joined
> into one to avoid this spurious notification.

I didn't want to make any assumptions or change the existing logic with this CL.
I agree that this could and should be improved. What I'd really like to see is
UserManager split into pieces that allow the interface to be moved here, without
waiting for all of the Chrome dependent parts to be migrated.

https://codereview.chromium.org/13495003/diff/52001/chromeos/login/login_state.h
File chromeos/login/login_state.h (right):

https://codereview.chromium.org/13495003/diff/52001/chromeos/login/login_stat...
chromeos/login/login_state.h:18: LOGGED_IN_OOBE,             // Out of box
experience not completed
On 2013/04/05 13:16:33, bartfab wrote:
> Full stops missing at the ends of all comments.
Not a requirement. This is a phrase not a sentence.

https://codereview.chromium.org/13495003/diff/52001/chromeos/login/login_stat...
chromeos/login/login_state.h:19: LOGGED_IN_NONE,             // Not logged in
On 2013/04/05 13:16:33, bartfab wrote:
> Nit: A better comment would be "No session in progress." (for some types of
> sessions you log in, for others you do not - the term "session in progress" is
> neutral and covers both).
I'm not sure if that level of detail helps us here, that didn't make a lot of
sense to me actually. Maybe for a future refactor?

https://codereview.chromium.org/13495003/diff/52001/chromeos/login/login_stat...
chromeos/login/login_state.h:21: LOGGED_IN_USER,             // A normal user is
logged in
On 2013/04/05 04:08:53, Nikita Kostylev wrote:
> nit: normal > regular
> To match naming that we have in User

Done.

https://codereview.chromium.org/13495003/diff/52001/chromeos/login/login_stat...
chromeos/login/login_state.h:26: LOGGED_IN_KIOSK_APP         // Is in kiosk app
mode
On 2013/04/05 13:16:33, bartfab wrote:
> Nit: I presume that this constant means a kiosk app mode session is actually
> running. A better comment would then be "Kiosk app session in progress."
> 
> @xiyuan: I strongly object to yet another usage of "kiosk". It is way
overloaded
> already. Can we please rename this to something like "LOGGED_IN_SINGLE_APP"
for
> "single-app mode"?

All of the above seem like good candidates for future re-factoring, not this CL.

https://codereview.chromium.org/13495003/diff/52001/chromeos/login/login_stat...
chromeos/login/login_state.h:32: virtual void
LoginStateChanged(LoginState::State state) = 0;
On 2013/04/05 13:16:33, bartfab wrote:
> Nit: No need for the |LoginState::| prefix here.

Done.

https://codereview.chromium.org/13495003/diff/52001/chromeos/login/login_stat...
chromeos/login/login_state.h:41: static LoginState* Get();
On 2013/04/05 13:16:33, bartfab wrote:
> One of the goals for our upcoming hackathon is to get rid of the copious use
of
> singletons in the Chrome OS user management code. Adding yet another singleton
> is a bad idea IMHO.

Implicit singletons are bad. We use explicit singletons extensively when there
is no clear owner. Not sure what the specific objectives of the Hackathon are,
but if there is a preferred pattern (where there is not an obvious higher level
owner) I know of a handful of classes that will need to be updated.

https://codereview.chromium.org/13495003/diff/52001/chromeos/login/login_stat...
chromeos/login/login_state.h:58: // Tracks wherther the user is logged in and
the browser process has started.
On 2013/04/05 04:08:53, Nikita Kostylev wrote:
> >>  the browser process has started
> Browser instance has been created to be more specific.

Done.

https://codereview.chromium.org/13495003/diff/52001/chromeos/login/login_stat...
chromeos/login/login_state.h:66: // SessionManagerClient::Observer
On 2013/04/05 13:16:33, bartfab wrote:
> Nit: Full stop or colon at the end of line.
We use this pattern throughout the code for label comments that are not
sentences.

https://codereview.chromium.org/13495003/diff/52001/chromeos/login/login_stat...
chromeos/login/login_state.h:67: virtual void LockScreen() OVERRIDE;
On 2013/04/05 13:16:33, bartfab wrote:
> #include "base/compiler_specific.h"

Done.

https://codereview.chromium.org/13495003/diff/52001/chromeos/login/login_stat...
chromeos/login/login_state.h:75: ObserverList<LoginState::Observer>
observer_list_;
On 2013/04/05 13:16:33, bartfab wrote:
> Nit: No need for the |LoginState::| prefix here.

Done.

https://codereview.chromium.org/13495003/diff/52001/chromeos/login/login_stat...
chromeos/login/login_state.h:77: DISALLOW_COPY_AND_ASSIGN(LoginState);
On 2013/04/05 13:16:33, bartfab wrote:
> #include "base/basictypes.h"

Done.

https://codereview.chromium.org/13495003/diff/52001/chromeos/login/login_stat...
File chromeos/login/login_state_unittest.cc (right):

https://codereview.chromium.org/13495003/diff/52001/chromeos/login/login_stat...
chromeos/login/login_state_unittest.cc:9: #include
"chromeos/dbus/session_manager_client.h"
On 2013/04/05 13:16:33, bartfab wrote:
> Nit: Already included by login_state.h.

Done.

[bartfab (slow), 2013-04-08 15:18:36]

I still don't like the addition of another enum but since there is a strong
desire to get more information about the logged-in user into chromeos/ and
Nikita is fine with the change as an OWNER, I am OK with it. LGTM with a few
more minor comments.

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

https://codereview.chromium.org/13495003/diff/52001/chrome/browser/chromeos/s...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:384:
LoginState::State state = LoginState::Get()->GetLoginState();
On 2013/04/05 18:17:15, stevenjb (chromium) wrote:
> On 2013/04/05 13:16:33, bartfab wrote:
> > I really dislike this - two almost-identical enums getting mapped to each
> other
> > with some subtle differences (one knows about OOBE, the other does not) that
> are
> > very easy to miss and not explained or justified anywhere.
> 
> One of them is Ash-specific (which may be on Windows), the other is Chrome OS
> specific. Ash does not know about OOBE; I will add a comment to that effect.

I don't see much reason to abstract away OOBE when very Chrome-OS-specific
things like retial mode, single-app-kiosk mode and public session are all passed
on to ash verbatim.

https://codereview.chromium.org/13495003/diff/52001/chromeos/login/login_stat...
File chromeos/login/login_state.cc (right):

https://codereview.chromium.org/13495003/diff/52001/chromeos/login/login_stat...
chromeos/login/login_state.cc:80: // |state_| may get set before
|session_started_|.
On 2013/04/05 18:17:15, stevenjb (chromium) wrote:
> On 2013/04/05 13:16:33, bartfab wrote:
> > |state_| actually always gets set before |session_started_| from
> > UserManagerImpl::UpdateLoginState(). This will cause the observers to be
> > notified that the state changed to LOGGED_IN_NONE and then to be notified
> again
> > a moment later when |session_started_| is set.
> > 
> > I think the SetLoginState() and SetSessionStarted() methods should be joined
> > into one to avoid this spurious notification.
> 
> I didn't want to make any assumptions or change the existing logic with this
CL.
> I agree that this could and should be improved. What I'd really like to see is
> UserManager split into pieces that allow the interface to be moved here,
without
> waiting for all of the Chrome dependent parts to be migrated.

I agree that changing existing functionality is always a challenge, especially
if the code is outside your regular field of expertise. But this here is new
code with new methods you implemented from scratch. Why not have |state_| and
|session_started_| be set at the same time?

https://codereview.chromium.org/13495003/diff/52001/chromeos/login/login_state.h
File chromeos/login/login_state.h (right):

https://codereview.chromium.org/13495003/diff/52001/chromeos/login/login_stat...
chromeos/login/login_state.h:18: LOGGED_IN_OOBE,             // Out of box
experience not completed
On 2013/04/05 18:17:15, stevenjb (chromium) wrote:
> On 2013/04/05 13:16:33, bartfab wrote:
> > Full stops missing at the ends of all comments.
> Not a requirement. This is a phrase not a sentence.
> 

True, not required by the style guide but generally followed nevertheless.

https://codereview.chromium.org/13495003/diff/63001/chromeos/login/login_stat...
File chromeos/login/login_state_unittest.cc (right):

https://codereview.chromium.org/13495003/diff/63001/chromeos/login/login_stat...
chromeos/login/login_state_unittest.cc:37: // LoginState::Observer
Nit: Still missing a sentence terminator, either . or :

https://codereview.chromium.org/13495003/diff/69002/chrome/browser/chromeos/p...
File chrome/browser/chromeos/power/power_button_observer.cc (right):

https://codereview.chromium.org/13495003/diff/69002/chrome/browser/chromeos/p...
chrome/browser/chromeos/power/power_button_observer.cc:24: return 
ash::user::LOGGED_IN_NONE;
The double space after return is still there.

[stevenjb (google-dont-use), 2013-04-08 23:56:13]

On Mon, Apr 8, 2013 at 8:18 AM, <bartfab@chromium.org> wrote:

> I still don't like the addition of another enum but since there is a strong
> desire to get more information about the logged-in user into chromeos/ and
> Nikita is fine with the change as an OWNER, I am OK with it. LGTM with a
> few
> more minor comments.
>
> I'm not a fan of the duplicate state, but I don't have a reasonable
intermediate suggestion.

I really want to see this re-factored, conflating IsLoggedIn(),
IsScreenLocked(), and LoggedInUserType() is terribly confusing. I started
down that path, but it's a pretty huge re-factor.



>  One of them is Ash-specific (which may be on Windows), the other is
>>
> Chrome OS
>
>> specific. Ash does not know about OOBE; I will add a comment to that
>>
> effect.
>
> I don't see much reason to abstract away OOBE when very
> Chrome-OS-specific things like retial mode, single-app-kiosk mode and
> public session are all passed on to ash verbatim.


The specific reason I didn't add this state to LoginStatus was to avoid
modifying a bunch of existing Ash logic that compares LoginStatus to NONE
(and sometimes LOCKED) but not OOBE (which doesn't exist).


>  I didn't want to make any assumptions or change the existing logic
>>
> with this CL.
>
>> I agree that this could and should be improved. What I'd really like
>>
> to see is
>
>> UserManager split into pieces that allow the interface to be moved
>>
> here, without
>
>> waiting for all of the Chrome dependent parts to be migrated.
>>
>
> I agree that changing existing functionality is always a challenge,
> especially if the code is outside your regular field of expertise. But
> this here is new code with new methods you implemented from scratch. Why
> not have |state_| and |session_started_| be set at the same time?
>
> state_ and session_started_ are set in LoginState at the same time
in UserManagerImpl::UpdateLoginState(), but the UserManagerImpl code is
pretty complex, and it is not clear to me
that UserManagerImpl::SessionStarted() is guaranteed to be called
before logged_in_user_ is set. It may be, but the code does not appear
structured that way, and I would hate to break something subtle by adding
that assumption.


>
>> > Full stops missing at the ends of all comments.
>> Not a requirement. This is a phrase not a sentence.
>>
>
>
> True, not required by the style guide but generally followed
> nevertheless.
>
> Not so much in the code I work with :) I really think this is pretty
minor, but personally it grates to see incorrect grammer (i.e. a non
sentence with a period at the end).

[stevenjb, 2013-04-09 15:12:26]

https://codereview.chromium.org/13495003/diff/69002/chrome/browser/chromeos/p...
File chrome/browser/chromeos/power/power_button_observer.cc (right):

https://codereview.chromium.org/13495003/diff/69002/chrome/browser/chromeos/p...
chrome/browser/chromeos/power/power_button_observer.cc:24: return 
ash::user::LOGGED_IN_NONE;
On 2013/04/08 15:18:36, bartfab wrote:
> The double space after return is still there.

Done.

[stevenjb, 2013-04-09 21:44:41]

OK, I thought about this some, made some time, and did the following:
* LoginState now tracks "LoggedInState" and "LoggedInUserType" separately. I
think conflating those is confusing.
* I eliminated session_started_ from LoginState; I don't think we need it in
src/chromeos.
* I eliminated lock screen tracking from LoginState; I don't currently have any
use cases for it in src/chromeos.

PTAL. (The previously approved patchset was #15 if you want to diff from there).

[Nikita (slow), 2013-04-10 08:09:22]

lgtm

https://codereview.chromium.org/13495003/diff/87004/chrome/browser/chromeos/b...
File chrome/browser/chromeos/background/ash_user_wallpaper_delegate.cc (right):

https://codereview.chromium.org/13495003/diff/87004/chrome/browser/chromeos/b...
chrome/browser/chromeos/background/ash_user_wallpaper_delegate.cc:92: return
true;
nit: add {} as condition takes more than one line.

https://codereview.chromium.org/13495003/diff/87004/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/wizard_controller.cc (right):

https://codereview.chromium.org/13495003/diff/87004/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/wizard_controller.cc:186:
LoginState::Get()->SetLoggedInState(
This has to be moved to base_login_display_host.cc / ShowLoginWizard

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr...

When OOBE is complete, WizardController instance is not created on boot.
We're planning refactoring that addresses inconsistencies like that.

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

https://codereview.chromium.org/13495003/diff/87004/chrome/browser/chromeos/s...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:388: return
ash::user::LOGGED_IN_NONE;
nit: Add {}

https://codereview.chromium.org/13495003/diff/87004/chrome/browser/google_api...
File chrome/browser/google_apis/auth_service.cc (right):

https://codereview.chromium.org/13495003/diff/87004/chrome/browser/google_api...
chrome/browser/google_apis/auth_service.cc:300: return false;
nit: Add {}

https://codereview.chromium.org/13495003/diff/87004/chromeos/login/login_stat...
File chromeos/login/login_state.cc (right):

https://codereview.chromium.org/13495003/diff/87004/chromeos/login/login_stat...
chromeos/login/login_state.cc:78: if (logged_in_state == LOGGED_IN_OOBE ||
logged_in_state == LOGGED_IN_NONE)
nit: this could be now simplified to

return GetLoggedInState() == LOGGED_IN_ACTIVE;

https://codereview.chromium.org/13495003/diff/87004/chromeos/login/login_state.h
File chromeos/login/login_state.h (right):

https://codereview.chromium.org/13495003/diff/87004/chromeos/login/login_stat...
chromeos/login/login_state.h:21: LOGGED_IN_ACTIVE   // A user has logged in
I like that it is now called "active". In my multi-profiles change I'll be
replacing GetLoggedInUser() to GetActiveUser().

[stevenjb, 2013-04-10 16:32:17]

https://codereview.chromium.org/13495003/diff/87004/chrome/browser/chromeos/b...
File chrome/browser/chromeos/background/ash_user_wallpaper_delegate.cc (right):

https://codereview.chromium.org/13495003/diff/87004/chrome/browser/chromeos/b...
chrome/browser/chromeos/background/ash_user_wallpaper_delegate.cc:92: return
true;
On 2013/04/10 08:09:22, Nikita Kostylev wrote:
> nit: add {} as condition takes more than one line.
Heh. I've been going back and forth with different reviewers / reviewees
regarding whether or not {} is required with a multi-line if() clause and single
line statement. There doesn't seem to be a hard rule, but I do tend to prefer
{}, so done.

https://codereview.chromium.org/13495003/diff/87004/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/wizard_controller.cc (right):

https://codereview.chromium.org/13495003/diff/87004/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/wizard_controller.cc:186:
LoginState::Get()->SetLoggedInState(
On 2013/04/10 08:09:22, Nikita Kostylev wrote:
> This has to be moved to base_login_display_host.cc / ShowLoginWizard
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr...
> 
> When OOBE is complete, WizardController instance is not created on boot.
> We're planning refactoring that addresses inconsistencies like that.

Ah, thanks. Done.

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

https://codereview.chromium.org/13495003/diff/87004/chrome/browser/chromeos/s...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:388: return
ash::user::LOGGED_IN_NONE;
On 2013/04/10 08:09:22, Nikita Kostylev wrote:
> nit: Add {}

Done.

https://codereview.chromium.org/13495003/diff/87004/chrome/browser/google_api...
File chrome/browser/google_apis/auth_service.cc (right):

https://codereview.chromium.org/13495003/diff/87004/chrome/browser/google_api...
chrome/browser/google_apis/auth_service.cc:300: return false;
On 2013/04/10 08:09:22, Nikita Kostylev wrote:
> nit: Add {}

Done.

https://codereview.chromium.org/13495003/diff/87004/chromeos/login/login_stat...
File chromeos/login/login_state.cc (right):

https://codereview.chromium.org/13495003/diff/87004/chromeos/login/login_stat...
chromeos/login/login_state.cc:78: if (logged_in_state == LOGGED_IN_OOBE ||
logged_in_state == LOGGED_IN_NONE)
On 2013/04/10 08:09:22, Nikita Kostylev wrote:
> nit: this could be now simplified to
> 
> return GetLoggedInState() == LOGGED_IN_ACTIVE;

Done.

https://codereview.chromium.org/13495003/diff/87004/chromeos/login/login_state.h
File chromeos/login/login_state.h (right):

https://codereview.chromium.org/13495003/diff/87004/chromeos/login/login_stat...
chromeos/login/login_state.h:21: LOGGED_IN_ACTIVE   // A user has logged in
On 2013/04/10 08:09:22, Nikita Kostylev wrote:
> I like that it is now called "active". In my multi-profiles change I'll be
> replacing GetLoggedInUser() to GetActiveUser().
Good to hear. I struggled with the name for a bit; LOGGED_IN_LOGGED_IN seemed
pretty strange, and _COMPLETE seemed potentially misleading / confusing.

[bartfab (slow), 2013-04-10 18:51:20]

LGTM++

Thanks so much for taking the time to implement this in a really nice way. Just
a couple of nits, then ship it.

https://chromiumcodereview.appspot.com/13495003/diff/110001/chrome/browser/ch...
File chrome/browser/chromeos/background/ash_user_wallpaper_delegate.cc (right):

https://chromiumcodereview.appspot.com/13495003/diff/110001/chrome/browser/ch...
chrome/browser/chromeos/background/ash_user_wallpaper_delegate.cc:91: user_type
== LoginState::LOGGED_IN_USER_LOCALLY_MANAGED) {
Nice one. I think you actually fixed a bug there: The old code allowed the
public session user to modify the wallpaper which I think we do not actually
want to happen.

https://chromiumcodereview.appspot.com/13495003/diff/110001/chrome/browser/ch...
File chrome/browser/chromeos/login/user_manager_impl.cc (right):

https://chromiumcodereview.appspot.com/13495003/diff/110001/chrome/browser/ch...
chrome/browser/chromeos/login/user_manager_impl.cc:1312: if (!logged_in_user_ ||
!session_started_)
You can simplify this: |session_started_| always implies a |logged_in_user_|

And at that point, it may all even neatly fit on one line as a ternary operator.

https://chromiumcodereview.appspot.com/13495003/diff/110001/chrome/browser/go...
File chrome/browser/google_apis/auth_service.cc (right):

https://chromiumcodereview.appspot.com/13495003/diff/110001/chrome/browser/go...
chrome/browser/google_apis/auth_service.cc:299: user_type ==
chromeos::LoginState::LOGGED_IN_USER_RETAIL_MODE) {
Can you add a TODO(bartfab): Check whether public session users should be
excluded here as well?

I cannot quite figure it out by looking at this snippet, but I think it is
possible we want to filter out public session users as well.

https://chromiumcodereview.appspot.com/13495003/diff/110001/chromeos/login/lo...
File chromeos/login/login_state.h (right):

https://chromiumcodereview.appspot.com/13495003/diff/110001/chromeos/login/lo...
chromeos/login/login_state.h:9: #include "base/compiler_specific.h"
Nit: Where is this used?

https://chromiumcodereview.appspot.com/13495003/diff/110001/chromeos/login/lo...
File chromeos/login/login_state_unittest.cc (right):

https://chromiumcodereview.appspot.com/13495003/diff/110001/chromeos/login/lo...
chromeos/login/login_state_unittest.cc:26: virtual void SetUp() OVERRIDE {
This file should actually include base/compiler_specific.h.

[stevenjb, 2013-04-10 20:46:41]

xiyuan@ - can you look at the diffs for:

ash_user_wallpaper_delegate.cc
auth_service.cc
And check the logic in LoginState::IsUserAuthenticated()

to make sure I didn't break any logic?

https://chromiumcodereview.appspot.com/13495003/diff/110001/chrome/browser/ch...
File chrome/browser/chromeos/login/user_manager_impl.cc (right):

https://chromiumcodereview.appspot.com/13495003/diff/110001/chrome/browser/ch...
chrome/browser/chromeos/login/user_manager_impl.cc:1312: if (!logged_in_user_ ||
!session_started_)
On 2013/04/10 18:51:20, bartfab wrote:
> You can simplify this: |session_started_| always implies a |logged_in_user_|
> 
> And at that point, it may all even neatly fit on one line as a ternary
operator.
OK, I'll add a DCHECK to ensure that. Done.

https://chromiumcodereview.appspot.com/13495003/diff/110001/chrome/browser/go...
File chrome/browser/google_apis/auth_service.cc (right):

https://chromiumcodereview.appspot.com/13495003/diff/110001/chrome/browser/go...
chrome/browser/google_apis/auth_service.cc:299: user_type ==
chromeos::LoginState::LOGGED_IN_USER_RETAIL_MODE) {
On 2013/04/10 18:51:20, bartfab wrote:
> Can you add a TODO(bartfab): Check whether public session users should be
> excluded here as well?
> 
> I cannot quite figure it out by looking at this snippet, but I think it is
> possible we want to filter out public session users as well.

Yeah, I thought about that, and you're right, we should fix it here or add a
TODO. I'll ping Zel and fix it or add a comment.

https://chromiumcodereview.appspot.com/13495003/diff/110001/chromeos/login/lo...
File chromeos/login/login_state.h (right):

https://chromiumcodereview.appspot.com/13495003/diff/110001/chromeos/login/lo...
chromeos/login/login_state.h:9: #include "base/compiler_specific.h"
On 2013/04/10 18:51:20, bartfab wrote:
> Nit: Where is this used?

Done.

https://chromiumcodereview.appspot.com/13495003/diff/110001/chromeos/login/lo...
File chromeos/login/login_state_unittest.cc (right):

https://chromiumcodereview.appspot.com/13495003/diff/110001/chromeos/login/lo...
chromeos/login/login_state_unittest.cc:26: virtual void SetUp() OVERRIDE {
On 2013/04/10 18:51:20, bartfab wrote:
> This file should actually include base/compiler_specific.h.

Done.

[xiyuan, 2013-04-10 20:55:01]

LGTM

[stevenjb, 2013-04-10 21:16:35]

https://chromiumcodereview.appspot.com/13495003/diff/127001/chrome/browser/ch...
File chrome/browser/chromeos/login/user_manager_impl.cc (right):

https://chromiumcodereview.appspot.com/13495003/diff/127001/chrome/browser/ch...
chrome/browser/chromeos/login/user_manager_impl.cc:1312: logged_in_state =
logged_in_user_ ? LoginState::LOGGED_IN_ACTIVE
It turns out that we actually want to check just logged_in_user_ here (which
matches the logic in UserManagerImpl::IsUserLoggedIn()). I discovered while
testing that waiting for session_started_ breaks the cert loading logic. 

For now, LoginState is entirely independent of session_started_ (which, now that
I'm a bit more familiar with the code, makes sense).

In a follow up CL, it might make sense to add a LOGGED_IN_SESSION_STARTED state
to LoginState, but that isn't currently needed so I will not further complicate
this CL!

[commit-bot: I haz the power, 2013-04-11 16:20:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevenjb@chromium.org/13495003/130001

[commit-bot: I haz the power, 2013-04-11 16:20:32]

Failed to apply patch for
chrome/browser/chromeos/extensions/file_browser_event_router.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/chromeos/extensions/file_browser_event_router.cc
  Hunk #1 FAILED at 20.
  Hunk #2 FAILED at 34.
  Hunk #3 FAILED at 310.
  3 out of 3 hunks FAILED -- saving rejects to file
chrome/browser/chromeos/extensions/file_browser_event_router.cc.rej

Patch:       chrome/browser/chromeos/extensions/file_browser_event_router.cc
Index: chrome/browser/chromeos/extensions/file_browser_event_router.cc
diff --git a/chrome/browser/chromeos/extensions/file_browser_event_router.cc
b/chrome/browser/chromeos/extensions/file_browser_event_router.cc
index
1f4cb7d90697d9e0c34e9617e51a2a88123dba25..5222d80f309426e1e4d2ba6a02d45aa83ffe67db
100644
--- a/chrome/browser/chromeos/extensions/file_browser_event_router.cc
+++ b/chrome/browser/chromeos/extensions/file_browser_event_router.cc
@@ -20,7 +20,6 @@
 #include "chrome/browser/chromeos/extensions/file_manager/file_manager_util.h"
 #include "chrome/browser/chromeos/login/base_login_display_host.h"
 #include "chrome/browser/chromeos/login/screen_locker.h"
-#include "chrome/browser/chromeos/login/user_manager.h"
 #include "chrome/browser/chromeos/net/connectivity_state_helper.h"
 #include "chrome/browser/extensions/event_names.h"
 #include "chrome/browser/extensions/event_router.h"
@@ -34,6 +33,7 @@
 #include "chromeos/dbus/dbus_thread_manager.h"
 #include "chromeos/dbus/power_manager_client.h"
 #include "chromeos/dbus/session_manager_client.h"
+#include "chromeos/login/login_state.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/notification_source.h"
 #include "grit/generated_resources.h"
@@ -310,8 +310,10 @@ void FileBrowserEventRouter::ObserveFileSystemEvents() {
     NOTREACHED();
     return;
   }
-  if (!chromeos::UserManager::Get()->IsUserLoggedIn())
+  if (!chromeos::LoginState::IsInitialized() ||
+      !chromeos::LoginState::Get()->IsUserLoggedIn()) {
     return;
+  }
 
   DiskMountManager* disk_mount_manager = DiskMountManager::GetInstance();
   if (disk_mount_manager) {

[commit-bot: I haz the power, 2013-04-11 16:21:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevenjb@chromium.org/13495003/134002

[commit-bot: I haz the power, 2013-04-11 16:21:59]

Presubmit check for 13495003-134002 failed and returned exit status 1.

INFO:root:Found 25 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/ash/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/browser/extensions/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/browser/ui/webui/PRESUBMIT.py

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    chrome/browser/ui/webui/screenshot_source.cc
    chrome/browser/automation/automation_provider.cc
    chrome/browser/ui/ash/chrome_shell_delegate_chromeos.cc
    chrome/browser/google_apis/auth_service.cc
    chrome/browser/ui/ash/ash_init.cc
    chrome/browser/extensions/extension_system.cc

Presubmit checks took 2.9s to calculate.

[commit-bot: I haz the power, 2013-04-11 16:26:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevenjb@chromium.org/13495003/134002

[commit-bot: I haz the power, 2013-04-11 18:58:22]

Change committed as 193699