Download filename determination refactor (3/3)

content/browser/download/download_manager_impl.cc

Index: content/browser/download/download_manager_impl.cc
diff --git a/content/browser/download/download_manager_impl.cc b/content/browser/download/download_manager_impl.cc
index a34d25ebcf1d3cc2e9b9b90a583a1261d063e82e..8f6a212db3f614c0704a1966ce5a10d415859f03 100644
--- a/content/browser/download/download_manager_impl.cc
+++ b/content/browser/download/download_manager_impl.cc
@@ -221,6 +221,10 @@ DownloadId DownloadManagerImpl::GetNextId() {
   return id;
 }
 
+DownloadFileManager* DownloadManagerImpl::GetDownloadFileManager() {
+  return file_manager_;
+}
+
 bool DownloadManagerImpl::ShouldOpenDownload(DownloadItem* item) {
   if (!delegate_)
     return true;
@@ -417,8 +421,39 @@ void DownloadManagerImpl::OnDownloadFileCreated(
     // persistence we should replace this comment with a "return;".
   }
 
-  if (!delegate_ || delegate_->ShouldStartDownload(download_id))
-    RestartDownload(download_id);
+  DownloadItem* download = GetActiveDownloadItem(download_id);
+  if (!download)
+    return;
+
+  content::DownloadTargetCallback callback =
+      base::Bind(&DownloadManagerImpl::OnDownloadTargetDetermined,
+                 this, download_id);
+  if (!delegate_ || !delegate_->DetermineDownloadTarget(download, callback)) {
+    FilePath target_path = download->GetForcedFilePath();
+    // TODO(asanka): Determine a useful path if |target_path| is empty.
+    callback.Run(target_path,
+                 DownloadItem::TARGET_DISPOSITION_OVERWRITE,
+                 content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS,
+                 target_path);
+  }
+  // Once DownloadItem::OnDownloadTargetDetermined() is called, we expect a
+  // DownloadRenamedToIntermediateName() callback. This is necessary for the
+  // download to proceed.

[Randy Smith (Not in Mondays), 2012/07/10 18:33:09]

nit: Wouldn't the best place for this comment be above the direct call to
DI::OnDownloadTargetDetermined() below?

[asanka, 2012/07/11 20:03:32]

Done.

+}
+
+void DownloadManagerImpl::OnDownloadTargetDetermined(
+    int32 download_id,
+    const FilePath& target_path,
+    DownloadItem::TargetDisposition disposition,
+    content::DownloadDangerType danger_type,
+    const FilePath& intermediate_path) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DownloadItem* download = GetActiveDownloadItem(download_id);
+  if (!download)
+    return;
+
+  download->OnDownloadTargetDetermined(target_path, disposition, danger_type,
+                                       intermediate_path);
 }
 
 void DownloadManagerImpl::CheckForHistoryFilesRemoval() {
@@ -462,40 +497,10 @@ void DownloadManagerImpl::OnFileRemovalDetected(int32 download_id) {
     download->OnDownloadedFileRemoved();
 }
 
-void DownloadManagerImpl::RestartDownload(int32 download_id) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-
-  DownloadItem* download = GetActiveDownloadItem(download_id);
-  if (!download)
-    return;
-
-  VLOG(20) << __FUNCTION__ << "()"
-           << " download = " << download->DebugString(true);
-
-  if (download->GetTargetDisposition() ==
-      DownloadItem::TARGET_DISPOSITION_PROMPT) {
-    // We must ask the user for the place to put the download.
-    if (delegate_) {
-      delegate_->ChooseDownloadPath(download);
-      FOR_EACH_OBSERVER(Observer, observers_,
-                        SelectFileDialogDisplayed(this, download_id));
-    } else {
-      FileSelectionCanceled(download_id);
-    }
-  } else {
-    // No prompting for download, just continue with the current target path.
-    OnTargetPathAvailable(download);
-  }
-}
-
 content::BrowserContext* DownloadManagerImpl::GetBrowserContext() const {
   return browser_context_;
 }
 
-FilePath DownloadManagerImpl::LastDownloadPath() {
-  return last_download_path_;
-}
-
 net::BoundNetLog DownloadManagerImpl::CreateDownloadItem(
     DownloadCreateInfo* info) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
@@ -549,36 +554,6 @@ DownloadItem* DownloadManagerImpl::CreateSavePackageDownloadItem(
   return download;
 }
 
