[NTP Snippets] Revert RemoteSuggestionsFetcher usage of IdentityManager

This CL reverts the following two CLs:
- "[NTP Snippets] RemoteSuggestionsFetcherImpl test uses IdentityTestEnv"
  (fce90bed)
- "[NTP Snippets] Have RemoteSuggestionsFetcherImpl use IdentityManager"
  (683639a4)

The reason is that the production change caused a regression in NTP
Snippets: on signout, an unauthenticated fetch of snippets is no longer
performed. The underlying cause is that IdentityManager being notified of
signout (and changing its internal state) currently races with NTP Snippets
itself being notified of signout (and starting its flow to refetch
snippets).

This is a straight revert with three minor exceptions:
- There was a minor amount of conflict resolution to do due to
  intervening changes of base::MakeUnique to std::make_unique
- The //services/identity API additions in fce90bed were *not* reverted
  as they are used by code that has subsequently landed in the tree.
- Similarly, the //components/ntp_snippets/BUILD.gn changes were not
  reverted as those dependencies are now used on trunk even without
  these changes.

This CL itself should be able to be reverted once I have fixed the above
race by ensuring that IdentityManager is notified of signin/signout events
before any observers of SigninManager. Note that in the long term this
won't be a problem as there will be no external observers of SigninManager.

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

TBR=gambard@chromium.org

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

chrome/browser/ntp_snippets/content_suggestions_service_factory.cc

@@ -25,7 +25,6 @@
 #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"
@@ -339,8 +338,8 @@ void RegisterArticleProviderIfEnabled(ContentSuggestionsService* service,
   }
 
   PrefService* pref_service = profile->GetPrefs();
-  identity::IdentityManager* identity_manager =
-      IdentityManagerFactory::GetForProfile(profile);
+  OAuth2TokenService* token_service =
+      ProfileOAuth2TokenServiceFactory::GetForProfile(profile);
   UrlLanguageHistogram* language_histogram =
       UrlLanguageHistogramFactory::GetForBrowserContext(profile);
 
