Make "Save As" control flow play better with safe browsing checks.

                                                                                 
When safe browsing is enabled, downloads that will be subject to                 
content based checked will be marked as non-safe even when we are                
prompting the user for the download location.  During filename                   
determination |DownloadItem::suggested_path_| will be set to an                  
intermediate filename for these downloads.  For safe downloads,                  
|suggested_path_| is the final download path.                                    
                                                                                 
This patch:                                                                      
                                                                                 
- Changes target filename determination when prompting for the                   
  download location in DownloadManagerImpl::RestartDownload() to use             
  the |target_name| instead of |suggested_path_| for non-safe                    
  downloads.                                                                     
                                                                                 
- Allows the target filename to be altered in                                    
  DownloadItemImpl::OnPathDetermined after prompting.                            
                                                                                 
- Uses the previously determined intermediate filename for non-safe              
  downloads if prompted in                                                       
  ChromeDownloadManagerDelegate::OverrideIntermediatePath.                       
                                                                                 
- Only uniquifies the target filename if it is a non-safe download for           
  which no file selection prompts have been shown, in                            
  DownloadItemImpl::OnDownloadCompleting.                                        
                                                                                 
BUG=106194                                                                       
TEST=unit_tests --gtest_filter=DownloadManagerTest.*


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

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

2 files changed, 31 insertions, 7 deletions

diff --git a/content/browser/download/download_item_impl.cc b/content/browser/download/download_item_impl.cc
index 23d2092..55d25a2 100644
--- a/content/browser/download/download_item_impl.cc
+++ b/content/browser/download/download_item_impl.cc
@@ -571,6 +571,10 @@ int DownloadItemImpl::PercentComplete() const {
 
 void DownloadItemImpl::OnPathDetermined(const FilePath& path) {
   full_path_ = path;
+  // If we prompted the user, then target_name is stale.  Allow it to be
+  // populated by UpdateTarget().
+  if (PromptUserForSaveLocation())
+    state_info_.target_name.clear();
   UpdateTarget();
 }
 
@@ -608,11 +612,16 @@ void DownloadItemImpl::OnDownloadCompleting(DownloadFileManager* file_manager) {
   DCHECK_NE(DANGEROUS, GetSafetyState());
   DCHECK(file_manager);
 
+  // If we prompted the user for save location, then we should overwrite the
+  // target.  Otherwise, if the danger state was NOT_DANGEROUS, we already
+  // uniquified the path and should overwrite.
+  bool should_overwrite = (PromptUserForSaveLocation() ||
+                           GetDangerType() == DownloadStateInfo::NOT_DANGEROUS);
   if (NeedsRename()) {
     BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
         base::Bind(&DownloadFileManager::RenameCompletingDownloadFile,
                    file_manager, download_id_,
-                   GetTargetFilePath(), GetSafetyState() == SAFE));
+                   GetTargetFilePath(), should_overwrite));
     return;
   }
 
diff --git a/content/browser/download/download_manager_impl.cc b/content/browser/download/download_manager_impl.cc
index 6045f1d..257de76 100644
--- a/content/browser/download/download_manager_impl.cc
+++ b/content/browser/download/download_manager_impl.cc
@@ -313,10 +313,17 @@ void DownloadManagerImpl::RestartDownload(
     // FileSelectionCancelled().
     int32* id_ptr = new int32;
     *id_ptr = download_id;
-
-    delegate_->ChooseDownloadPath(
-        contents, suggested_path, reinterpret_cast<void*>(id_ptr));
-
+    FilePath target_path;
+    // If |download| is a potentially dangerous file, then |suggested_path|
+    // contains the intermediate name instead of the final download
+    // filename. The latter is GetTargetName().
+    if (download->GetDangerType() != DownloadStateInfo::NOT_DANGEROUS)
+      target_path = suggested_path.DirName().Append(download->GetTargetName());
+    else
+      target_path = suggested_path;
+
+    delegate_->ChooseDownloadPath(contents, target_path,
+                                  reinterpret_cast<void*>(id_ptr));
     FOR_EACH_OBSERVER(Observer, observers_,
                       SelectFileDialogDisplayed(download_id));
   } else {
@@ -369,6 +376,11 @@ DownloadItem* DownloadManagerImpl::CreateSavePackageDownloadItem(
   return download;
 }
 
+// For non-safe downloads with no prompting, |chosen_file| is the intermediate
+// path for saving the in-progress download. The final target filename for these
+// is |download->GetTargetName()|.  For all other downloads (non-safe downloads
+// for which we have prompted for a save location, and all safe downloads),
+// |chosen_file| is the final target download path.
 void DownloadManagerImpl::ContinueDownloadWithPath(
     DownloadItem* download, const FilePath& chosen_file) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
@@ -562,8 +574,11 @@ void DownloadManagerImpl::OnDownloadRenamedToFinalName(
   if (!item)
     return;
 
-  if (item->GetSafetyState() == DownloadItem::SAFE) {
-    DCHECK_EQ(0, uniquifier) << "We should not uniquify SAFE downloads twice";
+  if (item->GetDangerType() == DownloadStateInfo::NOT_DANGEROUS ||
+      item->PromptUserForSaveLocation()) {
+    DCHECK_EQ(0, uniquifier)
+        << "We should not uniquify user supplied filenames or safe filenames "
+           "that have already been uniquified.";
   }
 
   BrowserThread::PostTask(