-// The target path for the download item is now valid. We proceed with the
-// determination of an intermediate path.
-void DownloadManagerImpl::OnTargetPathAvailable(DownloadItem* download) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  DCHECK(download);
-  DCHECK(ContainsKey(downloads_, download->GetId()));
-  DCHECK(ContainsKey(active_downloads_, download->GetId()));
-
-  VLOG(20) << __FUNCTION__ << "()"
-           << " download = " << download->DebugString(true);
-
-  // Rename to intermediate name.
-  // TODO(asanka): Skip this rename if download->AllDataSaved() is true. This
-  //               avoids a spurious rename when we can just rename to the final
-  //               filename. Unnecessary renames may cause bugs like
-  //               http://crbug.com/74187.
-  FilePath intermediate_path;
-  if (delegate_)
-    intermediate_path = delegate_->GetIntermediatePath(*download);
-  else
-    intermediate_path = download->GetTargetFilePath();
-
-  // We want the intermediate and target paths to refer to the same directory so
-  // that they are both on the same device and subject to same
-  // space/permission/availability constraints.
-  DCHECK(intermediate_path.DirName() ==
-         download->GetTargetFilePath().DirName());
-  download->OnIntermediatePathDetermined(file_manager_, intermediate_path);
-}
-
 void DownloadManagerImpl::UpdateDownload(int32 download_id,
                                          int64 bytes_so_far,
                                          int64 bytes_per_sec,
@@ -891,42 +866,6 @@ void DownloadManagerImpl::RemoveObserver(Observer* observer) {
   observers_.RemoveObserver(observer);
 }
 
-void DownloadManagerImpl::FileSelected(const FilePath& path,
-                                       int32 download_id) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  DCHECK(!path.empty());
-
-  DownloadItem* download = GetActiveDownloadItem(download_id);
-  if (!download)
-    return;
-  VLOG(20) << __FUNCTION__ << "()" << " path = \"" << path.value() << "\""
-           << " download = " << download->DebugString(true);
-
-  // Retain the last directory. Exclude temporary downloads since the path
-  // likely points at the location of a temporary file.
-  if (!download->IsTemporary())
-    last_download_path_ = path.DirName();
-
-  // Make sure the initial file name is set only once.
-  download->OnTargetPathSelected(path);
-  OnTargetPathAvailable(download);
-}
-
-void DownloadManagerImpl::FileSelectionCanceled(int32 download_id) {
-  // The user didn't pick a place to save the file, so need to cancel the
-  // download that's already in progress to the temporary location.
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-
-  DownloadItem* download = GetActiveDownloadItem(download_id);
-  if (!download)
-    return;
-
-  VLOG(20) << __FUNCTION__ << "()"
-           << " download = " << download->DebugString(true);
-
-  download->Cancel(true);
-}
-
 // Operations posted to us from the history service ----------------------------
 
 // The history service has retrieved all download entries. 'entries' contains
@@ -1048,8 +987,9 @@ int DownloadManagerImpl::InProgressCount() const {
 }
 
 // Clears the last download path, used to initialize "save as" dialogs.
-void DownloadManagerImpl::ClearLastDownloadPath() {
-  last_download_path_ = FilePath();
+void DownloadManagerImpl::ClearTransientState() {
+  if (delegate_)
+    delegate_->ClearTransientState();
 }
 
 void DownloadManagerImpl::NotifyModelChanged() {
@@ -1175,10 +1115,16 @@ void DownloadManagerImpl::DownloadOpened(DownloadItem* download) {
 void DownloadManagerImpl::DownloadRenamedToIntermediateName(
     DownloadItem* download) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  // If the rename failed, we receive an OnDownloadInterrupted() call before we
-  // receive the DownloadRenamedToIntermediateName() call.
-  if (delegate_)
+  // download->GetFullPath() is only expected to be meaningful after this
+  // callback is received. Therefore we can now add the download to a persistent
+  // store. If the rename failed, we receive an OnDownloadInterrupted() call
+  // before we receive the DownloadRenamedToIntermediateName() call.
+  if (delegate_) {
     delegate_->AddItemToPersistentStore(download);
+  } else {
+    OnItemAddedToPersistentStore(download->GetId(),
+                                 DownloadItem::kUninitializedHandle);
+  }
 }
 
 void DownloadManagerImpl::DownloadRenamedToFinalName(