Migrate PopularSitesImpl to SimpleURLLoader

As part of the effort to bring the new Network Service online, we are
deprecating URLFetcher in favor of SimpleURLLoader.

Background: https://docs.google.com/document/d/1YZDPeg3bf46QPU_vUotFwOEPHquww36b-UdRlO3ZpMU/edit

Bug: 844943
Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: I77ec8b2c4ff6a11995f3b6c2ec911c94b16789e4
Reviewed-on: https://chromium-review.googlesource.com/1076596
Commit-Queue: Mark Pilgrim <pilgrim@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Reviewed-by: Bernhard Bauer <bauerb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#562838}

chrome/browser/ntp_tiles/chrome_popular_sites_factory.cc

@@ -9,7 +9,9 @@
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/search_engines/template_url_service_factory.h"
 #include "components/ntp_tiles/popular_sites_impl.h"
+#include "content/public/browser/browser_context.h"
 #include "content/public/browser/browser_thread.h"
+#include "content/public/browser/storage_partition.h"
 #include "content/public/common/service_manager_connection.h"
 #include "services/data_decoder/public/cpp/safe_json_parser.h"
 
@@ -17,7 +19,9 @@ std::unique_ptr<ntp_tiles::PopularSites>
 ChromePopularSitesFactory::NewForProfile(Profile* profile) {
   return std::make_unique<ntp_tiles::PopularSitesImpl>(
       profile->GetPrefs(), TemplateURLServiceFactory::GetForProfile(profile),
-      g_browser_process->variations_service(), profile->GetRequestContext(),
+      g_browser_process->variations_service(),
+      content::BrowserContext::GetDefaultStoragePartition(profile)
+          ->GetURLLoaderFactoryForBrowserProcess(),
       base::Bind(
           data_decoder::SafeJsonParser::Parse,
           content::ServiceManagerConnection::GetForProcess()->GetConnector()));

components/ntp_tiles/BUILD.gn

@@ -59,6 +59,7 @@ static_library("ntp_tiles") {
     "//components/url_formatter",
     "//components/variations",
     "//components/variations/service",
+    "//services/network/public/cpp",
     "//ui/base",
   ]
 }
@@ -100,6 +101,8 @@ source_set("unit_tests") {
     "//components/rappor:test_support",
     "//components/sync_preferences:test_support",
     "//net:test_support",
+    "//services/network:test_support",
+    "//services/network/public/cpp",
     "//testing/gmock",
     "//testing/gtest",
     "//ui/base",

components/ntp_tiles/DEPS

@@ -16,6 +16,8 @@ include_rules = [
   "+components/variations",
   "+jni",
   "+net",
+  "+services/network/public/cpp",
+  "+services/network/test",
   "+ui/gfx",
   "+ui/base",
 ]

components/ntp_tiles/most_visited_sites_unittest.cc

@@ -33,8 +33,9 @@
 #include "components/ntp_tiles/section_type.h"
 #include "components/ntp_tiles/switches.h"
 #include "components/sync_preferences/testing_pref_service_syncable.h"
-#include "net/url_request/test_url_fetcher_factory.h"
-#include "net/url_request/url_request_test_util.h"
+#include "services/network/public/cpp/shared_url_loader_factory.h"
+#include "services/network/public/cpp/weak_wrapper_shared_url_loader_factory.h"
+#include "services/network/test/test_url_loader_factory.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -275,10 +276,10 @@ class PopularSitesFactoryForTest {
  public:
   PopularSitesFactoryForTest(
       sync_preferences::TestingPrefServiceSyncable* pref_service)
-      : prefs_(pref_service),
-        url_fetcher_factory_(/*default_factory=*/nullptr),
-        url_request_context_(new net::TestURLRequestContextGetter(
-            base::ThreadTaskRunnerHandle::Get())) {
+      : prefs_(pref_service) {
+    test_shared_loader_factory_ =
+        base::MakeRefCounted<network::WeakWrapperSharedURLLoaderFactory>(
+            &test_url_loader_factory_);
     PopularSitesImpl::RegisterProfilePrefs(pref_service->registry());
   }
 
@@ -286,9 +287,9 @@ class PopularSitesFactoryForTest {
     prefs_->SetString(prefs::kPopularSitesOverrideCountry, "IN");
     prefs_->SetString(prefs::kPopularSitesOverrideVersion, "5");
 
-    url_fetcher_factory_.ClearFakeResponses();
-    url_fetcher_factory_.SetFakeResponse(
-        GURL("https://www.gstatic.com/chrome/ntp/suggested_sites_IN_5.json"),
+    test_url_loader_factory_.ClearResponses();
+    test_url_loader_factory_.AddResponse(
+        "https://www.gstatic.com/chrome/ntp/suggested_sites_IN_5.json",
         R"([{
               "title": "PopularSite1",
               "url": "http://popularsite1/",
@@ -299,11 +300,10 @@ class PopularSitesFactoryForTest {
               "url": "http://popularsite2/",
               "favicon_url": "http://popularsite2/favicon.ico"
             },
-           ])",
-        net::HTTP_OK, net::URLRequestStatus::SUCCESS);
+           ])");
 
