[NTP Snippets] Reland RemoteSuggestionsFetcher usage of IdentityManager

This CL reverts d8bd981f, in effect relanding the changes to have
RemoteSuggestionsFetcherImpl and its unittest use IdentityManager. In
the interim I have landed a fix to the bug that necessitated
reversion of those CLs, i.e. the raciness between IdentityManager
getting updated on signin/signout and other SigninManager::Observers
getting updated on those same events
(fix is here:
 https://chromium-review.googlesource.com/c/chromium/src/+/883347).

To test this change, do the following:
- Sign in to Chrome on Android (in a chrome_apk build if building locally)
- Observe that there are personalized suggestions on the NTP
- Sign out
- Observe that there are different, generic suggestions on the NTP

You can also more generally that the suggestions get updated on various
transitions between a signed-out state and a signed-in state.

TBR=gambard@chromium.org

Bug: 798413
Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs
Change-Id: I37d182718ef0b53a85be1a083f203aa0acdb3d8d
Reviewed-on: https://chromium-review.googlesource.com/888617
Commit-Queue: Colin Blundell <blundell@chromium.org>
Reviewed-by: Jan Krcal <jkrcal@chromium.org>
Cr-Commit-Position: refs/heads/master@{#532888}

chrome/browser/ntp_snippets/content_suggestions_service_factory.cc

@@ -25,6 +25,7 @@
 #include "chrome/browser/ntp_snippets/dependent_features.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/search/suggestions/image_decoder_impl.h"
+#include "chrome/browser/signin/identity_manager_factory.h"
 #include "chrome/browser/signin/profile_oauth2_token_service_factory.h"
 #include "chrome/browser/signin/signin_manager_factory.h"
 #include "chrome/browser/sync/profile_sync_service_factory.h"
@@ -338,8 +339,8 @@ void RegisterArticleProviderIfEnabled(ContentSuggestionsService* service,
   }
 
   PrefService* pref_service = profile->GetPrefs();
-  OAuth2TokenService* token_service =
-      ProfileOAuth2TokenServiceFactory::GetForProfile(profile);
+  identity::IdentityManager* identity_manager =
+      IdentityManagerFactory::GetForProfile(profile);
   UrlLanguageHistogram* language_histogram =
       UrlLanguageHistogramFactory::GetForBrowserContext(profile);
 
@@ -375,8 +376,7 @@ void RegisterArticleProviderIfEnabled(ContentSuggestionsService* service,
   }
 #endif  // BUILDFLAG(ENABLE_OFFLINE_PAGES)
   auto suggestions_fetcher = base::MakeUnique<RemoteSuggestionsFetcherImpl>(
-      signin_manager, token_service, request_context, pref_service,
-      language_histogram,
+      identity_manager, request_context, pref_service, language_histogram,
       base::Bind(
           &data_decoder::SafeJsonParser::Parse,
           content::ServiceManagerConnection::GetForProcess()->GetConnector()),
@@ -454,6 +454,7 @@ ContentSuggestionsServiceFactory::ContentSuggestionsServiceFactory()
           BrowserContextDependencyManager::GetInstance()) {
   DependsOn(BookmarkModelFactory::GetInstance());
   DependsOn(HistoryServiceFactory::GetInstance());
+  DependsOn(IdentityManagerFactory::GetInstance());
   DependsOn(LargeIconServiceFactory::GetInstance());
 #if BUILDFLAG(ENABLE_OFFLINE_PAGES)
   DependsOn(OfflinePageModelFactory::GetInstance());

components/ntp_snippets/remote/remote_suggestions_fetcher_impl.cc

@@ -15,9 +15,9 @@
 #include "components/ntp_snippets/category.h"
 #include "components/ntp_snippets/ntp_snippets_constants.h"
 #include "components/ntp_snippets/user_classifier.h"
-#include "components/signin/core/browser/signin_manager_base.h"
 #include "net/url_request/url_fetcher.h"
 #include "net/url_request/url_request_status.h"
+#include "services/identity/public/cpp/identity_manager.h"
 #include "services/identity/public/cpp/primary_account_access_token_fetcher.h"
 
 using language::UrlLanguageHistogram;
@@ -124,8 +124,7 @@ void FilterCategories(FetchedCategoriesVector* categories,
 }  // namespace
 
 RemoteSuggestionsFetcherImpl::RemoteSuggestionsFetcherImpl(
-    SigninManagerBase* signin_manager,
-    OAuth2TokenService* token_service,
+    identity::IdentityManager* identity_manager,
     scoped_refptr<URLRequestContextGetter> url_request_context_getter,
     PrefService* pref_service,
     UrlLanguageHistogram* language_histogram,
@@ -133,8 +132,7 @@ RemoteSuggestionsFetcherImpl::RemoteSuggestionsFetcherImpl(
     const GURL& api_endpoint,
     const std::string& api_key,
     const UserClassifier* user_classifier)
-    : signin_manager_(signin_manager),
-      token_service_(token_service),
+    : identity_manager_(identity_manager),
       url_request_context_getter_(std::move(url_request_context_getter)),
       language_histogram_(language_histogram),
       parse_json_callback_(parse_json_callback),
@@ -184,7 +182,7 @@ void RemoteSuggestionsFetcherImpl::FetchSnippets(
       .SetUrlRequestContextGetter(url_request_context_getter_)
       .SetUserClassifier(*user_classifier_);
 
-  if (signin_manager_->IsAuthenticated()) {
+  if (identity_manager_->HasPrimaryAccount()) {
     // Signed-in: get OAuth token --> fetch suggestions.
     pending_requests_.emplace(std::move(builder), std::move(callback));
     StartTokenRequest();
@@ -217,7 +215,7 @@ void RemoteSuggestionsFetcherImpl::FetchSnippetsAuthenticated(
     SnippetsAvailableCallback callback,
     const std::string& oauth_access_token) {
   builder.SetUrl(fetch_url_)
-      .SetAuthentication(signin_manager_->GetAuthenticatedAccountId(),
+      .SetAuthentication(identity_manager_->GetPrimaryAccountInfo().account_id,
                          base::StringPrintf(kAuthorizationRequestHeaderFormat,
                                             oauth_access_token.c_str()));
   StartRequest(std::move(builder), std::move(callback),
@@ -242,8 +240,8 @@ void RemoteSuggestionsFetcherImpl::StartTokenRequest() {
   }
 
   OAuth2TokenService::ScopeSet scopes{kContentSuggestionsApiScope};
-  token_fetcher_ = std::make_unique<identity::PrimaryAccountAccessTokenFetcher>(
-      "ntp_snippets", signin_manager_, token_service_, scopes,
+  token_fetcher_ = identity_manager_->CreateAccessTokenFetcherForPrimaryAccount(
+      "ntp_snippets", scopes,
       base::BindOnce(&RemoteSuggestionsFetcherImpl::AccessTokenFetchFinished,
                      base::Unretained(this)),
       identity::PrimaryAccountAccessTokenFetcher::Mode::kWaitUntilAvailable);

components/ntp_snippets/remote/remote_suggestions_fetcher_impl.h

@@ -20,15 +20,14 @@
 #include "components/ntp_snippets/remote/request_params.h"
 #include "net/url_request/url_request_context_getter.h"
 
-class OAuth2TokenService;
 class PrefService;
-class SigninManagerBase;
 
 namespace base {
 class Value;
 }  // namespace base
 
 namespace identity {
+class IdentityManager;
 class PrimaryAccountAccessTokenFetcher;
 }
 
@@ -43,8 +42,7 @@ class UserClassifier;
 class RemoteSuggestionsFetcherImpl : public RemoteSuggestionsFetcher {
  public:
   RemoteSuggestionsFetcherImpl(
-      SigninManagerBase* signin_manager,
-      OAuth2TokenService* token_service,
+      identity::IdentityManager* identity_manager,
       scoped_refptr<net::URLRequestContextGetter> url_request_context_getter,
       PrefService* pref_service,
       language::UrlLanguageHistogram* language_histogram,
@@ -94,8 +92,7 @@ class RemoteSuggestionsFetcherImpl : public RemoteSuggestionsFetcher {
                      bool is_authenticated);
 
   // Authentication for signed-in users.
-  SigninManagerBase* signin_manager_;
-  OAuth2TokenService* token_service_;
+  identity::IdentityManager* identity_manager_;
 
   std::unique_ptr<identity::PrimaryAccountAccessTokenFetcher> token_fetcher_;
 

components/ntp_snippets/remote/remote_suggestions_fetcher_impl_unittest.cc

@@ -10,6 +10,7 @@
 
 #include "base/containers/circular_deque.h"
 #include "base/json/json_reader.h"
+#include "base/message_loop/message_loop.h"
 #include "base/optional.h"
 #include "base/test/histogram_tester.h"
 #include "base/test/test_mock_time_task_runner.h"
@@ -26,14 +27,12 @@
 #include "components/ntp_snippets/remote/test_utils.h"
 #include "components/ntp_snippets/user_classifier.h"
 #include "components/prefs/testing_pref_service.h"
-#include "components/signin/core/browser/fake_profile_oauth2_token_service.h"
-#include "components/signin/core/browser/fake_signin_manager.h"
 #include "components/variations/entropy_provider.h"
 #include "components/variations/variations_params_manager.h"
-#include "google_apis/gaia/fake_oauth2_token_service_delegate.h"
 #include "net/traffic_annotation/network_traffic_annotation_test_helper.h"
 #include "net/url_request/test_url_fetcher_factory.h"
 #include "net/url_request/url_request_test_util.h"
+#include "services/identity/public/cpp/identity_test_environment.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -59,7 +58,7 @@ const char kTestChromeContentSuggestionsSignedOutUrl[] =
 const char kTestChromeContentSuggestionsSignedInUrl[] =
     "https://chromecontentsuggestions-pa.googleapis.com/v1/suggestions/fetch";
 
-const char kTestEmail[] = "foo@bar.com";
+const char kTestAccount[] = "foo@bar.com";
 
 // Artificial time delay for JSON parsing.
 const int64_t kTestJsonParsingLatencyMs = 20;
@@ -259,9 +258,7 @@ void ParseJsonDelayed(const std::string& json,
 
 }  // namespace
 
-class RemoteSuggestionsFetcherImplTestBase
-    : public testing::Test,
-      public OAuth2TokenService::DiagnosticsObserver {
+class RemoteSuggestionsFetcherImplTestBase : public testing::Test {
  public:
   explicit RemoteSuggestionsFetcherImplTestBase(const GURL& gurl)
       : default_variation_params_(
@@ -281,8 +278,6 @@ class RemoteSuggestionsFetcherImplTestBase
   }
 
   ~RemoteSuggestionsFetcherImplTestBase() override {
-    if (fake_token_service_)
-      fake_token_service_->RemoveDiagnosticsObserver(this);
   }
 
   void ResetFetcher() { ResetFetcherWithAPIKey(kAPIKey); }
@@ -291,16 +286,8 @@ class RemoteSuggestionsFetcherImplTestBase
     scoped_refptr<net::TestURLRequestContextGetter> request_context_getter =
         new net::TestURLRequestContextGetter(mock_task_runner_.get());
 
-    if (fake_token_service_)
-      fake_token_service_->RemoveDiagnosticsObserver(this);
-    fake_token_service_ = std::make_unique<FakeProfileOAuth2TokenService>(
-        std::make_unique<FakeOAuth2TokenServiceDelegate>(
-            request_context_getter.get()));
-
-    fake_token_service_->AddDiagnosticsObserver(this);
-
     fetcher_ = std::make_unique<RemoteSuggestionsFetcherImpl>(
-        utils_.fake_signin_manager(), fake_token_service_.get(),
+        identity_test_env_.identity_manager(),
         std::move(request_context_getter), utils_.pref_service(), nullptr,
         base::BindRepeating(&ParseJsonDelayed),
         GetFetchEndpoint(version_info::Channel::STABLE), api_key,
@@ -311,26 +298,8 @@ class RemoteSuggestionsFetcherImplTestBase
   }
 
   void SignIn() {
-#if defined(OS_CHROMEOS)
-    utils_.fake_signin_manager()->SignIn(kTestEmail);
-#else
-    utils_.fake_signin_manager()->SignIn(kTestEmail, "user", "password");
-#endif
-  }
-
-  void IssueRefreshToken() {
-    fake_token_service_->GetDelegate()->UpdateCredentials(kTestEmail, "token");
-  }
-
-  void IssueOAuth2Token() {
-    fake_token_service_->IssueAllTokensForAccount(kTestEmail, "access_token",
-                                                  base::Time::Max());
-  }
-
-  void CancelOAuth2TokenRequests() {
-    fake_token_service_->IssueErrorForAllPendingRequestsForAccount(
-        kTestEmail, GoogleServiceAuthError(
-                        GoogleServiceAuthError::State::REQUEST_CANCELED));
+    identity_test_env_.MakePrimaryAccountAvailable(kTestAccount, kTestAccount,
+                                                   "token");
   }
 
   RemoteSuggestionsFetcher::SnippetsAvailableCallback
@@ -380,23 +349,12 @@ class RemoteSuggestionsFetcherImplTestBase
     fake_url_fetcher_factory_->SetFakeResponse(test_url_, response_data,
                                                response_code, status);
   }
-  void set_on_access_token_request_callback(base::OnceClosure callback) {
-    on_access_token_request_callback_ = std::move(callback);
-  }
 
  protected:
   std::map<std::string, std::string> default_variation_params_;
+  identity::IdentityTestEnvironment identity_test_env_;
 
  private:
-  // OAuth2TokenService::DiagnosticsObserver:
-  void OnAccessTokenRequested(
-      const std::string& account_id,
-      const std::string& consumer_id,
-      const OAuth2TokenService::ScopeSet& scopes) override {
-    if (on_access_token_request_callback_)
-      std::move(on_access_token_request_callback_).Run();
-  }
-
   // TODO(tzik): Remove |clock_| after updating GetMockTickClock to own the
   // instance. http://crbug.com/789079
   std::unique_ptr<base::Clock> clock_;
@@ -407,13 +365,11 @@ class RemoteSuggestionsFetcherImplTestBase
   FailingFakeURLFetcherFactory failing_url_fetcher_factory_;
   // Initialized lazily in SetFakeResponse().
   std::unique_ptr<net::FakeURLFetcherFactory> fake_url_fetcher_factory_;
-  std::unique_ptr<FakeProfileOAuth2TokenService> fake_token_service_;
   std::unique_ptr<RemoteSuggestionsFetcherImpl> fetcher_;
   std::unique_ptr<UserClassifier> user_classifier_;
   MockSnippetsAvailableCallback mock_callback_;
   const GURL test_url_;
   base::HistogramTester histogram_tester_;
-  base::OnceClosure on_access_token_request_callback_;
 
   DISALLOW_COPY_AND_ASSIGN(RemoteSuggestionsFetcherImplTestBase);
 };
@@ -489,11 +445,7 @@ TEST_F(RemoteSuggestionsSignedOutFetcherTest, ShouldFetchSuccessfully) {
 }
 
 TEST_F(RemoteSuggestionsSignedInFetcherTest, ShouldFetchSuccessfully) {
-  base::RunLoop run_loop;
-  set_on_access_token_request_callback(run_loop.QuitClosure());
-
   SignIn();
-  IssueRefreshToken();
 
   const std::string kJsonStr =
       "{\"categories\" : [{"
@@ -523,9 +475,9 @@ TEST_F(RemoteSuggestionsSignedInFetcherTest, ShouldFetchSuccessfully) {
   fetcher().FetchSnippets(test_params(),
                           ToSnippetsAvailableCallback(&mock_callback()));
 
-  run_loop.Run();
+  identity_test_env_.WaitForAccessTokenRequestAndRespondWithToken(
+      "access_token", base::Time::Max());
 
-  IssueOAuth2Token();
   // Wait for the fake response.
   FastForwardUntilNoTasksRemain();
 
@@ -540,11 +492,7 @@ TEST_F(RemoteSuggestionsSignedInFetcherTest, ShouldFetchSuccessfully) {
 }
 
 TEST_F(RemoteSuggestionsSignedInFetcherTest, ShouldRetryWhenOAuthCancelled) {
-  base::RunLoop run_loop;
-  set_on_access_token_request_callback(run_loop.QuitClosure());
-
   SignIn();
-  IssueRefreshToken();
 
   const std::string kJsonStr =
       "{\"categories\" : [{"
@@ -574,20 +522,15 @@ TEST_F(RemoteSuggestionsSignedInFetcherTest, ShouldRetryWhenOAuthCancelled) {
   fetcher().FetchSnippets(test_params(),
                           ToSnippetsAvailableCallback(&mock_callback()));
 
-  // Wait for the first access token request to be made.
-  run_loop.Run();
-
-  // Before cancelling the outstanding access token request, prepare to wait for
-  // the second access token request to be made in response to the cancellation.
-  base::RunLoop run_loop2;
-  set_on_access_token_request_callback(run_loop2.QuitClosure());
-
-  CancelOAuth2TokenRequests();
+  // Cancel the first access token request that's made.
+  identity_test_env_.WaitForAccessTokenRequestAndRespondWithError(
+      GoogleServiceAuthError(GoogleServiceAuthError::State::REQUEST_CANCELED));
 
-  // Wait for the second access token request to be made.
-  run_loop2.Run();
+  // RemoteSuggestionsFetcher should retry fetching an access token if the first
+  // attempt is cancelled. Respond with a valid access token on the retry.
+  identity_test_env_.WaitForAccessTokenRequestAndRespondWithToken(
+      "access_token", base::Time::Max());
 
-  IssueOAuth2Token();
   // Wait for the fake response.
   FastForwardUntilNoTasksRemain();
 

ios/chrome/browser/ntp_snippets/BUILD.gn

@@ -33,5 +33,6 @@ source_set("ntp_snippets") {
     "//ios/chrome/common",
     "//ios/web",
     "//net",
+    "//services/identity/public/cpp",
   ]
 }

ios/chrome/browser/ntp_snippets/ios_chrome_content_suggestions_service_factory.cc

@@ -15,6 +15,7 @@
 #include "ios/chrome/browser/ntp_snippets/ios_chrome_content_suggestions_service_factory_util.h"
 #include "ios/chrome/browser/pref_names.h"
 #include "ios/chrome/browser/reading_list/reading_list_model_factory.h"
+#include "ios/chrome/browser/signin/identity_manager_factory.h"
 #include "ios/chrome/browser/signin/oauth2_token_service_factory.h"
 #include "ios/chrome/browser/signin/signin_manager_factory.h"
 
@@ -46,6 +47,7 @@ IOSChromeContentSuggestionsServiceFactory::
     : BrowserStateKeyedServiceFactory(
           "ContentSuggestionsService",
           BrowserStateDependencyManager::GetInstance()) {
+  DependsOn(IdentityManagerFactory::GetInstance());
   DependsOn(ios::HistoryServiceFactory::GetInstance());
   DependsOn(IOSChromeLargeIconServiceFactory::GetInstance());
   DependsOn(OAuth2TokenServiceFactory::GetInstance());

ios/chrome/browser/ntp_snippets/ios_chrome_content_suggestions_service_factory_util.cc

@@ -44,7 +44,7 @@
 #include "ios/chrome/browser/history/history_service_factory.h"
 #include "ios/chrome/browser/pref_names.h"
 #include "ios/chrome/browser/reading_list/reading_list_model_factory.h"
-#include "ios/chrome/browser/signin/oauth2_token_service_factory.h"
+#include "ios/chrome/browser/signin/identity_manager_factory.h"
 #include "ios/chrome/browser/signin/signin_manager_factory.h"
 #include "ios/chrome/common/channel_info.h"
 #include "ios/web/public/browser_state.h"
@@ -160,9 +160,8 @@ void RegisterRemoteSuggestionsProvider(ContentSuggestionsService* service,
   PrefService* prefs = chrome_browser_state->GetPrefs();
   SigninManager* signin_manager =
       ios::SigninManagerFactory::GetForBrowserState(chrome_browser_state);
-
-  OAuth2TokenService* token_service =
-      OAuth2TokenServiceFactory::GetForBrowserState(chrome_browser_state);
+  identity::IdentityManager* identity_manager =
+      IdentityManagerFactory::GetForBrowserState(chrome_browser_state);
   scoped_refptr<net::URLRequestContextGetter> request_context =
       browser_state->GetRequestContext();
   base::FilePath database_dir(
@@ -177,7 +176,7 @@ void RegisterRemoteSuggestionsProvider(ContentSuggestionsService* service,
                                 : google_apis::GetNonStableAPIKey();
   }
   auto suggestions_fetcher = std::make_unique<RemoteSuggestionsFetcherImpl>(
-      signin_manager, token_service, request_context, prefs, nullptr,
+      identity_manager, request_context, prefs, nullptr,
       base::BindRepeating(&ParseJson), GetFetchEndpoint(GetChannel()), api_key,
       service->user_classifier());