Migate SyncStoppedReporter to SimpleURLLoader

Bug: 844966
Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: I3c198d95735770668eed4a1342a3979224239529
Reviewed-on: https://chromium-review.googlesource.com/1089132
Commit-Queue: Mark Pilgrim <pilgrim@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#565321}

chrome/browser/sync/profile_sync_service_factory.cc

@@ -43,8 +43,12 @@
 #include "components/sync/driver/signin_manager_wrapper.h"
 #include "components/sync/driver/startup_controller.h"
 #include "components/sync/driver/sync_util.h"
+#include "content/public/browser/browser_context.h"
 #include "content/public/browser/browser_thread.h"
+#include "content/public/browser/storage_partition.h"
 #include "extensions/buildflags/buildflags.h"
+#include "net/url_request/url_request_context_getter.h"
+#include "services/network/public/cpp/shared_url_loader_factory.h"
 #include "url/gurl.h"
 
 #if BUILDFLAG(ENABLE_EXTENSIONS)
@@ -173,6 +177,9 @@ KeyedService* ProfileSyncServiceFactory::BuildServiceInstanceFor(
   init_params.network_time_update_callback = base::Bind(&UpdateNetworkTime);
   init_params.base_directory = profile->GetPath();
   init_params.url_request_context = profile->GetRequestContext();
+  init_params.url_loader_factory =
+      content::BrowserContext::GetDefaultStoragePartition(profile)
+          ->GetURLLoaderFactoryForBrowserProcess();
   init_params.debug_identifier = profile->GetDebugName();
   init_params.channel = chrome::GetChannel();
 

chrome/browser/sync/profile_sync_test_util.cc

@@ -23,6 +23,9 @@
 #include "components/sync/driver/startup_controller.h"
 #include "components/sync/driver/sync_api_component_factory_mock.h"
 #include "components/sync/model/model_type_store_test_util.h"
+#include "content/public/browser/browser_context.h"
+#include "content/public/browser/storage_partition.h"
+#include "services/network/public/cpp/shared_url_loader_factory.h"
 
 using browser_sync::ProfileSyncService;
 using testing::NiceMock;
@@ -55,6 +58,9 @@ ProfileSyncService::InitParams CreateProfileSyncServiceParamsForTest(
   init_params.network_time_update_callback = base::DoNothing();
   init_params.base_directory = profile->GetPath();
   init_params.url_request_context = profile->GetRequestContext();
+  init_params.url_loader_factory =
+      content::BrowserContext::GetDefaultStoragePartition(profile)
+          ->GetURLLoaderFactoryForBrowserProcess();
   init_params.debug_identifier = profile->GetDebugName();
   init_params.channel = chrome::GetChannel();
   init_params.model_type_store_factory =

chrome/browser/ui/webui/signin/dice_turn_sync_on_helper_unittest.cc

@@ -37,6 +37,7 @@
 #include "components/signin/core/browser/signin_metrics.h"
 #include "components/signin/core/browser/signin_pref_names.h"
 #include "content/public/test/test_browser_thread_bundle.h"
+#include "net/url_request/url_request_context_getter.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 using ::testing::AtLeast;

components/browser_sync/BUILD.gn

@@ -46,6 +46,7 @@ static_library("browser_sync") {
     "//google_apis",
     "//net",
     "//services/identity/public/cpp",
+    "//services/network/public/cpp",
   ]
 }
 
@@ -92,6 +93,8 @@ source_set("unit_tests") {
     "//google_apis",
     "//net",
     "//services/identity/public/cpp:test_support",
+    "//services/network:test_support",
+    "//services/network/public/cpp",
     "//testing/gmock",
     "//testing/gtest",
   ]

components/browser_sync/DEPS

@@ -24,4 +24,6 @@ include_rules = [
   "+google_apis",
   "+net",
   "+services/identity/public/cpp",
+  "+services/network/public/cpp",
+  "+services/network/test",
 ]

components/browser_sync/profile_sync_service.cc

@@ -68,6 +68,7 @@
 #include "components/sync_sessions/sync_sessions_client.h"
 #include "components/version_info/version_info_values.h"
 #include "net/url_request/url_request_context_getter.h"
+#include "services/network/public/cpp/shared_url_loader_factory.h"
 
 using syncer::BackendMigrator;
 using syncer::ClientTagBasedModelTypeProcessor;
@@ -176,6 +177,7 @@ ProfileSyncService::ProfileSyncService(InitParams init_params)
       network_time_update_callback_(
           std::move(init_params.network_time_update_callback)),
       url_request_context_(init_params.url_request_context),
+      url_loader_factory_(std::move(init_params.url_loader_factory)),
       is_first_time_sync_configure_(false),
       engine_initialized_(false),
       sync_disabled_by_admin_(false),
@@ -243,8 +245,8 @@ void ProfileSyncService::Initialize() {
                       ->CreateLocalDeviceInfoProvider();
   DCHECK(local_device_);
   sync_stopped_reporter_ = std::make_unique<syncer::SyncStoppedReporter>(
-      sync_service_url_, local_device_->GetSyncUserAgent(),
-      url_request_context_, syncer::SyncStoppedReporter::ResultCallback());
+      sync_service_url_, local_device_->GetSyncUserAgent(), url_loader_factory_,
+      syncer::SyncStoppedReporter::ResultCallback());
 
   if (base::FeatureList::IsEnabled(switches::kSyncUSSSessions)) {
     sessions_sync_manager_ = std::make_unique<sync_sessions::SessionSyncBridge>(

components/browser_sync/profile_sync_service.h

@@ -52,6 +52,10 @@ namespace base {
 class MessageLoop;
 }
 
+namespace network {
+class SharedURLLoaderFactory;
+}  // namespace network
+
 namespace sync_sessions {
 class AbstractSessionsSyncManager;
 class FaviconCache;
@@ -216,6 +220,7 @@ class ProfileSyncService : public syncer::SyncService,
     syncer::NetworkTimeUpdateCallback network_time_update_callback;
     base::FilePath base_directory;
     scoped_refptr<net::URLRequestContextGetter> url_request_context;
+    scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory;
     std::string debug_identifier;
     version_info::Channel channel = version_info::Channel::UNKNOWN;
     syncer::RepeatingModelTypeStoreFactory model_type_store_factory;
@@ -735,6 +740,9 @@ class ProfileSyncService : public syncer::SyncService,
   // The request context in which sync should operate.
   scoped_refptr<net::URLRequestContextGetter> url_request_context_;
 
+  // The URL loader factory for the sync.
+  scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory_;
+
   // Indicates if this is the first time sync is being configured.  This value
   // is equal to !IsFirstSetupComplete() at the time of OnEngineInitialized().
   bool is_first_time_sync_configure_;

components/browser_sync/profile_sync_test_util.cc

@@ -22,6 +22,9 @@
 #include "components/sync/engine/ui_model_worker.h"
 #include "components/sync/model/model_type_store_test_util.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"
 
 namespace browser_sync {
 
@@ -260,6 +263,9 @@ ProfileSyncService::InitParams ProfileSyncServiceBundle::CreateBasicInitParams(
     EXPECT_TRUE(base_directory_.CreateUniqueTempDir());
   init_params.base_directory = base_directory_.GetPath();
   init_params.url_request_context = url_request_context();
+  init_params.url_loader_factory =
+      base::MakeRefCounted<network::WeakWrapperSharedURLLoaderFactory>(
+          &test_url_loader_factory_);
   init_params.debug_identifier = "dummyDebugName";
   init_params.channel = version_info::Channel::UNKNOWN;
   init_params.model_type_store_factory =

components/browser_sync/profile_sync_test_util.h

@@ -31,6 +31,10 @@ namespace net {
 class URLRequestContextGetter;
 }
 
+namespace network {
+class TestURLLoaderFactory;
+}  // namespace network
+
 namespace user_prefs {
 class PrefRegistrySyncable;
 }
@@ -125,6 +129,10 @@ class ProfileSyncServiceBundle {
     return url_request_context_.get();
   }
 
+  network::TestURLLoaderFactory* url_loader_factory() {
+    return &test_url_loader_factory_;
+  }
+
   sync_preferences::TestingPrefServiceSyncable* pref_service() {
     return &pref_service_;
   }
@@ -168,7 +176,10 @@ class ProfileSyncServiceBundle {
   testing::NiceMock<sync_sessions::MockSyncSessionsClient>
       sync_sessions_client_;
   invalidation::FakeInvalidationService fake_invalidation_service_;
+  // TODO(https://crbug.com/844968): Remove references to url_request_context_
+  // once the rest of the sync engine is migrated to network service.
   scoped_refptr<net::URLRequestContextGetter> url_request_context_;
+  network::TestURLLoaderFactory test_url_loader_factory_;
   base::ScopedTempDir base_directory_;
 
   DISALLOW_COPY_AND_ASSIGN(ProfileSyncServiceBundle);

components/sync/BUILD.gn

@@ -561,6 +561,7 @@ jumbo_static_library("sync") {
     "//crypto",
     "//google_apis",
     "//services/identity/public/cpp",
+    "//services/network/public/cpp",
     "//sql",
     "//third_party/cacheinvalidation",
     "//third_party/crc32c",
@@ -930,6 +931,7 @@ source_set("unit_tests") {
     "//google_apis:test_support",
     "//net",
     "//net:test_support",
+    "//services/network:test_support",
     "//sql",
     "//sql:test_support",
     "//testing/gmock",

components/sync/driver/DEPS

@@ -20,4 +20,6 @@ include_rules = [
   "+net",
   "+policy",
   "+services/identity/public/cpp",
+  "+services/network/public/cpp",
+  "+services/network/test",
 ]

components/sync/driver/sync_stopped_reporter.cc

@@ -4,6 +4,8 @@
 
 #include "components/sync/driver/sync_stopped_reporter.h"
 
+#include <utility>
+
 #include "base/location.h"
 #include "base/logging.h"
 #include "base/single_thread_task_runner.h"
@@ -14,6 +16,9 @@
 #include "net/base/load_flags.h"
 #include "net/http/http_status_code.h"
 #include "net/traffic_annotation/network_traffic_annotation.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"
 
 namespace {
 
@@ -32,15 +37,15 @@ namespace syncer {
 SyncStoppedReporter::SyncStoppedReporter(
     const GURL& sync_service_url,
     const std::string& user_agent,
-    const scoped_refptr<net::URLRequestContextGetter>& request_context,
+    scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
     const ResultCallback& callback)
     : sync_event_url_(GetSyncEventURL(sync_service_url)),
       user_agent_(user_agent),
-      request_context_(request_context),
+      url_loader_factory_(std::move(url_loader_factory)),
       callback_(callback) {
   DCHECK(!sync_service_url.is_empty());
   DCHECK(!user_agent_.empty());
-  DCHECK(request_context);
+  DCHECK(url_loader_factory_);
 }
 
 SyncStoppedReporter::~SyncStoppedReporter() {}
@@ -83,29 +88,35 @@ void SyncStoppedReporter::ReportSyncStopped(const std::string& access_token,
             }
           }
         })");
-  fetcher_ = net::URLFetcher::Create(sync_event_url_, net::URLFetcher::POST,
-                                     this, traffic_annotation);
-  fetcher_->AddExtraRequestHeader(base::StringPrintf(
-      "%s: Bearer %s", net::HttpRequestHeaders::kAuthorization,
-      access_token.c_str()));
-  fetcher_->AddExtraRequestHeader(base::StringPrintf(
-      "%s: %s", net::HttpRequestHeaders::kUserAgent, user_agent_.c_str()));
-  fetcher_->SetRequestContext(request_context_.get());
-  fetcher_->SetUploadData("application/octet-stream", msg);
-  fetcher_->SetLoadFlags(net::LOAD_BYPASS_CACHE | net::LOAD_DISABLE_CACHE |
-                         net::LOAD_DO_NOT_SAVE_COOKIES |
-                         net::LOAD_DO_NOT_SEND_COOKIES);
-  data_use_measurement::DataUseUserData::AttachToFetcher(
-      fetcher_.get(), data_use_measurement::DataUseUserData::SYNC);
-  fetcher_->Start();
+  auto resource_request = std::make_unique<network::ResourceRequest>();
+  resource_request->url = sync_event_url_;
+  resource_request->load_flags =
+      net::LOAD_BYPASS_CACHE | net::LOAD_DISABLE_CACHE |
+      net::LOAD_DO_NOT_SAVE_COOKIES | net::LOAD_DO_NOT_SEND_COOKIES;
+  resource_request->method = "POST";
+  resource_request->headers.SetHeader(
+      net::HttpRequestHeaders::kAuthorization,
+      base::StringPrintf("Bearer %s", access_token.c_str()));
+  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::SYNC
+  simple_url_loader_ = network::SimpleURLLoader::Create(
+      std::move(resource_request), traffic_annotation);
+  simple_url_loader_->AttachStringForUpload(msg, "application/octet-stream");
+  simple_url_loader_->DownloadToStringOfUnboundedSizeUntilCrashAndDie(
+      url_loader_factory_.get(),
+      base::BindOnce(&SyncStoppedReporter::OnSimpleLoaderComplete,
+                     base::Unretained(this)));
   timer_.Start(FROM_HERE, base::TimeDelta::FromSeconds(kRequestTimeoutSeconds),
                this, &SyncStoppedReporter::OnTimeout);
 }
 
-void SyncStoppedReporter::OnURLFetchComplete(const net::URLFetcher* source) {
-  Result result =
-      source->GetResponseCode() == net::HTTP_OK ? RESULT_SUCCESS : RESULT_ERROR;
-  fetcher_.reset();
+void SyncStoppedReporter::OnSimpleLoaderComplete(
+    std::unique_ptr<std::string> response_body) {
+  Result result = response_body ? RESULT_SUCCESS : RESULT_ERROR;
+  simple_url_loader_.reset();
   timer_.Stop();
   if (!callback_.is_null()) {
     base::ThreadTaskRunnerHandle::Get()->PostTask(
@@ -114,7 +125,7 @@ void SyncStoppedReporter::OnURLFetchComplete(const net::URLFetcher* source) {
 }
 
 void SyncStoppedReporter::OnTimeout() {
-  fetcher_.reset();
+  simple_url_loader_.reset();
   if (!callback_.is_null()) {
     base::ThreadTaskRunnerHandle::Get()->PostTask(
         FROM_HERE, base::Bind(callback_, RESULT_TIMEOUT));

components/sync/driver/sync_stopped_reporter.h

@@ -11,17 +11,17 @@
 #include "base/callback.h"
 #include "base/macros.h"
 #include "base/timer/timer.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 "url/gurl.h"
 
+namespace network {
+class SharedURLLoaderFactory;
+class SimpleURLLoader;
+}  // namespace network
+
 namespace syncer {
 
 // Manages informing the sync server that sync has been disabled.
-// An implementation of URLFetcherDelegate was needed in order to
-// clean up the fetcher_ pointer when the request completes.
-class SyncStoppedReporter : public net::URLFetcherDelegate {
+class SyncStoppedReporter {
  public:
   enum Result { RESULT_SUCCESS, RESULT_ERROR, RESULT_TIMEOUT };
 
@@ -30,9 +30,9 @@ class SyncStoppedReporter : public net::URLFetcherDelegate {
   SyncStoppedReporter(
       const GURL& sync_service_url,
       const std::string& user_agent,
-      const scoped_refptr<net::URLRequestContextGetter>& request_context,
+      scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
       const ResultCallback& callback);
-  ~SyncStoppedReporter() override;
+  ~SyncStoppedReporter();
 
   // Inform the sync server that sync was stopped on this device.
   // |access_token|, |cache_guid|, and |birthday| must not be empty.
@@ -40,16 +40,17 @@ class SyncStoppedReporter : public net::URLFetcherDelegate {
                          const std::string& cache_guid,
                          const std::string& birthday);
 
-  // net::URLFetcherDelegate implementation.
-  void OnURLFetchComplete(const net::URLFetcher* source) override;
-
- private:
   // Convert the base sync URL into the sync event URL.
+  // Public so tests can use it.
   static GURL GetSyncEventURL(const GURL& sync_service_url);
 
   // Callback for a request timing out.
+  // Public so tests can use it.
   void OnTimeout();
 
+ private:
+  void OnSimpleLoaderComplete(std::unique_ptr<std::string> response_body);
+
   // Handles timing out requests.
   base::OneShotTimer timer_;
 
@@ -59,11 +60,11 @@ class SyncStoppedReporter : public net::URLFetcherDelegate {
   // The user agent for the browser.
   const std::string user_agent_;
 
-  // Stored to simplify the API; needed for URLFetcher::Create().
-  scoped_refptr<net::URLRequestContextGetter> request_context_;
+  // The URL loader for the network request.
+  scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory_;
 
-  // The current URLFetcher. Null unless a request is in progress.
-  std::unique_ptr<net::URLFetcher> fetcher_;
+  // The current URL loader. Null unless a request is in progress.
+  std::unique_ptr<network::SimpleURLLoader> simple_url_loader_;
 
   // A callback for request completion or timeout.
   ResultCallback callback_;

ios/chrome/browser/sync/ios_chrome_profile_sync_test_util.cc

@@ -19,6 +19,7 @@
 #include "ios/chrome/browser/signin/signin_manager_factory.h"
 #include "ios/chrome/browser/sync/ios_chrome_sync_client.h"
 #include "ios/chrome/common/channel_info.h"
+#include "services/network/public/cpp/shared_url_loader_factory.h"
 
 browser_sync::ProfileSyncService::InitParams
 CreateProfileSyncServiceParamsForTest(
@@ -38,6 +39,7 @@ CreateProfileSyncServiceParamsForTest(
   init_params.network_time_update_callback = base::DoNothing();
   init_params.base_directory = browser_state->GetStatePath();
   init_params.url_request_context = browser_state->GetRequestContext();
+  init_params.url_loader_factory = browser_state->GetSharedURLLoaderFactory();
   init_params.debug_identifier = browser_state->GetDebugName();
   init_params.channel = ::GetChannel();
 

ios/chrome/browser/sync/profile_sync_service_factory.cc

@@ -37,6 +37,8 @@
 #include "ios/chrome/browser/web_data_service_factory.h"
 #include "ios/chrome/common/channel_info.h"
 #include "ios/web/public/web_thread.h"
+#include "net/url_request/url_request_context_getter.h"
+#include "services/network/public/cpp/shared_url_loader_factory.h"
 #include "url/gurl.h"
 
 using browser_sync::ProfileSyncService;
@@ -141,6 +143,7 @@ ProfileSyncServiceFactory::BuildServiceInstanceFor(
   init_params.network_time_update_callback = base::Bind(&UpdateNetworkTime);
   init_params.base_directory = browser_state->GetStatePath();
   init_params.url_request_context = browser_state->GetRequestContext();
+  init_params.url_loader_factory = browser_state->GetSharedURLLoaderFactory();
   init_params.debug_identifier = browser_state->GetDebugName();
   init_params.channel = ::GetChannel();