DownloadFile no longer keeps track of whether or not the final rename has occurred.

Instead, DownloadFileManager puts the DownloadFile in a new map when it has the final name.

This is a step towards moving the 'final rename' determination happen in the UI thread.

BUG=None
TEST=None

Review URL: http://codereview.chromium.org/6480079

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

chrome/browser/download/base_file.cc

@@ -25,7 +25,6 @@ BaseFile::BaseFile(const FilePath& full_path,
                    int64 received_bytes,
                    const linked_ptr<net::FileStream>& file_stream)
     : full_path_(full_path),
-      path_renamed_(false),
       source_url_(source_url),
       referrer_url_(referrer_url),
       file_stream_(file_stream),
@@ -79,7 +78,7 @@ bool BaseFile::AppendDataToFile(const char* data, size_t data_len) {
   return true;
 }
 
-bool BaseFile::Rename(const FilePath& new_path, bool is_final_rename) {
+bool BaseFile::Rename(const FilePath& new_path) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
 
   // Save the information whether the download is in progress because
@@ -89,8 +88,6 @@ bool BaseFile::Rename(const FilePath& new_path, bool is_final_rename) {
   // If the new path is same as the old one, there is no need to perform the
   // following renaming logic.
   if (new_path == full_path_) {
-    path_renamed_ = is_final_rename;
-
     // Don't close the file if we're not done (finished or canceled).
     if (!saved_in_progress)
       Close();
@@ -131,7 +128,6 @@ bool BaseFile::Rename(const FilePath& new_path, bool is_final_rename) {
 #endif
 
   full_path_ = new_path;
-  path_renamed_ = is_final_rename;
 
   // We don't need to re-open the file if we're done (finished or canceled).
   if (!saved_in_progress)

chrome/browser/download/base_file.h

@@ -40,9 +40,7 @@ class BaseFile {
   bool AppendDataToFile(const char* data, size_t data_len);
 
   // Rename the download file. Returns true on success.
-  // |path_renamed_| is set to true only if |is_final_rename| is true.
-  // Marked virtual for testing.
-  virtual bool Rename(const FilePath& full_path, bool is_final_rename);
+  virtual bool Rename(const FilePath& full_path);
 
   // Abort the download and automatically close the file.
   void Cancel();
@@ -54,7 +52,6 @@ class BaseFile {
   void AnnotateWithSourceInformation();
 
   FilePath full_path() const { return full_path_; }
-  bool path_renamed() const { return path_renamed_; }
   bool in_progress() const { return file_stream_ != NULL; }
   int64 bytes_so_far() const { return bytes_so_far_; }
 
@@ -71,9 +68,6 @@ class BaseFile {
   // Full path to the file including the file name.
   FilePath full_path_;
 
-  // Whether the download is still using its initial temporary path.
-  bool path_renamed_;
-
  private:
   static const size_t kSha256HashLen = 32;
 

chrome/browser/download/base_file_unittest.cc

@@ -77,7 +77,6 @@ class BaseFileTest : public testing::Test {
 // in production, where we would at least Initialize it.
 TEST_F(BaseFileTest, CreateDestroy) {
   EXPECT_EQ(FilePath().value(), base_file_->full_path().value());
-  EXPECT_FALSE(base_file_->path_renamed());
 }
 
 // Cancel the download explicitly.
@@ -87,7 +86,6 @@ TEST_F(BaseFileTest, Cancel) {
   base_file_->Cancel();
   EXPECT_FALSE(file_util::PathExists(base_file_->full_path()));
   EXPECT_NE(FilePath().value(), base_file_->full_path().value());
-  EXPECT_FALSE(base_file_->path_renamed());
 }
 
 // Write data to the file once.
@@ -95,8 +93,6 @@ TEST_F(BaseFileTest, SingleWrite) {
   ASSERT_TRUE(base_file_->Initialize(false));
   AppendDataToFile(kTestData1);
   base_file_->Finish();
-
-  EXPECT_FALSE(base_file_->path_renamed());
 }
 
 // Write data to the file multiple times.
@@ -108,8 +104,6 @@ TEST_F(BaseFileTest, MultipleWrites) {
   std::string hash;
   EXPECT_FALSE(base_file_->GetSha256Hash(&hash));
   base_file_->Finish();
-
-  EXPECT_FALSE(base_file_->path_renamed());
 }
 
 // Write data to the file once and calculate its sha256 hash.
@@ -118,8 +112,6 @@ TEST_F(BaseFileTest, SingleWriteWithHash) {
   AppendDataToFile(kTestData1);
   base_file_->Finish();
 
-  EXPECT_FALSE(base_file_->path_renamed());
-
   std::string hash;
   base_file_->GetSha256Hash(&hash);
   EXPECT_EQ("0B2D3F3F7943AD64B860DF94D05CB56A8A97C6EC5768B5B70B930C5AA7FA9ADE",
@@ -138,7 +130,6 @@ TEST_F(BaseFileTest, MultipleWritesWithHash) {
   EXPECT_FALSE(base_file_->GetSha256Hash(&hash));
   base_file_->Finish();
 
-  EXPECT_FALSE(base_file_->path_renamed());
   EXPECT_TRUE(base_file_->GetSha256Hash(&hash));
   EXPECT_EQ("CBF68BF10F8003DB86B31343AFAC8C7175BD03FB5FC905650F8C80AF087443A8",
             base::HexEncode(hash.data(), hash.size()));
@@ -155,13 +146,11 @@ TEST_F(BaseFileTest, WriteThenRename) {
 
   AppendDataToFile(kTestData1);
 
-  EXPECT_TRUE(base_file_->Rename(new_path, true));
+  EXPECT_TRUE(base_file_->Rename(new_path));
   EXPECT_FALSE(file_util::PathExists(initial_path));
   EXPECT_TRUE(file_util::PathExists(new_path));
 
   base_file_->Finish();
-
-  EXPECT_TRUE(base_file_->path_renamed());
 }
 
 // Rename the file while the download is still in progress.
@@ -176,15 +165,13 @@ TEST_F(BaseFileTest, RenameWhileInProgress) {
   AppendDataToFile(kTestData1);
 
   EXPECT_TRUE(base_file_->in_progress());
-  EXPECT_TRUE(base_file_->Rename(new_path, true));
+  EXPECT_TRUE(base_file_->Rename(new_path));
   EXPECT_FALSE(file_util::PathExists(initial_path));
   EXPECT_TRUE(file_util::PathExists(new_path));
 
   AppendDataToFile(kTestData2);
 
   base_file_->Finish();
-
-  EXPECT_TRUE(base_file_->path_renamed());
 }
 
 }  // namespace

chrome/browser/download/download_file_manager.cc

@@ -54,6 +54,7 @@ DownloadFileManager::DownloadFileManager(ResourceDispatcherHost* rdh)
 
 DownloadFileManager::~DownloadFileManager() {
   DCHECK(downloads_.empty());
+  DCHECK(downloads_with_final_name_.empty());
 }
 
 void DownloadFileManager::Shutdown() {
@@ -66,6 +67,7 @@ void DownloadFileManager::Shutdown() {
 void DownloadFileManager::OnShutdown() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
   StopUpdateTimer();
+  downloads_with_final_name_.clear();
   STLDeleteValues(&downloads_);
 }
 
@@ -224,10 +226,8 @@ void DownloadFileManager::OnResponseCompleted(int id, DownloadBuffer* buffer) {
 
     // We need to keep the download around until the UI thread has finalized
     // the name.
-    if (download->path_renamed()) {
-      downloads_.erase(it);
-      delete download;
-    }
+    if (ContainsKey(downloads_with_final_name_, id))
+      EraseDownload(id);
   }
 
   if (downloads_.empty())
@@ -247,10 +247,8 @@ void DownloadFileManager::CancelDownload(int id) {
              << " download = " << download->DebugString();
     download->Cancel();
 
-    if (download->path_renamed()) {
-      downloads_.erase(it);
-      delete download;
-    }
+    if (ContainsKey(downloads_with_final_name_, id))
+      EraseDownload(id);
   }
 
   if (downloads_.empty())
@@ -269,6 +267,7 @@ void DownloadFileManager::OnDownloadManagerShutdown(DownloadManager* manager) {
     if (download_file->GetDownloadManager() == manager) {
       download_file->CancelDownloadRequest(resource_dispatcher_host_);
       to_remove.insert(download_file);
+      downloads_with_final_name_.erase(download_file->id());
     }
   }
 
@@ -292,10 +291,11 @@ void DownloadFileManager::OnIntermediateDownloadName(
   DownloadFileMap::iterator it = downloads_.find(id);
   if (it == downloads_.end())
     return;
+  DCHECK(!ContainsKey(downloads_with_final_name_, id));
 
   DownloadFile* download = it->second;
   VLOG(20) << __FUNCTION__ << "()" << " download = " << download->DebugString();
-  if (!download->Rename(full_path, false /* is_final_rename */)) {
+  if (!download->Rename(full_path)) {
     // Error. Between the time the UI thread generated 'full_path' to the time
     // this code runs, something happened that prevents us from renaming.
     CancelDownloadOnRename(id);
@@ -309,7 +309,8 @@ void DownloadFileManager::OnIntermediateDownloadName(
 // There are 2 possible rename cases where this method can be called:
 // 1. foo.crdownload -> foo
 // 2. tmp-> Unconfirmed.xxx.crdownload
-// We don't call this function before the download is complete.
+// We don't call this function before a safe temp file has been renamed (in
+// that case tmp -> foo.crdownload occurs in |OnIntermediateDownloadName|).
 void DownloadFileManager::OnFinalDownloadName(
     int id, const FilePath& full_path, DownloadManager* download_manager) {
   VLOG(20) << __FUNCTION__ << "()" << " id = " << id
@@ -320,7 +321,9 @@ void DownloadFileManager::OnFinalDownloadName(
   if (!download)
     return;
   VLOG(20) << __FUNCTION__ << "()" << " download = " << download->DebugString();
-  if (download->Rename(full_path, true /* is_final_rename */)) {
+  DCHECK(!ContainsKey(downloads_with_final_name_, id));
+  if (download->Rename(full_path)) {
+    downloads_with_final_name_[id] = download;
 #if defined(OS_MACOSX)
     // Done here because we only want to do this once; see
     // http://crbug.com/13120 for details.
@@ -339,10 +342,8 @@ void DownloadFileManager::OnFinalDownloadName(
 
   // If the download has completed before we got this final name, we remove it
   // from our in progress map.
-  if (!download->in_progress()) {
-    downloads_.erase(id);
-    delete download;
-  }
+  if (!download->in_progress())
+    EraseDownload(id);
 
   if (downloads_.empty())
     StopUpdateTimer();
@@ -368,3 +369,17 @@ void DownloadFileManager::CancelDownloadOnRename(int id) {
       NewRunnableMethod(download_manager,
                         &DownloadManager::DownloadCancelled, id));
 }
+
+void DownloadFileManager::EraseDownload(int id) {
+  if (!ContainsKey(downloads_, id))
+    return;
+
+  DownloadFile* download_file = downloads_[id];
+
+  downloads_.erase(id);
+
+  if (ContainsKey(downloads_with_final_name_, id))
+    downloads_with_final_name_.erase(id);
+
+  delete download_file;
+}

chrome/browser/download/download_file_manager.h

@@ -126,13 +126,22 @@ class DownloadFileManager
   // on the FILE thread.
   void CancelDownloadOnRename(int id);
 
+  // Erases the download file with the given the download |id| and removes
+  // it from the maps.
+  void EraseDownload(int id);
+
   // Unique ID for each DownloadFile.
   int next_id_;
 
-  // A map of all in progress downloads.
   typedef base::hash_map<int, DownloadFile*> DownloadFileMap;
+
+  // A map of all in progress downloads.  It owns the download files,
+  // although they may also show up in |downloads_with_final_name_|.
   DownloadFileMap downloads_;
 
+  // download files are owned by |downloads_|.
+  DownloadFileMap downloads_with_final_name_;
+
   // Schedule periodic updates of the download progress. This timer
   // is controlled from the FILE thread, and posts updates to the UI thread.
   base::RepeatingTimer<DownloadFileManager> update_timer_;

chrome/browser/download/download_file_unittest.cc

@@ -125,7 +125,7 @@ const int DownloadFileTest::kDummyChildId = 3;
 const int DownloadFileTest::kDummyRequestId = 67;
 
 // Rename the file before any data is downloaded, after some has, after it all
-// has, and after it's closed.  File is marked as having the final rename.
+// has, and after it's closed.
 TEST_F(DownloadFileTest, RenameFileFinal) {
   CreateDownloadFile(&download_file_, 0);
   ASSERT_TRUE(download_file_->Initialize(true));
@@ -137,7 +137,7 @@ TEST_F(DownloadFileTest, RenameFileFinal) {
   FilePath path_4(initial_path.InsertBeforeExtensionASCII("_4"));
 
   // Rename the file before downloading any data.
-  EXPECT_TRUE(download_file_->Rename(path_1, true));
+  EXPECT_TRUE(download_file_->Rename(path_1));
   FilePath renamed_path = download_file_->full_path();
   EXPECT_EQ(path_1, renamed_path);
 
@@ -150,7 +150,7 @@ TEST_F(DownloadFileTest, RenameFileFinal) {
   AppendDataToFile(&download_file_, kTestData2);
 
   // Rename the file after downloading some data.
-  EXPECT_TRUE(download_file_->Rename(path_2, true));
+  EXPECT_TRUE(download_file_->Rename(path_2));
   renamed_path = download_file_->full_path();
   EXPECT_EQ(path_2, renamed_path);
 
@@ -161,7 +161,7 @@ TEST_F(DownloadFileTest, RenameFileFinal) {
   AppendDataToFile(&download_file_, kTestData3);
 
   // Rename the file after downloading all the data.
-  EXPECT_TRUE(download_file_->Rename(path_3, true));
+  EXPECT_TRUE(download_file_->Rename(path_3));
   renamed_path = download_file_->full_path();
   EXPECT_EQ(path_3, renamed_path);
 
@@ -176,7 +176,7 @@ TEST_F(DownloadFileTest, RenameFileFinal) {
   download_file_->Finish();
 
   // Rename the file after downloading all the data and closing the file.
-  EXPECT_TRUE(download_file_->Rename(path_4, true));
+  EXPECT_TRUE(download_file_->Rename(path_4));
   renamed_path = download_file_->full_path();
   EXPECT_EQ(path_4, renamed_path);
 
@@ -185,77 +185,8 @@ TEST_F(DownloadFileTest, RenameFileFinal) {
   EXPECT_TRUE(file_util::PathExists(path_4));
 
   // Check the hash.
-  EXPECT_TRUE(download_file_->path_renamed());
   EXPECT_TRUE(download_file_->GetSha256Hash(&hash));
   EXPECT_EQ(kDataHash, base::HexEncode(hash.data(), hash.size()));
 
   DestroyDownloadFile(&download_file_, 0);
 }
-
-// Rename the file before any data is downloaded, after some has, after it all
-// has, and after it's closed.  File is not marked as having the final rename.
-TEST_F(DownloadFileTest, RenameFileNoFinal) {
-  CreateDownloadFile(&download_file_, 1);
-  ASSERT_TRUE(download_file_->Initialize(true));
-  FilePath initial_path(download_file_->full_path());
-  EXPECT_TRUE(file_util::PathExists(initial_path));
-  FilePath path_1(initial_path.InsertBeforeExtensionASCII("_1"));
-  FilePath path_2(initial_path.InsertBeforeExtensionASCII("_2"));
-  FilePath path_3(initial_path.InsertBeforeExtensionASCII("_3"));
-  FilePath path_4(initial_path.InsertBeforeExtensionASCII("_4"));
-
-  // Rename the file before downloading any data.
-  EXPECT_TRUE(download_file_->Rename(path_1, false));
-  FilePath renamed_path = download_file_->full_path();
-  EXPECT_EQ(path_1, renamed_path);
-
-  // Check the files.
-  EXPECT_FALSE(file_util::PathExists(initial_path));
-  EXPECT_TRUE(file_util::PathExists(path_1));
-
-  // Download the data.
-  AppendDataToFile(&download_file_, kTestData1);
-  AppendDataToFile(&download_file_, kTestData2);
-
-  // Rename the file after downloading some data.
-  EXPECT_TRUE(download_file_->Rename(path_2, false));
-  renamed_path = download_file_->full_path();
-  EXPECT_EQ(path_2, renamed_path);
-
-  // Check the files.
-  EXPECT_FALSE(file_util::PathExists(path_1));
-  EXPECT_TRUE(file_util::PathExists(path_2));
-
-  AppendDataToFile(&download_file_, kTestData3);
-
-  // Rename the file after downloading all the data.
-  EXPECT_TRUE(download_file_->Rename(path_3, false));
-  renamed_path = download_file_->full_path();
-  EXPECT_EQ(path_3, renamed_path);
-
-  // Check the files.
-  EXPECT_FALSE(file_util::PathExists(path_2));
-  EXPECT_TRUE(file_util::PathExists(path_3));
-
-  // Should not be able to get the hash until the file is closed.
-  std::string hash;
-  EXPECT_FALSE(download_file_->GetSha256Hash(&hash));
-
-  download_file_->Finish();
-
-  // Rename the file after downloading all the data and closing the file.
-  EXPECT_TRUE(download_file_->Rename(path_4, false));
-  renamed_path = download_file_->full_path();
-  EXPECT_EQ(path_4, renamed_path);
-
-  // Check the files.
-  EXPECT_FALSE(file_util::PathExists(path_3));
-  EXPECT_TRUE(file_util::PathExists(path_4));
-
-  // Check the hash.
-  EXPECT_FALSE(download_file_->path_renamed());
-  EXPECT_TRUE(download_file_->GetSha256Hash(&hash));
-  EXPECT_EQ(kDataHash, base::HexEncode(hash.data(), hash.size()));
-
-  DestroyDownloadFile(&download_file_, 1);
-}

chrome/browser/download/download_manager_unittest.cc

@@ -178,15 +178,14 @@ class MockDownloadFile : public DownloadFile {
   explicit MockDownloadFile(DownloadCreateInfo* info)
       : DownloadFile(info, NULL), renamed_count_(0) { }
   virtual ~MockDownloadFile() { Destructed(); }
-  MOCK_METHOD2(Rename, bool(const FilePath&, bool));
+  MOCK_METHOD1(Rename, bool(const FilePath&));
   MOCK_METHOD0(Destructed, void());
 
   bool TestMultipleRename(
-      int expected_count, bool expected_final, const FilePath& expected,
-      const FilePath& path, bool is_final_rename) {
+      int expected_count, const FilePath& expected,
+      const FilePath& path) {
     ++renamed_count_;
     EXPECT_EQ(expected_count, renamed_count_);
-    EXPECT_EQ(expected_final, is_final_rename);
     EXPECT_EQ(expected.value(), path.value());
     return true;
   }
@@ -221,19 +220,18 @@ TEST_F(DownloadManagerTest, DownloadRenameTest) {
     EXPECT_CALL(*download, Destructed()).Times(1);
 
     if (kDownloadRenameCases[i].expected_rename_count == 1) {
-      EXPECT_CALL(*download, Rename(new_path, true)).WillOnce(Return(true));
+      EXPECT_CALL(*download, Rename(new_path)).WillOnce(Return(true));
     } else {
       ASSERT_EQ(2, kDownloadRenameCases[i].expected_rename_count);
       FilePath crdownload(download_util::GetCrDownloadPath(new_path));
-      EXPECT_CALL(*download, Rename(_, _))
-          .WillOnce(testing::WithArgs<0, 1>(Invoke(CreateFunctor(
+      EXPECT_CALL(*download, Rename(_))
+          .WillOnce(testing::WithArgs<0>(Invoke(CreateFunctor(
               download, &MockDownloadFile::TestMultipleRename,
-              1, false, crdownload))))
-          .WillOnce(testing::WithArgs<0, 1>(Invoke(CreateFunctor(
+              1, crdownload))))
+          .WillOnce(testing::WithArgs<0>(Invoke(CreateFunctor(
               download, &MockDownloadFile::TestMultipleRename,
-              2, true, new_path))));
+              2, new_path))));
     }
-
     download_manager_->CreateDownloadItem(info);
 
     if (kDownloadRenameCases[i].finish_before_rename) {

chrome/browser/download/save_file_manager.cc

@@ -432,7 +432,6 @@ void SaveFileManager::SaveLocalFile(const GURL& original_file_url,
   SaveFile* save_file = LookupSaveFile(save_id);
   if (!save_file)
     return;
-  DCHECK(!save_file->path_renamed());
   // If it has finished, just return.
   if (!save_file->in_progress())
     return;
@@ -490,7 +489,7 @@ void SaveFileManager::RenameAllFiles(
     if (it != save_file_map_.end()) {
       SaveFile* save_file = it->second;
       DCHECK(!save_file->in_progress());
-      save_file->Rename(i->second, true);
+      save_file->Rename(i->second);
       delete save_file;
       save_file_map_.erase(it);
     }