Add LoginState class to src/chromeos/login

chromeos/login/login_state.h

+// Copyright (c) 2013 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CHROMEOS_LOGIN_LOGIN_STATE_H_
+#define CHROMEOS_LOGIN_LOGIN_STATE_H_
+
+#include "base/observer_list.h"
+#include "chromeos/chromeos_export.h"
+#include "chromeos/dbus/session_manager_client.h"
+
+namespace chromeos {
+
+// Tracks the login state of chrome, accessible to Ash and other chromeos code.
+class CHROMEOS_EXPORT LoginState : public SessionManagerClient::Observer {
+ public:
+  enum State {

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

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.

+    LOGGED_IN_OOBE,             // Out of box experience not completed

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

Full stops missing at the ends of all comments.

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

Not a requirement. This is a phrase not a sentence.

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

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

+    LOGGED_IN_NONE,             // Not logged in

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

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

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

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?

+    LOGGED_IN_LOCKED,           // A user has locked the screen
+    LOGGED_IN_USER,             // A normal user is logged in

[Nikita (slow), 2013/04/05 04:08:53]

nit: normal > regular
To match naming that we have in User

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

Done.

+    LOGGED_IN_OWNER,            // The owner of the device is logged in
+    LOGGED_IN_GUEST,            // A guest is logged in (i.e. incognito)

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

Nit: A better comment would be "Guest session in progress."

+    LOGGED_IN_RETAIL_MODE,      // Is in retail mode

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

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."

+    LOGGED_IN_PUBLIC,           // A public account is logged in

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

Nit: A better comment would be "Public session in progress." (we recently
renamed public accounts to public sessions).

+    LOGGED_IN_KIOSK_APP         // Is in kiosk app mode

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

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"?

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

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

+  };
+
+  class Observer {
+   public:
+    // Called when the login state changes.
+    virtual void LoginStateChanged(LoginState::State state) = 0;

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

Nit: No need for the |LoginState::| prefix here.

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

Done.

+
+   protected:
+    virtual ~Observer() {}
+  };
+
+  // Manage singleton instance. Must be initialized after DBusThreadManager.
+  static void Initialize();
+  static void Shutdown();
+  static LoginState* Get();

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

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.

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

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.

+  static bool IsInitialized();
+
+  // Add/remove observers.
+  void AddObserver(Observer* observer);
+  void RemoveObserver(Observer* observer);
+
+  // Set the base login state to |state|. Note, other factors (e.g. screen lock)
+  // may affect the result of GetLoginState().
+  void SetLoginState(LoginState::State state);
+
+  // Returns the current login state.
+  LoginState::State GetLoginState() const;
+
+  // Returns true if |state_| is a logged in state.
+  bool IsUserLoggedIn();
+
+  // Tracks wherther the user is logged in and the browser process has started.

[Nikita (slow), 2013/04/05 04:08:53]

Browser instance has been created to be more specific.

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

Done.

+  void SetSessionStarted(bool session_started);
+  bool IsSessionStarted() const;
+
+ private:
+  LoginState();
+  virtual ~LoginState();
+
+  // SessionManagerClient::Observer

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

Nit: Full stop or colon at the end of line.

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

We use this pattern throughout the code for label comments that are not
sentences.

+  virtual void LockScreen() OVERRIDE;

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

#include "base/compiler_specific.h"

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

Done.

+  virtual void UnlockScreen() OVERRIDE;
+
+  void NotifyObservers();
+
+  State state_;
+  bool session_started_;
+  bool screen_locked_;
+  ObserverList<LoginState::Observer> observer_list_;

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

Nit: No need for the |LoginState::| prefix here.

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

Done.

+
+  DISALLOW_COPY_AND_ASSIGN(LoginState);

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

#include "base/basictypes.h"

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

Done.

+};
+
+}  // namespace chromeos
+
+#endif  // CHROMEOS_LOGIN_LOGIN_STATE_H_