Avoid useless fetches of user ID from GCM account tracker.

For every account available in the account tracker service, both the
Gaia ID and the email are available. The GCM AccountTracker was fetching
again the gaia ID from the server on every start-up and every time a new
account was added to the profile. This CL removes this useless fetch
and changed the GCM AccountTracker to reuse the information already
available in the IdentityManager.

Additional clean-up to completely remove the GCM AccountTracker would
also be good, but I do not know this code well enough to do this cleanup.

Fixed: 1028522

Change-Id: I77790a3203a94ea8c7baf19ca0eb2449b096b630
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1944492Reviewed-by: Colin Blundell <blundell@chromium.org>
Commit-Queue: Mihai Sardarescu <msarda@chromium.org>
Cr-Commit-Position: refs/heads/master@{#720515}

components/gcm_driver/account_tracker.h

@@ -10,52 +10,22 @@
 #include <string>
 #include <vector>
 
-#include "base/memory/scoped_refptr.h"
 #include "base/observer_list.h"
-#include "components/signin/public/identity_manager/access_token_fetcher.h"
 #include "components/signin/public/identity_manager/identity_manager.h"
 #include "google_apis/gaia/core_account_id.h"
-#include "google_apis/gaia/gaia_oauth_client.h"
-
-class GoogleServiceAuthError;
-
-namespace signin {
-struct AccessTokenInfo;
-}
-
-namespace network {
-class SharedURLLoaderFactory;
-}
 
 namespace gcm {
 
-struct AccountIds {
-  CoreAccountId account_key;  // The account ID used by IdentityManager.
-  std::string gaia;
-  std::string email;
-};
-
-class AccountIdFetcher;
-
 // The AccountTracker keeps track of what accounts exist on the
-// profile and the state of their credentials. The tracker fetches the
-// gaia ID of each account it knows about.
-//
-// The AccountTracker maintains these invariants:
-// 1. Events are only fired after the gaia ID has been fetched.
-// 2. Add/Remove and SignIn/SignOut pairs are always generated in order.
-// 3. SignIn follows Add, and there will be a SignOut between SignIn & Remove.
-// 4. If there is no primary account, there are no other accounts.
+// profile and the state of their credentials.
 class AccountTracker : public signin::IdentityManager::Observer {
  public:
-  AccountTracker(
-      signin::IdentityManager* identity_manager,
-      scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory);
+  explicit AccountTracker(signin::IdentityManager* identity_manager);
   ~AccountTracker() override;
 
   class Observer {
    public:
-    virtual void OnAccountSignInChanged(const AccountIds& ids,
+    virtual void OnAccountSignInChanged(const CoreAccountInfo& account,
                                         bool is_signed_in) = 0;
   };
 
@@ -64,20 +34,14 @@ class AccountTracker : public signin::IdentityManager::Observer {
   void AddObserver(Observer* observer);
   void RemoveObserver(Observer* observer);
 
-  // Returns the list of accounts that are signed in, and for which gaia IDs
+  // Returns the list of accounts that are signed in, and for which gaia account
   // have been fetched. The primary account for the profile will be first
-  // in the vector. Additional accounts will be in order of their gaia IDs.
-  std::vector<AccountIds> GetAccounts() const;
-
-  // Indicates if all user information has been fetched. If the result is false,
-  // there are still unfinished fetchers.
-  virtual bool IsAllUserInfoFetched() const;
+  // in the vector. Additional accounts will be in order of their gaia account.
+  std::vector<CoreAccountInfo> GetAccounts() const;
 
  private:
-  friend class AccountIdFetcher;
-
   struct AccountState {
-    AccountIds ids;
+    CoreAccountInfo account;
     bool is_signed_in;
   };
 
@@ -91,65 +55,26 @@ class AccountTracker : public signin::IdentityManager::Observer {
   void OnRefreshTokenRemovedForAccount(
       const CoreAccountId& account_id) override;
 
-  void OnUserInfoFetchSuccess(AccountIdFetcher* fetcher,
-                              const std::string& gaia_id);
-  void OnUserInfoFetchFailure(AccountIdFetcher* fetcher);
-
-  void NotifySignInChanged(const AccountState& account);
+  // Add |account_info| to the lists of accounts tracked by this AccountTracker.
+  void StartTrackingAccount(const CoreAccountInfo& account_info);
 
-  void UpdateSignInState(const CoreAccountId& account_key, bool is_signed_in);
-
-  void StartTrackingAccount(const CoreAccountId& account_key);
-
-  // Note: |account_key| is passed by value here, because the original
-  // object may be stored in |accounts_| and if so, it will be destroyed
-  // after erasing the key from the map.
-  void StopTrackingAccount(const CoreAccountId account_key);
+  // Stops tracking |account_id|. Notifies all observers if the account was
+  // previously signed in.
+  void StopTrackingAccount(const CoreAccountId account_id);
 
+  // Stops tracking all accounts.
   void StopTrackingAllAccounts();
-  void StartFetchingUserInfo(const CoreAccountId& account_key);
-  void DeleteFetcher(AccountIdFetcher* fetcher);
+
+  // Updates the is_signed_in corresponding to the given account. Notifies all
+  // observers of the signed in state changes.
+  void UpdateSignInState(const CoreAccountId& account_id, bool is_signed_in);
 
   signin::IdentityManager* identity_manager_;
-  scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory_;
-  std::map<CoreAccountId, std::unique_ptr<AccountIdFetcher>>
-      user_info_requests_;
   std::map<CoreAccountId, AccountState> accounts_;
   base::ObserverList<Observer>::Unchecked observer_list_;
   bool shutdown_called_;
 };
 
-class AccountIdFetcher : public gaia::GaiaOAuthClient::Delegate {
- public:
-  AccountIdFetcher(
-      signin::IdentityManager* identity_manager,
-      scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
-      AccountTracker* tracker,
-      const CoreAccountId& account_key);
-  ~AccountIdFetcher() override;
-
-  const CoreAccountId& account_key() { return account_key_; }
-
-  void Start();
-
-  void AccessTokenFetched(GoogleServiceAuthError error,
-                          signin::AccessTokenInfo access_token_info);
-
-  // gaia::GaiaOAuthClient::Delegate implementation.
-  void OnGetUserIdResponse(const std::string& gaia_id) override;
-  void OnOAuthError() override;
-  void OnNetworkError(int response_code) override;
-
- private:
-  signin::IdentityManager* identity_manager_;
-  scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory_;
-  AccountTracker* tracker_;
-  const CoreAccountId account_key_;
-
-  std::unique_ptr<signin::AccessTokenFetcher> access_token_fetcher_;
-  std::unique_ptr<gaia::GaiaOAuthClient> gaia_oauth_client_;
-};
-
 }  // namespace gcm
 
 #endif  // COMPONENTS_GCM_DRIVER_ACCOUNT_TRACKER_H_

components/gcm_driver/gcm_account_tracker.cc

@@ -72,12 +72,12 @@ void GCMAccountTracker::Start() {
   account_tracker_->AddObserver(this);
   driver_->AddConnectionObserver(this);
 
-  std::vector<AccountIds> accounts = account_tracker_->GetAccounts();
-  for (std::vector<AccountIds>::const_iterator iter = accounts.begin();
+  std::vector<CoreAccountInfo> accounts = account_tracker_->GetAccounts();
+  for (std::vector<CoreAccountInfo>::const_iterator iter = accounts.begin();
        iter != accounts.end(); ++iter) {
     if (!iter->email.empty()) {
       account_infos_.insert(std::make_pair(
-          iter->account_key, AccountInfo(iter->email, TOKEN_NEEDED)));
+          iter->account_id, AccountInfo(iter->email, TOKEN_NEEDED)));
     }
   }
 
@@ -104,12 +104,12 @@ void GCMAccountTracker::ScheduleReportTokens() {
       GetTimeToNextTokenReporting());
 }
 
-void GCMAccountTracker::OnAccountSignInChanged(const AccountIds& ids,
+void GCMAccountTracker::OnAccountSignInChanged(const CoreAccountInfo& account,
                                                bool is_signed_in) {
   if (is_signed_in)
-    OnAccountSignedIn(ids);
+    OnAccountSignedIn(account);
   else
-    OnAccountSignedOut(ids);
+    OnAccountSignedOut(account);
 }
 
 void GCMAccountTracker::OnAccessTokenFetchCompleteForAccount(
@@ -163,11 +163,9 @@ void GCMAccountTracker::ReportTokens() {
     return;
   }
 
-  // Wait for AccountTracker to be done with fetching the user info, as
-  // well as all of the pending token requests from GCMAccountTracker to be done
-  // before you report the results.
-  if (!account_tracker_->IsAllUserInfoFetched() ||
-      !pending_token_requests_.empty()) {
+  // Wait for all of the pending token requests from GCMAccountTracker to be
+  // done before you report the results.
+  if (!pending_token_requests_.empty()) {
     return;
   }
 
@@ -311,13 +309,13 @@ void GCMAccountTracker::GetToken(AccountInfos::iterator& account_iter) {
   account_iter->second.state = GETTING_TOKEN;
 }
 
-void GCMAccountTracker::OnAccountSignedIn(const AccountIds& ids) {
-  DVLOG(1) << "Account signed in: " << ids.email;
-  auto iter = account_infos_.find(ids.account_key);
+void GCMAccountTracker::OnAccountSignedIn(const CoreAccountInfo& account) {
+  DVLOG(1) << "Account signed in: " << account.email;
+  auto iter = account_infos_.find(account.account_id);
   if (iter == account_infos_.end()) {
-    DCHECK(!ids.email.empty());
-    account_infos_.insert(
-        std::make_pair(ids.account_key, AccountInfo(ids.email, TOKEN_NEEDED)));
+    DCHECK(!account.email.empty());
+    account_infos_.insert(std::make_pair(
+        account.account_id, AccountInfo(account.email, TOKEN_NEEDED)));
   } else if (iter->second.state == ACCOUNT_REMOVED) {
     iter->second.state = TOKEN_NEEDED;
   }
@@ -325,9 +323,9 @@ void GCMAccountTracker::OnAccountSignedIn(const AccountIds& ids) {
   GetAllNeededTokens();
 }
 
-void GCMAccountTracker::OnAccountSignedOut(const AccountIds& ids) {
-  DVLOG(1) << "Account signed out: " << ids.email;
-  auto iter = account_infos_.find(ids.account_key);
+void GCMAccountTracker::OnAccountSignedOut(const CoreAccountInfo& account) {
+  DVLOG(1) << "Account signed out: " << account.email;
+  auto iter = account_infos_.find(account.account_id);
   if (iter == account_infos_.end())
     return;
 
@@ -338,7 +336,7 @@ void GCMAccountTracker::OnAccountSignedOut(const AccountIds& ids) {
   // re-added and a new access token request made, we do not break this class'
   // invariant that there is at most one ongoing access token request per
   // account.
-  pending_token_requests_.erase(ids.account_key);
+  pending_token_requests_.erase(account.account_id);
   ReportTokens();
 }
 

components/gcm_driver/gcm_account_tracker.h

@@ -96,12 +96,12 @@ class GCMAccountTracker : public AccountTracker::Observer,
  private:
   friend class GCMAccountTrackerTest;
 
-  // Maps account keys to account states. Keyed by account_ids as used by
+  // Maps account keys to account states. Keyed by account_id as used by
   // IdentityManager.
   typedef std::map<CoreAccountId, AccountInfo> AccountInfos;
 
   // AccountTracker::Observer overrides.
-  void OnAccountSignInChanged(const AccountIds& ids,
+  void OnAccountSignInChanged(const CoreAccountInfo& account,
                               bool is_signed_in) override;
 
   void OnAccessTokenFetchCompleteForAccount(
@@ -136,8 +136,8 @@ class GCMAccountTracker : public AccountTracker::Observer,
   void GetToken(AccountInfos::iterator& account_iter);
 
   // Handling of actual sign in and sign out for accounts.
-  void OnAccountSignedIn(const AccountIds& ids);
-  void OnAccountSignedOut(const AccountIds& ids);
+  void OnAccountSignedIn(const CoreAccountInfo& account);
+  void OnAccountSignedOut(const CoreAccountInfo& account);
 
   // Account tracker.
   std::unique_ptr<AccountTracker> account_tracker_;

components/gcm_driver/gcm_profile_service.cc

@@ -112,7 +112,7 @@ void GCMProfileService::IdentityObserver::StartAccountTracker(
     return;
 
   std::unique_ptr<AccountTracker> gaia_account_tracker(
-      new AccountTracker(identity_manager_, std::move(url_loader_factory)));
+      new AccountTracker(identity_manager_));
 
   gcm_account_tracker_.reset(new GCMAccountTracker(
       std::move(gaia_account_tracker), identity_manager_, driver_));