Migrate WebHistoryService to SimpleURLLoader

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

chrome/browser/browsing_data/counters/history_counter_browsertest.cc

@@ -36,8 +36,8 @@ class HistoryCounterTest : public InProcessBrowserTest {
     time_ = base::Time::Now();
     history_service_ = HistoryServiceFactory::GetForProfileWithoutCreating(
         browser()->profile());
-    fake_web_history_service_.reset(new history::FakeWebHistoryService(
-        browser()->profile()->GetRequestContext()));
+    fake_web_history_service_ =
+        std::make_unique<history::FakeWebHistoryService>();
 
     SetHistoryDeletionPref(true);
     SetDeletionPeriodPref(browsing_data::TimePeriod::ALL_TIME);

chrome/browser/browsing_data/counters/sync_aware_counter_browsertest.cc

@@ -36,8 +36,8 @@ class SyncAwareCounterTest : public SyncTest {
   ~SyncAwareCounterTest() override {}
 
   void SetUpOnMainThread() override {
-    fake_web_history_service_.reset(new history::FakeWebHistoryService(
-        browser()->profile()->GetRequestContext()));
+    fake_web_history_service_ =
+        std::make_unique<history::FakeWebHistoryService>();
     run_loop_.reset(new base::RunLoop());
   }
 

chrome/browser/history/web_history_service_factory.cc

@@ -11,7 +11,8 @@
 #include "components/signin/core/browser/profile_oauth2_token_service.h"
 #include "components/signin/core/browser/signin_manager.h"
 #include "components/sync/driver/sync_service.h"
-#include "net/url_request/url_request_context_getter.h"
+#include "content/public/browser/browser_context.h"
+#include "content/public/browser/storage_partition.h"
 
 namespace {
 // Returns true if the user is signed in and full history sync is enabled,
@@ -51,7 +52,8 @@ KeyedService* WebHistoryServiceFactory::BuildServiceInstanceFor(
 
   return new history::WebHistoryService(
       IdentityManagerFactory::GetForProfile(profile),
-      profile->GetRequestContext());
+      content::BrowserContext::GetDefaultStoragePartition(profile)
+          ->GetURLLoaderFactoryForBrowserProcess());
 }
 
 WebHistoryServiceFactory::WebHistoryServiceFactory()

chrome/browser/ui/webui/browsing_history_handler_unittest.cc

@@ -143,11 +143,8 @@ class BrowsingHistoryHandlerTest : public ::testing::Test {
 
   static std::unique_ptr<KeyedService> BuildFakeWebHistoryService(
       content::BrowserContext* context) {
-    Profile* profile = static_cast<TestingProfile*>(context);
-
     std::unique_ptr<history::FakeWebHistoryService> service =
-        std::make_unique<history::FakeWebHistoryService>(
-            profile->GetRequestContext());
+        std::make_unique<history::FakeWebHistoryService>();
     service->SetupFakeResponse(true /* success */, net::HTTP_OK);
     return std::move(service);
   }

components/browsing_data/core/history_notice_utils_unittest.cc

@@ -63,8 +63,7 @@ class HistoryNoticeUtilsTest : public ::testing::Test {
 
   void SetUp() override {
     sync_service_.reset(new TestSyncService());
-    history_service_.reset(new history::FakeWebHistoryService(
-        url_request_context_));
+    history_service_ = std::make_unique<history::FakeWebHistoryService>();
     history_service_->SetupFakeResponse(true /* success */, net::HTTP_OK);
   }
 

components/history/DEPS

@@ -6,6 +6,9 @@ include_rules = [
   "+components/sync",
   "+google_apis/gaia",
   "+net",
+  "+services/network/public/cpp",
+  "+services/network/public/mojom",
+  "+services/network/test",
   "+sql",
   "+third_party/skia",
   "+third_party/sqlite",

components/history/core/browser/BUILD.gn

@@ -114,6 +114,7 @@ static_library("browser") {
     "//google_apis",
     "//net",
     "//services/identity/public/cpp",
+    "//services/network/public/cpp",
     "//sql",
     "//third_party/sqlite",
     "//ui/base",
@@ -226,6 +227,8 @@ source_set("unit_tests") {
     "//components/sync:test_support_driver",
     "//components/sync:test_support_model",
     "//net:test_support",
+    "//services/network:test_support",
+    "//services/network/public/cpp",
     "//sql",
     "//sql:test_support",
     "//testing/gtest",

components/history/core/browser/browsing_history_service_unittest.cc

@@ -120,8 +120,7 @@ class TestBrowsingHistoryDriver : public BrowsingHistoryDriver {
 
 class TestWebHistoryService : public FakeWebHistoryService {
  public:
-  TestWebHistoryService()
-      : FakeWebHistoryService(scoped_refptr<net::URLRequestContextGetter>()) {}
+  TestWebHistoryService() : FakeWebHistoryService() {}
 
   void TriggerOnWebHistoryDeleted() {
     TestRequest request;

components/history/core/browser/web_history_service.cc

@@ -28,10 +28,10 @@
 #include "net/base/url_util.h"
 #include "net/http/http_status_code.h"
 #include "net/http/http_util.h"
-#include "net/url_request/url_fetcher.h"
-#include "net/url_request/url_fetcher_delegate.h"
-#include "net/url_request/url_request_context_getter.h"
 #include "services/identity/public/cpp/identity_manager.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"
 #include "ui/base/device_form_factor.h"
 #include "url/gurl.h"
 
@@ -63,11 +63,10 @@ const char kPostDataMimeType[] = "text/plain";
 
 const char kSyncProtoMimeType[] = "application/octet-stream";
 
-// The maximum number of retries for the URLFetcher requests.
+// The maximum number of retries for the SimpleURLLoader requests.
 const size_t kMaxRetries = 1;
 
-class RequestImpl : public WebHistoryService::Request,
-                    private net::URLFetcherDelegate {
+class RequestImpl : public WebHistoryService::Request {
  public:
   ~RequestImpl() override {}
 
@@ -85,12 +84,12 @@ class RequestImpl : public WebHistoryService::Request,
 
   RequestImpl(
       identity::IdentityManager* identity_manager,
-      const scoped_refptr<net::URLRequestContextGetter>& request_context,
+      scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
       const GURL& url,
       const WebHistoryService::CompletionCallback& callback,
       const net::PartialNetworkTrafficAnnotationTag& partial_traffic_annotation)
       : identity_manager_(identity_manager),
-        request_context_(request_context),
+        url_loader_factory_(std::move(url_loader_factory)),
         url_(url),
         post_data_mime_type_(kPostDataMimeType),
         response_code_(0),
@@ -99,7 +98,7 @@ class RequestImpl : public WebHistoryService::Request,
         is_pending_(false),
         partial_traffic_annotation_(partial_traffic_annotation) {
     DCHECK(identity_manager_);
-    DCHECK(request_context_);
+    DCHECK(url_loader_factory_);
   }
 
   void OnAccessTokenFetchComplete(const GoogleServiceAuthError& error,
@@ -122,8 +121,49 @@ class RequestImpl : public WebHistoryService::Request,
     UMA_HISTOGRAM_BOOLEAN("WebHistory.OAuthTokenCompletion", true);
 
     // Got an access token -- start the actual API request.
-    url_fetcher_ = CreateUrlFetcher(access_token);
-    url_fetcher_->Start();
+    net::NetworkTrafficAnnotationTag traffic_annotation =
+        net::CompleteNetworkTrafficAnnotation("web_history_service",
+                                              partial_traffic_annotation_,
+                                              R"(
+          semantics {
+            sender: "Web History"
+            destination: GOOGLE_OWNED_SERVICE
+          }
+          policy {
+            cookies_allowed: NO
+            setting:
+              "To disable this feature, users can either sign out or disable "
+              "history sync via unchecking 'History' setting under 'Advanced "
+              "sync settings."
+          })");
+    auto resource_request = std::make_unique<network::ResourceRequest>();
+    resource_request->url = url_;
+    resource_request->load_flags =
+        net::LOAD_DO_NOT_SEND_COOKIES | net::LOAD_DO_NOT_SAVE_COOKIES;
+    resource_request->method = post_data_ ? "POST" : "GET";
+    resource_request->headers.SetHeader(net::HttpRequestHeaders::kAuthorization,
+                                        "Bearer " + access_token);
+    resource_request->headers.SetHeader(
+        "X-Developer-Key", GaiaUrls::GetInstance()->oauth2_chrome_client_id());
+    if (!user_agent_.empty()) {
+      resource_request->headers.SetHeader(net::HttpRequestHeaders::kUserAgent,
+                                          user_agent_);
+    }
+    // TODO(https://crbug.com/808498): Re-add data use measurement once
+    // SimpleURLLoader supports it.
+    // ID=data_use_measurement::DataUseUserData::WEB_HISTORY_SERVICE
+    simple_url_loader_ = network::SimpleURLLoader::Create(
+        std::move(resource_request), traffic_annotation);
+    if (post_data_) {
+      simple_url_loader_->AttachStringForUpload(post_data_.value(),
+                                                post_data_mime_type_);
+    }
+    simple_url_loader_->SetRetryOptions(kMaxRetries,
+                                        network::SimpleURLLoader::RETRY_ON_5XX);
+    simple_url_loader_->DownloadToStringOfUnboundedSizeUntilCrashAndDie(
+        url_loader_factory_.get(),
+        base::BindOnce(&RequestImpl::OnSimpleLoaderComplete,
+                       base::Unretained(this)));
   }
 
   // Tells the request to do its thang.
@@ -140,10 +180,14 @@ class RequestImpl : public WebHistoryService::Request,
     is_pending_ = true;
   }
 
-  // content::URLFetcherDelegate interface.
-  void OnURLFetchComplete(const net::URLFetcher* source) override {
-    DCHECK_EQ(source, url_fetcher_.get());
-    response_code_ = url_fetcher_->GetResponseCode();
+  void OnSimpleLoaderComplete(std::unique_ptr<std::string> response_body) {
+    response_code_ = -1;
+    if (simple_url_loader_->ResponseInfo() &&
+        simple_url_loader_->ResponseInfo()->headers) {
+      response_code_ =
+          simple_url_loader_->ResponseInfo()->headers->response_code();
+    }
+    simple_url_loader_.reset();
 
     UMA_HISTOGRAM_CUSTOM_ENUMERATION("WebHistory.OAuthTokenResponseCode",
         net::HttpUtil::MapStatusCodeForHistogram(response_code_),
@@ -162,58 +206,17 @@ class RequestImpl : public WebHistoryService::Request,
       Start();
       return;
     }
-    url_fetcher_->GetResponseAsString(&response_body_);
-    url_fetcher_.reset();
+    if (response_body) {
+      response_body_ = std::move(*response_body);
+    } else {
+      response_body_.clear();
+    }
     is_pending_ = false;
     callback_.Run(this, true);
     // It is valid for the callback to delete |this|, so do not access any
     // members below here.
   }
 
-  // Helper for creating a new URLFetcher for the API request.
-  std::unique_ptr<net::URLFetcher> CreateUrlFetcher(
-      const std::string& access_token) {
-    net::URLFetcher::RequestType request_type = post_data_ ?
-        net::URLFetcher::POST : net::URLFetcher::GET;
-    net::NetworkTrafficAnnotationTag traffic_annotation =
-        net::CompleteNetworkTrafficAnnotation("web_history_service",
-                                              partial_traffic_annotation_,
-                                              R"(
-          semantics {
-            sender: "Web History"
-            destination: GOOGLE_OWNED_SERVICE
-          }
-          policy {
-            cookies_allowed: NO
-            setting:
-              "To disable this feature, users can either sign out or disable "
-              "history sync via unchecking 'History' setting under 'Advanced "
-              "sync settings."
-          })");
-    std::unique_ptr<net::URLFetcher> fetcher =
-        net::URLFetcher::Create(url_, request_type, this, traffic_annotation);
-    data_use_measurement::DataUseUserData::AttachToFetcher(
-        fetcher.get(),
-        data_use_measurement::DataUseUserData::WEB_HISTORY_SERVICE);
-    fetcher->SetRequestContext(request_context_.get());
-    fetcher->SetMaxRetriesOn5xx(kMaxRetries);
-    fetcher->SetLoadFlags(net::LOAD_DO_NOT_SEND_COOKIES |
-                          net::LOAD_DO_NOT_SAVE_COOKIES);
-    fetcher->AddExtraRequestHeader("Authorization: Bearer " + access_token);
-    fetcher->AddExtraRequestHeader("X-Developer-Key: " +
-        GaiaUrls::GetInstance()->oauth2_chrome_client_id());
-
-    if (!user_agent_.empty()) {
-      fetcher->AddExtraRequestHeader(
-          std::string(net::HttpRequestHeaders::kUserAgent) +
-          ": " + user_agent_);
-    }
-
-    if (post_data_)
-      fetcher->SetUploadData(post_data_mime_type_, post_data_.value());
-    return fetcher;
-  }
-
   void SetPostData(const std::string& post_data) override {
     SetPostDataAndType(post_data, kPostDataMimeType);
   }
@@ -229,7 +232,7 @@ class RequestImpl : public WebHistoryService::Request,
   }
 
   identity::IdentityManager* identity_manager_;
-  scoped_refptr<net::URLRequestContextGetter> request_context_;
+  scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory_;
 
   // The URL of the API endpoint.
   GURL url_;
@@ -250,7 +253,7 @@ class RequestImpl : public WebHistoryService::Request,
   std::string access_token_;
 
   // Handles the actual API requests after the OAuth token is acquired.
-  std::unique_ptr<net::URLFetcher> url_fetcher_;
+  std::unique_ptr<network::SimpleURLLoader> simple_url_loader_;
 
   // Holds the response code received from the server.
   int response_code_;
@@ -268,7 +271,7 @@ class RequestImpl : public WebHistoryService::Request,
   // True if the request was started and has not yet completed, otherwise false.
   bool is_pending_;
 
-  // Partial Network traffic annotation used to create URLFetcher for this
+  // Partial Network traffic annotation used to create SimpleURLLoader for this
   // request.
   const net::PartialNetworkTrafficAnnotationTag partial_traffic_annotation_;
 };
@@ -348,9 +351,9 @@ WebHistoryService::Request::~Request() {
 
 WebHistoryService::WebHistoryService(
     identity::IdentityManager* identity_manager,
-    const scoped_refptr<net::URLRequestContextGetter>& request_context)
+    scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory)
     : identity_manager_(identity_manager),
-      request_context_(request_context),
+      url_loader_factory_(std::move(url_loader_factory)),
       weak_ptr_factory_(this) {}
 
 WebHistoryService::~WebHistoryService() {
@@ -368,7 +371,7 @@ WebHistoryService::Request* WebHistoryService::CreateRequest(
     const GURL& url,
     const CompletionCallback& callback,
     const net::PartialNetworkTrafficAnnotationTag& partial_traffic_annotation) {
-  return new RequestImpl(identity_manager_, request_context_, url, callback,
+  return new RequestImpl(identity_manager_, url_loader_factory_, url, callback,
                          partial_traffic_annotation);
 }
 

components/history/core/browser/web_history_service.h

@@ -29,8 +29,8 @@ namespace identity {
 class IdentityManager;
 }
 
-namespace net {
-class URLRequestContextGetter;
+namespace network {
+class SharedURLLoaderFactory;
 }
 
 namespace version_info {
@@ -98,7 +98,7 @@ class WebHistoryService : public KeyedService {
 
   WebHistoryService(
       identity::IdentityManager* identity_manager,
-      const scoped_refptr<net::URLRequestContextGetter>& request_context);
+      scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory);
   ~WebHistoryService() override;
 
   void AddObserver(WebHistoryServiceObserver* observer);
@@ -223,7 +223,7 @@ class WebHistoryService : public KeyedService {
   identity::IdentityManager* identity_manager_;
 
   // Request context getter to use.
-  scoped_refptr<net::URLRequestContextGetter> request_context_;
+  scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory_;
 
   // Stores the version_info token received from the server in response to
   // a mutation operation (e.g., deleting history). This is used to ensure that

components/history/core/browser/web_history_service_unittest.cc

@@ -15,7 +15,9 @@
 #include "base/values.h"
 #include "net/http/http_status_code.h"
 #include "net/traffic_annotation/network_traffic_annotation_test_helper.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"
 
@@ -30,12 +32,12 @@ namespace {
 class TestingWebHistoryService : public WebHistoryService {
  public:
   explicit TestingWebHistoryService(
-      const scoped_refptr<net::URLRequestContextGetter>& request_context)
+      scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory)
       // NOTE: Simply pass null object for IdentityManager. WebHistoryService's
       // only usage of this object is to fetch access tokens via RequestImpl,
       // and TestWebHistoryService deliberately replaces this flow with
       // TestRequest.
-      : WebHistoryService(nullptr, request_context),
+      : WebHistoryService(nullptr, url_loader_factory),
         expected_url_(GURL()),
         expected_audio_history_value_(false),
         current_expected_post_data_("") {}
@@ -215,9 +217,10 @@ std::string TestingWebHistoryService::GetExpectedAudioHistoryValue() {
 class WebHistoryServiceTest : public testing::Test {
  public:
   WebHistoryServiceTest()
-      : url_request_context_(new net::TestURLRequestContextGetter(
-            base::ThreadTaskRunnerHandle::Get())),
-        web_history_service_(url_request_context_) {}
+      : test_shared_loader_factory_(
+            base::MakeRefCounted<network::WeakWrapperSharedURLLoaderFactory>(
+                &test_url_loader_factory_)),
+        web_history_service_(test_shared_loader_factory_) {}
 
   ~WebHistoryServiceTest() override {}
 
@@ -234,7 +237,8 @@ class WebHistoryServiceTest : public testing::Test {
 
  private:
   base::test::ScopedTaskEnvironment scoped_task_environment_;
-  scoped_refptr<net::URLRequestContextGetter> url_request_context_;
+  network::TestURLLoaderFactory test_url_loader_factory_;
+  scoped_refptr<network::SharedURLLoaderFactory> test_shared_loader_factory_;
   TestingWebHistoryService web_history_service_;
 
   DISALLOW_COPY_AND_ASSIGN(WebHistoryServiceTest);

components/history/core/test/BUILD.gn

@@ -33,6 +33,7 @@ static_library("test") {
     "//components/history/core/browser",
     "//components/sync/protocol:protocol",
     "//net",
+    "//services/network/public/cpp",
     "//sql",
     "//sql:test_support",
     "//testing/gtest",

components/history/core/test/fake_web_history_service.cc

@@ -18,7 +18,7 @@
 #include "components/sync/protocol/history_status.pb.h"
 #include "net/base/url_util.h"
 #include "net/http/http_status_code.h"
-#include "net/url_request/url_request_context_getter.h"
+#include "services/network/public/cpp/shared_url_loader_factory.h"
 
 namespace history {
 
@@ -177,13 +177,12 @@ void FakeWebHistoryService::FakeRequest::Start() {
 
 // FakeWebHistoryService -------------------------------------------------------
 
-FakeWebHistoryService::FakeWebHistoryService(
-    const scoped_refptr<net::URLRequestContextGetter>& request_context)
+FakeWebHistoryService::FakeWebHistoryService()
     // NOTE: Simply pass null object for IdentityManager. WebHistoryService's
     // only usage of this object is to fetch access tokens via RequestImpl, and
     // FakeWebHistoryService deliberately replaces this flow with
     // FakeWebHistoryService::FakeRequest.
-    : history::WebHistoryService(nullptr, request_context),
+    : history::WebHistoryService(nullptr, nullptr),
       emulate_success_(true),
       emulate_response_code_(net::HTTP_OK),
       web_and_app_activity_enabled_(false),

components/history/core/test/fake_web_history_service.h

@@ -29,8 +29,7 @@ namespace history {
 // TODO(msramek): This class might need its own set of tests.
 class FakeWebHistoryService : public WebHistoryService {
  public:
-  FakeWebHistoryService(
-      const scoped_refptr<net::URLRequestContextGetter>& request_context);
+  FakeWebHistoryService();
   ~FakeWebHistoryService() override;
 
   // Sets up the behavior of the fake response returned when calling

ios/chrome/browser/history/web_history_service_factory.cc

@@ -15,7 +15,8 @@
 #include "ios/chrome/browser/browser_state/chrome_browser_state.h"
 #include "ios/chrome/browser/signin/identity_manager_factory.h"
 #include "ios/chrome/browser/sync/ios_chrome_profile_sync_service_factory.h"
-#include "net/url_request/url_request_context_getter.h"
+#include "services/network/public/cpp/shared_url_loader_factory.h"
+#include "services/network/public/cpp/weak_wrapper_shared_url_loader_factory.h"
 
 namespace ios {
 namespace {
@@ -66,7 +67,8 @@ std::unique_ptr<KeyedService> WebHistoryServiceFactory::BuildServiceInstanceFor(
       ios::ChromeBrowserState::FromBrowserState(context);
   return std::make_unique<history::WebHistoryService>(
       IdentityManagerFactory::GetForBrowserState(browser_state),
-      browser_state->GetRequestContext());
+      base::MakeRefCounted<network::WeakWrapperSharedURLLoaderFactory>(
+          browser_state->GetURLLoaderFactory()));
 }
 
 }  // namespace ios