Make SigninManagerBase observe OAuth2TokenService

This CL moves SigninManager’s observing of
ProfileOAuth2TokenService::OnRefreshTokensLoaded() to SigninManagerBase,
if’def-d out on ChromeOS.

This is a step in eliminating the inheritance relationship between
SigninManager and SigninManagerBase, as motivated in this design doc:

https://docs.google.com/document/d/15y-Db27BV08vrIyelHB-3CwiAfDYh-FigNKGXrqSWto/edit#heading=h.8jjdy95t6p7x

and described concretely here:

https://docs.google.com/document/d/15y-Db27BV08vrIyelHB-3CwiAfDYh-FigNKGXrqSWto/edit#heading=h.mbkrv9nkb93w

The functionality is ifdef-d out on ChromeOS, as it is currently not
used on that platform.

Bug: 952766
Change-Id: Icafd01997b140c34c560fca642356c41197eb24a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1635614
Commit-Queue: Henrique Ferreiro <hferreiro@igalia.com>
Reviewed-by: Colin Blundell <blundell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#666237}

components/signin/core/browser/signin_manager.cc

@@ -35,7 +35,6 @@ SigninManager::SigninManager(
       weak_pointer_factory_(this) {}
 
 SigninManager::~SigninManager() {
-  token_service()->RemoveObserver(this);
   local_state_pref_registrar_.RemoveAll();
 }
 
@@ -76,10 +75,6 @@ void SigninManager::FinalizeInitBeforeLoadingRefreshTokens(
     SignOutAndKeepAllAccounts(signin_metrics::SIGNIN_PREF_CHANGED_DURING_SIGNIN,
                               signin_metrics::SignoutDelete::IGNORE_METRIC);
   }
-
-  // It is important to only load credentials after starting to observe the
-  // token service.
-  token_service()->AddObserver(this);
 }
 
 void SigninManager::OnGoogleServicesUsernamePatternChanged() {
@@ -113,26 +108,3 @@ bool SigninManager::IsAllowedUsername(const std::string& username) const {
       local_state->GetString(prefs::kGoogleServicesUsernamePattern);
   return identity::IsUsernameAllowedByPattern(username, pattern);
 }
-
-void SigninManager::OnRefreshTokensLoaded() {
-  token_service()->RemoveObserver(this);
-
-  if (account_tracker_service()->GetMigrationState() ==
-      AccountTrackerService::MIGRATION_IN_PROGRESS) {
-    account_tracker_service()->SetMigrationDone();
-  }
-
-  // Remove account information from the account tracker service if needed.
-  if (token_service()->HasLoadCredentialsFinishedWithNoErrors()) {
-    std::vector<AccountInfo> accounts_in_tracker_service =
-        account_tracker_service()->GetAccounts();
-    for (const auto& account : accounts_in_tracker_service) {
-      if (GetAuthenticatedAccountId() != account.account_id &&
-          !token_service()->RefreshTokenIsAvailable(account.account_id)) {
-        DVLOG(0) << "Removed account from account tracker service: "
-                 << account.account_id;
-        account_tracker_service()->RemoveAccount(account.account_id);
-      }
-    }
-  }
-}

components/signin/core/browser/signin_manager.h

@@ -32,19 +32,18 @@
 #include "components/prefs/pref_member.h"
 #include "components/signin/core/browser/account_info.h"
 #include "components/signin/core/browser/account_tracker_service.h"
-#include "components/signin/core/browser/profile_oauth2_token_service.h"
 #include "components/signin/core/browser/signin_manager_base.h"
 #include "components/signin/core/browser/signin_metrics.h"
 #include "net/cookies/canonical_cookie.h"
 
 class PrefService;
+class ProfileOAuth2TokenService;
 
 namespace identity {
 class IdentityManager;
 }  // namespace identity
 
-class SigninManager : public SigninManagerBase,
-                      public OAuth2TokenService::Observer {
+class SigninManager : public SigninManagerBase {
  public:
   SigninManager(SigninClient* client,
                 ProfileOAuth2TokenService* token_service,
@@ -66,9 +65,6 @@ class SigninManager : public SigninManagerBase,
   // Returns true if a signin to Chrome is allowed (by policy or pref).
   bool IsSigninAllowed() const;
 
-  // OAuth2TokenService::Observer:
-  void OnRefreshTokensLoaded() override;
-
   void OnSigninAllowedPrefChanged();
   void OnGoogleServicesUsernamePatternChanged();
 

components/signin/core/browser/signin_manager_base.cc

@@ -41,6 +41,8 @@ SigninManagerBase::SigninManagerBase(
 
 SigninManagerBase::~SigninManagerBase() {
   DCHECK(!observer_);
+
+  token_service_->RemoveObserver(this);
 }
 
 // static
@@ -152,7 +154,10 @@ void SigninManagerBase::Initialize(PrefService* local_state) {
     SetAuthenticatedAccountId(account_id);
   }
   FinalizeInitBeforeLoadingRefreshTokens(local_state);
-  token_service()->LoadCredentials(GetAuthenticatedAccountId());
+  // It is important to only load credentials after starting to observe the
+  // token service.
+  token_service_->AddObserver(this);
+  token_service_->LoadCredentials(GetAuthenticatedAccountId());
 }
 
 void SigninManagerBase::FinalizeInitBeforeLoadingRefreshTokens(
@@ -249,8 +254,7 @@ void SigninManagerBase::ClearObserver() {
 
 #if !defined(OS_CHROMEOS)
 void SigninManagerBase::SignIn(const std::string& username) {
-  AccountInfo info =
-      account_tracker_service()->FindAccountInfoByEmail(username);
+  AccountInfo info = account_tracker_service_->FindAccountInfoByEmail(username);
   DCHECK(!info.gaia.empty());
   DCHECK(!info.email.empty());
 
@@ -348,13 +352,13 @@ void SigninManagerBase::OnSignoutDecisionReached(
   switch (remove_option) {
     case RemoveAccountsOption::kRemoveAllAccounts:
       VLOG(0) << "Revoking all refresh tokens on server. Reason: sign out";
-      token_service()->RevokeAllCredentials(
+      token_service_->RevokeAllCredentials(
           signin_metrics::SourceForRefreshTokenOperation::
               kSigninManager_ClearPrimaryAccount);
       break;
     case RemoveAccountsOption::kRemoveAuthenticatedAccountIfInError:
-      if (token_service()->RefreshTokenHasError(account_id))
-        token_service()->RevokeCredentials(
+      if (token_service_->RefreshTokenHasError(account_id))
+        token_service_->RevokeCredentials(
             account_id, signin_metrics::SourceForRefreshTokenOperation::
                             kSigninManager_ClearPrimaryAccount);
       break;
@@ -371,4 +375,27 @@ void SigninManagerBase::FireGoogleSignedOut(const AccountInfo& account_info) {
     observer_->GoogleSignedOut(account_info);
   }
 }
+
+void SigninManagerBase::OnRefreshTokensLoaded() {
+  token_service_->RemoveObserver(this);
+
+  if (account_tracker_service_->GetMigrationState() ==
+      AccountTrackerService::MIGRATION_IN_PROGRESS) {
+    account_tracker_service_->SetMigrationDone();
+  }
+
+  // Remove account information from the account tracker service if needed.
+  if (token_service_->HasLoadCredentialsFinishedWithNoErrors()) {
+    std::vector<AccountInfo> accounts_in_tracker_service =
+        account_tracker_service_->GetAccounts();
+    for (const auto& account : accounts_in_tracker_service) {
+      if (GetAuthenticatedAccountId() != account.account_id &&
+          !token_service_->RefreshTokenIsAvailable(account.account_id)) {
+        DVLOG(0) << "Removed account from account tracker service: "
+                 << account.account_id;
+        account_tracker_service_->RemoveAccount(account.account_id);
+      }
+    }
+  }
+}
 #endif  // !defined(OS_CHROMEOS)

components/signin/core/browser/signin_manager_base.h

@@ -37,6 +37,7 @@
 #include "components/signin/core/browser/account_info.h"
 #include "components/signin/core/browser/signin_client.h"
 #include "google_apis/gaia/google_service_auth_error.h"
+#include "google_apis/gaia/oauth2_token_service.h"
 
 class AccountTrackerService;
 class PrefRegistrySimple;
@@ -44,7 +45,7 @@ class PrefService;
 class ProfileOAuth2TokenService;
 class SigninClient;
 
-class SigninManagerBase {
+class SigninManagerBase : public OAuth2TokenService::Observer {
  public:
   class Observer {
    public:
@@ -97,7 +98,7 @@ class SigninManagerBase {
   };
 #endif
 
-  virtual ~SigninManagerBase();
+  ~SigninManagerBase() override;
 
   // Registers per-profile prefs.
   static void RegisterProfilePrefs(PrefRegistrySimple* registry);
@@ -181,12 +182,6 @@ class SigninManagerBase {
  protected:
   SigninClient* signin_client() const { return client_; }
 
-  ProfileOAuth2TokenService* token_service() const { return token_service_; }
-
-  AccountTrackerService* account_tracker_service() const {
-    return account_tracker_service_;
-  }
-
   // Invoked at the end of |Initialize| before the refresh token for the primary
   // account is loaded.
   virtual void FinalizeInitBeforeLoadingRefreshTokens(PrefService* local_state);
@@ -231,6 +226,9 @@ class SigninManagerBase {
 
   // Send all observers |GoogleSignedOut| notifications.
   void FireGoogleSignedOut(const AccountInfo& account_info);
+
+  // OAuth2TokenService::Observer:
+  void OnRefreshTokensLoaded() override;
 #endif
 
   SigninClient* client_;