DownloadItem::NeedsRename should not return true for dangerous downloads.

Dangerous downloads always have different target_name from its full_path.BaseName().  Returning true from NeedsRename() wrongly makes DownloadManager::DownloadRenamedToFinalName() to call ContinueDownloadFinished() while it's not finished yet.

Renamed DownloadItem::NeedsRename() to NeedsRenameForSafeDownload() to avoid further confusion.

BUG=62398
TEST=downloads.DownloadsTest.testNoUnsafeDownloadsOnRestart

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

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

5 files changed, 51 insertions, 36 deletions

diff --git a/chrome/browser/download/download_item.cc b/chrome/browser/download/download_item.cc
index 7b4b134..3069e94 100644
--- a/chrome/browser/download/download_item.cc
+++ b/chrome/browser/download/download_item.cc
@@ -11,6 +11,7 @@
 #include "base/utf_string_conversions.h"
 #include "net/base/net_util.h"
 #include "chrome/browser/browser_thread.h"
+#include "chrome/browser/download/download_file_manager.h"
 #include "chrome/browser/download/download_history.h"
 #include "chrome/browser/download/download_manager.h"
 #include "chrome/browser/download/download_prefs.h"
@@ -359,6 +360,34 @@ void DownloadItem::OnNameFinalized() {
     NotifyObserversDownloadFileCompleted();
 }
 
