Move Rename functionality from DownloadFileManager to DownloadFileImple.

BUG=123998
R=asanka@chromium.org


Review URL: https://chromiumcodereview.appspot.com/10689093

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@146162 0039d316-1c4b-4281-b951-d872f2087c98

17 files changed, 332 insertions, 190 deletions

diff --git a/content/browser/download/download_file.h b/content/browser/download/download_file.h
index 59e8b9b..e707a26 100644
--- a/content/browser/download/download_file.h
+++ b/content/browser/download/download_file.h
@@ -8,6 +8,7 @@
 #include <string>
 
 #include "base/basictypes.h"
+#include "base/callback_forward.h"
 #include "base/file_path.h"
 #include "content/common/content_export.h"
 #include "content/public/browser/download_id.h"
@@ -23,6 +24,13 @@ class DownloadManager;
 // cancelled, the DownloadFile is destroyed.
 class CONTENT_EXPORT DownloadFile {
  public:
+  // Callback used with Rename().  On a successful rename |reason| will be
+  // DOWNLOAD_INTERRUPT_REASON_NONE and |path| the path the rename
+  // was done to.  On a failed rename, |reason| will contain the
+  // error.
+  typedef base::Callback<void(content::DownloadInterruptReason reason,
+                              const FilePath& path)> RenameCompletionCallback;
+
   virtual ~DownloadFile() {}
 
   // If calculate_hash is true, sha256 hash will be calculated.
@@ -30,9 +38,14 @@ class CONTENT_EXPORT DownloadFile {
   // error code on failure.
   virtual DownloadInterruptReason Initialize() = 0;
 
-  // Rename the download file.
-  // Returns net::OK on success, or a network error code on failure.
-  virtual DownloadInterruptReason Rename(const FilePath& full_path) = 0;
+  // Rename the download file to |full_path|.  If that file exists and
+  // |overwrite_existing_file| is false, |full_path| will be uniquified by
+  // suffixing " (<number>)" to the file name before the extension.
+  // Upon completion, |callback| will be called on the UI thread
+  // as per the comment above.
+  virtual void Rename(const FilePath& full_path,
+                      bool overwrite_existing_file,
+                      const RenameCompletionCallback& callback) = 0;
 
   // Detach the file so it is not deleted on destruction.
   virtual void Detach() = 0;
diff --git a/content/browser/download/download_file_impl.cc b/content/browser/download/download_file_impl.cc
index 9d4a6880..8c42515 100644
--- a/content/browser/download/download_file_impl.cc
+++ b/content/browser/download/download_file_impl.cc
@@ -88,11 +88,39 @@ content::DownloadInterruptReason DownloadFileImpl::AppendDataToFile(
       content::DOWNLOAD_INTERRUPT_FROM_DISK);
 }
 
-content::DownloadInterruptReason DownloadFileImpl::Rename(
-    const FilePath& full_path) {
-  return content::ConvertNetErrorToInterruptReason(
-      file_.Rename(full_path),
-      content::DOWNLOAD_INTERRUPT_FROM_DISK);
+void DownloadFileImpl::Rename(const FilePath& full_path,
+                              bool overwrite_existing_file,
+                              const RenameCompletionCallback& callback) {
+  FilePath new_path(full_path);
+  if (!overwrite_existing_file) {
+    // Make the file unique if requested.
+    int uniquifier =
+        file_util::GetUniquePathNumber(new_path, FILE_PATH_LITERAL(""));
+    if (uniquifier > 0) {
+      new_path = new_path.InsertBeforeExtensionASCII(
+          StringPrintf(" (%d)", uniquifier));
+    }
+  }
+
+  net::Error rename_error = file_.Rename(new_path);
+  content::DownloadInterruptReason reason(
+      content::DOWNLOAD_INTERRUPT_REASON_NONE);
+  if (net::OK != rename_error) {
+    // Make sure our information is updated, since we're about to
+    // error out.
+    SendUpdate();
+
+    reason =
+        content::ConvertNetErrorToInterruptReason(
+            rename_error,
+            content::DOWNLOAD_INTERRUPT_FROM_DISK);
+
+    new_path.clear();
+  }
+
+  BrowserThread::PostTask(
+      BrowserThread::UI, FROM_HERE,
+      base::Bind(callback, reason, new_path));
 }
 
 void DownloadFileImpl::Detach() {
@@ -190,6 +218,10 @@ void DownloadFileImpl::StreamActive() {
           base::TimeTicks write_start(base::TimeTicks::Now());
           reason = AppendDataToFile(
               incoming_data.get()->data(), incoming_data_size);
+          // Note that if we're after a rename failure but before any
+          // cancel that our owner generates based on that rename failure,
+          // we'll get an ERR_UNEXPECTED from the above.  Our consumers
+          // need to handle this situation.
           disk_writes_time_ += (base::TimeTicks::Now() - write_start);
           bytes_seen_ += incoming_data_size;
           total_incoming_data_size += incoming_data_size;
@@ -238,11 +270,11 @@ void DownloadFileImpl::StreamActive() {
     // Our controller will clean us up.
     stream_reader_->RegisterCallback(base::Closure());
     weak_factory_.InvalidateWeakPtrs();
+    SendUpdate();                       // Make info up to date before error.
     BrowserThread::PostTask(
         BrowserThread::UI, FROM_HERE,
         base::Bind(&DownloadManager::OnDownloadInterrupted,
-                   download_manager_, id_.local(),
-                   BytesSoFar(), GetHashState(), reason));
+                   download_manager_, id_.local(), reason));
   } else if (state == content::ByteStreamReader::STREAM_COMPLETE) {
     // Signal successful completion and shut down processing.
     stream_reader_->RegisterCallback(base::Closure());
diff --git a/content/browser/download/download_file_impl.h b/content/browser/download/download_file_impl.h
index 1083132..b3630e5 100644
--- a/content/browser/download/download_file_impl.h
+++ b/content/browser/download/download_file_impl.h
@@ -40,8 +40,9 @@ class CONTENT_EXPORT DownloadFileImpl : virtual public content::DownloadFile {
 
   // DownloadFile functions.
   virtual content::DownloadInterruptReason Initialize() OVERRIDE;
-  virtual content::DownloadInterruptReason Rename(
-      const FilePath& full_path) OVERRIDE;
+  virtual void Rename(const FilePath& full_path,
+                      bool overwrite_existing_file,
+                      const RenameCompletionCallback& callback) OVERRIDE;
   virtual void Detach() OVERRIDE;
   virtual void Cancel() OVERRIDE;
   virtual void AnnotateWithSourceInformation() OVERRIDE;
@@ -63,13 +64,13 @@ class CONTENT_EXPORT DownloadFileImpl : virtual public content::DownloadFile {
       const char* data, size_t data_len);
 
  private:
+  // Send an update on our progress.
+  void SendUpdate();
+
   // Called when there's some activity on stream_reader_ that needs to be
   // handled.
   void StreamActive();
 
-  // Send updates on our progress.
-  void SendUpdate();
-
   // The base file instance.
   BaseFile file_;
 
diff --git a/content/browser/download/download_file_manager.cc b/content/browser/download/download_file_manager.cc
index 4fc4a86..7bebd92 100644
--- a/content/browser/download/download_file_manager.cc
+++ b/content/browser/download/download_file_manager.cc
@@ -195,42 +195,14 @@ void DownloadFileManager::RenameDownloadFile(
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
   DownloadFile* download_file = GetDownloadFile(global_id);
   if (!download_file) {
-    BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
-                            base::Bind(callback, FilePath()));
-    return;
-  }
-
-  FilePath new_path(full_path);
-  if (!overwrite_existing_file) {
-    // Make the file unique if requested.
-    int uniquifier =
-        file_util::GetUniquePathNumber(new_path, FILE_PATH_LITERAL(""));
-    if (uniquifier > 0) {
-      new_path = new_path.InsertBeforeExtensionASCII(
-          StringPrintf(" (%d)", uniquifier));
-    }
-  }
-
-  // Do the actual rename
-  content::DownloadInterruptReason rename_error =
-      download_file->Rename(new_path);
-  if (content::DOWNLOAD_INTERRUPT_REASON_NONE != rename_error) {
-    DownloadManager* download_manager = download_file->GetDownloadManager();
-    DCHECK(download_manager);
-
     BrowserThread::PostTask(
         BrowserThread::UI, FROM_HERE,
-        base::Bind(&DownloadManager::OnDownloadInterrupted,
-                   download_manager,
-                   global_id.local(),
-                   download_file->BytesSoFar(),
-                   download_file->GetHashState(),
-                   rename_error));
-
-    new_path.clear();
+        base::Bind(callback, content::DOWNLOAD_INTERRUPT_REASON_FILE_FAILED,
+                   FilePath()));
+    return;
   }
-  BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
-                          base::Bind(callback, new_path));
+
+  download_file->Rename(full_path, overwrite_existing_file, callback);
 }
 
 int DownloadFileManager::NumberOfActiveDownloads() const {
diff --git a/content/browser/download/download_file_manager.h b/content/browser/download/download_file_manager.h
index bf2a53e..919f1c4 100644
--- a/content/browser/download/download_file_manager.h
+++ b/content/browser/download/download_file_manager.h
@@ -49,6 +49,7 @@
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/timer.h"
+#include "content/browser/download/download_file.h"
 #include "content/common/content_export.h"
 #include "content/public/browser/download_id.h"
 #include "content/public/browser/download_interrupt_reasons.h"
@@ -61,7 +62,6 @@ class FilePath;
 
 namespace content {
 class ByteStreamReader;
-class DownloadFile;
 class DownloadId;
 class DownloadManager;
 }
@@ -82,7 +82,8 @@ class CONTENT_EXPORT DownloadFileManager
       CreateDownloadFileCallback;
 
   // Callback used with RenameDownloadFile().
-  typedef base::Callback<void(const FilePath&)> RenameCompletionCallback;
+  typedef content::DownloadFile::RenameCompletionCallback
+      RenameCompletionCallback;
 
   class DownloadFileFactory {
    public:
diff --git a/content/browser/download/download_file_manager_unittest.cc b/content/browser/download/download_file_manager_unittest.cc
index 04290d4..52ce433 100644
--- a/content/browser/download/download_file_manager_unittest.cc
+++ b/content/browser/download/download_file_manager_unittest.cc
@@ -31,6 +31,7 @@ using ::testing::_;
 using ::testing::AtLeast;
 using ::testing::Mock;
 using ::testing::Return;
+using ::testing::SaveArg;
 using ::testing::StrictMock;
 using ::testing::StrEq;
 
@@ -39,8 +40,10 @@ namespace {
 // MockDownloadManager with the addition of a mock callback used for testing.
 class TestDownloadManager : public MockDownloadManager {
  public:
-  MOCK_METHOD2(OnDownloadRenamed,
-               void(int download_id, const FilePath& full_path));
+  MOCK_METHOD3(OnDownloadRenamed,
+               void(int download_id,
+                    content::DownloadInterruptReason reason,
+                    const FilePath& full_path));
  private:
   ~TestDownloadManager() {}
 };
@@ -212,48 +215,22 @@ class DownloadFileManagerTest : public testing::Test {
   // |should_overwrite| indicates whether to replace or uniquify the file.
   void RenameFile(const DownloadId& id,
                   const FilePath& new_path,
-                  const FilePath& unique_path,
-                  content::DownloadInterruptReason rename_error,
-                  RenameFileState state,
-                  RenameFileOverwrite should_overwrite) {
+                  bool should_overwrite) {
     MockDownloadFile* file = download_file_factory_->GetExistingFile(id);
     ASSERT_TRUE(file != NULL);
+    content::DownloadFile::RenameCompletionCallback rename_callback;
 
-    EXPECT_CALL(*file, Rename(unique_path))
-        .Times(1)
-        .WillOnce(Return(rename_error));
+    EXPECT_CALL(*file, Rename(new_path, should_overwrite, _))
+        .WillOnce(SaveArg<2>(&rename_callback));
 
-    if (rename_error != content::DOWNLOAD_INTERRUPT_REASON_NONE) {
-      EXPECT_CALL(*file, BytesSoFar())
-          .Times(AtLeast(1))
-          .WillRepeatedly(Return(byte_count_[id]));
-      EXPECT_CALL(*file, GetHashState())
-          .Times(AtLeast(1));
-      EXPECT_CALL(*file, GetDownloadManager())
-          .Times(AtLeast(1));
-    }
-
-    download_file_manager_->RenameDownloadFile(
-        id, new_path, (should_overwrite == OVERWRITE),
+    content::DownloadFile::RenameCompletionCallback passed_callback(
         base::Bind(&TestDownloadManager::OnDownloadRenamed,
                    download_manager_, id.local()));
 
-    if (rename_error != content::DOWNLOAD_INTERRUPT_REASON_NONE) {
-      EXPECT_CALL(*download_manager_,
-                  OnDownloadInterrupted(
-                      id.local(),
-                      byte_count_[id],
-                      "",
-                      rename_error));
-      EXPECT_CALL(*download_manager_,
-                  OnDownloadRenamed(id.local(), FilePath()));
-      ProcessAllPendingMessages();
-      ++error_count_[id];
-    } else {
-      EXPECT_CALL(*download_manager_,
-                  OnDownloadRenamed(id.local(), unique_path));
-      ProcessAllPendingMessages();
-    }
+    download_file_manager_->RenameDownloadFile(
+        id, new_path, should_overwrite, passed_callback);
+
+    EXPECT_TRUE(rename_callback.Equals(passed_callback));
   }
 
   void Complete(DownloadId id) {
@@ -345,42 +322,7 @@ TEST_F(DownloadFileManagerTest, Complete) {
   Complete(dummy_id);
 }
 
-TEST_F(DownloadFileManagerTest, RenameInProgress) {
-  scoped_ptr<DownloadCreateInfo> info(new DownloadCreateInfo);
-  DownloadId dummy_id(download_manager_.get(), kDummyDownloadId);
-  info->download_id = dummy_id;
-  ScopedTempDir download_dir;
-  ASSERT_TRUE(download_dir.CreateUniqueTempDir());
-
-  CreateDownloadFile(info.Pass());
-
-  FilePath foo(download_dir.path().Append(FILE_PATH_LITERAL("foo.txt")));
-  RenameFile(dummy_id, foo, foo, content::DOWNLOAD_INTERRUPT_REASON_NONE,
-             IN_PROGRESS, OVERWRITE);
-
-  CleanUp(dummy_id);
-}
-
-TEST_F(DownloadFileManagerTest, RenameInProgressWithUniquification) {
-  scoped_ptr<DownloadCreateInfo> info(new DownloadCreateInfo);
-  DownloadId dummy_id(download_manager_.get(), kDummyDownloadId);
-  info->download_id = dummy_id;
-  ScopedTempDir download_dir;
-  ASSERT_TRUE(download_dir.CreateUniqueTempDir());
-
-  CreateDownloadFile(info.Pass());
-
-  FilePath foo(download_dir.path().Append(FILE_PATH_LITERAL("foo.txt")));
-  FilePath unique_foo(foo.InsertBeforeExtension(FILE_PATH_LITERAL(" (1)")));
-  ASSERT_EQ(0, file_util::WriteFile(foo, "", 0));
-  RenameFile(dummy_id, foo, unique_foo,
-             content::DOWNLOAD_INTERRUPT_REASON_NONE, IN_PROGRESS,
-             DONT_OVERWRITE);
-
-  CleanUp(dummy_id);
-}
-
-TEST_F(DownloadFileManagerTest, RenameInProgressWithError) {
+TEST_F(DownloadFileManagerTest, Rename) {
   scoped_ptr<DownloadCreateInfo> info(new DownloadCreateInfo);
   DownloadId dummy_id(download_manager_.get(), kDummyDownloadId);
   info->download_id = dummy_id;
@@ -390,14 +332,12 @@ TEST_F(DownloadFileManagerTest, RenameInProgressWithError) {
   CreateDownloadFile(info.Pass());
 
   FilePath foo(download_dir.path().Append(FILE_PATH_LITERAL("foo.txt")));
-  RenameFile(dummy_id, foo, foo,
-             content::DOWNLOAD_INTERRUPT_REASON_FILE_NAME_TOO_LONG,
-             IN_PROGRESS, OVERWRITE);
+  RenameFile(dummy_id, foo, true);
 
   CleanUp(dummy_id);
 }
 
-TEST_F(DownloadFileManagerTest, RenameWithUniquification) {
+TEST_F(DownloadFileManagerTest, RenameNoOverwrite) {
   scoped_ptr<DownloadCreateInfo> info(new DownloadCreateInfo);
   DownloadId dummy_id(download_manager_.get(), kDummyDownloadId);
   info->download_id = dummy_id;
@@ -407,13 +347,7 @@ TEST_F(DownloadFileManagerTest, RenameWithUniquification) {
   CreateDownloadFile(info.Pass());
 
   FilePath foo(download_dir.path().Append(FILE_PATH_LITERAL("foo.txt")));
-  FilePath unique_foo(foo.InsertBeforeExtension(FILE_PATH_LITERAL(" (1)")));
-  // Create a file at |foo|. Since we are specifying DONT_OVERWRITE,
-  // RenameDownloadFile() should pick "foo (1).txt" instead of
-  // overwriting this file.
-  ASSERT_EQ(0, file_util::WriteFile(foo, "", 0));
-  RenameFile(dummy_id, foo, unique_foo,
-             content::DOWNLOAD_INTERRUPT_REASON_NONE, COMPLETE, DONT_OVERWRITE);
+  RenameFile(dummy_id, foo, false);
 
   CleanUp(dummy_id);
 }
@@ -429,12 +363,10 @@ TEST_F(DownloadFileManagerTest, RenameTwice) {
 
   FilePath crfoo(download_dir.path().Append(
       FILE_PATH_LITERAL("foo.txt.crdownload")));
-  RenameFile(dummy_id, crfoo, crfoo, content::DOWNLOAD_INTERRUPT_REASON_NONE,
-             IN_PROGRESS, OVERWRITE);
+  RenameFile(dummy_id, crfoo, true);
 
   FilePath foo(download_dir.path().Append(FILE_PATH_LITERAL("foo.txt")));
-  RenameFile(dummy_id, foo, foo, content::DOWNLOAD_INTERRUPT_REASON_NONE,
-             COMPLETE, OVERWRITE);
+  RenameFile(dummy_id, foo, true);
 
   CleanUp(dummy_id);
 }
@@ -455,24 +387,20 @@ TEST_F(DownloadFileManagerTest, TwoDownloads) {
 
   FilePath crbar(download_dir.path().Append(
       FILE_PATH_LITERAL("bar.txt.crdownload")));
-  RenameFile(dummy_id2, crbar, crbar, content::DOWNLOAD_INTERRUPT_REASON_NONE,
-             IN_PROGRESS, OVERWRITE);
+  RenameFile(dummy_id2, crbar, true);
 
   FilePath crfoo(download_dir.path().Append(
       FILE_PATH_LITERAL("foo.txt.crdownload")));
-  RenameFile(dummy_id, crfoo, crfoo, content::DOWNLOAD_INTERRUPT_REASON_NONE,
-             IN_PROGRESS, OVERWRITE);
+  RenameFile(dummy_id, crfoo, true);
 
 
   FilePath bar(download_dir.path().Append(FILE_PATH_LITERAL("bar.txt")));
-  RenameFile(dummy_id2, bar, bar, content::DOWNLOAD_INTERRUPT_REASON_NONE,
-             COMPLETE, OVERWRITE);
+  RenameFile(dummy_id2, bar, true);
 
   CleanUp(dummy_id2);
 
   FilePath foo(download_dir.path().Append(FILE_PATH_LITERAL("foo.txt")));
-  RenameFile(dummy_id, foo, foo, content::DOWNLOAD_INTERRUPT_REASON_NONE,
-             COMPLETE, OVERWRITE);
+  RenameFile(dummy_id, foo, true);
 
   CleanUp(dummy_id);
 }
diff --git a/content/browser/download/download_file_unittest.cc b/content/browser/download/download_file_unittest.cc
index 01009a8..aa07554 100644
--- a/content/browser/download/download_file_unittest.cc
+++ b/content/browser/download/download_file_unittest.cc
@@ -5,6 +5,7 @@
 #include "base/file_util.h"
 #include "base/message_loop.h"
 #include "base/string_number_conversions.h"
+#include "base/test/test_file_util.h"
 #include "content/browser/browser_thread_impl.h"
 #include "content/browser/download/byte_stream.h"
 #include "content/browser/download/download_create_info.h"
@@ -15,6 +16,7 @@
 #include "content/public/browser/download_manager.h"
 #include "content/public/test/mock_download_manager.h"
 #include "net/base/file_stream.h"
+#include "net/base/mock_file_stream.h"
 #include "net/base/net_errors.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -27,6 +29,7 @@ using content::DownloadManager;
 using ::testing::_;
 using ::testing::AnyNumber;
 using ::testing::DoAll;
+using ::testing::InSequence;
 using ::testing::Return;
 using ::testing::SetArgPointee;
 using ::testing::StrictMock;
@@ -45,6 +48,14 @@ class MockByteStreamReader : public content::ByteStreamReader {
   MOCK_METHOD1(RegisterCallback, void(const base::Closure&));
 };
 
+class LocalMockDownloadManager : public content::MockDownloadManager {
+ public:
+  MOCK_METHOD4(CurrentUpdateStatus,
+               void(int32, int64, int64, const std::string&));
+ protected:
+  virtual ~LocalMockDownloadManager() {}
+};
+
 }  // namespace
 
 DownloadId::Domain kValidIdDomain = "valid DownloadId::Domain";
@@ -65,6 +76,10 @@ class DownloadFileTest : public testing::Test {
   // calling Release() on |download_manager_| won't ever result in its
   // destructor being called and we get a leak.
   DownloadFileTest() :
+      update_download_id_(-1),
+      bytes_(-1),
+      bytes_per_sec_(-1),
+      hash_state_("xyzzy"),
       ui_thread_(BrowserThread::UI, &loop_),
       file_thread_(BrowserThread::FILE, &loop_) {
   }
@@ -74,13 +89,19 @@ class DownloadFileTest : public testing::Test {
 
   void SetUpdateDownloadInfo(int32 id, int64 bytes, int64 bytes_per_sec,
                              const std::string& hash_state) {
+    update_download_id_ = id;
     bytes_ = bytes;
     bytes_per_sec_ = bytes_per_sec;
     hash_state_ = hash_state;
   }
 
+  void ConfirmUpdateDownloadInfo() {
+    download_manager_->CurrentUpdateStatus(
+        update_download_id_, bytes_, bytes_per_sec_, hash_state_);
+  }
+
   virtual void SetUp() {
-    download_manager_ = new StrictMock<content::MockDownloadManager>;
+    download_manager_ = new StrictMock<LocalMockDownloadManager>;
     EXPECT_CALL(*(download_manager_.get()),
                 UpdateDownload(
                     DownloadId(kValidIdDomain, kDummyDownloadId + 0).local(),
@@ -117,8 +138,8 @@ class DownloadFileTest : public testing::Test {
         .RetiresOnSaturation();
 
     DownloadCreateInfo info;
-    info.download_id = DownloadId(kValidIdDomain, kDummyDownloadId + offset);
     // info.request_handle default constructed to null.
+    info.download_id = DownloadId(kValidIdDomain, kDummyDownloadId + offset);
     info.save_info.file_stream = file_stream_;
     download_file_.reset(
         new DownloadFileImpl(
@@ -228,8 +249,28 @@ class DownloadFileTest : public testing::Test {
     }
   }
 
+  content::DownloadInterruptReason Rename(
+      const FilePath& full_path, bool overwrite_existing_file,
+      FilePath* result_path_p) {
+    base::WeakPtrFactory<DownloadFileTest> weak_ptr_factory(this);
+    content::DownloadInterruptReason result_reason(
+        content::DOWNLOAD_INTERRUPT_REASON_NONE);
+    bool callback_was_called(false);
+    FilePath result_path;
+
+    download_file_->Rename(full_path, overwrite_existing_file,
+                           base::Bind(&DownloadFileTest::SetRenameResult,
+                                      weak_ptr_factory.GetWeakPtr(),
+                                      &callback_was_called,
+                                      &result_reason, result_path_p));
+    loop_.RunAllPending();
+
+    EXPECT_TRUE(callback_was_called);
+    return result_reason;
+  }
+
  protected:
-  scoped_refptr<StrictMock<content::MockDownloadManager> > download_manager_;
+  scoped_refptr<StrictMock<LocalMockDownloadManager> > download_manager_;
 
   linked_ptr<net::FileStream> file_stream_;
 
@@ -244,6 +285,7 @@ class DownloadFileTest : public testing::Test {
   base::Closure sink_callback_;
 
   // Latest update sent to the download manager.
+  int32 update_download_id_;
   int64 bytes_;
   int64 bytes_per_sec_;
   std::string hash_state_;
@@ -251,6 +293,19 @@ class DownloadFileTest : public testing::Test {
   MessageLoop loop_;
 
  private:
+  void SetRenameResult(bool* called_p,
+                       content::DownloadInterruptReason* reason_p,
+                       FilePath* result_path_p,
+                       content::DownloadInterruptReason reason,
+                       const FilePath& result_path) {
+    if (called_p)
+      *called_p = true;
+    if (reason_p)
+      *reason_p = reason;
+    if (result_path_p)
+      *result_path_p = result_path;
+  }
+
   // UI thread.
   BrowserThreadImpl ui_thread_;
   // File thread to satisfy debug checks in DownloadFile.
@@ -281,12 +336,15 @@ TEST_F(DownloadFileTest, RenameFileFinal) {
   FilePath path_2(initial_path.InsertBeforeExtensionASCII("_2"));
   FilePath path_3(initial_path.InsertBeforeExtensionASCII("_3"));
   FilePath path_4(initial_path.InsertBeforeExtensionASCII("_4"));
+  FilePath path_5(initial_path.InsertBeforeExtensionASCII("_5"));
+  FilePath output_path;
 
   // Rename the file before downloading any data.
   EXPECT_EQ(content::DOWNLOAD_INTERRUPT_REASON_NONE,
-            download_file_->Rename(path_1));
+            Rename(path_1, false, &output_path));
   FilePath renamed_path = download_file_->FullPath();
   EXPECT_EQ(path_1, renamed_path);
+  EXPECT_EQ(path_1, output_path);
 
   // Check the files.
   EXPECT_FALSE(file_util::PathExists(initial_path));
@@ -298,9 +356,10 @@ TEST_F(DownloadFileTest, RenameFileFinal) {
 
   // Rename the file after downloading some data.
   EXPECT_EQ(content::DOWNLOAD_INTERRUPT_REASON_NONE,
-            download_file_->Rename(path_2));
+            Rename(path_2, false, &output_path));
   renamed_path = download_file_->FullPath();
   EXPECT_EQ(path_2, renamed_path);
+  EXPECT_EQ(path_2, output_path);
 
   // Check the files.
   EXPECT_FALSE(file_util::PathExists(path_1));
@@ -311,9 +370,10 @@ TEST_F(DownloadFileTest, RenameFileFinal) {
 
   // Rename the file after downloading all the data.
   EXPECT_EQ(content::DOWNLOAD_INTERRUPT_REASON_NONE,
-            download_file_->Rename(path_3));
+            Rename(path_3, false, &output_path));
   renamed_path = download_file_->FullPath();
   EXPECT_EQ(path_3, renamed_path);
+  EXPECT_EQ(path_3, output_path);
 
   // Check the files.
   EXPECT_FALSE(file_util::PathExists(path_2));
@@ -327,9 +387,10 @@ TEST_F(DownloadFileTest, RenameFileFinal) {
 
   // Rename the file after downloading all the data and closing the file.
   EXPECT_EQ(content::DOWNLOAD_INTERRUPT_REASON_NONE,
-            download_file_->Rename(path_4));
+            Rename(path_4, false, &output_path));
   renamed_path = download_file_->FullPath();
   EXPECT_EQ(path_4, renamed_path);
+  EXPECT_EQ(path_4, output_path);
 
   // Check the files.
   EXPECT_FALSE(file_util::PathExists(path_3));
@@ -339,6 +400,81 @@ TEST_F(DownloadFileTest, RenameFileFinal) {
   EXPECT_TRUE(download_file_->GetHash(&hash));
   EXPECT_EQ(kDataHash, base::HexEncode(hash.data(), hash.size()));
 
+  // Check that a rename with overwrite to an existing file succeeds.
+  std::string file_contents;
+  ASSERT_FALSE(file_util::PathExists(path_5));
+  static const char file_data[] = "xyzzy";
+  ASSERT_EQ(static_cast<int>(sizeof(file_data) - 1),
+            file_util::WriteFile(path_5, file_data, sizeof(file_data) - 1));
+  ASSERT_TRUE(file_util::PathExists(path_5));
+  EXPECT_TRUE(file_util::ReadFileToString(path_5, &file_contents));
+  EXPECT_EQ(std::string(file_data), file_contents);
+
+  EXPECT_EQ(content::DOWNLOAD_INTERRUPT_REASON_NONE,
+            Rename(path_5, true, &output_path));
+  EXPECT_EQ(path_5, output_path);
+
+  file_contents = "";
+  EXPECT_TRUE(file_util::ReadFileToString(path_5, &file_contents));
+  EXPECT_NE(std::string(file_data), file_contents);
+
+  DestroyDownloadFile(0);
+}
+
+// Test to make sure the rename uniquifies if we aren't overwriting
+// and there's a file where we're aiming.
+TEST_F(DownloadFileTest, RenameUniquifies) {
+  ASSERT_TRUE(CreateDownloadFile(0, true));
+  FilePath initial_path(download_file_->FullPath());
+  EXPECT_TRUE(file_util::PathExists(initial_path));
+  FilePath path_1(initial_path.InsertBeforeExtensionASCII("_1"));
+  FilePath path_1_suffixed(path_1.InsertBeforeExtensionASCII(" (1)"));
+
+  ASSERT_FALSE(file_util::PathExists(path_1));
+  static const char file_data[] = "xyzzy";
+  ASSERT_EQ(static_cast<int>(sizeof(file_data)),
+            file_util::WriteFile(path_1, file_data, sizeof(file_data)));
+  ASSERT_TRUE(file_util::PathExists(path_1));
+
+  EXPECT_EQ(content::DOWNLOAD_INTERRUPT_REASON_NONE,
+            Rename(path_1, false, NULL));
+  EXPECT_TRUE(file_util::PathExists(path_1_suffixed));
+
+  FinishStream(content::DOWNLOAD_INTERRUPT_REASON_NONE, true);
+  loop_.RunAllPending();
+  DestroyDownloadFile(0);
+}
+
+// Test to make sure we get the proper error on failure.
+TEST_F(DownloadFileTest, RenameError) {
+  ASSERT_TRUE(CreateDownloadFile(0, true));
+  FilePath initial_path(download_file_->FullPath());
+
+  // Create a subdirectory and rename into it.
+  FilePath tempdir(initial_path.DirName().Append(FILE_PATH_LITERAL("tempdir")));
+  ASSERT_TRUE(file_util::CreateDirectory(tempdir));
+  FilePath new_path(tempdir.Append(initial_path.BaseName()));
+  ASSERT_EQ(content::DOWNLOAD_INTERRUPT_REASON_NONE,
+            Rename(new_path, true, NULL));
+
+  // Targets
+  FilePath path_1(new_path.InsertBeforeExtensionASCII("_1"));
+  FilePath path_1_suffixed(path_1.InsertBeforeExtensionASCII(" (1)"));
+  ASSERT_FALSE(file_util::PathExists(path_1));
+  ASSERT_FALSE(file_util::PathExists(path_1_suffixed));
+
+  // Make the directory unwritable and try to rename within it.
+  {
+    file_util::PermissionRestorer restorer(tempdir);
+    ASSERT_TRUE(file_util::MakeFileUnwritable(tempdir));
+
+    EXPECT_EQ(content::DOWNLOAD_INTERRUPT_REASON_FILE_ACCESS_DENIED,
+              Rename(path_1, true, NULL));
+    EXPECT_FALSE(file_util::PathExists(path_1_suffixed));
+  }
+
+  FinishStream(content::DOWNLOAD_INTERRUPT_REASON_NONE, true);
+  loop_.RunAllPending();
   DestroyDownloadFile(0);
 }
 
@@ -380,10 +516,22 @@ TEST_F(DownloadFileTest, StreamEmptyError) {
   EXPECT_CALL(*(download_manager_.get()),
               OnDownloadInterrupted(
                   DownloadId(kValidIdDomain, kDummyDownloadId + 0).local(),
-                  0, _,
-                  content::DOWNLOAD_INTERRUPT_REASON_NETWORK_DISCONNECTED));
+                  content::DOWNLOAD_INTERRUPT_REASON_NETWORK_DISCONNECTED))
+      .WillOnce(InvokeWithoutArgs(
+          this, &DownloadFileTest::ConfirmUpdateDownloadInfo));
+
+  // If this next EXPECT_CALL fails flakily, it's probably a real failure.
+  // We'll be getting a stream of UpdateDownload calls from the timer, and
+  // the last one may have the correct information even if the failure
+  // doesn't produce an update, as the timer update may have triggered at the
+  // same time.
+  EXPECT_CALL(*(download_manager_.get()),
+              CurrentUpdateStatus(kDummyDownloadId + 0, 0, _, _));
+
   FinishStream(content::DOWNLOAD_INTERRUPT_REASON_NETWORK_DISCONNECTED, false);
 
+  loop_.RunAllPending();
+
   DestroyDownloadFile(0);
 }
 
@@ -416,12 +564,26 @@ TEST_F(DownloadFileTest, StreamNonEmptyError) {
   SetupDataAppend(chunks1, 2, s1);
   SetupFinishStream(content::DOWNLOAD_INTERRUPT_REASON_NETWORK_DISCONNECTED,
                     s1);
+
   EXPECT_CALL(*(download_manager_.get()),
               OnDownloadInterrupted(
                   DownloadId(kValidIdDomain, kDummyDownloadId + 0).local(),
-                  strlen(kTestData1) + strlen(kTestData2), _,
-                  content::DOWNLOAD_INTERRUPT_REASON_NETWORK_DISCONNECTED));
+                  content::DOWNLOAD_INTERRUPT_REASON_NETWORK_DISCONNECTED))
+      .WillOnce(InvokeWithoutArgs(
+          this, &DownloadFileTest::ConfirmUpdateDownloadInfo));
+
+  // If this next EXPECT_CALL fails flakily, it's probably a real failure.
+  // We'll be getting a stream of UpdateDownload calls from the timer, and
+  // the last one may have the correct information even if the failure
+  // doesn't produce an update, as the timer update may have triggered at the
+  // same time.
+  EXPECT_CALL(*(download_manager_.get()),
+              CurrentUpdateStatus(kDummyDownloadId + 0,
+                                  strlen(kTestData1) + strlen(kTestData2),
+                                  _, _));
+
   sink_callback_.Run();
+  loop_.RunAllPending();
   VerifyStreamAndSize();
   DestroyDownloadFile(0);
 }
diff --git a/content/browser/download/download_item_impl.cc b/content/browser/download/download_item_impl.cc
index 3c9521d..1912cce 100644
--- a/content/browser/download/download_item_impl.cc
+++ b/content/browser/download/download_item_impl.cc
@@ -489,8 +489,19 @@ void DownloadItemImpl::Cancel(bool user_cancel) {
 
 // An error occurred somewhere.
 void DownloadItemImpl::Interrupt(content::DownloadInterruptReason reason) {
-  // It should not be possible both to have an error and complete.
-  DCHECK(IsInProgress());
+  // Somewhat counter-intuitively, it is possible for us to receive an
+  // interrupt after we've already been interrupted.  The generation of
+  // interrupts from the file thread Renames and the generation of
+  // interrupts from disk writes go through two different mechanisms (driven
+  // by rename requests from UI thread and by write requests from IO thread,
+  // respectively), and since we choose not to keep state on the File thread,
+  // this is the place where the races collide.  It's also possible for
+  // interrupts to race with cancels.
+
+  // Whatever happens, the first one to hit the UI thread wins.
+  if (!IsInProgress())
+    return;
+
   last_reason_ = reason;
   TransitionTo(INTERRUPTED);
   download_stats::RecordDownloadInterrupted(
@@ -739,6 +750,7 @@ void DownloadItemImpl::OnDownloadCompleting(DownloadFileManager* file_manager) {
 
 void DownloadItemImpl::OnDownloadRenamedToFinalName(
     DownloadFileManager* file_manager,
+    content::DownloadInterruptReason reason,
     const FilePath& full_path) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
@@ -748,12 +760,13 @@ void DownloadItemImpl::OnDownloadRenamedToFinalName(
            << " " << DebugString(false);
   DCHECK(NeedsRename());
 
-  if (full_path.empty())
-    // Indicates error; also reported
-    // by DownloadManagerImpl::OnDownloadInterrupted.
+  if (content::DOWNLOAD_INTERRUPT_REASON_NONE != reason) {
+    Interrupt(reason);
     return;
+  }
 
   // full_path is now the current and target file path.
+  DCHECK(!full_path.empty());
   target_path_ = full_path;
   SetFullPath(full_path);
   delegate_->DownloadRenamedToFinalName(this);
@@ -775,12 +788,16 @@ void DownloadItemImpl::OnDownloadFileReleased() {
 }
 
 void DownloadItemImpl::OnDownloadRenamedToIntermediateName(
+    content::DownloadInterruptReason reason,
     const FilePath& full_path) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  if (!full_path.empty()) {
+  if (content::DOWNLOAD_INTERRUPT_REASON_NONE != reason) {
+    Interrupt(reason);
+  } else {
     SetFullPath(full_path);
     UpdateObservers();
   }
+
   delegate_->DownloadRenamedToIntermediateName(this);
 }
 
diff --git a/content/browser/download/download_item_impl.h b/content/browser/download/download_item_impl.h
index af0b505..a48154c 100644
--- a/content/browser/download/download_item_impl.h
+++ b/content/browser/download/download_item_impl.h
@@ -253,11 +253,13 @@ class CONTENT_EXPORT DownloadItemImpl : public content::DownloadItem {
 
   // Callback invoked when the download has been renamed to its final name.
   void OnDownloadRenamedToFinalName(DownloadFileManager* file_manager,
+                                    content::DownloadInterruptReason reason,
                                     const FilePath& full_path);
 
   // Callback invoked when the download has been renamed to its intermediate
   // name.
-  void OnDownloadRenamedToIntermediateName(const FilePath& full_path);
+  void OnDownloadRenamedToIntermediateName(
+      content::DownloadInterruptReason reason, const FilePath& full_path);
 
   // Callback from file thread when we release the DownloadFile.
   void OnDownloadFileReleased();
diff --git a/content/browser/download/download_item_impl_unittest.cc b/content/browser/download/download_item_impl_unittest.cc
index c7ac84d..3224a83 100644
--- a/content/browser/download/download_item_impl_unittest.cc
+++ b/content/browser/download/download_item_impl_unittest.cc
@@ -101,8 +101,9 @@ class MockDownloadFileManager : public DownloadFileManager {
 //               RenameDownloadFile(_,_,_,_))
 //       .WillOnce(ScheduleRenameCallback(new_path));
 ACTION_P(ScheduleRenameCallback, new_path) {
-  BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
-                          base::Bind(arg3, new_path));
+  BrowserThread::PostTask(
+      BrowserThread::UI, FROM_HERE,
+      base::Bind(arg3, content::DOWNLOAD_INTERRUPT_REASON_NONE, new_path));
 }
 
 // Similarly for scheduling a completion callback.
diff --git a/content/browser/download/download_manager_impl.cc b/content/browser/download/download_manager_impl.cc
index f742c7c..b798899 100644
--- a/content/browser/download/download_manager_impl.cc
+++ b/content/browser/download/download_manager_impl.cc
@@ -428,7 +428,7 @@ content::DownloadId DownloadManagerImpl::StartDownload(
 void DownloadManagerImpl::OnDownloadFileCreated(
     int32 download_id, content::DownloadInterruptReason reason) {
   if (reason != content::DOWNLOAD_INTERRUPT_REASON_NONE) {
-    OnDownloadInterrupted(download_id, 0, "", reason);
+    OnDownloadInterrupted(download_id, reason);
     // TODO(rdsmith): It makes no sense to continue along the
     // regular download path after we've gotten an error.  But it's
     // the way the code has historically worked, and this allows us
@@ -771,15 +771,12 @@ void DownloadManagerImpl::DownloadStopped(DownloadItem* download) {
 
 void DownloadManagerImpl::OnDownloadInterrupted(
     int32 download_id,
-    int64 size,
-    const std::string& hash_state,
     content::DownloadInterruptReason reason) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   DownloadItem* download = GetActiveDownload(download_id);
   if (!download)
     return;
-  download->UpdateProgress(size, 0, hash_state);
   download->Interrupt(reason);
 }
 
diff --git a/content/browser/download/download_manager_impl.h b/content/browser/download/download_manager_impl.h
index ad79935..6edebaa 100644
--- a/content/browser/download/download_manager_impl.h
+++ b/content/browser/download/download_manager_impl.h
@@ -58,8 +58,6 @@ class CONTENT_EXPORT DownloadManagerImpl
   virtual void CancelDownload(int32 download_id) OVERRIDE;
   virtual void OnDownloadInterrupted(
       int32 download_id,
-      int64 size,
-      const std::string& hash_state,
       content::DownloadInterruptReason reason) OVERRIDE;
   virtual int RemoveDownloadsBetween(base::Time remove_begin,
                                      base::Time remove_end) OVERRIDE;
diff --git a/content/browser/download/download_manager_impl_unittest.cc b/content/browser/download/download_manager_impl_unittest.cc
index 124d4ce..da28c5e 100644
--- a/content/browser/download/download_manager_impl_unittest.cc
+++ b/content/browser/download/download_manager_impl_unittest.cc
@@ -513,15 +513,11 @@ TEST_F(DownloadManagerTest, OnDownloadInterrupted) {
   content::MockDownloadItem& item(AddItemToManager());
   int download_id = item.GetId();
 
-  int64 size = 0xdeadbeef;
-  const std::string hash_state("Undead beef");
   content::DownloadInterruptReason reason(
       content::DOWNLOAD_INTERRUPT_REASON_FILE_FAILED);
 
-  EXPECT_CALL(item, UpdateProgress(size, 0, hash_state));
   EXPECT_CALL(item, Interrupt(reason));
-  download_manager_->OnDownloadInterrupted(
-      download_id, size, hash_state, reason);
+  download_manager_->OnDownloadInterrupted(download_id, reason);
   EXPECT_EQ(&item, GetActiveDownloadItem(download_id));
 }
 
diff --git a/content/browser/download/mock_download_file.h b/content/browser/download/mock_download_file.h
index 8608136..34bcb54 100644
--- a/content/browser/download/mock_download_file.h
+++ b/content/browser/download/mock_download_file.h
@@ -29,6 +29,9 @@ class MockDownloadFile : virtual public content::DownloadFile {
       const char* data, size_t data_len));
   MOCK_METHOD1(Rename, content::DownloadInterruptReason(
       const FilePath& full_path));
+  MOCK_METHOD3(Rename, void(const FilePath& full_path,
+                            bool overwrite_existing_file,
+                            const RenameCompletionCallback& callback));
   MOCK_METHOD0(Detach, void());
   MOCK_METHOD0(Cancel, void());
   MOCK_METHOD0(Finish, void());
@@ -40,6 +43,7 @@ class MockDownloadFile : virtual public content::DownloadFile {
   MOCK_METHOD1(GetHash, bool(std::string* hash));
   MOCK_METHOD0(GetHashState, std::string());
   MOCK_METHOD0(CancelDownloadRequest, void());
+  MOCK_METHOD0(SendUpdate, void());
   MOCK_CONST_METHOD0(Id, int());
   MOCK_METHOD0(GetDownloadManager, content::DownloadManager*());
   MOCK_CONST_METHOD0(GlobalId, const content::DownloadId&());
diff --git a/content/public/browser/download_manager.h b/content/public/browser/download_manager.h
index 1d60213..36f0ae3 100644
--- a/content/public/browser/download_manager.h
+++ b/content/public/browser/download_manager.h
@@ -152,8 +152,6 @@ class CONTENT_EXPORT DownloadManager
   // |reason| is a download interrupt reason code.
   virtual void OnDownloadInterrupted(
       int32 download_id,
-      int64 size,
-      const std::string& hash_state,
       DownloadInterruptReason reason) = 0;
 
   // Remove downloads after remove_begin (inclusive) and before remove_end
diff --git a/content/public/test/mock_download_manager.h b/content/public/test/mock_download_manager.h
index 2e61ee5..3e9674c 100644
--- a/content/public/test/mock_download_manager.h
+++ b/content/public/test/mock_download_manager.h
@@ -49,10 +49,8 @@ class MockDownloadManager : public content::DownloadManager {
                                          int64 size,
                                          const std::string& hash));
   MOCK_METHOD1(CancelDownload, void(int32 download_id));
-  MOCK_METHOD4(OnDownloadInterrupted,
+  MOCK_METHOD2(OnDownloadInterrupted,
                void(int32 download_id,
-                    int64 size,
-                    const std::string& hash_state,
                     content::DownloadInterruptReason reason));
   MOCK_METHOD2(RemoveDownloadsBetween, int(base::Time remove_begin,
                                            base::Time remove_end));
diff --git a/content/test/test_file_error_injector.cc b/content/test/test_file_error_injector.cc
index 3af526a..26eaf8e 100644
--- a/content/test/test_file_error_injector.cc
+++ b/content/test/test_file_error_injector.cc
@@ -55,8 +55,9 @@ class DownloadFileWithErrors: public DownloadFileImpl {
   virtual content::DownloadInterruptReason Initialize() OVERRIDE;
   virtual content::DownloadInterruptReason AppendDataToFile(
       const char* data, size_t data_len) OVERRIDE;
-  virtual content::DownloadInterruptReason Rename(
-      const FilePath& full_path) OVERRIDE;
+  virtual void Rename(const FilePath& full_path,
+                      bool overwrite_existing_file,
+                      const RenameCompletionCallback& callback) OVERRIDE;
 
  private:
   // Error generating helper.
@@ -64,6 +65,13 @@ class DownloadFileWithErrors: public DownloadFileImpl {
       content::TestFileErrorInjector::FileOperationCode code,
       content::DownloadInterruptReason original_error);
 
+  // Used in place of original rename callback to intercept with
+  // ShouldReturnError.
+  void RenameErrorCallback(
+    const RenameCompletionCallback& original_callback,
+    content::DownloadInterruptReason original_error,
+    const FilePath& path_result);
+
   // Source URL for the file being downloaded.
   GURL source_url_;
 
@@ -118,11 +126,16 @@ content::DownloadInterruptReason DownloadFileWithErrors::AppendDataToFile(
       DownloadFileImpl::AppendDataToFile(data, data_len));
 }
 
-content::DownloadInterruptReason DownloadFileWithErrors::Rename(
-    const FilePath& full_path) {
-  return ShouldReturnError(
-      content::TestFileErrorInjector::FILE_OPERATION_RENAME,
-      DownloadFileImpl::Rename(full_path));
+void DownloadFileWithErrors::Rename(
+    const FilePath& full_path,
+    bool overwrite_existing_file,
+    const RenameCompletionCallback& callback) {
+  DownloadFileImpl::Rename(
+      full_path, overwrite_existing_file,
+      base::Bind(&DownloadFileWithErrors::RenameErrorCallback,
+                 // Unretained since this'll only be called from
+                 // the DownloadFileImpl slice of the same object.
+                 base::Unretained(this), callback));
 }
 
 content::DownloadInterruptReason DownloadFileWithErrors::ShouldReturnError(
@@ -152,6 +165,15 @@ content::DownloadInterruptReason DownloadFileWithErrors::ShouldReturnError(
   return error_info_.error;
 }
 
+void DownloadFileWithErrors::RenameErrorCallback(
+    const RenameCompletionCallback& original_callback,
+    content::DownloadInterruptReason original_error,
+    const FilePath& path_result) {
+  original_callback.Run(ShouldReturnError(
+      content::TestFileErrorInjector::FILE_OPERATION_RENAME,
+      original_error), path_result);
+}
+
 }  // namespace
 
 namespace content {
@@ -167,7 +189,7 @@ class DownloadFileWithErrorsFactory
   virtual ~DownloadFileWithErrorsFactory();
 
   // DownloadFileFactory interface.
-  virtual content::DownloadFile* CreateFile(
+  virtual DownloadFile* CreateFile(
       DownloadCreateInfo* info,
       scoped_ptr<content::ByteStreamReader> stream,
       content::DownloadManager* download_manager,