Download filename determination refactor (3/3)

chrome/browser/download/chrome_download_manager_delegate.cc

Index: chrome/browser/download/chrome_download_manager_delegate.cc
diff --git a/chrome/browser/download/chrome_download_manager_delegate.cc b/chrome/browser/download/chrome_download_manager_delegate.cc
index 9a7f18ee80c9b598105018017b320f4f160acfd5..38458ab6b77a8d18a6f67d43d598553d17d8a455 100644
--- a/chrome/browser/download/chrome_download_manager_delegate.cc
+++ b/chrome/browser/download/chrome_download_manager_delegate.cc
@@ -135,19 +135,9 @@ DownloadId ChromeDownloadManagerDelegate::GetNextId() {
       GetDelegate()->GetNextId();
 }
 
-bool ChromeDownloadManagerDelegate::ShouldStartDownload(int32 download_id) {
-  // We create a download item and store it in our download map, and inform the
-  // history system of a new download. Since this method can be called while the
-  // history service thread is still reading the persistent state, we do not
-  // insert the new DownloadItem into 'history_downloads_' or inform our
-  // observers at this point. OnCreateDownloadEntryComplete() handles that
-  // finalization of the the download creation as a callback from the history
-  // thread.
-  DownloadItem* download =
-      download_manager_->GetActiveDownloadItem(download_id);
-  if (!download)
-    return false;
-
+bool ChromeDownloadManagerDelegate::DetermineDownloadTarget(
+    DownloadItem* download,
+    const content::DownloadTargetCallback& callback) {
 #if defined(ENABLE_SAFE_BROWSING)
   DownloadProtectionService* service = GetDownloadProtectionService();
   if (service) {
@@ -158,15 +148,20 @@ bool ChromeDownloadManagerDelegate::ShouldStartDownload(int32 download_id) {
         base::Bind(
             &ChromeDownloadManagerDelegate::CheckDownloadUrlDone,
             this,
-            download->GetId()));
-    return false;
+            download->GetId(),
+            callback));
+    return true;
   }
 #endif
-  CheckDownloadUrlDone(download_id, DownloadProtectionService::SAFE);
-  return false;
+  CheckDownloadUrlDone(download->GetId(), callback,
+                       DownloadProtectionService::SAFE);
+  return true;
 }
 