-    url_fetcher_factory_.SetFakeResponse(
-        GURL("https://www.gstatic.com/chrome/ntp/suggested_sites_US_5.json"),
+    test_url_loader_factory_.AddResponse(
+        "https://www.gstatic.com/chrome/ntp/suggested_sites_US_5.json",
         R"([{
               "title": "ESPN",
               "url": "http://www.espn.com",
@@ -317,11 +317,10 @@ class PopularSitesFactoryForTest {
               "url": "http://news.google.com",
               "favicon_url": "http://news.google.com/favicon.ico"
             },
-           ])",
-        net::HTTP_OK, net::URLRequestStatus::SUCCESS);
+           ])");
 
-    url_fetcher_factory_.SetFakeResponse(
-        GURL("https://www.gstatic.com/chrome/ntp/suggested_sites_IN_6.json"),
+    test_url_loader_factory_.AddResponse(
+        "https://www.gstatic.com/chrome/ntp/suggested_sites_IN_6.json",
         R"([{
               "section": 1, // PERSONALIZED
               "sites": [{
@@ -363,22 +362,21 @@ class PopularSitesFactoryForTest {
                     // Intentionally empty site list.
                 ]
             }
-        ])",
-        net::HTTP_OK, net::URLRequestStatus::SUCCESS);
+        ])");
   }
 
   std::unique_ptr<PopularSites> New() {
     return std::make_unique<PopularSitesImpl>(
         prefs_,
         /*template_url_service=*/nullptr,
-        /*variations_service=*/nullptr, url_request_context_.get(),
+        /*variations_service=*/nullptr, test_shared_loader_factory_,
         base::Bind(JsonUnsafeParser::Parse));
   }
 
  private:
   PrefService* prefs_;
-  net::FakeURLFetcherFactory url_fetcher_factory_;
-  scoped_refptr<net::TestURLRequestContextGetter> url_request_context_;
+  network::TestURLLoaderFactory test_url_loader_factory_;
+  scoped_refptr<network::SharedURLLoaderFactory> test_shared_loader_factory_;
 };
 
 // CallbackList-like container without Subscription, mimicking the

components/ntp_tiles/popular_sites_impl.cc

@@ -33,8 +33,9 @@
 #include "net/base/load_flags.h"
 #include "net/http/http_status_code.h"
 #include "net/traffic_annotation/network_traffic_annotation.h"
-#include "net/url_request/url_fetcher.h"
-#include "net/url_request/url_request_status.h"
+#include "services/network/public/cpp/resource_request.h"
+#include "services/network/public/cpp/shared_url_loader_factory.h"
+#include "services/network/public/cpp/simple_url_loader.h"
 
 #if defined(OS_ANDROID) || defined(OS_IOS)
 #include "base/json/json_reader.h"
