Move AccountConsistencyService and its unittest away from signin internals

This CL removes GaiaCookieManagerService, AccountTrackerService,
ProfileOAuth2TokenService, AccountFetcherService and SigninClient.

It also updates DomainsClearedOnBrowsingDataRemoved
and DomainsClearedOnBrowsingDataRemoved2 to use IdentityManager,
not GaiaCookieManagerService. It checks whether
OnAccountsInCookieUpdated() is triggered or not after
OnBrowsingDataRemoved() is called as it forces to call
OnCookieChange().

Bug: 797931
Change-Id: I247370add1ed670a8b2aad99c7905bbe655f5d63
Reviewed-on: https://chromium-review.googlesource.com/c/1488476Reviewed-by: David Roger <droger@chromium.org>
Reviewed-by: Colin Blundell <blundell@chromium.org>
Commit-Queue: Julie Jeongeun Kim <jkim@igalia.com>
Cr-Commit-Position: refs/heads/master@{#635947}

components/signin/ios/browser/BUILD.gn

@@ -75,7 +75,6 @@ source_set("unit_tests") {
     ":test_support",
     "//components/prefs:test_support",
     "//components/signin/core/browser",
-    "//components/signin/core/browser:internals_test_support",
     "//components/sync_preferences:test_support",
     "//ios/web",
     "//ios/web/public/test",

components/signin/ios/browser/account_consistency_service.h

@@ -17,7 +17,6 @@
 #include "base/time/time.h"
 #include "components/content_settings/core/browser/cookie_settings.h"
 #include "components/keyed_service/core/keyed_service.h"
-#include "components/signin/core/browser/gaia_cookie_manager_service.h"
 #include "components/signin/ios/browser/active_state_manager.h"
 #import "components/signin/ios/browser/manage_accounts_delegate.h"
 #import "services/identity/public/cpp/identity_manager.h"
@@ -29,7 +28,6 @@ class WebStatePolicyDecider;
 }
 
 class AccountReconcilor;
-class SigninClient;
 
 @class AccountConsistencyNavigationDelegate;
 @class WKWebView;
@@ -40,7 +38,6 @@ class SigninClient;
 //
 // This is currently only used when WKWebView is enabled.
 class AccountConsistencyService : public KeyedService,
-                                  public GaiaCookieManagerService::Observer,
                                   public identity::IdentityManager::Observer,
                                   public ActiveStateManager::Observer {
  public:
@@ -50,10 +47,9 @@ class AccountConsistencyService : public KeyedService,
 
   AccountConsistencyService(
       web::BrowserState* browser_state,
+      PrefService* prefs,
       AccountReconcilor* account_reconcilor,
       scoped_refptr<content_settings::CookieSettings> cookie_settings,
-      GaiaCookieManagerService* gaia_cookie_manager_service,
-      SigninClient* signin_client,
       identity::IdentityManager* identity_manager);
   ~AccountConsistencyService() override;
 
@@ -146,16 +142,13 @@ class AccountConsistencyService : public KeyedService,
   // Adds CHROME_CONNECTED cookies on all the main Google domains.
   void AddChromeConnectedCookies();
 
-  // GaiaCookieManagerService::Observer implementation.
-  void OnGaiaAccountsInCookieUpdated(
-      const std::vector<gaia::ListedAccount>& accounts,
-      const std::vector<gaia::ListedAccount>& signed_out_accounts,
-      const GoogleServiceAuthError& error) override;
-
   // IdentityManager::Observer implementation.
   void OnPrimaryAccountSet(const CoreAccountInfo& account_info) override;
   void OnPrimaryAccountCleared(
       const CoreAccountInfo& previous_account_info) override;
+  void OnAccountsInCookieUpdated(
+      const identity::AccountsInCookieJarInfo& accounts_in_cookie_jar_info,
+      const GoogleServiceAuthError& error) override;
 
   // ActiveStateManager::Observer implementation.
   void OnActive() override;
@@ -163,17 +156,14 @@ class AccountConsistencyService : public KeyedService,
 
   // Browser state associated with the service, used to create WKWebViews.
   web::BrowserState* browser_state_;
+  // Used to update kDomainsWithCookiePref.
+  PrefService* prefs_;
   // Service managing accounts reconciliation, notified of GAIA responses with
   // the X-Chrome-Manage-Accounts header
   AccountReconcilor* account_reconcilor_;
   // Cookie settings currently in use for |browser_state_|, used to check if
   // setting CHROME_CONNECTED cookies is valid.
   scoped_refptr<content_settings::CookieSettings> cookie_settings_;
-  // Service managing the Gaia cookies, observed to be notified of the state of
-  // reconciliation.
-  GaiaCookieManagerService* gaia_cookie_manager_service_;
-  // Signin client, used to access prefs.
-  SigninClient* signin_client_;
   // Identity manager, observed to be notified of primary account signin and
   // signout events.
   identity::IdentityManager* identity_manager_;

components/signin/ios/browser/account_consistency_service.mm

@@ -16,7 +16,6 @@
 #include "components/prefs/scoped_user_pref_update.h"
 #include "components/signin/core/browser/account_consistency_method.h"
 #include "components/signin/core/browser/account_reconcilor.h"
-#include "components/signin/core/browser/signin_client.h"
 #include "components/signin/core/browser/signin_header_helper.h"
 #include "ios/web/public/browser_state.h"
 #include "ios/web/public/web_state/web_state_policy_decider.h"
@@ -218,19 +217,16 @@ AccountConsistencyService::CookieRequest::CookieRequest(
 
 AccountConsistencyService::AccountConsistencyService(
     web::BrowserState* browser_state,
+    PrefService* prefs,
     AccountReconcilor* account_reconcilor,
     scoped_refptr<content_settings::CookieSettings> cookie_settings,
-    GaiaCookieManagerService* gaia_cookie_manager_service,
-    SigninClient* signin_client,
     identity::IdentityManager* identity_manager)
     : browser_state_(browser_state),
+      prefs_(prefs),
       account_reconcilor_(account_reconcilor),
       cookie_settings_(cookie_settings),
-      gaia_cookie_manager_service_(gaia_cookie_manager_service),
-      signin_client_(signin_client),
       identity_manager_(identity_manager),
       applying_cookie_requests_(false) {
-  gaia_cookie_manager_service_->AddObserver(this);
   identity_manager_->AddObserver(this);
   ActiveStateManager::FromBrowserState(browser_state_)->AddObserver(this);
   LoadFromPrefs();
@@ -326,14 +322,13 @@ void AccountConsistencyService::RemoveChromeConnectedCookieFromDomain(
 
 void AccountConsistencyService::LoadFromPrefs() {
   const base::DictionaryValue* dict =
-      signin_client_->GetPrefs()->GetDictionary(kDomainsWithCookiePref);
+      prefs_->GetDictionary(kDomainsWithCookiePref);
   for (base::DictionaryValue::Iterator it(*dict); !it.IsAtEnd(); it.Advance()) {
     last_cookie_update_map_[it.key()] = base::Time();
   }
 }
 
 void AccountConsistencyService::Shutdown() {
-  gaia_cookie_manager_service_->RemoveObserver(this);
   identity_manager_->RemoveObserver(this);
   ActiveStateManager::FromBrowserState(browser_state_)->RemoveObserver(this);
   ResetWKWebView();
@@ -397,8 +392,7 @@ void AccountConsistencyService::FinishedApplyingCookieRequest(bool success) {
   CookieRequest& request = cookie_requests_.front();
   if (success) {
     DictionaryPrefUpdate update(
-        signin_client_->GetPrefs(),
-        AccountConsistencyService::kDomainsWithCookiePref);
+        prefs_, AccountConsistencyService::kDomainsWithCookiePref);
     switch (request.request_type) {
       case ADD_CHROME_CONNECTED_COOKIE:
         // Add request.domain to prefs, use |true| as a dummy value (that is
@@ -472,17 +466,12 @@ void AccountConsistencyService::OnBrowsingDataRemoved() {
   cookie_requests_.clear();
   last_cookie_update_map_.clear();
   base::DictionaryValue dict;
-  signin_client_->GetPrefs()->Set(kDomainsWithCookiePref, dict);
+  prefs_->Set(kDomainsWithCookiePref, dict);
 
   // APISID cookie has been removed, notify the GCMS.
-  gaia_cookie_manager_service_->ForceOnCookieChangeProcessing();
-}
-
-void AccountConsistencyService::OnGaiaAccountsInCookieUpdated(
-    const std::vector<gaia::ListedAccount>& accounts,
-    const std::vector<gaia::ListedAccount>& signed_out_accounts,
-    const GoogleServiceAuthError& error) {
-  AddChromeConnectedCookies();
+  // TODO(https://crbug.com/930582) : Remove the need to expose this method
+  // or move it to the network::CookieManager.
+  identity_manager_->ForceTriggerOnCookieChange();
 }
 
 void AccountConsistencyService::OnPrimaryAccountSet(
@@ -497,6 +486,12 @@ void AccountConsistencyService::OnPrimaryAccountCleared(
   // right before fetching the Gaia logout request.
 }
 
+void AccountConsistencyService::OnAccountsInCookieUpdated(
+    const identity::AccountsInCookieJarInfo& accounts_in_cookie_jar_info,
+    const GoogleServiceAuthError& error) {
+  AddChromeConnectedCookies();
+}
+
 void AccountConsistencyService::OnActive() {
   // |browser_state_| is now active. There might be some pending cookie requests
   // to apply.

components/signin/ios/browser/account_consistency_service_unittest.mm

@@ -13,12 +13,7 @@
 #include "base/values.h"
 #include "components/signin/core/browser/account_reconcilor.h"
 #include "components/signin/core/browser/account_reconcilor_delegate.h"
-#include "components/signin/core/browser/account_tracker_service.h"
-#include "components/signin/core/browser/fake_account_fetcher_service.h"
-#include "components/signin/core/browser/fake_profile_oauth2_token_service.h"
-#include "components/signin/core/browser/gaia_cookie_manager_service.h"
-#include "components/signin/core/browser/signin_manager.h"
-#include "components/signin/core/browser/signin_pref_names.h"
+#include "components/signin/core/browser/list_accounts_test_utils.h"
 #include "components/signin/core/browser/test_signin_client.h"
 #include "components/sync_preferences/testing_pref_service_syncable.h"
 #include "ios/web/public/test/fakes/test_browser_state.h"
@@ -27,6 +22,7 @@
 #include "ios/web/public/web_state/web_state_policy_decider.h"
 #import "services/identity/public/cpp/identity_manager.h"
 #import "services/identity/public/cpp/identity_test_environment.h"
+#include "services/identity/public/cpp/test_identity_manager_observer.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "testing/platform_test.h"
@@ -58,16 +54,14 @@ class FakeAccountConsistencyService : public AccountConsistencyService {
  public:
   FakeAccountConsistencyService(
       web::BrowserState* browser_state,
+      PrefService* prefs,
       AccountReconcilor* account_reconcilor,
       scoped_refptr<content_settings::CookieSettings> cookie_settings,
-      GaiaCookieManagerService* gaia_cookie_manager_service,
-      SigninClient* signin_client,
       identity::IdentityManager* identity_manager)
       : AccountConsistencyService(browser_state,
+                                  prefs,
                                   account_reconcilor,
                                   cookie_settings,
-                                  gaia_cookie_manager_service,
-                                  signin_client,
                                   identity_manager) {}
 
  private:
@@ -91,25 +85,6 @@ class MockAccountReconcilor : public AccountReconcilor {
   MOCK_METHOD1(OnReceivedManageAccountsResponse, void(signin::GAIAServiceType));
 };
 
-// Mock GaiaCookieManagerService to catch call to ForceOnCookieChangeProcessing.
-// TODO(https://crbug.com/907782): Update this to use gmock.
-class CustomGaiaCookieManagerService : public GaiaCookieManagerService {
- public:
-  CustomGaiaCookieManagerService()
-      : GaiaCookieManagerService(nullptr, nullptr),
-        calls_to_force_on_cookie_change_processing_(0) {}
-
-  uint8_t CallsToForceOnCookieChangeProcessing() {
-    return calls_to_force_on_cookie_change_processing_;
-  }
-
- private:
-  void ForceOnCookieChangeProcessing() override {
-    calls_to_force_on_cookie_change_processing_++;
-  }
-  uint8_t calls_to_force_on_cookie_change_processing_;
-};
-
 // TestWebState that allows control over its policy decider.
 class TestWebState : public web::TestWebState {
  public:
@@ -151,29 +126,14 @@ class AccountConsistencyServiceTest : public PlatformTest {
     PlatformTest::SetUp();
     ActiveStateManager::FromBrowserState(&browser_state_)->SetActive(true);
     AccountConsistencyService::RegisterPrefs(prefs_.registry());
-    AccountTrackerService::RegisterPrefs(prefs_.registry());
-    AccountFetcherService::RegisterPrefs(prefs_.registry());
-    ProfileOAuth2TokenService::RegisterProfilePrefs(prefs_.registry());
     content_settings::CookieSettings::RegisterProfilePrefs(prefs_.registry());
     HostContentSettingsMap::RegisterProfilePrefs(prefs_.registry());
-    SigninManagerBase::RegisterProfilePrefs(prefs_.registry());
 
     web_view_load_expection_count_ = 0;
-    gaia_cookie_manager_service_.reset(new CustomGaiaCookieManagerService());
     signin_client_.reset(new TestSigninClient(&prefs_));
-    account_tracker_service_.Initialize(&prefs_, base::FilePath());
-    token_service_.reset(new FakeProfileOAuth2TokenService(&prefs_));
-    account_fetcher_service_.Initialize(
-        signin_client_.get(), token_service_.get(), &account_tracker_service_,
-        std::make_unique<TestImageDecoder>());
-    signin_manager_.reset(new SigninManager(
-        signin_client_.get(), token_service_.get(), &account_tracker_service_,
-        nullptr, signin::AccountConsistencyMethod::kDisabled));
-    signin_manager_->Initialize(nullptr);
     identity_test_env_.reset(new identity::IdentityTestEnvironment(
-        &prefs_, &account_tracker_service_, &account_fetcher_service_,
-        token_service_.get(), signin_manager_.get(),
-        gaia_cookie_manager_service_.get()));
+        &test_url_loader_factory_, &prefs_,
+        signin::AccountConsistencyMethod::kDisabled, signin_client_.get()));
     settings_map_ = new HostContentSettingsMap(
         &prefs_, false /* incognito_profile */, false /* guest_profile */,
         false /* store_last_modified */,
@@ -191,8 +151,6 @@ class AccountConsistencyServiceTest : public PlatformTest {
     account_consistency_service_->Shutdown();
     settings_map_->ShutdownOnUIThread();
     ActiveStateManager::FromBrowserState(&browser_state_)->SetActive(false);
-    account_fetcher_service_.Shutdown();
-    account_tracker_service_.Shutdown();
     identity_test_env_.reset();
     PlatformTest::TearDown();
   }
@@ -221,8 +179,7 @@ class AccountConsistencyServiceTest : public PlatformTest {
       account_consistency_service_->Shutdown();
     }
     account_consistency_service_.reset(new FakeAccountConsistencyService(
-        &browser_state_, account_reconcilor_.get(), cookie_settings_,
-        gaia_cookie_manager_service_.get(), signin_client_.get(),
+        &browser_state_, &prefs_, account_reconcilor_.get(), cookie_settings_,
         identity_test_env_->identity_manager()));
   }
 
@@ -276,19 +233,16 @@ class AccountConsistencyServiceTest : public PlatformTest {
   // Creates test threads, necessary for ActiveStateManager that needs a UI
   // thread.
   web::TestWebThreadBundle thread_bundle_;
-  AccountTrackerService account_tracker_service_;
-  FakeAccountFetcherService account_fetcher_service_;
   web::TestBrowserState browser_state_;
   sync_preferences::TestingPrefServiceSyncable prefs_;
   TestWebState web_state_;
+  network::TestURLLoaderFactory test_url_loader_factory_;
+
   std::unique_ptr<identity::IdentityTestEnvironment> identity_test_env_;
   // AccountConsistencyService being tested. Actually a
   // FakeAccountConsistencyService to be able to use a mock web view.
   std::unique_ptr<AccountConsistencyService> account_consistency_service_;
   std::unique_ptr<TestSigninClient> signin_client_;
-  std::unique_ptr<FakeProfileOAuth2TokenService> token_service_;
-  std::unique_ptr<SigninManager> signin_manager_;
-  std::unique_ptr<CustomGaiaCookieManagerService> gaia_cookie_manager_service_;
   std::unique_ptr<MockAccountReconcilor> account_reconcilor_;
   scoped_refptr<HostContentSettingsMap> settings_map_;
   scoped_refptr<content_settings::CookieSettings> cookie_settings_;
@@ -477,11 +431,20 @@ TEST_F(AccountConsistencyServiceTest, DomainsClearedOnBrowsingDataRemoved) {
       prefs_.GetDictionary(AccountConsistencyService::kDomainsWithCookiePref);
   EXPECT_EQ(2u, dict->size());
 
+  // Sets Response to get IdentityManager::Observer::OnAccountsInCookieUpdated
+  // through GaiaCookieManagerService::OnCookieChange.
+  signin::SetListAccountsResponseNoAccounts(&test_url_loader_factory_);
+
+  base::RunLoop run_loop;
+  identity_test_env_->identity_manager_observer()
+      ->SetOnAccountsInCookieUpdatedCallback(run_loop.QuitClosure());
+  // OnBrowsingDataRemoved triggers IdentityManager::ForceTriggerOnCookieChange
+  // and finally IdentityManager::Observer::OnAccountsInCookieUpdated is called.
   account_consistency_service_->OnBrowsingDataRemoved();
+  run_loop.Run();
+
   dict =
       prefs_.GetDictionary(AccountConsistencyService::kDomainsWithCookiePref);
-  EXPECT_EQ(
-      1u, gaia_cookie_manager_service_->CallsToForceOnCookieChangeProcessing());
   EXPECT_EQ(0u, dict->size());
 }
 
@@ -494,9 +457,18 @@ TEST_F(AccountConsistencyServiceTest, DomainsClearedOnBrowsingDataRemoved2) {
 
   AddPageLoadedExpectation(kGoogleUrl, false /* continue_navigation */);
   SimulateGaiaCookieManagerServiceLogout(false);
+
+  // Sets Response to get IdentityManager::Observer::OnAccountsInCookieUpdated
+  // through GaiaCookieManagerService::OnCookieChange.
+  signin::SetListAccountsResponseNoAccounts(&test_url_loader_factory_);
+
+  base::RunLoop run_loop;
+  identity_test_env_->identity_manager_observer()
+      ->SetOnAccountsInCookieUpdatedCallback(run_loop.QuitClosure());
+  // OnBrowsingDataRemoved triggers IdentityManager::ForceTriggerOnCookieChange
+  // and finally IdentityManager::Observer::OnAccountsInCookieUpdated is called.
   account_consistency_service_->OnBrowsingDataRemoved();
-  EXPECT_EQ(
-      1u, gaia_cookie_manager_service_->CallsToForceOnCookieChangeProcessing());
+  run_loop.Run();
   EXPECT_TRUE(remove_cookie_callback_called_);
 }
 

ios/chrome/browser/signin/account_consistency_service_factory.mm

@@ -11,9 +11,7 @@
 #include "ios/chrome/browser/browser_state/chrome_browser_state.h"
 #include "ios/chrome/browser/content_settings/cookie_settings_factory.h"
 #include "ios/chrome/browser/signin/account_reconcilor_factory.h"
-#include "ios/chrome/browser/signin/gaia_cookie_manager_service_factory.h"
 #include "ios/chrome/browser/signin/identity_manager_factory.h"
-#include "ios/chrome/browser/signin/signin_client_factory.h"
 
 #if !defined(__has_feature) || !__has_feature(objc_arc)
 #error "This file requires ARC support."
@@ -27,8 +25,6 @@ AccountConsistencyServiceFactory::AccountConsistencyServiceFactory()
           BrowserStateDependencyManager::GetInstance()) {
   DependsOn(ios::AccountReconcilorFactory::GetInstance());
   DependsOn(ios::CookieSettingsFactory::GetInstance());
-  DependsOn(GaiaCookieManagerServiceFactory::GetInstance());
-  DependsOn(SigninClientFactory::GetInstance());
   DependsOn(IdentityManagerFactory::GetInstance());
 }
 
@@ -59,11 +55,9 @@ AccountConsistencyServiceFactory::BuildServiceInstanceFor(
   ios::ChromeBrowserState* chrome_browser_state =
       ios::ChromeBrowserState::FromBrowserState(context);
   return std::make_unique<AccountConsistencyService>(
-      chrome_browser_state,
+      chrome_browser_state, chrome_browser_state->GetPrefs(),
       ios::AccountReconcilorFactory::GetForBrowserState(chrome_browser_state),
       ios::CookieSettingsFactory::GetForBrowserState(chrome_browser_state),
-      GaiaCookieManagerServiceFactory::GetForBrowserState(chrome_browser_state),
-      SigninClientFactory::GetForBrowserState(chrome_browser_state),
       IdentityManagerFactory::GetForBrowserState(chrome_browser_state));
 }