Rename AuthenticationService::HaveAccountsChanged to HaveAccountsChangedWhileInBackground.

There was significant confusion about the name of the function AuthenticationService::HaveAccountsChanged
as it was not clear that it was only tracking if the accounts changed while Chrome was in
background. This CL attempts to rename the variable and all the corresponding functions to
make more clear its semantics.

Bug: None

Change-Id: Ic5a609de5ab04e60e9acadedbf72c1b978125e9f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1827157
Commit-Queue: Mihai Sardarescu <msarda@chromium.org>
Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
Cr-Commit-Position: refs/heads/master@{#707352}

ios/chrome/browser/signin/authentication_service.h

@@ -66,7 +66,7 @@ class AuthenticationService : public KeyedService,
   // they were stored in the browser state prefs. This storing happens every
   // time the accounts change in foreground.
   // This reloads the cached accounts if the information might be stale.
-  virtual bool HaveAccountsChanged() const;
+  virtual bool HaveAccountsChangedWhileInBackground() const;
 
   // ChromeIdentity management
 
@@ -126,14 +126,23 @@ class AuthenticationService : public KeyedService,
   // ids.
   void MigrateAccountsStoredInPrefsIfNeeded();
 
-  // Stores the token service accounts in the browser state prefs.
-  void StoreAccountsInPrefs();
-
-  // Gets the accounts previously stored in the  browser state prefs.
-  std::vector<std::string> GetAccountsInPrefs();
-
-  // Returns the cached MDM infos associated with |identity|. If the cache is
-  // stale for |identity|, the entry might be removed.
+  // Saves the last known list of accounts from the token service when
+  // the app is in foreground. This can be used when app comes back from
+  // background to detect if any changes occurred to the list. Must only
+  // be called when the application is in foreground.
+  // See HaveAccountsChangesWhileInBackground().
+  void StoreKnownAccountsWhileInForeground();
+
+  // Gets the accounts previously stored as the foreground accounts in the
+  // browser state prefs.
+  // Returns the list of previously stored known accounts. This list
+  // is only updated when the app is in foreground and used to detect
+  // if any change occurred while the app was in background.
+  // See HaveAccountsChangesWhileInBackground().
+  std::vector<std::string> GetLastKnownAccountsFromForeground();
+
+  // Returns the cached MDM infos associated with |identity|. If the cache
+  // is stale for |identity|, the entry might be removed.
   NSDictionary* GetCachedMDMInfo(ChromeIdentity* identity) const;
 
   // Handles an MDM notification |user_info| associated with |identity|.
@@ -147,7 +156,7 @@ class AuthenticationService : public KeyedService,
   //
   // |in_foreground| indicates whether the application was in foreground when
   // the identity list change notification was received.
-  void HandleIdentityListChanged(bool in_foreground);
+  void HandleIdentityListChanged();
 
   // Verifies that the authenticated user is still associated with a valid
   // ChromeIdentity. This method must only be called when the user is
@@ -177,6 +186,9 @@ class AuthenticationService : public KeyedService,
   // or when the application is entering foregorund.
   void UpdateHaveAccountsChangedWhileInBackground();
 
+  // Returns whether the application is currently in the foreground or not.
+  bool InForeground() const;
+
   // signin::IdentityManager::Observer implementation.
   void OnEndBatchOfRefreshTokenStateChanges() override;
 
@@ -203,7 +215,7 @@ class AuthenticationService : public KeyedService,
   // Whether the accounts have changed while the AuthenticationService was in
   // background. When the AuthenticationService is in background, this value
   // cannot be trusted.
-  bool have_accounts_changed_ = false;
+  bool have_accounts_changed_while_in_background_ = false;
 
   // Whether the AuthenticationService is currently reloading credentials, used
   // to avoid an infinite reloading loop.

ios/chrome/browser/signin/authentication_service.mm

@@ -123,7 +123,7 @@ void AuthenticationService::Shutdown() {
 }
 
 void AuthenticationService::OnApplicationWillEnterForeground() {
-  if (identity_manager_observer_.IsObservingSources())
+  if (InForeground())
     return;
 
   identity_manager_observer_.Add(identity_manager_);
@@ -133,7 +133,7 @@ void AuthenticationService::OnApplicationWillEnterForeground() {
   // changed (both are done by |UpdateHaveAccountsChangedWhileInBackground|).
   // After that, save the current list of accounts.
   UpdateHaveAccountsChangedWhileInBackground();
-  StoreAccountsInPrefs();
+  StoreKnownAccountsWhileInForeground();
 
   if (IsAuthenticated()) {
     bool sync_enabled = sync_setup_service_->IsSyncEnabled();
@@ -166,13 +166,23 @@ void AuthenticationService::OnApplicationWillEnterForeground() {
 }
 
 void AuthenticationService::OnApplicationDidEnterBackground() {
-  if (!identity_manager_observer_.IsObservingSources())
+  if (!InForeground())
     return;
 
   // Stop observing |identity_manager_| when in the background. Note that
   // this allows checking whether the app is in background without having a
   // separate bool by using identity_manager_observer_.IsObservingSources().
   identity_manager_observer_.Remove(identity_manager_);
+
+  // Reset the state |have_accounts_changed_while_in_background_| as the
+  // application just entered background.
+  have_accounts_changed_while_in_background_ = false;
+}
+
+bool AuthenticationService::InForeground() const {
+  // The application is in foreground when |identity_manager_observer_| is
+  // observing sources.
+  return identity_manager_observer_.IsObservingSources();
 }
 
 void AuthenticationService::SetPromptForSignIn() {
@@ -191,8 +201,9 @@ void AuthenticationService::UpdateHaveAccountsChangedWhileInBackground() {
   // Load accounts from preference before synchronizing the accounts with
   // the system, otherwiser we would never detect any changes to the list
   // of accounts.
-  std::vector<std::string> old_accounts = GetAccountsInPrefs();
-  std::sort(old_accounts.begin(), old_accounts.end());
+  std::vector<std::string> last_foreground_accounts =
+      GetLastKnownAccountsFromForeground();
+  std::sort(last_foreground_accounts.begin(), last_foreground_accounts.end());
 
   // Reload credentials to ensure the accounts from the token service are
   // up-to-date.
@@ -201,18 +212,19 @@ void AuthenticationService::UpdateHaveAccountsChangedWhileInBackground() {
   // must be set to true.
   ReloadCredentialsFromIdentities(/*should_prompt=*/true);
 
-  std::vector<CoreAccountInfo> new_accounts_info =
+  std::vector<CoreAccountInfo> current_accounts_info =
       identity_manager_->GetAccountsWithRefreshTokens();
-  std::vector<std::string> new_accounts;
-  for (const CoreAccountInfo& account_info : new_accounts_info)
-    new_accounts.push_back(account_info.account_id);
-  std::sort(new_accounts.begin(), new_accounts.end());
+  std::vector<std::string> current_accounts;
+  for (const CoreAccountInfo& account_info : current_accounts_info)
+    current_accounts.push_back(account_info.account_id);
+  std::sort(current_accounts.begin(), current_accounts.end());
 
-  have_accounts_changed_ = old_accounts != new_accounts;
+  have_accounts_changed_while_in_background_ =
+      last_foreground_accounts != current_accounts;
 }
 
-bool AuthenticationService::HaveAccountsChanged() const {
-  return have_accounts_changed_;
+bool AuthenticationService::HaveAccountsChangedWhileInBackground() const {
+  return have_accounts_changed_while_in_background_;
 }
 
 void AuthenticationService::MigrateAccountsStoredInPrefsIfNeeded() {
@@ -227,7 +239,7 @@ void AuthenticationService::MigrateAccountsStoredInPrefsIfNeeded() {
     return;
   }
 
-  std::vector<std::string> account_ids = GetAccountsInPrefs();
+  std::vector<std::string> account_ids = GetLastKnownAccountsFromForeground();
   std::vector<base::Value> accounts_pref_value;
   for (const std::string& account_id : account_ids) {
     if (identity_manager_->HasAccountWithRefreshToken(account_id)) {
@@ -235,8 +247,8 @@ void AuthenticationService::MigrateAccountsStoredInPrefsIfNeeded() {
     } else {
       // The account for |email| was removed since the last application cold
       // start. Insert |kFakeAccountIdForRemovedAccount| to ensure
-      // |have_accounts_changed_| will be set to true and the removal won't be
-      // silently ignored.
+      // |have_accounts_changed_while_in_background_| will be set to true and
+      // the removal won't be silently ignored.
       accounts_pref_value.emplace_back(kFakeAccountIdForRemovedAccount);
     }
   }
@@ -245,7 +257,8 @@ void AuthenticationService::MigrateAccountsStoredInPrefsIfNeeded() {
   pref_service_->SetBoolean(prefs::kSigninLastAccountsMigrated, true);
 }
 
-void AuthenticationService::StoreAccountsInPrefs() {
+void AuthenticationService::StoreKnownAccountsWhileInForeground() {
+  DCHECK(InForeground());
   std::vector<CoreAccountInfo> accounts(
       identity_manager_->GetAccountsWithRefreshTokens());
   std::vector<base::Value> accounts_pref_value;
@@ -255,7 +268,8 @@ void AuthenticationService::StoreAccountsInPrefs() {
                      base::Value(std::move(accounts_pref_value)));
 }
 
-std::vector<std::string> AuthenticationService::GetAccountsInPrefs() {
+std::vector<std::string>
+AuthenticationService::GetLastKnownAccountsFromForeground() {
   const base::Value* accounts_pref =
       pref_service_->GetList(prefs::kSigninLastAccounts);
 
@@ -418,7 +432,7 @@ void AuthenticationService::OnEndBatchOfRefreshTokenStateChanges() {
   // Accounts maybe have been excluded or included from the current browser
   // state, without any change to the identity list.
   // Store the current list of accounts to make sure it is up-to-date.
-  StoreAccountsInPrefs();
+  StoreKnownAccountsWhileInForeground();
 }
 
 void AuthenticationService::OnIdentityListChanged() {
@@ -429,8 +443,7 @@ void AuthenticationService::OnIdentityListChanged() {
   base::ThreadTaskRunnerHandle::Get()->PostTask(
       FROM_HERE,
       base::BindOnce(&AuthenticationService::HandleIdentityListChanged,
-                     GetWeakPtr(),
-                     identity_manager_observer_.IsObservingSources()));
+                     GetWeakPtr()));
 }
 
 bool AuthenticationService::HandleMDMNotification(ChromeIdentity* identity,
@@ -494,10 +507,10 @@ void AuthenticationService::OnChromeIdentityServiceWillBeDestroyed() {
   identity_service_observer_.RemoveAll();
 }
 
-void AuthenticationService::HandleIdentityListChanged(bool in_foreground) {
+void AuthenticationService::HandleIdentityListChanged() {
   // Only notify the user about an identity change notification if the
   // application was in background.
-  if (in_foreground) {
+  if (InForeground()) {
     // Do not update the have accounts change state when in foreground.
     ReloadCredentialsFromIdentities(/*should_prompt=*/false);
     return;

ios/chrome/browser/signin/authentication_service_fake.h

@@ -31,9 +31,9 @@ class AuthenticationServiceFake : public AuthenticationService {
   void SignOut(signin_metrics::ProfileSignout signout_source,
                ProceduralBlock completion) override;
 
-  void SetHaveAccountsChanged(bool changed);
+  void SetHaveAccountsChangedWhileInBackground(bool changed);
 
-  bool HaveAccountsChanged() const override;
+  bool HaveAccountsChangedWhileInBackground() const override;
 
   bool IsAuthenticated() const override;
 
@@ -46,7 +46,7 @@ class AuthenticationServiceFake : public AuthenticationService {
                             syncer::SyncService* sync_service);
 
   __strong ChromeIdentity* authenticated_identity_;
-  bool have_accounts_changed_;
+  bool have_accounts_changed_while_in_background_;
 };
 
 #endif  // IOS_CHROME_BROWSER_SIGNIN_AUTHENTICATION_SERVICE_FAKE_H_

ios/chrome/browser/signin/authentication_service_fake.mm

@@ -29,7 +29,7 @@ AuthenticationServiceFake::AuthenticationServiceFake(
                             sync_setup_service,
                             identity_manager,
                             sync_service),
-      have_accounts_changed_(false) {}
+      have_accounts_changed_while_in_background_(false) {}
 
 AuthenticationServiceFake::~AuthenticationServiceFake() {}
 
@@ -48,12 +48,13 @@ void AuthenticationServiceFake::SignOut(
     completion();
 }
 
-void AuthenticationServiceFake::SetHaveAccountsChanged(bool changed) {
-  have_accounts_changed_ = changed;
+void AuthenticationServiceFake::SetHaveAccountsChangedWhileInBackground(
+    bool changed) {
+  have_accounts_changed_while_in_background_ = changed;
 }
 
-bool AuthenticationServiceFake::HaveAccountsChanged() const {
-  return have_accounts_changed_;
+bool AuthenticationServiceFake::HaveAccountsChangedWhileInBackground() const {
+  return have_accounts_changed_while_in_background_;
 }
 
 bool AuthenticationServiceFake::IsAuthenticated() const {

ios/chrome/browser/signin/authentication_service_unittest.mm

@@ -108,12 +108,12 @@ class AuthenticationServiceTest : public PlatformTest {
     EXPECT_CALL(*sync_setup_service_mock(), PrepareForFirstSyncSetup());
   }
 
-  void StoreAccountsInPrefs() {
-    authentication_service()->StoreAccountsInPrefs();
+  void StoreKnownAccountsWhileInForeground() {
+    authentication_service()->StoreKnownAccountsWhileInForeground();
   }
 
-  std::vector<std::string> GetAccountsInPrefs() {
-    return authentication_service()->GetAccountsInPrefs();
+  std::vector<std::string> GetLastKnownAccountsFromForeground() {
+    return authentication_service()->GetLastKnownAccountsFromForeground();
   }
 
   void FireApplicationWillEnterForeground() {
@@ -249,7 +249,7 @@ TEST_F(AuthenticationServiceTest, TestHandleForgottenIdentityPromptSignIn) {
 
 TEST_F(AuthenticationServiceTest, StoreAndGetAccountsInPrefs) {
   // Profile starts empty.
-  std::vector<std::string> accounts = GetAccountsInPrefs();
+  std::vector<std::string> accounts = GetLastKnownAccountsFromForeground();
   EXPECT_TRUE(accounts.empty());
 
   // Sign in.
@@ -258,12 +258,11 @@ TEST_F(AuthenticationServiceTest, StoreAndGetAccountsInPrefs) {
 
   // Store the accounts and get them back from the prefs. They should be the
   // same as the token service accounts.
-  StoreAccountsInPrefs();
-  accounts = GetAccountsInPrefs();
+  StoreKnownAccountsWhileInForeground();
+  accounts = GetLastKnownAccountsFromForeground();
   ASSERT_EQ(2u, accounts.size());
-
-      EXPECT_EQ("foo2ID", accounts[0]);
-      EXPECT_EQ("fooID", accounts[1]);
+  EXPECT_EQ("foo2ID", accounts[0]);
+  EXPECT_EQ("fooID", accounts[1]);
 }
 
 TEST_F(AuthenticationServiceTest,
@@ -282,9 +281,8 @@ TEST_F(AuthenticationServiceTest,
       identity_manager()->GetAccountsWithRefreshTokens();
   std::sort(accounts.begin(), accounts.end(), account_compare_func);
   ASSERT_EQ(2u, accounts.size());
-
-      EXPECT_EQ("foo2ID", accounts[0].account_id);
-      EXPECT_EQ("fooID", accounts[1].account_id);
+  EXPECT_EQ("foo2ID", accounts[0].account_id);
+  EXPECT_EQ("fooID", accounts[1].account_id);
 
   // Simulate a switching to background and back to foreground, triggering a
   // credentials reload.
@@ -296,13 +294,14 @@ TEST_F(AuthenticationServiceTest,
   accounts = identity_manager()->GetAccountsWithRefreshTokens();
   std::sort(accounts.begin(), accounts.end(), account_compare_func);
   ASSERT_EQ(3u, accounts.size());
-      EXPECT_EQ("foo2ID", accounts[0].account_id);
-      EXPECT_EQ("foo3ID", accounts[1].account_id);
-      EXPECT_EQ("fooID", accounts[2].account_id);
+  EXPECT_EQ("foo2ID", accounts[0].account_id);
+  EXPECT_EQ("foo3ID", accounts[1].account_id);
+  EXPECT_EQ("fooID", accounts[2].account_id);
 }
 
 TEST_F(AuthenticationServiceTest, HaveAccountsChanged_Default) {
-  EXPECT_FALSE(authentication_service()->HaveAccountsChanged());
+  EXPECT_FALSE(
+      authentication_service()->HaveAccountsChangedWhileInBackground());
 }
 
 TEST_F(AuthenticationServiceTest, HaveAccountsChanged_NoChange) {
@@ -315,15 +314,18 @@ TEST_F(AuthenticationServiceTest, HaveAccountsChanged_NoChange) {
 
   // If an account is added while the application is in foreground, then the
   // have accounts changed state should stay false.
-  EXPECT_FALSE(authentication_service()->HaveAccountsChanged());
+  EXPECT_FALSE(
+      authentication_service()->HaveAccountsChangedWhileInBackground());
 
   // Backgrounding the app should not change the have accounts changed state.
   FireApplicationDidEnterBackground();
-  EXPECT_FALSE(authentication_service()->HaveAccountsChanged());
+  EXPECT_FALSE(
+      authentication_service()->HaveAccountsChangedWhileInBackground());
 
   // Foregrounding the app should not change the have accounts changed state.
   FireApplicationWillEnterForeground();
-  EXPECT_FALSE(authentication_service()->HaveAccountsChanged());
+  EXPECT_FALSE(
+      authentication_service()->HaveAccountsChangedWhileInBackground());
 }
 
 TEST_F(AuthenticationServiceTest, HaveAccountsChanged_ChangedInBackground) {
@@ -333,14 +335,15 @@ TEST_F(AuthenticationServiceTest, HaveAccountsChanged_ChangedInBackground) {
   identity_service()->AddIdentities(@[ @"foo3" ]);
   FireIdentityListChanged();
   base::RunLoop().RunUntilIdle();
-  EXPECT_FALSE(authentication_service()->HaveAccountsChanged());
+  EXPECT_FALSE(
+      authentication_service()->HaveAccountsChangedWhileInBackground());
 
   // Simulate a switching to background and back to foreground, changing the
   // accounts while in background (no notification fired by |identity_service|).
   FireApplicationDidEnterBackground();
   identity_service()->AddIdentities(@[ @"foo4" ]);
   FireApplicationWillEnterForeground();
-  EXPECT_TRUE(authentication_service()->HaveAccountsChanged());
+  EXPECT_TRUE(authentication_service()->HaveAccountsChangedWhileInBackground());
 }
 
 TEST_F(AuthenticationServiceTest, HaveAccountsChanged_CalledInBackground) {
@@ -350,7 +353,8 @@ TEST_F(AuthenticationServiceTest, HaveAccountsChanged_CalledInBackground) {
   identity_service()->AddIdentities(@[ @"foo3" ]);
   FireIdentityListChanged();
   base::RunLoop().RunUntilIdle();
-  EXPECT_FALSE(authentication_service()->HaveAccountsChanged());
+  EXPECT_FALSE(
+      authentication_service()->HaveAccountsChangedWhileInBackground());
 
   // Simulate a switching to background, changing the accounts while in
   // background.
@@ -358,11 +362,11 @@ TEST_F(AuthenticationServiceTest, HaveAccountsChanged_CalledInBackground) {
   identity_service()->AddIdentities(@[ @"foo4" ]);
   FireIdentityListChanged();
   base::RunLoop().RunUntilIdle();
-  EXPECT_TRUE(authentication_service()->HaveAccountsChanged());
+  EXPECT_TRUE(authentication_service()->HaveAccountsChangedWhileInBackground());
 
   // Entering foreground should not change the have accounts changed state.
   FireApplicationWillEnterForeground();
-  EXPECT_TRUE(authentication_service()->HaveAccountsChanged());
+  EXPECT_TRUE(authentication_service()->HaveAccountsChangedWhileInBackground());
 }
 
 // Regression test for http://crbug.com/1006717
@@ -373,7 +377,8 @@ TEST_F(AuthenticationServiceTest, HaveAccountsChanged_ResetOntwoBackgrounds) {
   identity_service()->AddIdentities(@[ @"foo3" ]);
   FireIdentityListChanged();
   base::RunLoop().RunUntilIdle();
-  EXPECT_FALSE(authentication_service()->HaveAccountsChanged());
+  EXPECT_FALSE(
+      authentication_service()->HaveAccountsChangedWhileInBackground());
 
   // Simulate a switching to background, changing the accounts while in
   // background.
@@ -386,14 +391,15 @@ TEST_F(AuthenticationServiceTest, HaveAccountsChanged_ResetOntwoBackgrounds) {
   // When entering foreground, the have accounts changed state should be
   // updated.
   FireApplicationWillEnterForeground();
-  EXPECT_TRUE(authentication_service()->HaveAccountsChanged());
+  EXPECT_TRUE(authentication_service()->HaveAccountsChangedWhileInBackground());
 
   // Backgrounding and foregrounding the application a second time should update
   // the list of accounts in |kSigninLastAccounts| and should reset the have
   // account changed state.
   FireApplicationDidEnterBackground();
   FireApplicationWillEnterForeground();
-  EXPECT_FALSE(authentication_service()->HaveAccountsChanged());
+  EXPECT_FALSE(
+      authentication_service()->HaveAccountsChangedWhileInBackground());
 }
 
 TEST_F(AuthenticationServiceTest, IsAuthenticatedBackground) {

ios/chrome/browser/ui/authentication/signed_in_accounts_view_controller.mm

@@ -206,11 +206,12 @@ BOOL gSignedInAccountsViewControllerIsShown = NO;
   if (prevSessionInfo.isFirstSessionAfterUpgrade &&
       [prevSessionInfo.previousSessionVersion hasPrefix:@"77."]) {
     // In M77, showing the signed-in account view was disabled due to the fact
-    // that the preferences used to compute authService->HaveAccountsChanged()
-    // were not correctly updated (see crbug.com/1006717).
-    // To avoid user confusion, it is important to avoid showing the signed-in
-    // accounts dialog on the first session after an update from M77 in order
-    // to allow the authentication service to update its internal preferences.
+    // that the preferences used to compute
+    // authService->HaveAccountsChangedWhileInBackground() were not correctly
+    // updated (see crbug.com/1006717). To avoid user confusion, it is important
+    // to avoid showing the signed-in accounts dialog on the first session after
+    // an update from M77 in order to allow the authentication service to update
+    // its internal preferences.
     //
     // TODO(crbug.com/1007990) Remove this code after M81 (revert
     // https://chromium-review.googlesource.com/c/chromium/src/+/1824259 ).
@@ -220,7 +221,8 @@ BOOL gSignedInAccountsViewControllerIsShown = NO;
   AuthenticationService* authService =
       AuthenticationServiceFactory::GetForBrowserState(browserState);
   return !gSignedInAccountsViewControllerIsShown &&
-         authService->IsAuthenticated() && authService->HaveAccountsChanged();
+         authService->IsAuthenticated() &&
+         authService->HaveAccountsChangedWhileInBackground();
 }
 
 #pragma mark Initialization

ios/chrome/browser/ui/authentication/signed_in_accounts_view_controller_unittest.mm

@@ -63,7 +63,7 @@ TEST_F(SignedInAccountsViewControllerTest,
 // have changed.
 TEST_F(SignedInAccountsViewControllerTest,
        ShouldBePresentedForBrowserStateNecessary) {
-  auth_service_->SetHaveAccountsChanged(true);
+  auth_service_->SetHaveAccountsChangedWhileInBackground(true);
   EXPECT_TRUE([SignedInAccountsViewController
       shouldBePresentedForBrowserState:browser_state_.get()]);
 }
@@ -72,7 +72,7 @@ TEST_F(SignedInAccountsViewControllerTest,
 // session after upgrade.
 TEST_F(SignedInAccountsViewControllerTest,
        ShouldBePresentedForBrowserStateAfterUpgrade) {
-  auth_service_->SetHaveAccountsChanged(true);
+  auth_service_->SetHaveAccountsChangedWhileInBackground(true);
 
   {
     [PreviousSessionInfo resetSharedInstanceForTesting];