@@ -46,7 +47,6 @@
 #include "components/ntp_tiles/country_code_ios.h"
 #endif
 
-using net::URLFetcher;
 using variations::VariationsService;
 
 namespace ntp_tiles {
@@ -271,12 +271,12 @@ PopularSitesImpl::PopularSitesImpl(
     PrefService* prefs,
     const TemplateURLService* template_url_service,
     VariationsService* variations_service,
-    net::URLRequestContextGetter* download_context,
+    scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
     ParseJSONCallback parse_json)
     : prefs_(prefs),
       template_url_service_(template_url_service),
       variations_(variations_service),
-      download_context_(download_context),
+      url_loader_factory_(std::move(url_loader_factory)),
       parse_json_(std::move(parse_json)),
       is_fallback_(false),
       sections_(
@@ -452,30 +452,33 @@ void PopularSitesImpl::FetchPopularSites() {
           policy_exception_justification:
             "Not implemented, considered not useful."
         })");
-  fetcher_ = URLFetcher::Create(pending_url_, URLFetcher::GET, this,
-                                traffic_annotation);
-  data_use_measurement::DataUseUserData::AttachToFetcher(
-      fetcher_.get(), data_use_measurement::DataUseUserData::NTP_TILES);
-  fetcher_->SetRequestContext(download_context_);
-  fetcher_->SetLoadFlags(net::LOAD_DO_NOT_SEND_COOKIES |
-                         net::LOAD_DO_NOT_SAVE_COOKIES);
-  fetcher_->SetAutomaticallyRetryOnNetworkChanges(1);
-  fetcher_->Start();
+  auto resource_request = std::make_unique<network::ResourceRequest>();
+  resource_request->url = pending_url_;
+  resource_request->load_flags =
+      net::LOAD_DO_NOT_SEND_COOKIES | net::LOAD_DO_NOT_SAVE_COOKIES;
+  // TODO(https://crbug.com/808498): Re-add data use measurement once
+  // SimpleURLLoader supports it.
+  // ID=data_use_measurement::DataUseUserData::NTP_TILES
+  simple_url_loader_ = network::SimpleURLLoader::Create(
+      std::move(resource_request), traffic_annotation);
+  simple_url_loader_->SetRetryOptions(
+      1, network::SimpleURLLoader::RETRY_ON_NETWORK_CHANGE);
+  simple_url_loader_->DownloadToStringOfUnboundedSizeUntilCrashAndDie(
+      url_loader_factory_.get(),
+      base::BindOnce(&PopularSitesImpl::OnSimpleLoaderComplete,
+                     base::Unretained(this)));
 }
 
