Remove some instances of NOTIFICATION_SESSION_STARTED

This patch changes SessionManagerObserver::OnPrimaryUserSessionStarted
to notify for non-primary user sessions as well. This makes it align
with NOTIFICATION_SESSION_STARTED.

Two notification observers are updated:
- LocaleChangeGuard
- first_run.cc (some dead code is also deleted)

On[Primary]UserSessionStarted(), and NOTIFICATION_SESSION_STARTED,
are different from UserSessionStateObserver::OnActiveUserChanged
in that they are called when the UI is fully ready. This is appropriate
for the two observers that are updated in this CL, both of which want
to show UI right after login.

Bug: 268984

Change-Id: Iac5ba9541e99a74a29758db6e3c678e1dcbfa630
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1767494Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Commit-Queue: Evan Stade <estade@chromium.org>
Cr-Commit-Position: refs/heads/master@{#690074}

chrome/browser/chromeos/first_run/first_run.cc

@@ -11,7 +11,6 @@
 #include "chrome/browser/apps/app_service/app_launch_params.h"
 #include "chrome/browser/apps/launch_service/launch_service.h"
 #include "chrome/browser/browser_process.h"
-#include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/chromeos/arc/arc_util.h"
 #include "chrome/browser/chromeos/first_run/first_run_controller.h"
 #include "chrome/browser/chromeos/login/ui/login_display_host.h"
@@ -25,17 +24,17 @@
 #include "chrome/common/extensions/extension_constants.h"
 #include "chrome/common/pref_names.h"
 #include "chromeos/constants/chromeos_switches.h"
+#include "chromeos/login/login_state/login_state.h"
 #include "components/arc/arc_prefs.h"
 #include "components/arc/arc_service_manager.h"
 #include "components/pref_registry/pref_registry_syncable.h"
 #include "components/prefs/pref_service.h"
+#include "components/session_manager/core/session_manager.h"
+#include "components/session_manager/core/session_manager_observer.h"
 #include "components/signin/public/identity_manager/account_info.h"
 #include "components/signin/public/identity_manager/identity_manager.h"
 #include "components/sync_preferences/pref_service_syncable.h"
 #include "components/user_manager/user_manager.h"
-#include "content/public/browser/notification_observer.h"
-#include "content/public/browser/notification_registrar.h"
-#include "content/public/browser/notification_service.h"
 #include "content/public/common/content_switches.h"
 #include "extensions/browser/extension_registry.h"
 #include "extensions/common/constants.h"
@@ -106,34 +105,25 @@ void TryLaunchFirstRunDialog(Profile* profile) {
 // Object of this class waits for session start. Then it launches or not
 // launches first-run dialog depending on user prefs and flags. Than object
 // deletes itself.
-class DialogLauncher : public content::NotificationObserver {
+class DialogLauncher : public session_manager::SessionManagerObserver {
  public:
-  explicit DialogLauncher(Profile* profile)
-      : profile_(profile) {
-    DCHECK(profile);
-    registrar_.Add(this,
-                   chrome::NOTIFICATION_SESSION_STARTED,
-                   content::NotificationService::AllSources());
+  DialogLauncher() {
+    session_manager::SessionManager::Get()->AddObserver(this);
   }
 
-  ~DialogLauncher() override {}
-
-  void Observe(int type,
-               const content::NotificationSource& source,
-               const content::NotificationDetails& details) override {
-    DCHECK(type == chrome::NOTIFICATION_SESSION_STARTED);
-    DCHECK(content::Details<const user_manager::User>(details).ptr() ==
-           ProfileHelper::Get()->GetUserByProfile(profile_));
-
-    TryLaunchFirstRunDialog(profile_);
+  ~DialogLauncher() override {
+    session_manager::SessionManager::Get()->RemoveObserver(this);
+  }
 
+  // session_manager::SessionManagerObserver:
+  void OnUserSessionStarted(bool is_primary_user) override {
+    auto* profile = ProfileHelper::Get()->GetProfileByUser(
+        user_manager::UserManager::Get()->GetActiveUser());
+    TryLaunchFirstRunDialog(profile);
     delete this;
   }
 
  private:
-  Profile* profile_;
-  content::NotificationRegistrar registrar_;
-
   DISALLOW_COPY_AND_ASSIGN(DialogLauncher);
 };
 
@@ -147,13 +137,7 @@ void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry) {
 }
 
 void MaybeLaunchDialogAfterSessionStart() {
-  new DialogLauncher(ProfileHelper::Get()->GetProfileByUserUnsafe(
-      user_manager::UserManager::Get()->GetActiveUser()));
-}
-
-void MaybeLaunchDialogImmediately() {
-  TryLaunchFirstRunDialog(ProfileHelper::Get()->GetProfileByUser(
-      user_manager::UserManager::Get()->GetActiveUser()));
+  new DialogLauncher();
 }
 
 void LaunchTutorial() {

chrome/browser/chromeos/first_run/first_run.h

@@ -15,11 +15,6 @@ namespace first_run {
 // Registers preferences related to ChromeOS first-run tutorial.
 void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry);
 
-// Launches the first-run dialog, based on synced user prefs. Unlike
-// MaybeLaunchDialogAfterSessionStart, this function can launch the dialog
-// at any point instead of just before session start.
-void MaybeLaunchDialogImmediately();
-
 // Probably launches first-run dialog after session start depending on synced
 // user prefs. This method should be called after user already logged in but
 // session didn't started yet.

chrome/browser/chromeos/locale_change_guard.cc

@@ -43,11 +43,7 @@ const char* const kSkipShowNotificationLanguages[4] = {"en", "de", "fr", "it"};
 
 }  // anonymous namespace
 
-LocaleChangeGuard::LocaleChangeGuard(Profile* profile)
-    : profile_(profile),
-      reverted_(false),
-      session_started_(false),
-      main_frame_loaded_(false) {
+LocaleChangeGuard::LocaleChangeGuard(Profile* profile) : profile_(profile) {
   DCHECK(profile_);
   DeviceSettingsService::Get()->AddObserver(this);
 }
@@ -58,16 +54,13 @@ LocaleChangeGuard::~LocaleChangeGuard() {
 }
 
 void LocaleChangeGuard::OnLogin() {
-  registrar_.Add(this, chrome::NOTIFICATION_SESSION_STARTED,
-                 content::NotificationService::AllSources());
+  session_observer_.Add(session_manager::SessionManager::Get());
   registrar_.Add(this, content::NOTIFICATION_LOAD_COMPLETED_MAIN_FRAME,
                  content::NotificationService::AllBrowserContextsAndSources());
 }
 
 void LocaleChangeGuard::RevertLocaleChange() {
-  if (profile_ == NULL ||
-      from_locale_.empty() ||
-      to_locale_.empty()) {
+  if (from_locale_.empty() || to_locale_.empty()) {
     NOTREACHED();
     return;
   }
@@ -75,44 +68,30 @@ void LocaleChangeGuard::RevertLocaleChange() {
     return;
   reverted_ = true;
   base::RecordAction(UserMetricsAction("LanguageChange_Revert"));
-  profile_->ChangeAppLocale(
-      from_locale_, Profile::APP_LOCALE_CHANGED_VIA_REVERT);
+  profile_->ChangeAppLocale(from_locale_,
+                            Profile::APP_LOCALE_CHANGED_VIA_REVERT);
   chrome::AttemptUserExit();
 }
 
 void LocaleChangeGuard::Observe(int type,
                                 const content::NotificationSource& source,
                                 const content::NotificationDetails& details) {
-  if (profile_ == NULL) {
-    NOTREACHED();
+  DCHECK_EQ(type, content::NOTIFICATION_LOAD_COMPLETED_MAIN_FRAME);
+  if (profile_ != content::Source<WebContents>(source)->GetBrowserContext())
     return;
-  }
-  switch (type) {
-    case chrome::NOTIFICATION_SESSION_STARTED: {
-      session_started_ = true;
-      registrar_.Remove(this, chrome::NOTIFICATION_SESSION_STARTED,
-                        content::NotificationService::AllSources());
-      if (main_frame_loaded_)
-        Check();
-      break;
-    }
-    case content::NOTIFICATION_LOAD_COMPLETED_MAIN_FRAME: {
-      if (profile_ ==
-          content::Source<WebContents>(source)->GetBrowserContext()) {
-        main_frame_loaded_ = true;
-        // We need to perform locale change check only once, so unsubscribe.
-        registrar_.Remove(this, content::NOTIFICATION_LOAD_COMPLETED_MAIN_FRAME,
-                          content::NotificationService::AllSources());
-        if (session_started_)
-          Check();
-      }
-      break;
-    }
-    default: {
-      NOTREACHED();
-      break;
-    }
-  }
+
+  main_frame_loaded_ = true;
+  // We need to perform locale change check only once, so unsubscribe.
+  registrar_.Remove(this, content::NOTIFICATION_LOAD_COMPLETED_MAIN_FRAME,
+                    content::NotificationService::AllSources());
+  if (session_manager::SessionManager::Get()->IsSessionStarted())
+    Check();
+}
+
+void LocaleChangeGuard::OnUserSessionStarted(bool is_primary_user) {
+  session_observer_.RemoveAll();
+  if (main_frame_loaded_)
+    Check();
 }
 
 void LocaleChangeGuard::OwnershipStatusChanged() {
@@ -171,8 +150,8 @@ void LocaleChangeGuard::Check() {
   // Showing notification.
   if (from_locale_ != from_locale || to_locale_ != to_locale) {
     // Falling back to showing message in current locale.
-    LOG(ERROR) <<
-        "Showing locale change notification in current (not previous) language";
+    LOG(ERROR) << "Showing locale change notification in current (not "
+                  "previous) language";
     PrepareChangingLocale(from_locale, to_locale);
   }
 
@@ -193,9 +172,7 @@ void LocaleChangeGuard::OnResult(ash::LocaleNotificationResult result) {
 }
 
 void LocaleChangeGuard::AcceptLocaleChange() {
-  if (profile_ == NULL ||
-      from_locale_.empty() ||
-      to_locale_.empty()) {
+  if (from_locale_.empty() || to_locale_.empty()) {
     NOTREACHED();
     return;
   }
@@ -216,8 +193,8 @@ void LocaleChangeGuard::AcceptLocaleChange() {
   prefs->SetString(prefs::kApplicationLocaleAccepted, to_locale_);
 }
 
-void LocaleChangeGuard::PrepareChangingLocale(
-    const std::string& from_locale, const std::string& to_locale) {
+void LocaleChangeGuard::PrepareChangingLocale(const std::string& from_locale,
+                                              const std::string& to_locale) {
   std::string cur_locale = g_browser_process->GetApplicationLocale();
   if (!from_locale.empty())
     from_locale_ = from_locale;

chrome/browser/chromeos/locale_change_guard.h

@@ -15,8 +15,11 @@
 #include "base/gtest_prod_util.h"
 #include "base/lazy_instance.h"
 #include "base/memory/weak_ptr.h"
+#include "base/scoped_observer.h"
 #include "base/strings/string16.h"
 #include "chrome/browser/chromeos/settings/device_settings_service.h"
+#include "components/session_manager/core/session_manager.h"
+#include "components/session_manager/core/session_manager_observer.h"
 #include "content/public/browser/notification_observer.h"
 #include "content/public/browser/notification_registrar.h"
 #include "content/public/browser/notification_types.h"
@@ -29,6 +32,7 @@ namespace chromeos {
 // (based on synchronized user preference).  If so: shows notification that
 // allows user to revert change.
 class LocaleChangeGuard : public content::NotificationObserver,
+                          public session_manager::SessionManagerObserver,
                           public DeviceSettingsService::Observer,
                           public base::SupportsWeakPtr<LocaleChangeGuard> {
  public:
@@ -59,6 +63,9 @@ class LocaleChangeGuard : public content::NotificationObserver,
                const content::NotificationSource& source,
                const content::NotificationDetails& details) override;
 
+  // session_manager::SessionManagerObserver:
+  void OnUserSessionStarted(bool is_primary_user) override;
+
   // DeviceSettingsService::Observer
   void OwnershipStatusChanged() override;
 
@@ -72,10 +79,12 @@ class LocaleChangeGuard : public content::NotificationObserver,
   std::string from_locale_;
   std::string to_locale_;
   Profile* profile_;
-  bool reverted_;
-  bool session_started_;
-  bool main_frame_loaded_;
+  bool reverted_ = false;
+  bool main_frame_loaded_ = false;
   content::NotificationRegistrar registrar_;
+  ScopedObserver<session_manager::SessionManager,
+                 session_manager::SessionManagerObserver>
+      session_observer_{this};
 
   DISALLOW_COPY_AND_ASSIGN(LocaleChangeGuard);
 };

chrome/browser/chromeos/system/automatic_reboot_manager.cc

@@ -246,7 +246,10 @@ void AutomaticRebootManager::OnUserActivity(const ui::Event* event) {
                  false));
 }
 
-void AutomaticRebootManager::OnPrimaryUserSessionStarted() {
+void AutomaticRebootManager::OnUserSessionStarted(bool is_primary_user) {
+  if (!is_primary_user)
+    return;
+
   // A session is starting. Stop listening for user activity as it no longer is
   // a relevant criterion.
   if (ui::UserActivityDetector::Get())

chrome/browser/chromeos/system/automatic_reboot_manager.h

@@ -109,7 +109,7 @@ class AutomaticRebootManager : public PowerManagerClient::Observer,
   void OnUserActivity(const ui::Event* event) override;
 
   // session_manager::SessionManagerObserver:
-  void OnPrimaryUserSessionStarted() override;
+  void OnUserSessionStarted(bool is_primary_user) override;
 
   // content::NotificationObserver:
   void Observe(int type,

chrome/browser/ui/ash/assistant/assistant_client.cc

@@ -251,7 +251,7 @@ void AssistantClient::OnUserProfileLoaded(const AccountId& account_id) {
     return;
 
   // Initialize Assistant when primary user profile is loaded so that it could
-  // be used in post oobe steps. OnPrimaryUserSessionStarted() is too late
+  // be used in post oobe steps. OnUserSessionStarted() is too late
   // because it happens after post oobe steps
   Profile* user_profile =
       chromeos::ProfileHelper::Get()->GetProfileByAccountId(account_id);
@@ -261,7 +261,7 @@ void AssistantClient::OnUserProfileLoaded(const AccountId& account_id) {
   MaybeInit(user_profile);
 }
 
-void AssistantClient::OnPrimaryUserSessionStarted() {
-  if (!chromeos::switches::ShouldSkipOobePostLogin())
+void AssistantClient::OnUserSessionStarted(bool is_primary_user) {
+  if (is_primary_user && !chromeos::switches::ShouldSkipOobePostLogin())
     MaybeStartAssistantOptInFlow();
 }

chrome/browser/ui/ash/assistant/assistant_client.h

@@ -97,7 +97,7 @@ class AssistantClient : chromeos::assistant::mojom::Client,
 
   // session_manager::SessionManagerObserver:
   void OnUserProfileLoaded(const AccountId& account_id) override;
-  void OnPrimaryUserSessionStarted() override;
+  void OnUserSessionStarted(bool is_primary_user) override;
 
   mojo::Binding<chromeos::assistant::mojom::Client> client_binding_{this};
 

components/session_manager/core/session_manager.cc

@@ -72,9 +72,9 @@ void SessionManager::SessionStarted() {
 
   session_started_ = true;
 
-  // SessionStarted() could be called in tests without any session created.
-  if (sessions_.size() == 1)
-    NotifyPrimaryUserSessionStarted();
+  bool is_primary = sessions_.size() == 1;
+  for (auto& observer : observers_)
+    observer.OnUserSessionStarted(is_primary);
 }
 
 bool SessionManager::HasSessionForAccountId(
@@ -110,11 +110,6 @@ void SessionManager::NotifyUserProfileLoaded(const AccountId& account_id) {
     observer.OnUserProfileLoaded(account_id);
 }
 
-void SessionManager::NotifyPrimaryUserSessionStarted() {
-  for (auto& observer : observers_)
-    observer.OnPrimaryUserSessionStarted();
-}
-
 void SessionManager::NotifyUserLoggedIn(const AccountId& user_account_id,
                                         const std::string& user_id_hash,
                                         bool browser_restart,

components/session_manager/core/session_manager.h

@@ -64,7 +64,6 @@ class SESSION_EXPORT SessionManager {
 
   // Various helpers to notify observers.
   void NotifyUserProfileLoaded(const AccountId& account_id);
-  void NotifyPrimaryUserSessionStarted();
 
   SessionState session_state() const { return session_state_; }
   const std::vector<Session>& sessions() const { return sessions_; }

components/session_manager/core/session_manager_observer.h

@@ -22,8 +22,12 @@ class SessionManagerObserver : public base::CheckedObserver {
   // Invoked when a user profile is loaded.
   virtual void OnUserProfileLoaded(const AccountId& account_id) {}
 
-  // Invoked when the primary user session is started.
-  virtual void OnPrimaryUserSessionStarted() {}
+  // Invoked when a user session is started. If this is a new user on the
+  // machine this will not be called until after post-login steps are finished
+  // (for example a profile picture has been selected). In contrast,
+  // UserSessionStateObserver::OnActiveUserChanged() is invoked immediately
+  // after the user has logged in.
+  virtual void OnUserSessionStarted(bool is_primary_user) {}
 };
 
 }  // namespace session_manager