@@ -376,7 +375,8 @@ void RegisterArticleProviderIfEnabled(ContentSuggestionsService* service,
   }
 #endif  // BUILDFLAG(ENABLE_OFFLINE_PAGES)
   auto suggestions_fetcher = base::MakeUnique<RemoteSuggestionsFetcherImpl>(
-      identity_manager, request_context, pref_service, language_histogram,
+      signin_manager, token_service, request_context, pref_service,
+      language_histogram,
       base::Bind(
           &data_decoder::SafeJsonParser::Parse,
           content::ServiceManagerConnection::GetForProcess()->GetConnector()),
@@ -454,7 +454,6 @@ 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,7 +124,8 @@ void FilterCategories(FetchedCategoriesVector* categories,
 }  // namespace
 
 RemoteSuggestionsFetcherImpl::RemoteSuggestionsFetcherImpl(
-    identity::IdentityManager* identity_manager,
+    SigninManagerBase* signin_manager,
+    OAuth2TokenService* token_service,
     scoped_refptr<URLRequestContextGetter> url_request_context_getter,
     PrefService* pref_service,
     UrlLanguageHistogram* language_histogram,
@@ -132,7 +133,8 @@ RemoteSuggestionsFetcherImpl::RemoteSuggestionsFetcherImpl(
     const GURL& api_endpoint,
     const std::string& api_key,
     const UserClassifier* user_classifier)
-    : identity_manager_(identity_manager),
+    : signin_manager_(signin_manager),
+      token_service_(token_service),
       url_request_context_getter_(std::move(url_request_context_getter)),
       language_histogram_(language_histogram),
       parse_json_callback_(parse_json_callback),
@@ -177,7 +179,7 @@ void RemoteSuggestionsFetcherImpl::FetchSnippets(
       .SetUrlRequestContextGetter(url_request_context_getter_)
       .SetUserClassifier(*user_classifier_);
 
-  if (identity_manager_->HasPrimaryAccount()) {
+  if (signin_manager_->IsAuthenticated()) {
     // Signed-in: get OAuth token --> fetch suggestions.
     pending_requests_.emplace(std::move(builder), std::move(callback));
     StartTokenRequest();
@@ -209,7 +211,7 @@ void RemoteSuggestionsFetcherImpl::FetchSnippetsAuthenticated(
     const std::string& oauth_access_token) {
   // TODO(jkrcal, treib): Add unit-tests for authenticated fetches.
   builder.SetUrl(fetch_url_)
-      .SetAuthentication(identity_manager_->GetPrimaryAccountInfo().account_id,
+      .SetAuthentication(signin_manager_->GetAuthenticatedAccountId(),
                          base::StringPrintf(kAuthorizationRequestHeaderFormat,
                                             oauth_access_token.c_str()));
   StartRequest(std::move(builder), std::move(callback));
@@ -232,8 +234,8 @@ void RemoteSuggestionsFetcherImpl::StartTokenRequest() {
   }
 
   OAuth2TokenService::ScopeSet scopes{kContentSuggestionsApiScope};
-  token_fetcher_ = identity_manager_->CreateAccessTokenFetcherForPrimaryAccount(
-      "ntp_snippets", scopes,
+  token_fetcher_ = std::make_unique<identity::PrimaryAccountAccessTokenFetcher>(
+      "ntp_snippets", signin_manager_, token_service_, scopes,
       base::BindOnce(&RemoteSuggestionsFetcherImpl::AccessTokenFetchFinished,
                      base::Unretained(this)),
       identity::PrimaryAccountAccessTokenFetcher::Mode::kWaitUntilAvailable);

components/ntp_snippets/remote/remote_suggestions_fetcher_impl.h

@@ -20,14 +20,15 @@
 #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;
 }
 
@@ -42,7 +43,8 @@ class UserClassifier;
 class RemoteSuggestionsFetcherImpl : public RemoteSuggestionsFetcher {
  public:
   RemoteSuggestionsFetcherImpl(
-      identity::IdentityManager* identity_manager,
+      SigninManagerBase* signin_manager,
+      OAuth2TokenService* token_service,
       scoped_refptr<net::URLRequestContextGetter> url_request_context_getter,
       PrefService* pref_service,
       language::UrlLanguageHistogram* language_histogram,
@@ -88,7 +90,8 @@ class RemoteSuggestionsFetcherImpl : public RemoteSuggestionsFetcher {
                      const std::string& error_details);
 
   // Authentication for signed-in users.
-  identity::IdentityManager* identity_manager_;
+  SigninManagerBase* signin_manager_;
+  OAuth2TokenService* token_service_;
 
   std::unique_ptr<identity::PrimaryAccountAccessTokenFetcher> token_fetcher_;
 

components/ntp_snippets/remote/remote_suggestions_fetcher_impl_unittest.cc

@@ -10,7 +10,6 @@
 
 #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"
@@ -27,12 +26,14 @@
 #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"
 
@@ -58,7 +59,7 @@ const char kTestChromeContentSuggestionsSignedOutUrl[] =
 const char kTestChromeContentSuggestionsSignedInUrl[] =
     "https://chromecontentsuggestions-pa.googleapis.com/v1/suggestions/fetch";
 
-const char kTestAccount[] = "foo@bar.com";
+const char kTestEmail[] = "foo@bar.com";
 
 // Artificial time delay for JSON parsing.
 const int64_t kTestJsonParsingLatencyMs = 20;
@@ -258,7 +259,9 @@ void ParseJsonDelayed(const std::string& json,
 
 }  // namespace
 
-class RemoteSuggestionsFetcherImplTestBase : public testing::Test {
+class RemoteSuggestionsFetcherImplTestBase
+    : public testing::Test,
+      public OAuth2TokenService::DiagnosticsObserver {
  public:
   explicit RemoteSuggestionsFetcherImplTestBase(const GURL& gurl)
       : default_variation_params_(
@@ -278,6 +281,8 @@ class RemoteSuggestionsFetcherImplTestBase : public testing::Test {
   }
 
   ~RemoteSuggestionsFetcherImplTestBase() override {
+    if (fake_token_service_)
+      fake_token_service_->RemoveDiagnosticsObserver(this);
   }
 
   void ResetFetcher() { ResetFetcherWithAPIKey(kAPIKey); }
@@ -286,8 +291,16 @@ class RemoteSuggestionsFetcherImplTestBase : public testing::Test {
     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>(
-        identity_test_env_.identity_manager(),
+        utils_.fake_signin_manager(), fake_token_service_.get(),
         std::move(request_context_getter), utils_.pref_service(), nullptr,
         base::BindRepeating(&ParseJsonDelayed),
         GetFetchEndpoint(version_info::Channel::STABLE), api_key,
@@ -298,8 +311,26 @@ class RemoteSuggestionsFetcherImplTestBase : public testing::Test {
   }
 
   void SignIn() {
-    identity_test_env_.MakePrimaryAccountAvailable(kTestAccount, kTestAccount,
-                                                   "token");
+#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));
   }
 
   RemoteSuggestionsFetcher::SnippetsAvailableCallback
@@ -349,12 +380,23 @@ class RemoteSuggestionsFetcherImplTestBase : public testing::Test {
     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_;
@@ -365,11 +407,13 @@ class RemoteSuggestionsFetcherImplTestBase : public testing::Test {
   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);
 };
@@ -445,7 +489,11 @@ 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\" : [{"
@@ -475,9 +523,9 @@ TEST_F(RemoteSuggestionsSignedInFetcherTest, ShouldFetchSuccessfully) {
   fetcher().FetchSnippets(test_params(),
                           ToSnippetsAvailableCallback(&mock_callback()));
 
-  identity_test_env_.WaitForAccessTokenRequestAndRespondWithToken(
-      "access_token", base::Time::Max());
+  run_loop.Run();
 
+  IssueOAuth2Token();
   // Wait for the fake response.
   FastForwardUntilNoTasksRemain();
 
@@ -492,7 +540,11 @@ 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\" : [{"
@@ -522,15 +574,20 @@ TEST_F(RemoteSuggestionsSignedInFetcherTest, ShouldRetryWhenOAuthCancelled) {
   fetcher().FetchSnippets(test_params(),
                           ToSnippetsAvailableCallback(&mock_callback()));
 
-  // Cancel the first access token request that's made.
-  identity_test_env_.WaitForAccessTokenRequestAndRespondWithError(
-      GoogleServiceAuthError(GoogleServiceAuthError::State::REQUEST_CANCELED));
+  // 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();
 
-  // 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());
+  // Wait for the second access token request to be made.
+  run_loop2.Run();
 
+  IssueOAuth2Token();
   // Wait for the fake response.
   FastForwardUntilNoTasksRemain();
 

ios/chrome/browser/ntp_snippets/BUILD.gn

@@ -33,6 +33,5 @@ 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,7 +15,6 @@
 #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"
 
@@ -47,7 +46,6 @@ 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

@@ -45,7 +45,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/identity_manager_factory.h"
+#include "ios/chrome/browser/signin/oauth2_token_service_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"
@@ -161,8 +161,9 @@ void RegisterRemoteSuggestionsProvider(ContentSuggestionsService* service,
   PrefService* prefs = chrome_browser_state->GetPrefs();
   SigninManager* signin_manager =
       ios::SigninManagerFactory::GetForBrowserState(chrome_browser_state);
-  identity::IdentityManager* identity_manager =
-      IdentityManagerFactory::GetForBrowserState(chrome_browser_state);
+
+  OAuth2TokenService* token_service =
+      OAuth2TokenServiceFactory::GetForBrowserState(chrome_browser_state);
   scoped_refptr<net::URLRequestContextGetter> request_context =
       browser_state->GetRequestContext();
   base::FilePath database_dir(
@@ -177,7 +178,7 @@ void RegisterRemoteSuggestionsProvider(ContentSuggestionsService* service,
                                 : google_apis::GetNonStableAPIKey();
   }
   auto suggestions_fetcher = std::make_unique<RemoteSuggestionsFetcherImpl>(
-      identity_manager, request_context, prefs, nullptr,
+      signin_manager, token_service, request_context, prefs, nullptr,
       base::BindRepeating(&ParseJson), GetFetchEndpoint(GetChannel()), api_key,
       service->user_classifier());