-void PopularSitesImpl::OnURLFetchComplete(const net::URLFetcher* source) {
-  DCHECK_EQ(fetcher_.get(), source);
-  std::unique_ptr<net::URLFetcher> free_fetcher = std::move(fetcher_);
+void PopularSitesImpl::OnSimpleLoaderComplete(
+    std::unique_ptr<std::string> response_body) {
+  simple_url_loader_.reset();
 
-  std::string json_string;
-  if (!(source->GetStatus().is_success() &&
-        source->GetResponseCode() == net::HTTP_OK &&
-        source->GetResponseAsString(&json_string))) {
+  if (!response_body) {
     OnDownloadFailed();
     return;
   }
 
-  parse_json_.Run(json_string,
+  parse_json_.Run(*response_body,
                   base::Bind(&PopularSitesImpl::OnJsonParsed,
                              weak_ptr_factory_.GetWeakPtr()),
                   base::Bind(&PopularSitesImpl::OnJsonParseFailed,

components/ntp_tiles/popular_sites_impl.h

@@ -15,15 +15,15 @@
 #include "base/memory/weak_ptr.h"
 #include "base/strings/string16.h"
 #include "components/ntp_tiles/popular_sites.h"
-#include "net/url_request/url_fetcher_delegate.h"
 #include "url/gurl.h"
 
 namespace base {
 class Value;
 }
 
-namespace net {
-class URLRequestContextGetter;
+namespace network {
+class SimpleURLLoader;
+class SharedURLLoaderFactory;
 }
 
 namespace user_prefs {
@@ -46,13 +46,13 @@ using ParseJSONCallback = base::Callback<void(
 
 // Actual (non-test) implementation of the PopularSites interface. Caches the
 // downloaded file on disk to avoid re-downloading on every startup.
-class PopularSitesImpl : public PopularSites, public net::URLFetcherDelegate {
+class PopularSitesImpl : public PopularSites {
  public:
   PopularSitesImpl(
       PrefService* prefs,
       const TemplateURLService* template_url_service,
       variations::VariationsService* variations_service,
-      net::URLRequestContextGetter* download_context,
+      scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
       ParseJSONCallback parse_json);
 
   ~PopularSitesImpl() override;
@@ -77,8 +77,8 @@ class PopularSitesImpl : public PopularSites, public net::URLFetcherDelegate {
   // that already exists.
   void FetchPopularSites();
 
-  // net::URLFetcherDelegate implementation.
-  void OnURLFetchComplete(const net::URLFetcher* source) override;
+  // Called once SimpleURLLoader completes the network request.
+  void OnSimpleLoaderComplete(std::unique_ptr<std::string> response_body);
 
   void OnJsonParsed(std::unique_ptr<base::Value> json);
   void OnJsonParseFailed(const std::string& error_message);
@@ -88,13 +88,13 @@ class PopularSitesImpl : public PopularSites, public net::URLFetcherDelegate {
   PrefService* const prefs_;
   const TemplateURLService* const template_url_service_;
   variations::VariationsService* const variations_;
-  net::URLRequestContextGetter* const download_context_;
+  scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory_;
   ParseJSONCallback parse_json_;
 
   // Set by MaybeStartFetch() and called after fetch completes.
   FinishedCallback callback_;
 
-  std::unique_ptr<net::URLFetcher> fetcher_;
+  std::unique_ptr<network::SimpleURLLoader> simple_url_loader_;
   bool is_fallback_;
   std::map<SectionType, SitesVector> sections_;
   GURL pending_url_;

components/ntp_tiles/popular_sites_impl_unittest.cc

@@ -17,6 +17,7 @@
 #include "base/optional.h"
 #include "base/run_loop.h"
 #include "base/strings/string_number_conversions.h"
+#include "base/strings/utf_string_conversions.h"
 #include "base/test/scoped_feature_list.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/values.h"
@@ -28,9 +29,9 @@
 #include "components/pref_registry/pref_registry_syncable.h"
 #include "components/sync_preferences/testing_pref_service_syncable.h"
 #include "net/http/http_status_code.h"
-#include "net/url_request/test_url_fetcher_factory.h"
-#include "net/url_request/url_request_status.h"
-#include "net/url_request/url_request_test_util.h"
+#include "services/network/public/cpp/shared_url_loader_factory.h"
+#include "services/network/public/cpp/weak_wrapper_shared_url_loader_factory.h"
+#include "services/network/test/test_url_loader_factory.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -98,7 +99,9 @@ class PopularSitesTest : public ::testing::Test {
             // No "title_source" (like in v5 or earlier). Defaults to TITLE_TAG.
         },
         prefs_(new sync_preferences::TestingPrefServiceSyncable()),
-        url_fetcher_factory_(nullptr) {
+        test_shared_loader_factory_(
+            base::MakeRefCounted<network::WeakWrapperSharedURLLoaderFactory>(
+                &test_url_loader_factory_)) {
     PopularSitesImpl::RegisterProfilePrefs(prefs_->registry());
   }
 
@@ -132,8 +135,7 @@ class PopularSitesTest : public ::testing::Test {
                          const TestPopularSiteVector& sites) {
     std::string sites_string;
     base::JSONWriter::Write(*CreateListFromTestSites(sites), &sites_string);
-    url_fetcher_factory_.SetFakeResponse(GURL(url), sites_string, net::HTTP_OK,
-                                         net::URLRequestStatus::SUCCESS);
+    test_url_loader_factory_.AddResponse(url, sites_string);
   }
 
   void RespondWithV6JSON(const std::string& url,
@@ -147,18 +149,15 @@ class PopularSitesTest : public ::testing::Test {
     }
     std::string sites_string;
     base::JSONWriter::Write(sections_value, &sites_string);
-    url_fetcher_factory_.SetFakeResponse(GURL(url), sites_string, net::HTTP_OK,
-                                         net::URLRequestStatus::SUCCESS);
+    test_url_loader_factory_.AddResponse(url, sites_string);
   }
 
   void RespondWithData(const std::string& url, const std::string& data) {
-    url_fetcher_factory_.SetFakeResponse(GURL(url), data, net::HTTP_OK,
-                                         net::URLRequestStatus::SUCCESS);
+    test_url_loader_factory_.AddResponse(url, data);
   }
 
   void RespondWith404(const std::string& url) {
-    url_fetcher_factory_.SetFakeResponse(GURL(url), "404", net::HTTP_NOT_FOUND,
-                                         net::URLRequestStatus::SUCCESS);
+    test_url_loader_factory_.AddResponse(url, "", net::HTTP_NOT_FOUND);
   }
 
   void ReregisterProfilePrefs() {
@@ -182,11 +181,7 @@ class PopularSitesTest : public ::testing::Test {
   base::Optional<bool> FetchAllSections(
       bool force_download,
       std::map<SectionType, PopularSites::SitesVector>* sections) {
-    scoped_refptr<net::TestURLRequestContextGetter> url_request_context(
-        new net::TestURLRequestContextGetter(
-            base::ThreadTaskRunnerHandle::Get()));
-    std::unique_ptr<PopularSites> popular_sites =
-        CreatePopularSites(url_request_context.get());
+    std::unique_ptr<PopularSites> popular_sites = CreatePopularSites();
 
     base::RunLoop loop;
     base::Optional<bool> save_success;
@@ -204,12 +199,11 @@ class PopularSitesTest : public ::testing::Test {
     return save_success;
   }
 
-  std::unique_ptr<PopularSites> CreatePopularSites(
-      net::URLRequestContextGetter* context) {
+  std::unique_ptr<PopularSites> CreatePopularSites() {
     return std::make_unique<PopularSitesImpl>(
         prefs_.get(),
         /*template_url_service=*/nullptr,
-        /*variations_service=*/nullptr, context,
+        /*variations_service=*/nullptr, test_shared_loader_factory_,
         base::Bind(JsonUnsafeParser::Parse));
   }
 
@@ -219,15 +213,12 @@ class PopularSitesTest : public ::testing::Test {
 
   base::MessageLoopForUI ui_loop_;
   std::unique_ptr<sync_preferences::TestingPrefServiceSyncable> prefs_;
-  net::FakeURLFetcherFactory url_fetcher_factory_;
+  network::TestURLLoaderFactory test_url_loader_factory_;
+  scoped_refptr<network::SharedURLLoaderFactory> test_shared_loader_factory_;
 };
 
 TEST_F(PopularSitesTest, ContainsDefaultTilesRightAfterConstruction) {
-  scoped_refptr<net::TestURLRequestContextGetter> url_request_context(
-      new net::TestURLRequestContextGetter(
-          base::ThreadTaskRunnerHandle::Get()));
-
-  auto popular_sites = CreatePopularSites(url_request_context.get());
+  auto popular_sites = CreatePopularSites();
   EXPECT_THAT(
       popular_sites->sections(),
       ElementsAre(Pair(SectionType::PERSONALIZED,
@@ -239,10 +230,7 @@ TEST_F(PopularSitesTest, IsEmptyOnConstructionIfDisabledByTrial) {
   override_features.InitAndDisableFeature(kPopularSitesBakedInContentFeature);
   ReregisterProfilePrefs();
 
-  scoped_refptr<net::TestURLRequestContextGetter> url_request_context(
-      new net::TestURLRequestContextGetter(
-          base::ThreadTaskRunnerHandle::Get()));
-  auto popular_sites = CreatePopularSites(url_request_context.get());
+  auto popular_sites = CreatePopularSites();
 
   EXPECT_THAT(popular_sites->sections(),
               ElementsAre(Pair(SectionType::PERSONALIZED, IsEmpty())));
@@ -311,11 +299,7 @@ TEST_F(PopularSitesTest, PopulatesWithDefaultResoucesOnFailure) {
 
 #if defined(OS_ANDROID) || defined(OS_IOS)
 TEST_F(PopularSitesTest, AddsIconResourcesToDefaultPages) {
-  scoped_refptr<net::TestURLRequestContextGetter> url_request_context(
-      new net::TestURLRequestContextGetter(
-          base::ThreadTaskRunnerHandle::Get()));
-  std::unique_ptr<PopularSites> popular_sites =
-      CreatePopularSites(url_request_context.get());
+  std::unique_ptr<PopularSites> popular_sites = CreatePopularSites();
 
   const PopularSites::SitesVector& sites =
       popular_sites->sections().at(SectionType::PERSONALIZED);
@@ -334,11 +318,7 @@ TEST_F(PopularSitesTest, ProvidesDefaultSitesUntilCallbackReturns) {
   RespondWithV5JSON(
       "https://www.gstatic.com/chrome/ntp/suggested_sites_ZZ_5.json",
       {kWikipedia});
-  scoped_refptr<net::TestURLRequestContextGetter> url_request_context(
-      new net::TestURLRequestContextGetter(
-          base::ThreadTaskRunnerHandle::Get()));
-  std::unique_ptr<PopularSites> popular_sites =
-      CreatePopularSites(url_request_context.get());
+  std::unique_ptr<PopularSites> popular_sites = CreatePopularSites();
 
   base::RunLoop loop;
   base::Optional<bool> save_success = false;

ios/chrome/browser/ntp_tiles/BUILD.gn

@@ -27,6 +27,7 @@ source_set("ntp_tiles") {
     "//ios/chrome/browser/search_engines",
     "//ios/chrome/browser/suggestions",
     "//ios/web",
+    "//services/network/public/cpp",
   ]
 }
 

ios/chrome/browser/ntp_tiles/ios_popular_sites_factory.cc

@@ -12,6 +12,8 @@
 #include "ios/chrome/browser/browser_state/chrome_browser_state.h"
 #include "ios/chrome/browser/search_engines/template_url_service_factory.h"
 #include "ios/web/public/web_thread.h"
+#include "services/network/public/cpp/shared_url_loader_factory.h"
+#include "services/network/public/cpp/weak_wrapper_shared_url_loader_factory.h"
 
 std::unique_ptr<ntp_tiles::PopularSites>
 IOSPopularSitesFactory::NewForBrowserState(
@@ -20,6 +22,7 @@ IOSPopularSitesFactory::NewForBrowserState(
       browser_state->GetPrefs(),
       ios::TemplateURLServiceFactory::GetForBrowserState(browser_state),
       GetApplicationContext()->GetVariationsService(),
-      browser_state->GetRequestContext(),
+      base::MakeRefCounted<network::WeakWrapperSharedURLLoaderFactory>(
+          browser_state->GetURLLoaderFactory()),
       base::Bind(ntp_tiles::JsonUnsafeParser::Parse));
 }