-void ChromeDownloadManagerDelegate::ChooseDownloadPath(DownloadItem* item) {
+void ChromeDownloadManagerDelegate::ChooseDownloadPath(
+    DownloadItem* item,
+    const FilePath& suggested_path,
+    const base::Callback<void(const FilePath&)>& file_selected_callback) {
   // Deletes itself.
   DownloadFilePicker* file_picker =
 #if defined(OS_CHROMEOS)
@@ -174,20 +169,22 @@ void ChromeDownloadManagerDelegate::ChooseDownloadPath(DownloadItem* item) {
 #else
       new DownloadFilePicker();
 #endif
-  file_picker->Init(download_manager_, item);
+  file_picker->Init(download_manager_, item, suggested_path,
+                    file_selected_callback);
 }
 
 FilePath ChromeDownloadManagerDelegate::GetIntermediatePath(
-    const DownloadItem& download) {
+    const FilePath& target_path,
+    content::DownloadDangerType danger_type) {
   // If the download is not dangerous, just append .crdownload to the target
   // path.
-  if (download.GetDangerType() == content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS)
-    return download_util::GetCrDownloadPath(download.GetTargetFilePath());
+  if (danger_type == content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS)
+    return download_util::GetCrDownloadPath(target_path);
 
   // If the download is potentially dangerous we create a filename of the form
   // 'Unconfirmed <random>.crdownload'.
   FilePath::StringType file_name;
-  FilePath dir = download.GetTargetFilePath().DirName();
+  FilePath dir = target_path.DirName();
 #if defined(OS_WIN)
   string16 unconfirmed_prefix =
       l10n_util::GetStringUTF16(IDS_DOWNLOAD_UNCONFIRMED_PREFIX);
@@ -480,6 +477,10 @@ void ChromeDownloadManagerDelegate::ChooseSavePath(
 #endif
 }
 
+void ChromeDownloadManagerDelegate::ClearLastDownloadPath() {
+  last_download_path_.clear();
+}
+
 DownloadProtectionService*
     ChromeDownloadManagerDelegate::GetDownloadProtectionService() {
 #if defined(ENABLE_SAFE_BROWSING)
@@ -532,7 +533,7 @@ void ChromeDownloadManagerDelegate::GetReservedPath(
     const FilePath& target_path,
     const FilePath& default_download_path,
     bool should_uniquify_path,
-    const DownloadPathReservationTracker::ReservedPathCallback callback) {
+    const DownloadPathReservationTracker::ReservedPathCallback& callback) {
   DownloadPathReservationTracker::GetReservedPath(
       download, target_path, default_download_path, should_uniquify_path,
       callback);
@@ -540,6 +541,7 @@ void ChromeDownloadManagerDelegate::GetReservedPath(
 
 void ChromeDownloadManagerDelegate::CheckDownloadUrlDone(
     int32 download_id,
+    const content::DownloadTargetCallback& callback,
     DownloadProtectionService::DownloadCheckResult result) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DownloadItem* download =
@@ -556,7 +558,7 @@ void ChromeDownloadManagerDelegate::CheckDownloadUrlDone(
   download_history_->CheckVisitedReferrerBefore(
       download_id, download->GetReferrerUrl(),
       base::Bind(&ChromeDownloadManagerDelegate::CheckVisitedReferrerBeforeDone,
-                 base::Unretained(this), download_id, danger_type));
+                 base::Unretained(this), download_id, callback, danger_type));
 }
 
 void ChromeDownloadManagerDelegate::CheckClientDownloadDone(
@@ -614,6 +616,7 @@ void ChromeDownloadManagerDelegate::Observe(
 
 void ChromeDownloadManagerDelegate::CheckVisitedReferrerBeforeDone(
     int32 download_id,
+    const content::DownloadTargetCallback& callback,
     content::DownloadDangerType danger_type,
     bool visited_referrer_before) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
@@ -655,8 +658,8 @@ void ChromeDownloadManagerDelegate::CheckVisitedReferrerBeforeDone(
     // 1) using the default download directory.
     // 2) prompting the user.
     FilePath target_directory;
-    if (should_prompt && !download_manager_->LastDownloadPath().empty())
-      target_directory = download_manager_->LastDownloadPath();
+    if (should_prompt && !last_download_path_.empty())
+      target_directory = last_download_path_;
     else
       target_directory = download_prefs_->download_path();
     suggested_path = target_directory.Append(generated_name);
@@ -699,19 +702,23 @@ void ChromeDownloadManagerDelegate::CheckVisitedReferrerBeforeDone(
       profile_, suggested_path, download,
       base::Bind(
           &ChromeDownloadManagerDelegate::SubstituteGDataDownloadPathCallback,
-          this, download->GetId(), should_prompt, is_forced_path, danger_type));
+          this, download->GetId(), callback, should_prompt, is_forced_path,
+          danger_type));
 #else
   GetReservedPath(
       *download, suggested_path, download_prefs_->download_path(),
       !is_forced_path,
       base::Bind(&ChromeDownloadManagerDelegate::OnPathReservationAvailable,
-                 this, download->GetId(), should_prompt, danger_type));
+                 this, download->GetId(), callback, should_prompt,
+                 danger_type));
 #endif
 }
 
 #if defined (OS_CHROMEOS)
+// TODO(asanka): Merge this logic with the logic in DownloadFilePickerChromeOS.

[Randy Smith (Not in Mondays), 2012/07/12 17:01:22]

Probably worth pulling Achuith into the review at some point, to keep him
updated and in case he feels like taking this one.

[asanka, 2012/07/18 21:50:56]

Yeah. Hopefully we can get it down to a single GData path substitution instead
of two for a single download. The code change seems non-trivial so I wanted to
do it (or someone else should) on a separate CL.

 void ChromeDownloadManagerDelegate::SubstituteGDataDownloadPathCallback(
     int32 download_id,
+    const content::DownloadTargetCallback& callback,
     bool should_prompt,
     bool is_forced_path,
     content::DownloadDangerType danger_type,
@@ -726,30 +733,70 @@ void ChromeDownloadManagerDelegate::SubstituteGDataDownloadPathCallback(
       *download, suggested_path, download_prefs_->download_path(),
       !is_forced_path,
       base::Bind(&ChromeDownloadManagerDelegate::OnPathReservationAvailable,
-                 this, download->GetId(), should_prompt, danger_type));
+                 this, download->GetId(), callback, should_prompt,
+                 danger_type));
 }
 #endif
 
 void ChromeDownloadManagerDelegate::OnPathReservationAvailable(
     int32 download_id,
+    const content::DownloadTargetCallback& callback,
     bool should_prompt,
     content::DownloadDangerType danger_type,
-    const FilePath& target_path,
-    bool target_path_verified) {
+    const FilePath& reserved_path,
+    bool reserved_path_verified) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DownloadItem* download =
       download_manager_->GetActiveDownloadItem(download_id);
   if (!download)
     return;
