Add URLFetcher::SaveResponseToFileAtPath

content/common/net/url_fetcher_impl.cc

Index: content/common/net/url_fetcher_impl.cc
diff --git a/content/common/net/url_fetcher_impl.cc b/content/common/net/url_fetcher_impl.cc
index be442898bdd0e5ee55900bcb1948c63207ad150f..78aecc167968be5e92b30dc76faf90ca74fb82f0 100644
--- a/content/common/net/url_fetcher_impl.cc
+++ b/content/common/net/url_fetcher_impl.cc
@@ -110,6 +110,11 @@ class URLFetcherImpl::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,
@@ -282,6 +287,10 @@ class URLFetcherImpl::Core
   // 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.
@@ -339,20 +348,23 @@ 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) {
@@ -371,6 +383,24 @@ void URLFetcherImpl::Core::FileWriter::DidCreateTempFile(
       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());
 
@@ -590,9 +620,12 @@ void URLFetcherImpl::Core::StartOnIOThread() {
       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:
@@ -1032,10 +1065,19 @@ 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 {