Add download to temp file support to SimpleURLLoader.

Bug: 746977
Change-Id: Ib0439337359486f1523e85963d8549fbae58ee08
Reviewed-on: https://chromium-review.googlesource.com/705695
Commit-Queue: Matt Menke <mmenke@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Randy Smith <rdsmith@chromium.org>
Cr-Commit-Position: refs/heads/master@{#509669}

content/public/common/simple_url_loader.cc

@@ -252,17 +252,21 @@ class SaveToFileBodyHandler : public BodyHandler {
  public:
   // |net_priority| is the priority from the ResourceRequest, and is used to
   // determine the TaskPriority of the sequence used to read from the response
-  // body and write to the file.
+  // body and write to the file. If |create_temp_file| is true, a temp file is
+  // created instead of using |path|.
   SaveToFileBodyHandler(SimpleURLLoaderImpl* simple_url_loader,
                         SimpleURLLoader::DownloadToFileCompleteCallback
                             download_to_file_complete_callback,
                         const base::FilePath& path,
+                        bool create_temp_file,
                         uint64_t max_body_size,
                         net::RequestPriority request_priority)
       : BodyHandler(simple_url_loader),
         download_to_file_complete_callback_(
             std::move(download_to_file_complete_callback)),
         weak_ptr_factory_(this) {
+    DCHECK(create_temp_file || !path.empty());
+
     // Choose the TaskPriority based on the net request priority.
     // TODO(mmenke): Can something better be done here?
     base::TaskPriority task_priority;
@@ -275,8 +279,8 @@ class SaveToFileBodyHandler : public BodyHandler {
     }
 
     // Can only do this after initializing the WeakPtrFactory.
-    file_writer_ =
-        std::make_unique<FileWriter>(path, max_body_size, task_priority);
+    file_writer_ = std::make_unique<FileWriter>(path, create_temp_file,
+                                                max_body_size, task_priority);
   }
 
   ~SaveToFileBodyHandler() override {
@@ -356,16 +360,19 @@ class SaveToFileBodyHandler : public BodyHandler {
                                                    int64_t total_bytes,
                                                    const base::FilePath& path)>;
 
-    explicit FileWriter(const base::FilePath& path,
-                        int64_t max_body_size,
-                        base::TaskPriority priority)
+    FileWriter(const base::FilePath& path,
+               bool create_temp_file,
+               int64_t max_body_size,
+               base::TaskPriority priority)
         : body_handler_task_runner_(base::SequencedTaskRunnerHandle::Get()),
           file_writer_task_runner_(base::CreateSequencedTaskRunnerWithTraits(
               {base::MayBlock(), priority,
                base::TaskShutdownBehavior::BLOCK_SHUTDOWN})),
           path_(path),
+          create_temp_file_(create_temp_file),
           max_body_size_(max_body_size) {
       DCHECK(body_handler_task_runner_->RunsTasksInCurrentSequence());
+      DCHECK(create_temp_file_ || !path_.empty());
     }
 
     // Starts reading from |body_data_pipe| and writing to the file.
@@ -420,9 +427,22 @@ class SaveToFileBodyHandler : public BodyHandler {
       DCHECK(!file_.IsValid());
       DCHECK(!body_reader_);
 
-      // Try to create the file.
-      file_.Initialize(path_,
-                       base::File::FLAG_WRITE | base::File::FLAG_CREATE_ALWAYS);
+      bool have_path = !create_temp_file_;
+      if (!have_path) {
+        DCHECK(create_temp_file_);
+        have_path = base::CreateTemporaryFile(&path_);
+        // CreateTemporaryFile() creates an empty file.
+        if (have_path)
+          owns_file_ = true;
+      }
+
+      if (have_path) {
+        // Try to initialize |file_|, creating the file if needed.
+        file_.Initialize(
+            path_, base::File::FLAG_WRITE | base::File::FLAG_CREATE_ALWAYS);
+      }
+
+      // If CreateTemporaryFile() or File::Initialize() failed, report failure.
       if (!file_.IsValid()) {
         body_handler_task_runner_->PostTask(
             FROM_HERE, base::BindOnce(std::move(on_done_callback),
@@ -502,6 +522,7 @@ class SaveToFileBodyHandler : public BodyHandler {
     // |file_writer_task_runner_|.
 
     base::FilePath path_;
+    const bool create_temp_file_;
     const int64_t max_body_size_;
 
     // File being downloaded to. Created just before reading from the data pipe.
@@ -567,6 +588,12 @@ class SimpleURLLoaderImpl : public SimpleURLLoader,
       DownloadToFileCompleteCallback download_to_file_complete_callback,
       const base::FilePath& file_path,
       int64_t max_body_size) override;
+  void DownloadToTempFile(
+      const ResourceRequest& resource_request,
+      mojom::URLLoaderFactory* url_loader_factory,
+      const net::NetworkTrafficAnnotationTag& annotation_tag,
+      DownloadToFileCompleteCallback download_to_file_complete_callback,
+      int64_t max_body_size) override;
   void SetAllowPartialResults(bool allow_partial_results) override;
   void SetAllowHttpErrorResults(bool allow_http_error_results) override;
   void SetRetryOptions(int max_retries, int retry_mode) override;
@@ -767,9 +794,22 @@ void SimpleURLLoaderImpl::DownloadToFile(
     DownloadToFileCompleteCallback download_to_file_complete_callback,
     const base::FilePath& file_path,
     int64_t max_body_size) {
+  DCHECK(!file_path.empty());
   body_handler_ = std::make_unique<SaveToFileBodyHandler>(
       this, std::move(download_to_file_complete_callback), file_path,
-      max_body_size, resource_request.priority);
+      false /* create_temp_file */, max_body_size, resource_request.priority);
+  Start(resource_request, url_loader_factory, annotation_tag);
+}
+
+void SimpleURLLoaderImpl::DownloadToTempFile(
+    const ResourceRequest& resource_request,
+    mojom::URLLoaderFactory* url_loader_factory,
+    const net::NetworkTrafficAnnotationTag& annotation_tag,
+    DownloadToFileCompleteCallback download_to_file_complete_callback,
+    int64_t max_body_size) {
+  body_handler_ = std::make_unique<SaveToFileBodyHandler>(
+      this, std::move(download_to_file_complete_callback), base::FilePath(),
+      true /* create_temp_file */, max_body_size, resource_request.priority);
   Start(resource_request, url_loader_factory, annotation_tag);
 }
 

content/public/common/simple_url_loader.h

@@ -41,7 +41,6 @@ class URLLoaderFactory;
 // Each SimpleURLLoader can only be used for a single request.
 //
 // TODO(mmenke): Support the following:
-// * Save to temp file.
 // * Consumer-provided methods to receive streaming (with backpressure).
 // * Monitoring (And cancelling during) redirects.
 // * Uploads (Fixed strings, files, data streams (with backpressure), chunked
@@ -129,6 +128,15 @@ class CONTENT_EXPORT SimpleURLLoader {
       const base::FilePath& file_path,
       int64_t max_body_size = std::numeric_limits<int64_t>::max()) = 0;
 
+  // Same as DownloadToFile, but creates a temporary file instead of taking a
+  // FilePath.
+  virtual void DownloadToTempFile(
+      const ResourceRequest& resource_request,
+      mojom::URLLoaderFactory* url_loader_factory,
+      const net::NetworkTrafficAnnotationTag& annotation_tag,
+      DownloadToFileCompleteCallback download_to_file_complete_callback,
+      int64_t max_body_size = std::numeric_limits<int64_t>::max()) = 0;
+
   // Sets whether partially received results are allowed. Defaults to false.
   // When true, if an error is received after reading the body starts or the max
   // allowed body size exceeded, the partial response body that was received

content/public/common/simple_url_loader_unittest.cc

@@ -66,8 +66,9 @@ const char kTruncatedBody[] = "Truncated Body";
 // and check the result.
 class SimpleLoaderTestHelper {
  public:
-  // What the response should be downloaded to.
-  enum class DownloadType { TO_STRING, TO_FILE };
+  // What the response should be downloaded to. Running all tests for all types
+  // is more than strictly needed, but simplest just to cover all cases.
+  enum class DownloadType { TO_STRING, TO_FILE, TO_TEMP_FILE };
 
   explicit SimpleLoaderTestHelper(DownloadType download_type)
       : download_type_(download_type),
@@ -130,6 +131,22 @@ class SimpleLoaderTestHelper {
               dest_path_, max_body_size);
         }
         break;
+      case DownloadType::TO_TEMP_FILE:
+        if (max_body_size < 0) {
+          simple_url_loader_->DownloadToTempFile(
+              resource_request, url_loader_factory,
+              TRAFFIC_ANNOTATION_FOR_TESTS,
+              base::BindOnce(&SimpleLoaderTestHelper::DownloadedToFile,
+                             base::Unretained(this)));
+        } else {
+          simple_url_loader_->DownloadToTempFile(
+              resource_request, url_loader_factory,
+              TRAFFIC_ANNOTATION_FOR_TESTS,
+              base::BindOnce(&SimpleLoaderTestHelper::DownloadedToFile,
+                             base::Unretained(this)),
+              max_body_size);
+        }
+        break;
     }
   }
 
@@ -224,20 +241,34 @@ class SimpleLoaderTestHelper {
   void DownloadedToFile(const base::FilePath& file_path) {
     DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
     EXPECT_FALSE(done_);
-    EXPECT_EQ(DownloadType::TO_FILE, download_type_);
+    EXPECT_TRUE(download_type_ == DownloadType::TO_FILE ||
+                download_type_ == DownloadType::TO_TEMP_FILE);
     EXPECT_FALSE(response_body_);
 
-    // Make sure the destination file exists if |file_path| is non-empty, or the
-    // file is expected to exist on error.
-    EXPECT_EQ(!file_path.empty() || expect_path_exists_on_error_,
-              base::PathExists(dest_path_));
-
     if (!file_path.empty()) {
-      EXPECT_EQ(dest_path_, file_path);
+      EXPECT_TRUE(base::PathExists(file_path));
       response_body_ = std::make_unique<std::string>();
-      EXPECT_TRUE(base::ReadFileToString(dest_path_, response_body_.get()));
+      EXPECT_TRUE(base::ReadFileToString(file_path, response_body_.get()));
     }
 
+    // Can do some additional checks in the TO_FILE case. Unfortunately, in the
+    // temp file case, can't check that temp files were cleaned up, since it's
+    // a shared directory.
+    if (download_type_ == DownloadType::TO_FILE) {
+      // Make sure the destination file exists if |file_path| is non-empty, or
+      // the file is expected to exist on error.
+      EXPECT_EQ(!file_path.empty() || expect_path_exists_on_error_,
+                base::PathExists(dest_path_));
+
+      if (!file_path.empty())
+        EXPECT_EQ(dest_path_, file_path);
+    }
+
+    // Clean up file, so tests don't leave around files in the temp directory.
+    // Only matters in the TO_TEMP_FILE case.
+    if (!file_path.empty())
+      base::DeleteFile(file_path, false);
+
     done_ = true;
     run_loop_.Quit();
   }
@@ -1457,7 +1488,8 @@ INSTANTIATE_TEST_CASE_P(
     /* No prefix */,
     SimpleURLLoaderTest,
     testing::Values(SimpleLoaderTestHelper::DownloadType::TO_STRING,
-                    SimpleLoaderTestHelper::DownloadType::TO_FILE));
+                    SimpleLoaderTestHelper::DownloadType::TO_FILE,
+                    SimpleLoaderTestHelper::DownloadType::TO_TEMP_FILE));
 
 class SimpleURLLoaderFileTest : public SimpleURLLoaderTestBase,
                                 public testing::Test {