-  DownloadItem::TargetDisposition disposition;
-  // If the target path could not be verified then the path was non-existant,
-  // non writeable or could not be uniquified. Prompt the user.
-  if (should_prompt || !target_path_verified)
-    disposition = DownloadItem::TARGET_DISPOSITION_PROMPT;
-  else
-    disposition = DownloadItem::TARGET_DISPOSITION_OVERWRITE;
-  download->OnTargetPathDetermined(target_path, disposition, danger_type);
-  download_manager_->RestartDownload(download_id);
+  if (should_prompt || !reserved_path_verified) {
+    // If the target path could not be verified then the path was non-existant,
+    // non writeable or could not be uniquified. Prompt the user.
+    ChooseDownloadPath(
+        download, reserved_path,
+        base::Bind(&ChromeDownloadManagerDelegate::OnTargetPathDetermined,
+                   this, download_id, callback,
+                   DownloadItem::TARGET_DISPOSITION_PROMPT, danger_type));
+  } else {
+    OnTargetPathDetermined(download_id, callback,
+                           DownloadItem::TARGET_DISPOSITION_OVERWRITE,
+                           danger_type, reserved_path);
+  }
+}
+
+void ChromeDownloadManagerDelegate::OnTargetPathDetermined(
+    int32 download_id,
+    const content::DownloadTargetCallback& callback,
+    DownloadItem::TargetDisposition disposition,
+    content::DownloadDangerType danger_type,
+    const FilePath& target_path) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DownloadItem* download =
+      download_manager_->GetActiveDownloadItem(download_id);
+  if (!download)
+    return;
+
+  // If |target_path| is empty, then that means that the user wants to cancel
+  // the download.
+  if (target_path.empty()) {
+    DCHECK_EQ(DownloadItem::TARGET_DISPOSITION_PROMPT, disposition);
+    download->Cancel(true);
+    return;
+  }
+
+  // Retain the last directory. Exclude temporary downloads since the path
+  // likely points at the location of a temporary file.
+  // TODO(asanka): This logic is a hack. DownloadFilePicker should give us a
+  //               directory to persist. Or perhaps, if the GData path
+  //               substitution logic is moved here, then we would have a
+  //               persistable path after the DownloadFilePicker is done.

[Randy Smith (Not in Mondays), 2012/07/12 17:01:22]

This is only for GData, right?  It should go away after we do the
DownloadDetermination refactor.

[asanka, 2012/07/18 21:50:56]

Yeah. I didn't want to do it here since such a fix would become redundant once
the GData path substitution is refactored.

+  if (disposition == DownloadItem::TARGET_DISPOSITION_PROMPT &&
+      !download->IsTemporary())
+    last_download_path_ = target_path.DirName();
+
+  FilePath intermediate_path = GetIntermediatePath(target_path, danger_type);
+  callback.Run(target_path, disposition, danger_type, intermediate_path);
 }
 
 void ChromeDownloadManagerDelegate::OnItemAddedToPersistentStore(