[Resumption 9/11] Handle filename determination for resumed downloads.

chrome/browser/download/download_target_determiner.cc

Index: chrome/browser/download/download_target_determiner.cc
diff --git a/chrome/browser/download/download_target_determiner.cc b/chrome/browser/download/download_target_determiner.cc
index caf1083c83c2e3415d404b55ec3a3fb7f0ebd39d..f7b2a244d4d1756802b5a37231fdaf12983092c8 100644
--- a/chrome/browser/download/download_target_determiner.cc
+++ b/chrome/browser/download/download_target_determiner.cc
@@ -22,6 +22,7 @@
 #include "chrome/common/pref_names.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/browser_thread.h"
+#include "content/public/browser/download_interrupt_reasons.h"
 #include "grit/generated_resources.h"
 #include "net/base/net_util.h"
 #include "ui/base/l10n/l10n_util.h"
@@ -58,6 +59,7 @@ DownloadTargetDeterminerDelegate::~DownloadTargetDeterminerDelegate() {
 
 DownloadTargetDeterminer::DownloadTargetDeterminer(
     DownloadItem* download,
+    const base::FilePath& initial_virtual_path,
     DownloadPrefs* download_prefs,
     const base::FilePath& last_selected_directory,
     DownloadTargetDeterminerDelegate* delegate,
@@ -69,7 +71,11 @@ DownloadTargetDeterminer::DownloadTargetDeterminer(
                        DownloadPathReservationTracker::UNIQUIFY :
                        DownloadPathReservationTracker::OVERWRITE),
       danger_type_(download->GetDangerType()),
+      virtual_path_(initial_virtual_path),
       download_(download),
+      is_resumption_(download_->GetLastReason() !=
+                     content::DOWNLOAD_INTERRUPT_REASON_NONE &&

[Randy Smith (Not in Mondays), 2013/05/15 19:02:13]

This is relying on an aspect of the API that I don't think is intuitive, which
is that GetLastReason() persists through transitions into IN_PROGRESS.  In fact,
the doc on that in download_item.h indicates that it's only valid if the
download is interrupted.  Could you update the doc?

[asanka, 2013/05/23 20:30:37]

Updated.

+                     !initial_virtual_path.empty()),
       download_prefs_(download_prefs),
       delegate_(delegate),
       last_selected_directory_(last_selected_directory),
@@ -137,18 +143,32 @@ void DownloadTargetDeterminer::DoLoop() {
 DownloadTargetDeterminer::Result
     DownloadTargetDeterminer::DoGenerateTargetPath() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  DCHECK(virtual_path_.empty());
   DCHECK(local_path_.empty());
   bool is_forced_path = !download_->GetForcedFilePath().empty();
 
   next_state_ = STATE_NOTIFY_EXTENSIONS;
 
-  // If we don't have a forced path, we should construct a path for the
-  // download. Forced paths are only specified for programmatic downloads
-  // (WebStore, Drag&Drop). Treat the path as a virtual path. We will eventually
-  // determine whether this is a local path and if not, figure out a local path.
-  if (!is_forced_path) {
-   std::string default_filename(
+  if (!virtual_path_.empty()) {
+    DCHECK(!is_forced_path || virtual_path_ == download_->GetForcedFilePath());
+    DCHECK(!should_prompt_);
+    // The download is being resumed and virtual_path_ is the path that was
+    // determined prior to the interruption. If the path was selected by the
+    // user or if is a forced path, then assume that it's okay to overwrite the
+    // target.
+    // Note: |virtual_path_| may already be uniquified. In this case setting
+    // conflict_action_ to UNIQUIFY may cause additional levels of uniquifiers
+    // (e.g. "foo (1) (1).txt").

[Randy Smith (Not in Mondays), 2013/05/15 19:00:18]

I consider this sub-ideal.  Are there other options?  A simple thought would be
that if we haven't previously prompted the user, we just discard the old name
and rederive a name.

[asanka, 2013/05/23 20:30:37]

Yeah. I think that's better as well. Done.

+    conflict_action_ = (is_forced_path || HasPromptedForPath())
+        ? DownloadPathReservationTracker::OVERWRITE
+        : DownloadPathReservationTracker::PROMPT;
+    should_prompt_ = ShouldPromptForDownload(virtual_path_);
+  } else if (!is_forced_path) {
+    // If we don't have a forced path, we should construct a path for the
+    // download. Forced paths are only specified for programmatic downloads
+    // (WebStore, Drag&Drop). Treat the path as a virtual path. We will
+    // eventually determine whether this is a local path and if not, figure out
+    // a local path.
+    std::string default_filename(
         l10n_util::GetStringUTF8(IDS_DEFAULT_DOWNLOAD_FILENAME));
     base::FilePath generated_filename = net::GenerateFileName(
         download_->GetURL(),
@@ -195,8 +215,10 @@ DownloadTargetDeterminer::Result
   next_state_ = STATE_RESERVE_VIRTUAL_PATH;
 
   // If the target path is forced or if we don't have an extensions event
-  // router, then proceed with the original path.
-  if (!download_->GetForcedFilePath().empty())
+  // router, then proceed with the original path. Also don't notify extensions
+  // for a resumed download since extensions would have been notified once
+  // already.
+  if (!download_->GetForcedFilePath().empty() || is_resumption_)
     return CONTINUE;
 
   delegate_->NotifyExtensions(download_, virtual_path_,
@@ -348,7 +370,7 @@ DownloadTargetDeterminer::Result
   // where the download has already been deemed dangerous, or where the user is
   // going to be prompted or where this is a programmatic download.
   if (danger_type_ != content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS ||
-      should_prompt_ ||
+      HasPromptedForPath() ||
       !download_->GetForcedFilePath().empty()) {
     return CONTINUE;
   }
@@ -398,6 +420,8 @@ DownloadTargetDeterminer::Result
   DCHECK(!virtual_path_.empty());
   DCHECK(!local_path_.empty());
   DCHECK(intermediate_path_.empty());
+  DCHECK(!virtual_path_.MatchesExtension(FILE_PATH_LITERAL(".crdownload")));
+  DCHECK(!local_path_.MatchesExtension(FILE_PATH_LITERAL(".crdownload")));
 
   next_state_ = STATE_NONE;
 
@@ -464,8 +488,9 @@ void DownloadTargetDeterminer::ScheduleCallbackAndDeleteSelf() {
       FROM_HERE,
       base::Bind(completion_callback_,
                  local_path_,
-                 (should_prompt_ ? DownloadItem::TARGET_DISPOSITION_PROMPT :
-                                   DownloadItem::TARGET_DISPOSITION_OVERWRITE),
+                 (HasPromptedForPath()
+                  ? DownloadItem::TARGET_DISPOSITION_PROMPT
+                  : DownloadItem::TARGET_DISPOSITION_OVERWRITE),
                  danger_type_,
                  intermediate_path_));
   completion_callback_.Reset();
@@ -486,12 +511,23 @@ Profile* DownloadTargetDeterminer::GetProfile() {
 }
 
 bool DownloadTargetDeterminer::ShouldPromptForDownload(
-    const base::FilePath& filename) {
+    const base::FilePath& filename) const {
+  if (is_resumption_) {
+    // If the target disposition or prefs require prompting, the user has
+    // already been prompted. Try to respect the user's selection, unless we've
+    // discovered that the target path cannot be used for some reason.
+    content::DownloadInterruptReason reason = download_->GetLastReason();
+    return (reason == content::DOWNLOAD_INTERRUPT_REASON_FILE_ACCESS_DENIED ||
+            reason == content::DOWNLOAD_INTERRUPT_REASON_FILE_NO_SPACE ||
+            reason == content::DOWNLOAD_INTERRUPT_REASON_FILE_TOO_LARGE);
+  }
+
   // If the download path is forced, don't prompt.
   if (!download_->GetForcedFilePath().empty()) {
     // 'Save As' downloads shouldn't have a forced path.
-    DCHECK_NE(DownloadItem::TARGET_DISPOSITION_PROMPT,
-              download_->GetTargetDisposition());
+    DCHECK(is_resumption_ ||
+           (DownloadItem::TARGET_DISPOSITION_PROMPT !=
+            download_->GetTargetDisposition()));
     return false;
   }
 
@@ -526,6 +562,12 @@ bool DownloadTargetDeterminer::ShouldPromptForDownload(
   return false;
 }
 
+bool DownloadTargetDeterminer::HasPromptedForPath() const {
+  return should_prompt_ ||
+      (is_resumption_ && download_->GetTargetDisposition() ==
+       DownloadItem::TARGET_DISPOSITION_PROMPT);
+}
+
 bool DownloadTargetDeterminer::IsDangerousFile(PriorVisitsToReferrer visits) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   const bool is_extension_download =
@@ -584,6 +626,7 @@ void DownloadTargetDeterminer::OnDownloadDestroyed(
 // static
 void DownloadTargetDeterminer::Start(
     content::DownloadItem* download,
+    const base::FilePath& initial_virtual_path,
     DownloadPrefs* download_prefs,
     const base::FilePath& last_selected_directory,
     DownloadTargetDeterminerDelegate* delegate,
@@ -591,7 +634,7 @@ void DownloadTargetDeterminer::Start(
   // DownloadTargetDeterminer owns itself and will self destruct when the job is
   // complete or the download item is destroyed. The callback is always invoked
   // asynchronously.
-  new DownloadTargetDeterminer(download, download_prefs,
+  new DownloadTargetDeterminer(download, initial_virtual_path, download_prefs,
                                last_selected_directory, delegate, callback);
 }