Use previous target path when resuming a download after crash

chrome/browser/download/download_target_determiner.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/download/download_target_determiner.h"
 
 #include "base/location.h"
 #include "base/rand_util.h"
 #include "base/single_thread_task_runner.h"
 #include "base/strings/stringprintf.h"
@@ skipping 177 matching lines @@
     DownloadTargetDeterminer::DoGenerateTargetPath() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   DCHECK(local_path_.empty());
   DCHECK(!should_prompt_);
   DCHECK(!should_notify_extensions_);
   DCHECK_EQ(DownloadPathReservationTracker::OVERWRITE, conflict_action_);
   bool is_forced_path = !download_->GetForcedFilePath().empty();
 
   next_state_ = STATE_NOTIFY_EXTENSIONS;
 
   if (!virtual_path_.empty() && HasPromptedForPath() && !is_forced_path) {

[asanka, 2017/01/12 18:58:35]

Apologies. I didn't read this CL as carefully as I should have during the first
round.

virtual_path_ here should contain the current virtual path for a resumed
download.  I.e. for the cases where GetTargetFilePath() is non-empty,
virtual_path_ would already contain the corresponding virtual path.

[fyi: for all cases except Drive downloads on Chrome OS, virtual path is the
same as the real path].

So, there should be no Chrome visible effect to the new code that's been added.
The behavior changes at the DownloadTargetDeterminer level, but Chrome should
never be invoking this with an empty virtual_path_ if there's a valid target
path.

Is that not what you are seeing? Does this CL fix the issue for you? In the
latter case we'd need to dig in and find out what's really going on.

[qinmin, 2017/01/13 01:08:24]

Based on our offline discussions, a non empty virtual_path_ means the download
is in resumption. So changed the CL to use virtual_path_ if it is provided. And
HasPromptedForPath() should only determine whether we should change the
conflict_action_

     // The download is being resumed and the user has already been prompted for
     // a path. Assume that it's okay to overwrite the file if there's a conflict
     // and reuse the selection.
     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.
+    // If the download already has a target path, use it.
+    if (!download_->GetTargetFilePath().empty()) {
+      virtual_path_ = download_->GetTargetFilePath();
+      // If the download is interrupted due to file errors, prompt user for
+      // a new path.
+      should_prompt_ = ShouldPromptForDownload(virtual_path_);
+    } else {
+      // 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 suggested_filename = download_->GetSuggestedFilename();
-    if (suggested_filename.empty() &&
-        download_->GetMimeType() == "application/x-x509-user-cert") {
-      suggested_filename = "user.crt";
+      std::string suggested_filename = download_->GetSuggestedFilename();
+      if (suggested_filename.empty() &&
+          download_->GetMimeType() == "application/x-x509-user-cert") {
+        suggested_filename = "user.crt";
+      }
+
+      std::string default_filename(
+          l10n_util::GetStringUTF8(IDS_DEFAULT_DOWNLOAD_FILENAME));
+      base::FilePath generated_filename = net::GenerateFileName(
+          download_->GetURL(),
+          download_->GetContentDisposition(),
+          GetProfile()->GetPrefs()->GetString(prefs::kDefaultCharset),
+          suggested_filename,
+          download_->GetMimeType(),
+          default_filename);
+      should_prompt_ = ShouldPromptForDownload(generated_filename);
+      base::FilePath target_directory;
+      if (should_prompt_) {
+        DCHECK(!download_prefs_->IsDownloadPathManaged());
+        // If the user is going to be prompted and the user has been prompted
+        // before, then always prefer the last directory that the user selected.
+        target_directory = download_prefs_->SaveFilePath();
+      } else {
+        target_directory = download_prefs_->DownloadPath();
+      }
+      virtual_path_ = target_directory.Append(generated_filename);
+      should_notify_extensions_ = true;
     }
-
-    std::string default_filename(
-        l10n_util::GetStringUTF8(IDS_DEFAULT_DOWNLOAD_FILENAME));
-    base::FilePath generated_filename = net::GenerateFileName(
-        download_->GetURL(),
-        download_->GetContentDisposition(),
-        GetProfile()->GetPrefs()->GetString(prefs::kDefaultCharset),
-        suggested_filename,
-        download_->GetMimeType(),
-        default_filename);
-    should_prompt_ = ShouldPromptForDownload(generated_filename);
-    base::FilePath target_directory;
-    if (should_prompt_) {
-      DCHECK(!download_prefs_->IsDownloadPathManaged());
-      // If the user is going to be prompted and the user has been prompted
-      // before, then always prefer the last directory that the user selected.
-      target_directory = download_prefs_->SaveFilePath();
-    } else {
-      target_directory = download_prefs_->DownloadPath();
-    }
-    virtual_path_ = target_directory.Append(generated_filename);
 #if defined(OS_ANDROID)
     conflict_action_ = DownloadPathReservationTracker::PROMPT;
 #else
     conflict_action_ = DownloadPathReservationTracker::UNIQUIFY;
 #endif
-    should_notify_extensions_ = true;
   } else {
     virtual_path_ = download_->GetForcedFilePath();
     // If this is a resumed download which was previously interrupted due to an
     // issue with the forced path, the user is still not prompted. If the path
     // supplied to a programmatic download is invalid, then the caller needs to
     // intervene.
   }
   DCHECK(virtual_path_.IsAbsolute());
   DVLOG(20) << "Generated virtual path: " << virtual_path_.AsUTF8Unsafe();
 
@@ skipping 691 matching lines @@
     const base::FilePath& suggested_path) {
   return base::FilePath(suggested_path.value() + kCrdownloadSuffix);
 }
 
 #if defined(OS_WIN)
 // static
 bool DownloadTargetDeterminer::IsAdobeReaderUpToDate() {
   return g_is_adobe_reader_up_to_date_;
 }
 #endif