+void DownloadItem::OnSafeDownloadFinished(DownloadFileManager* file_manager) {
+  DCHECK_EQ(SAFE, safety_state());
+  DCHECK(file_manager);
+  if (NeedsRename()) {
+    BrowserThread::PostTask(
+        BrowserThread::FILE, FROM_HERE,
+        NewRunnableMethod(
+            file_manager, &DownloadFileManager::OnFinalDownloadName,
+            id(), GetTargetFilePath(), false, download_manager_));
+    return;
+  }
+
+  download_manager_->DownloadFinished(this);
+}
+
+void DownloadItem::OnDownloadRenamedToFinalName(const FilePath& full_path) {
+  bool needed_rename = NeedsRename();
+
+  Rename(full_path);
+  OnNameFinalized();
+
+  if (needed_rename && safety_state() == SAFE) {
+    // This was called from OnSafeDownloadFinished; continue to call
+    // DownloadFinished.
+    download_manager_->DownloadFinished(this);
+  }
+}
+
 bool DownloadItem::MatchesQuery(const string16& query) const {
   if (query.empty())
     return true;
diff --git a/chrome/browser/download/download_item.h b/chrome/browser/download/download_item.h
index 4bc2b39..a9b736e 100644
--- a/chrome/browser/download/download_item.h
+++ b/chrome/browser/download/download_item.h
@@ -27,6 +27,7 @@
 #include "base/timer.h"
 #include "googleurl/src/gurl.h"
 
+class DownloadFileManager;
 class DownloadManager;
 struct DownloadCreateInfo;
 
@@ -159,6 +160,14 @@ class DownloadItem {
   // Called when the name of the download is finalized.
   void OnNameFinalized();
 
+  // Called when the download is finished for safe downloads.
+  // This may perform final rename if necessary and will eventually call
+  // DownloadManager::DownloadFinished().
+  void OnSafeDownloadFinished(DownloadFileManager* file_manager);
+
+  // Called when the file name for the download is renamed to its final name.
+  void OnDownloadRenamedToFinalName(const FilePath& full_path);
+
   // Returns true if this item matches |query|. |query| must be lower-cased.
   bool MatchesQuery(const string16& query) const;
 
diff --git a/chrome/browser/download/download_manager.cc b/chrome/browser/download/download_manager.cc
index 59476b47..5100236 100644
--- a/chrome/browser/download/download_manager.cc
+++ b/chrome/browser/download/download_manager.cc
@@ -531,16 +531,7 @@ void DownloadManager::OnAllDataSaved(int32 download_id, int64 size) {
     return;
   }
 
-  if (download->NeedsRename()) {
-    BrowserThread::PostTask(
-        BrowserThread::FILE, FROM_HERE,
-        NewRunnableMethod(
-            file_manager_, &DownloadFileManager::OnFinalDownloadName,
-            download->id(), download->GetTargetFilePath(), false, this));
-    return;
-  }
-
-  ContinueDownloadFinished(download);
+  download->OnSafeDownloadFinished(file_manager_);
 }
 
 void DownloadManager::DownloadRenamedToFinalName(int download_id,
@@ -549,20 +540,10 @@ void DownloadManager::DownloadRenamedToFinalName(int download_id,
   DownloadItem* item = GetDownloadItem(download_id);
   if (!item)
     return;
-
-  bool needed_rename = item->NeedsRename();
-  item->Rename(full_path);
-
-  item->OnNameFinalized();
-
-  if (needed_rename) {
-    // This was called from OnAllDataSaved; continue to call
-    // ContinueDownloadFinished.
-    ContinueDownloadFinished(item);
-  }
+  item->OnDownloadRenamedToFinalName(full_path);
 }
 
-void DownloadManager::ContinueDownloadFinished(DownloadItem* download) {
+void DownloadManager::DownloadFinished(DownloadItem* download) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   // If this was a dangerous download, it has now been approved and must be
@@ -622,7 +603,7 @@ void DownloadManager::DangerousDownloadRenamed(int64 download_handle,
   }
 
   // Continue the download finished sequence.
-  ContinueDownloadFinished(download);
+  DownloadFinished(download);
 }
 
 void DownloadManager::DownloadCancelled(int32 download_id) {
diff --git a/chrome/browser/download/download_manager.h b/chrome/browser/download/download_manager.h
index 6608af2..2d8bf15 100644
--- a/chrome/browser/download/download_manager.h
+++ b/chrome/browser/download/download_manager.h
@@ -204,6 +204,13 @@ class DownloadManager
   // Called when the user has validated the download of a dangerous file.
   void DangerousDownloadValidated(DownloadItem* download);
 
+  // Performs the last steps required when a download has been completed.
+  // It is necessary to break down the flow when a download is finished as
+  // dangerous downloads are downloaded to temporary files that need to be
+  // renamed on the file thread first.
+  // Invoked on the UI thread.
+  void DownloadFinished(DownloadItem* download);
+
  private:
   // This class is used to let an incognito DownloadManager observe changes to
   // a normal DownloadManager, to propagate ModelChanged() calls from the parent
@@ -253,13 +260,6 @@ class DownloadManager
                                  int render_process_id,
                                  int request_id);
 
-  // Performs the last steps required when a download has been completed.
-  // It is necessary to break down the flow when a download is finished as
-  // dangerous downloads are downloaded to temporary files that need to be
-  // renamed on the file thread first.
-  // Invoked on the UI thread.
-  void ContinueDownloadFinished(DownloadItem* download);
-
   // Renames a finished dangerous download from its temporary file name to its
   // real file name.
   // Invoked on the file thread.
@@ -353,7 +353,7 @@ class DownloadManager
   // destination, so that observers are appropriately notified of completion
   // after this determination is made.
   // The map is of download_id->remaining size (bytes), both of which are
-  // required when calling DownloadFinished.
+  // required when calling OnAllDataSaved.
   typedef std::map<int32, int64> PendingFinishedMap;
   PendingFinishedMap pending_finished_downloads_;
 
diff --git a/chrome/test/functional/PYAUTO_TESTS b/chrome/test/functional/PYAUTO_TESTS
index 91b13bc..5f4d2a1 100644
--- a/chrome/test/functional/PYAUTO_TESTS
+++ b/chrome/test/functional/PYAUTO_TESTS
@@ -31,8 +31,6 @@
     'cookies',
     'crash_reporter',
     'downloads',
-    # crbug.com/62398. Also look in the chromeos section while removing this.
-    '-downloads.DownloadsTest.testNoUnsafeDownloadsOnRestart',
     'find_in_page',
     # Turkish I problem. crbug.com/60638
     '-find_in_page.FindMatchTests.testLocalizationAndCaseOrder',
@@ -108,9 +106,7 @@
     '-prefs.PrefsTest.testSessionRestoreURLs',
     '-prefs.PrefsTest.testSessionRestore',
     '-translate.TranslateTest.testSessionRestore',
-    # Uncomment the next line when the suppression for bug 62398 (above)
-    # is removed.
-    # '-downloads.DownloadsTest.testNoUnsafeDownloadsOnRestart',
+    '-downloads.DownloadsTest.testNoUnsafeDownloadsOnRestart',
     '-downloads.DownloadsTest.testZip',
     '-downloads.DownloadsTest.testZipInIncognito',
     '-downloads.DownloadsTest.testCrazyFilenames',