Use SimpleURLLoader in background download service.

Currently we use URLFetcher as network backend, which will be
deprecated soon, this CL converts InMemoryDownload to use
SimpleURLLoader.

Bug: 766299
Change-Id: I809766479804bb0a4c289095264c4ce5e788b54f
Reviewed-on: https://chromium-review.googlesource.com/952364Reviewed-by: David Trainor <dtrainor@chromium.org>
Reviewed-by: Min Qin <qinmin@chromium.org>
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Reviewed-by: Shakti Sahu <shaktisahu@chromium.org>
Commit-Queue: Xing Liu <xingliu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#542304}

chrome/browser/download/download_service_factory.cc

@@ -29,7 +29,6 @@
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/storage_partition.h"
-#include "net/url_request/url_request_context_getter.h"
 
 #if defined(OS_ANDROID)
 #include "chrome/browser/android/download/service/download_task_scheduler.h"
@@ -79,10 +78,6 @@ KeyedService* DownloadServiceFactory::BuildServiceInstanceFor(
   // Build in memory download service for incognito profile.
   if (context->IsOffTheRecord() &&
       base::FeatureList::IsEnabled(download::kDownloadServiceIncognito)) {
-    scoped_refptr<net::URLRequestContextGetter> url_request_context =
-        content::BrowserContext::GetDefaultStoragePartition(
-            Profile::FromBrowserContext(context))
-            ->GetURLRequestContext();
     content::BrowserContext::BlobContextGetter blob_context_getter =
         content::BrowserContext::GetBlobStorageContext(context);
     scoped_refptr<base::SingleThreadTaskRunner> io_task_runner =
@@ -90,8 +85,8 @@ KeyedService* DownloadServiceFactory::BuildServiceInstanceFor(
             content::BrowserThread::IO);
 
     return download::BuildInMemoryDownloadService(
-        context, std::move(clients), base::FilePath(), url_request_context,
-        blob_context_getter, io_task_runner);
+        context, std::move(clients), base::FilePath(), blob_context_getter,
+        io_task_runner);
   } else {
     // Build download service for normal profile.
     base::FilePath storage_dir;

components/download/content/factory/download_service_factory.cc

@@ -23,7 +23,7 @@
 #include "components/download/internal/background_service/proto/entry.pb.h"
 #include "components/download/internal/background_service/scheduler/scheduler_impl.h"
 #include "components/leveldb_proto/proto_database_impl.h"
-#include "net/url_request/url_request_context_getter.h"
+#include "content/public/browser/storage_partition.h"
 
 #if defined(OS_ANDROID)
 #include "components/download/internal/background_service/android/battery_status_listener_android.h"
@@ -111,12 +111,16 @@ DownloadService* BuildInMemoryDownloadService(
     content::BrowserContext* browser_context,
     std::unique_ptr<DownloadClientMap> clients,
     const base::FilePath& storage_dir,
-    scoped_refptr<net::URLRequestContextGetter> request_context_getter,
     BlobTaskProxy::BlobContextGetter blob_context_getter,
     scoped_refptr<base::SingleThreadTaskRunner> io_task_runner) {
   auto config = Configuration::CreateFromFinch();
+  auto* url_loader_factory =
+      content::BrowserContext::GetDefaultStoragePartition(browser_context)
+          ->GetURLLoaderFactoryForBrowserProcess()
+          .get();
+  DCHECK(url_loader_factory);
   auto download_factory = std::make_unique<InMemoryDownloadFactory>(
-      request_context_getter, blob_context_getter, io_task_runner);
+      url_loader_factory, blob_context_getter, io_task_runner);
   auto driver =
       std::make_unique<InMemoryDownloadDriver>(std::move(download_factory));
   auto store = std::make_unique<NoopStore>();

components/download/content/factory/download_service_factory.h

@@ -17,10 +17,6 @@ namespace content {
 class BrowserContext;
 }  // namespace content
 
-namespace net {
-class URLRequestContextGetter;
-}  // namespace net
-
 namespace download {
 
 class DownloadService;
@@ -49,7 +45,6 @@ DownloadService* BuildInMemoryDownloadService(
     content::BrowserContext* browser_context,
     std::unique_ptr<DownloadClientMap> clients,
     const base::FilePath& storage_dir,
-    scoped_refptr<net::URLRequestContextGetter> request_context_getter,
     BlobTaskProxy::BlobContextGetter blob_context_getter,
     scoped_refptr<base::SingleThreadTaskRunner> io_task_runner);
 

components/download/internal/background_service/BUILD.gn

@@ -87,6 +87,7 @@ static_library("internal") {
     "//components/download/public/background_service:public",
     "//components/leveldb_proto",
     "//net",
+    "//services/network/public/cpp",
     "//storage/browser",
   ]
 
@@ -152,7 +153,7 @@ source_set("unit_tests") {
     "//components/download/internal/background_service/test:test_support",
     "//components/download/public/background_service/test:test_support",
     "//components/leveldb_proto:test_support",
-    "//net:test_support",
+    "//services/network:test_support",
     "//storage/browser",
     "//testing/gmock",
     "//testing/gtest",

components/download/internal/background_service/DEPS

@@ -5,6 +5,8 @@ include_rules = [
   "+base",
   "+jni",
   "+net",
+  "+services/network/public",
+  "+services/network/test",
   "+storage/browser",
   "+storage/common",
 ]

components/download/internal/background_service/in_memory_download.h

@@ -14,13 +14,11 @@
 #include "base/single_thread_task_runner.h"
 #include "components/download/internal/background_service/blob_task_proxy.h"
 #include "components/download/public/background_service/download_params.h"
-#include "net/base/completion_callback.h"
-#include "net/url_request/url_fetcher_delegate.h"
-#include "net/url_request/url_fetcher_response_writer.h"
-#include "net/url_request/url_request_context_getter.h"
+#include "services/network/public/cpp/simple_url_loader.h"
+#include "services/network/public/cpp/simple_url_loader_stream_consumer.h"
+#include "services/network/public/mojom/url_loader_factory.mojom.h"
 
 namespace net {
-class URLFetcher;
 struct NetworkTrafficAnnotationTag;
 }  // namespace net
 
@@ -78,9 +76,7 @@ class InMemoryDownload {
     FAILED,
 
     // Download is completed, and data is successfully saved as a blob.
-    // 1. We guarantee the states of network responses.
-    // 2. Do not guarantee the state of blob data. The consumer of blob
-    // should validate its state when using it on IO thread.
+    // Guarantee the blob is fully constructed.
     COMPLETE,
   };
 
@@ -129,14 +125,15 @@ class InMemoryDownload {
   DISALLOW_COPY_AND_ASSIGN(InMemoryDownload);
 };
 
-// Implementation of InMemoryDownload and uses URLFetcher as network backend.
+// Implementation of InMemoryDownload and uses SimpleURLLoader as network
+// backend.
 // Threading contract:
 // 1. This object lives on the main thread.
 // 2. Reading/writing IO buffer from network is done on another thread,
 // based on |request_context_getter_|. When complete, main thread is notified.
 // 3. After network IO is done, Blob related work is done on IO thread with
 // |blob_task_proxy_|, then notify the result to main thread.
-class InMemoryDownloadImpl : public net::URLFetcherDelegate,
+class InMemoryDownloadImpl : public network::SimpleURLLoaderStreamConsumer,
                              public InMemoryDownload {
  public:
   InMemoryDownloadImpl(
@@ -144,51 +141,13 @@ class InMemoryDownloadImpl : public net::URLFetcherDelegate,
       const RequestParams& request_params,
       const net::NetworkTrafficAnnotationTag& traffic_annotation,
       Delegate* delegate,
-      scoped_refptr<net::URLRequestContextGetter> request_context_getter,
+      network::mojom::URLLoaderFactory* url_loader_factory,
       BlobTaskProxy::BlobContextGetter blob_context_getter,
       scoped_refptr<base::SingleThreadTaskRunner> io_task_runner);
+
   ~InMemoryDownloadImpl() override;
 
  private:
-  // Response writer that supports pause and resume operations.
-  class ResponseWriter : public net::URLFetcherResponseWriter {
-   public:
-    ResponseWriter(scoped_refptr<base::SingleThreadTaskRunner> io_task_runner);
-    ~ResponseWriter() override;
-
-    // Pause writing data from pipe into |data_|.
-    void Pause();
-
-    // Resume writing data from the pipe into |data_|.
-    void Resume();
-
-    // Take the data, must be called after the network layer completes its job.
-    std::unique_ptr<std::string> TakeData();
-
-   private:
-    // net::URLFetcherResponseWriter implementation.
-    int Initialize(const net::CompletionCallback& callback) override;
-    int Write(net::IOBuffer* buffer,
-              int num_bytes,
-              const net::CompletionCallback& callback) override;
-    int Finish(int net_error, const net::CompletionCallback& callback) override;
-
-    void PauseOnIO();
-    void ResumeOnIO();
-
-    // Download data, should be moved to avoid extra copy.
-    std::unique_ptr<std::string> data_;
-
-    bool paused_on_io_;
-    scoped_refptr<base::SingleThreadTaskRunner> io_task_runner_;
-
-    // When paused, cached callback to trigger the next read. Must be set and
-    // called on fetcher's IO thread.
-    net::CompletionCallback write_callback_;
-
-    DISALLOW_COPY_AND_ASSIGN(ResponseWriter);
-  };
-
   // InMemoryDownload implementation.
   void Start() override;
   void Pause() override;
@@ -197,16 +156,11 @@ class InMemoryDownloadImpl : public net::URLFetcherDelegate,
   std::unique_ptr<storage::BlobDataHandle> ResultAsBlob() override;
   size_t EstimateMemoryUsage() const override;
 
-  // net::URLFetcherDelegate implementation.
-  void OnURLFetchDownloadProgress(const net::URLFetcher* source,
-                                  int64_t current,
-                                  int64_t total,
-                                  int64_t current_network_bytes) override;
-  void OnURLFetchComplete(const net::URLFetcher* source) override;
-
-  // Handles response code and change the state accordingly.
-  // Returns if the response code is considered as successful code.
-  bool HandleResponseCode(int response_code);
+  // network::SimpleURLLoaderStreamConsumer implementation.
+  void OnDataReceived(base::StringPiece string_piece,
+                      base::OnceClosure resume) override;
+  void OnComplete(bool success) override;
+  void OnRetry(base::OnceClosure start_retry) override;
 
   // Saves the download data into blob storage.
   void SaveAsBlob();
@@ -218,26 +172,23 @@ class InMemoryDownloadImpl : public net::URLFetcherDelegate,
   // call.
   void NotifyDelegateDownloadComplete();
 
-  // Sends the network request.
+  // Sends a new network request.
   void SendRequest();
 
+  // Resets local states.
+  void Reset();
+
   // Request parameters of the download.
   const RequestParams request_params_;
 
   // Traffic annotation of the request.
   const net::NetworkTrafficAnnotationTag traffic_annotation_;
 
-  // Used to send requests to servers. Also contains the download data in its
-  // string buffer. We should avoid extra copy on the data and release the
-  // memory when needed.
-  std::unique_ptr<net::URLFetcher> url_fetcher_;
-
-  // Owned by |url_fetcher_|. Lives on fetcher's delegate thread, perform
-  // network IO on fetcher's IO thread.
-  ResponseWriter* response_writer_;
+  // Used to send requests to servers.
+  std::unique_ptr<network::SimpleURLLoader> loader_;
 
-  // Request context getter used by |url_fetcher_|.
-  scoped_refptr<net::URLRequestContextGetter> request_context_getter_;
+  // Used to handle network response.
+  network::mojom::URLLoaderFactory* url_loader_factory_;
 
   // Worker that does blob related task on IO thread.
   std::unique_ptr<BlobTaskProxy> blob_task_proxy_;
@@ -253,6 +204,12 @@ class InMemoryDownloadImpl : public net::URLFetcherDelegate,
 
   bool paused_;
 
+  // Data downloaded from network, should be moved to avoid extra copy.
+  std::string data_;
+
+  // Cached callback to let network backend continue to pull data.
+  base::OnceClosure resume_callback_;
+
   // Ensures Delegate::OnDownloadComplete is only called once.
   bool completion_notified_;
 

components/download/internal/background_service/in_memory_download_driver.cc

@@ -47,10 +47,10 @@ DriverEntry CreateDriverEntry(const InMemoryDownload& download) {
 }  // namespace
 
 InMemoryDownloadFactory::InMemoryDownloadFactory(
-    scoped_refptr<net::URLRequestContextGetter> request_context_getter,
+    network::mojom::URLLoaderFactory* url_loader_factory,
     BlobTaskProxy::BlobContextGetter blob_context_getter,
     scoped_refptr<base::SingleThreadTaskRunner> io_task_runner)
-    : request_context_getter_(request_context_getter),
+    : url_loader_factory_(url_loader_factory),
       blob_context_getter_(blob_context_getter),
       io_task_runner_(io_task_runner) {}
 
@@ -61,9 +61,10 @@ std::unique_ptr<InMemoryDownload> InMemoryDownloadFactory::Create(
     const RequestParams& request_params,
     const net::NetworkTrafficAnnotationTag& traffic_annotation,
     InMemoryDownload::Delegate* delegate) {
+  DCHECK(url_loader_factory_);
   return std::make_unique<InMemoryDownloadImpl>(
-      guid, request_params, traffic_annotation, delegate,
-      request_context_getter_, blob_context_getter_, io_task_runner_);
+      guid, request_params, traffic_annotation, delegate, url_loader_factory_,
+      blob_context_getter_, io_task_runner_);
 }
 
 InMemoryDownloadDriver::InMemoryDownloadDriver(

components/download/internal/background_service/in_memory_download_driver.h

@@ -12,6 +12,7 @@
 
 #include "base/macros.h"
 #include "components/download/internal/background_service/in_memory_download.h"
+#include "services/network/public/mojom/url_loader_factory.mojom.h"
 
 namespace download {
 
@@ -21,7 +22,7 @@ class InMemoryDownload;
 class InMemoryDownloadFactory : public InMemoryDownload::Factory {
  public:
   InMemoryDownloadFactory(
-      scoped_refptr<net::URLRequestContextGetter> request_context_getter,
+      network::mojom::URLLoaderFactory* url_loader_factory,
       BlobTaskProxy::BlobContextGetter blob_context_getter,
       scoped_refptr<base::SingleThreadTaskRunner> io_task_runner);
   ~InMemoryDownloadFactory() override;
@@ -34,7 +35,8 @@ class InMemoryDownloadFactory : public InMemoryDownload::Factory {
       const net::NetworkTrafficAnnotationTag& traffic_annotation,
       InMemoryDownload::Delegate* delegate) override;
 
-  scoped_refptr<net::URLRequestContextGetter> request_context_getter_;
+  network::mojom::URLLoaderFactory* url_loader_factory_;
+
   BlobTaskProxy::BlobContextGetter blob_context_getter_;
   scoped_refptr<base::SingleThreadTaskRunner> io_task_runner_;
 

components/download/internal/background_service/in_memory_download_unittest.cc

@@ -4,16 +4,14 @@
 
 #include "components/download/internal/background_service/in_memory_download.h"
 
-#include "base/files/file_util.h"
 #include "base/guid.h"
-#include "base/path_service.h"
 #include "base/run_loop.h"
 #include "base/test/bind_test_util.h"
 #include "base/test/scoped_task_environment.h"
 #include "base/threading/thread.h"
-#include "net/test/embedded_test_server/embedded_test_server.h"
+#include "net/base/io_buffer.h"
 #include "net/traffic_annotation/network_traffic_annotation_test_helper.h"
-#include "net/url_request/url_request_test_util.h"
+#include "services/network/test/test_url_loader_factory.h"
 #include "storage/browser/blob/blob_reader.h"
 #include "storage/browser/blob/blob_storage_context.h"
 #include "testing/gmock/include/gmock/gmock.h"
@@ -22,18 +20,9 @@
 namespace download {
 namespace {
 
-// Posts a dummy task on |task_runner| and wait for its callback, to drain all
-// previous tasks on |task_runner|.
-void DrainPreviousTasks(
-    scoped_refptr<base::SingleThreadTaskRunner> task_runner) {
-  base::RunLoop run_loop;
-
-  auto dummy_task = []() {};
-  task_runner->PostTaskAndReply(FROM_HERE, base::BindRepeating(dummy_task),
-                                run_loop.QuitClosure());
-
-  run_loop.Run();
-}
+const char kTestDownloadData[] =
+    "In earlier tellings, the dog had a better reputation than the cat, "
+    "however the president veto it.";
 
 // Dummy callback used for IO_PENDING state in blob operations, this is not
 // called when the blob operation is done, but called when chained with other
@@ -78,14 +67,10 @@ class InMemoryDownloadTest : public testing::Test {
   ~InMemoryDownloadTest() override = default;
 
   void SetUp() override {
-    test_server_.ServeFilesFromDirectory(GetTestDataDirectory());
-    ASSERT_TRUE(test_server_.Start());
-
     io_thread_.reset(new base::Thread("Network and Blob IO thread"));
     base::Thread::Options options(base::MessageLoop::TYPE_IO, 0);
     io_thread_->StartWithOptions(options);
-    request_context_getter_ =
-        new net::TestURLRequestContextGetter(io_thread_->task_runner());
+
     base::RunLoop loop;
     io_thread_->task_runner()->PostTask(
         FROM_HERE, base::BindLambdaForTesting([&]() {
@@ -103,17 +88,11 @@ class InMemoryDownloadTest : public testing::Test {
   }
 
  protected:
-  base::FilePath GetTestDataDirectory() {
-    base::FilePath test_data_dir;
-    EXPECT_TRUE(base::PathService::Get(base::DIR_SOURCE_ROOT, &test_data_dir));
-    return test_data_dir.AppendASCII("components/test/data/download");
-  }
-
   // Helper method to create a download with request_params.
   void CreateDownload(const RequestParams& request_params) {
     download_ = std::make_unique<InMemoryDownloadImpl>(
         base::GenerateGUID(), request_params, TRAFFIC_ANNOTATION_FOR_TESTS,
-        delegate(), request_context_getter_,
+        delegate(), &url_loader_factory_,
         base::BindRepeating(&BlobStorageContextGetter,
                             blob_storage_context_.get()),
         io_thread_->task_runner());
@@ -121,9 +100,8 @@ class InMemoryDownloadTest : public testing::Test {
 
   InMemoryDownload* download() { return download_.get(); }
   MockDelegate* delegate() { return &mock_delegate_; }
-  net::EmbeddedTestServer* test_server() { return &test_server_; }
-  scoped_refptr<net::TestURLRequestContextGetter> request_context_getter() {
-    return request_context_getter_;
+  network::TestURLLoaderFactory* url_loader_factory() {
+    return &url_loader_factory_;
   }
 
   // Verifies if data read from |blob| is identical as |expected|.
@@ -174,56 +152,27 @@ class InMemoryDownloadTest : public testing::Test {
   std::unique_ptr<InMemoryDownloadImpl> download_;
   MockDelegate mock_delegate_;
 
-  // Used by URLFetcher network backend.
-  scoped_refptr<net::TestURLRequestContextGetter> request_context_getter_;
+  // Used by SimpleURLLoader network backend.
+  network::TestURLLoaderFactory url_loader_factory_;
 
   // Memory backed blob storage that can never page to disk.
   std::unique_ptr<storage::BlobStorageContext> blob_storage_context_;
 
-  net::EmbeddedTestServer test_server_;
-
   DISALLOW_COPY_AND_ASSIGN(InMemoryDownloadTest);
 };
 
 TEST_F(InMemoryDownloadTest, DownloadTest) {
   RequestParams request_params;
-  request_params.url = test_server()->GetURL("/text_data.json");
-  CreateDownload(request_params);
-  download()->Start();
-  delegate()->WaitForCompletion();
-
-  EXPECT_EQ(InMemoryDownload::State::COMPLETE, download()->state());
-  auto blob = download()->ResultAsBlob();
-
-  std::string expected;
-  EXPECT_TRUE(ReadFileToString(
-      GetTestDataDirectory().AppendASCII("text_data.json"), &expected));
-  VerifyBlobData(expected, blob.get());
-}
-
-TEST_F(InMemoryDownloadTest, PauseResume) {
-  RequestParams request_params;
-  request_params.url = test_server()->GetURL("/text_data.json");
   CreateDownload(request_params);
-
-  // Pause before sending request.
-  download()->Pause();
+  url_loader_factory()->AddResponse(request_params.url.spec(),
+                                    kTestDownloadData);
+  // TODO(xingliu): More tests on pause/resume.
   download()->Start();
-
-  // Force to return ERR_IO_PENDING on network thread in
-  // InMemoryDownloadImpl::ResponseWriter::Write.
-  DrainPreviousTasks(request_context_getter()->GetNetworkTaskRunner());
-
-  download()->Resume();
   delegate()->WaitForCompletion();
 
   EXPECT_EQ(InMemoryDownload::State::COMPLETE, download()->state());
   auto blob = download()->ResultAsBlob();
-
-  std::string expected;
-  EXPECT_TRUE(ReadFileToString(
-      GetTestDataDirectory().AppendASCII("text_data.json"), &expected));
-  VerifyBlobData(expected, blob.get());
+  VerifyBlobData(kTestDownloadData, blob.get());
 }
 
 }  // namespace

components/test/data/download/text_data.json

-{  
-   "data":"In earlier tellings, the dog had a better reputation than the cat, however the president veto it."
-}
\ No newline at end of file