Add URLFetcher::SaveResponseToFileAtPath

content/common/net/url_fetcher_impl.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/common/net/url_fetcher_impl.h"
 
 #include <set>
 
 #include "base/bind.h"
 #include "base/compiler_specific.h"
@@ skipping 92 matching lines @@
   // to a file. It is only used if |Core::response_destination_| == FILE.
   // Each instance of FileWriter is owned by a URLFetcher::Core, which manages
   // its lifetime and never transfers ownership.  While writing to
   // a file, all function calls happen on the IO thread.
   class FileWriter {
    public:
     FileWriter(URLFetcherImpl::Core* core,
                scoped_refptr<base::MessageLoopProxy> file_message_loop_proxy);
 
     ~FileWriter();
+    void CreateFile(const FilePath& file_path);
+    void DidCreateFile(const FilePath& file_path,
+                       base::PlatformFileError error_code,
+                       base::PassPlatformFile file_handle,
+                       bool created);
     void CreateTempFile();
     void DidCreateTempFile(base::PlatformFileError error_code,
                            base::PassPlatformFile file_handle,
                            const FilePath& file_path);
 
     // Record |num_bytes_| response bytes in |core_->buffer_| to the file.
     void WriteBuffer(int num_bytes);
 
     // Called when a write has been done.  Continues writing if there are
     // any more bytes to write.  Otherwise, initiates a read in core_.
@@ skipping 152 matching lines @@
   // True if the URLFetcher has been cancelled.
   bool was_cancelled_;
 
   // If writing results to a file, |file_writer_| will manage creation,
   // writing, and destruction of that file.
   scoped_ptr<FileWriter> file_writer_;
 
   // Where should responses be saved?
   ResponseDestinationType response_destination_;
 
+  // Path to the file where the response is written.
+  // We create a temporary file when the path is empty.
+  FilePath response_destination_file_path_;
+
   // If |automatically_retry_on_5xx_| is false, 5xx responses will be
   // propagated to the observer, if it is true URLFetcher will automatically
   // re-execute the request, after the back-off delay has expired.
   // true by default.
   bool automatically_retry_on_5xx_;
   // Maximum retries allowed.
   int max_retries_;
   // Back-off time delay. 0 by default.
   base::TimeDelta backoff_delay_;
 
@@ skipping 37 matching lines @@
       error_code_(base::PLATFORM_FILE_OK),
       ALLOW_THIS_IN_INITIALIZER_LIST(weak_factory_(this)),
       file_message_loop_proxy_(file_message_loop_proxy),
       file_handle_(base::kInvalidPlatformFileValue) {
 }
 
 URLFetcherImpl::Core::FileWriter::~FileWriter() {
   RemoveFile();
 }
 
-void URLFetcherImpl::Core::FileWriter::CreateTempFile() {
+void URLFetcherImpl::Core::FileWriter::CreateFile(const FilePath& file_path) {
   DCHECK(core_->io_message_loop_proxy_->BelongsToCurrentThread());
   DCHECK(file_message_loop_proxy_.get());
-  base::FileUtilProxy::CreateTemporary(
+  base::FileUtilProxy::CreateOrOpen(
       file_message_loop_proxy_,
-      0,  // No additional file flags.
-      base::Bind(&URLFetcherImpl::Core::FileWriter::DidCreateTempFile,
-                 weak_factory_.GetWeakPtr()));
+      file_path,
+      base::PLATFORM_FILE_CREATE_ALWAYS | base::PLATFORM_FILE_WRITE,

[willchan no longer on Chromium, 2012/03/06 02:33:04]

Just doublechecking...is this what we want? We always create the file even if it
already exists? Sounds potentially dangerous.

[hashimoto, 2012/03/06 10:16:01]

I think this is OK because zip::ZipReader is doing the same thing.
But if this is not an encouraged thing to do in code under content/, I would
like to
 1. Always make error when the file already exists, instead of always
overwriting it, or
 2. Add |file_flags| argument to SaveResponseToFile to allow user code to
determine the behavior.
What do you think?

+      base::Bind(&URLFetcherImpl::Core::FileWriter::DidCreateFile,
+                 weak_factory_.GetWeakPtr(),
+                 file_path));
 }
 
-void URLFetcherImpl::Core::FileWriter::DidCreateTempFile(
+void URLFetcherImpl::Core::FileWriter::DidCreateFile(
+    const FilePath& file_path,
     base::PlatformFileError error_code,
     base::PassPlatformFile file_handle,
-    const FilePath& file_path) {
+    bool created) {
   DCHECK(core_->io_message_loop_proxy_->BelongsToCurrentThread());
 
   if (base::PLATFORM_FILE_OK != error_code) {
     error_code_ = error_code;
     RemoveFile();
     core_->delegate_loop_proxy_->PostTask(
         FROM_HERE, base::Bind(&Core::InformDelegateFetchIsComplete, core_));
     return;
   }
 
   file_path_ = file_path;
   file_handle_ = file_handle.ReleaseValue();
   total_bytes_written_ = 0;
 
   core_->io_message_loop_proxy_->PostTask(
       FROM_HERE, base::Bind(&Core::StartURLRequestWhenAppropriate, core_));
 }
 
+void URLFetcherImpl::Core::FileWriter::CreateTempFile() {
+  DCHECK(core_->io_message_loop_proxy_->BelongsToCurrentThread());
+  DCHECK(file_message_loop_proxy_.get());
+  base::FileUtilProxy::CreateTemporary(
+      file_message_loop_proxy_,
+      0,  // No additional file flags.
+      base::Bind(&URLFetcherImpl::Core::FileWriter::DidCreateTempFile,
+                 weak_factory_.GetWeakPtr()));
+}
+
+void URLFetcherImpl::Core::FileWriter::DidCreateTempFile(
+    base::PlatformFileError error_code,
+    base::PassPlatformFile file_handle,
+    const FilePath& file_path) {
+  const bool created = true;
+  DidCreateFile(file_path, error_code, file_handle, created);
+}
+
 void URLFetcherImpl::Core::FileWriter::WriteBuffer(int num_bytes) {
   DCHECK(core_->io_message_loop_proxy_->BelongsToCurrentThread());
 
   // Start writing to the file by setting the initial state
   // of |pending_bytes_| and |buffer_offset_| to indicate that the
   // entire buffer has not yet been written.
   pending_bytes_ = num_bytes;
   buffer_offset_ = 0;
   ContinueWrite(base::PLATFORM_FILE_OK, 0);
 }
@@ skipping 199 matching lines @@
       StartURLRequestWhenAppropriate();
       break;
 
     case FILE:
       DCHECK(file_message_loop_proxy_.get())
           << "Need to set the file message loop proxy.";
 
       file_writer_.reset(
           new FileWriter(this, file_message_loop_proxy_));
 
-      // If the temp file is successfully created,
+      // If the file is successfully created,
       // Core::StartURLRequestWhenAppropriate() will be called.
-      file_writer_->CreateTempFile();
+      if (!response_destination_file_path_.empty())

[Sam Kerner (Chrome), 2012/03/06 00:23:06]

Looks like the signal that you want a specific file (and not a temp file) is the
emptiness of response_destination_file_path_ .  This if block is clear today,
but I worry that as we make the code more complex, this will no longer be easy
to understand.

Could you make response_destination_'s type one of { STRING, TEMP_FILE,
PERMANENT_FILE}?  This would make the differences between temp and permanent
paths easy to see.

[hashimoto, 2012/03/06 10:16:01]

Sounds good, done.

+        file_writer_->CreateFile(response_destination_file_path_);
+      else
+        file_writer_->CreateTempFile();
       break;
 
     default:
       NOTREACHED();
   }
 }
 
 void URLFetcherImpl::Core::Stop() {
   if (delegate_loop_proxy_)  // May be NULL in tests.
     DCHECK(delegate_loop_proxy_->BelongsToCurrentThread());
@@ skipping 419 matching lines @@
 
 int URLFetcherImpl::GetMaxRetries() const {
   return core_->max_retries_;
 }
 
 
 base::TimeDelta URLFetcherImpl::GetBackoffDelay() const {
   return core_->backoff_delay_;
 }
 
+void URLFetcherImpl::SaveResponseToFile(

[Sam Kerner (Chrome), 2012/03/06 00:23:06]

The name of this method should distinguish it from saving to a temp path.  How
about SaveResponseToFileAtPath(const FilePath& ...) ?

[hashimoto, 2012/03/06 10:16:01]

Sounds good, done.

+    const FilePath& file_path,
+    scoped_refptr<base::MessageLoopProxy> file_message_loop_proxy) {
+  core_->file_message_loop_proxy_ = file_message_loop_proxy;
+  core_->response_destination_ = FILE;

[Sam Kerner (Chrome), 2012/03/06 00:23:06]

Here is another example where adding a new enum value would enhance clarity.

[hashimoto, 2012/03/06 10:16:01]

Done.

+  core_->response_destination_file_path_ = file_path;
+}
+
 void URLFetcherImpl::SaveResponseToTemporaryFile(
     scoped_refptr<base::MessageLoopProxy> file_message_loop_proxy) {
   core_->file_message_loop_proxy_ = file_message_loop_proxy;
   core_->response_destination_ = FILE;
+  core_->response_destination_file_path_.clear();
 }
 
 net::HttpResponseHeaders* URLFetcherImpl::GetResponseHeaders() const {
   return core_->response_headers_;
 }
 
 void URLFetcherImpl::set_response_headers(
     scoped_refptr<net::HttpResponseHeaders> headers) {
   core_->response_headers_ = headers;
 }
@@ skipping 105 matching lines @@
 
 // static
 content::URLFetcherFactory* URLFetcherImpl::factory() {
   return g_factory;
 }
 
 // static
 void URLFetcherImpl::set_factory(content::URLFetcherFactory* factory) {
   g_factory = factory;
 }