Split DownloadFile::Rename into RenameAndUniquify and RenameAndAnnotate.

content/browser/download/download_item_impl.cc

Index: content/browser/download/download_item_impl.cc
diff --git a/content/browser/download/download_item_impl.cc b/content/browser/download/download_item_impl.cc
index 3cd5e2f3c9b5ae8e9bd4603c85c894e6322025d2..2a8a323c2757822e95f0aee7bd2e36acd2cddcf8 100644
--- a/content/browser/download/download_item_impl.cc
+++ b/content/browser/download/download_item_impl.cc
@@ -99,11 +99,9 @@ class NullDownloadRequestHandle : public DownloadRequestHandleInterface {
 // Wrapper around DownloadFile::Detach and DownloadFile::Cancel that
 // takes ownership of the DownloadFile and hence implicitly destroys it
 // at the end of the function.
-static void DownloadFileDetach(
-    scoped_ptr<DownloadFile> download_file,
-    const DownloadFile::DetachCompletionCallback& callback) {
+static void DownloadFileDetach(scoped_ptr<DownloadFile> download_file) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
-  download_file->Detach(callback);
+  download_file->Detach();
 }
 
 static void DownloadFileCancel(scoped_ptr<DownloadFile> download_file) {
@@ -828,7 +826,7 @@ void DownloadItemImpl::Interrupt(DownloadInterruptReason reason) {
   // interrupts to race with cancels.
 
   // Whatever happens, the first one to hit the UI thread wins.
-  if (state_ != IN_PROGRESS_INTERNAL && state_ != COMPLETING_INTERNAL)
+  if (state_ != IN_PROGRESS_INTERNAL)
     return;
 
   last_reason_ = reason;
@@ -1101,10 +1099,10 @@ void DownloadItemImpl::OnDownloadTargetDetermined(
                  weak_ptr_factory_.GetWeakPtr());
   BrowserThread::PostTask(
       BrowserThread::FILE, FROM_HERE,
-      base::Bind(&DownloadFile::Rename,
+      base::Bind(&DownloadFile::RenameAndUniquify,
                  // Safe because we control download file lifetime.
                  base::Unretained(download_file_.get()),
-                 intermediate_path, false, callback));
+                 intermediate_path, callback));
 }
 
 void DownloadItemImpl::OnDownloadRenamedToIntermediateName(
@@ -1132,6 +1130,7 @@ void DownloadItemImpl::OnDownloadRenamedToIntermediateName(
 // complete.
 void DownloadItemImpl::MaybeCompleteDownload() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DCHECK(!is_save_package_download_);
 
   if (!IsDownloadReadyForCompletion())
     return;
@@ -1171,7 +1170,6 @@ void DownloadItemImpl::ReadyForDownloadCompletionDone() {
     return;
 
   VLOG(20) << __FUNCTION__ << "()"
-           << " needs rename = " << NeedsRename()
            << " " << DebugString(true);
   DCHECK(!GetTargetFilePath().empty());
   DCHECK_NE(DANGEROUS, GetSafetyState());
@@ -1180,29 +1178,31 @@ void DownloadItemImpl::ReadyForDownloadCompletionDone() {
   if (is_save_package_download_) {
     // Avoid doing anything on the file thread; there's nothing we control
     // there.
-    OnDownloadFileReleased(DOWNLOAD_INTERRUPT_REASON_NONE);
+    // Strictly speaking, this skips giving the embedder a chance to open
+    // the download.  But on a save package download, there's no real
+    // concept of opening.
+    Completed();
     return;
   }
 
   DCHECK(download_file_.get());
-  if (NeedsRename()) {
-    DownloadFile::RenameCompletionCallback callback =
-        base::Bind(&DownloadItemImpl::OnDownloadRenamedToFinalName,
-                   weak_ptr_factory_.GetWeakPtr());
-    BrowserThread::PostTask(
-        BrowserThread::FILE, FROM_HERE,
-        base::Bind(&DownloadFile::Rename,
-                   base::Unretained(download_file_.get()),
-                   GetTargetFilePath(), true, callback));
-  } else {
-    ReleaseDownloadFile();
-  }
+  // Unilaterally rename; even if it already has the right name,
+  // we need theannotation.
+  DownloadFile::RenameCompletionCallback callback =
+      base::Bind(&DownloadItemImpl::OnDownloadRenamedToFinalName,
+                 weak_ptr_factory_.GetWeakPtr());
+  BrowserThread::PostTask(
+      BrowserThread::FILE, FROM_HERE,
+      base::Bind(&DownloadFile::RenameAndAnnotate,
+                 base::Unretained(download_file_.get()),
+                 GetTargetFilePath(), callback));
 }
 
 void DownloadItemImpl::OnDownloadRenamedToFinalName(
     DownloadInterruptReason reason,
     const FilePath& full_path) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DCHECK(!is_save_package_download_);
 
   // If a cancel or interrupt hit, we'll cancel the DownloadFile, which
   // will result in deleting the file on the file thread.  So we don't
@@ -1212,46 +1212,33 @@ void DownloadItemImpl::OnDownloadRenamedToFinalName(
 
   VLOG(20) << __FUNCTION__ << "()"
            << " full_path = \"" << full_path.value() << "\""
-           << " needed rename = " << NeedsRename()
            << " " << DebugString(false);
-  DCHECK(NeedsRename());
 
   if (DOWNLOAD_INTERRUPT_REASON_NONE != reason) {
     Interrupt(reason);
     return;
   }
 
-  // full_path is now the current and target file path.
-  DCHECK(!full_path.empty());
-  target_path_ = full_path;
-  SetFullPath(full_path);
-  delegate_->DownloadRenamedToFinalName(this);
-
-  ReleaseDownloadFile();
-}
+  if (full_path != current_path_) {
+    // full_path is now the current and target file path.
+    DCHECK(!full_path.empty());
+    DCHECK(target_path_ == full_path);
+    SetFullPath(full_path);
+    delegate_->DownloadRenamedToFinalName(this);

[asanka, 2012/11/07 16:11:03]

Move the DCHECK(target_path_ == full_path) out of the conditional. It should be
true regardless.

[Randy Smith (Not in Mondays), 2012/11/07 21:22:36]

Done.

+  }
 
-void DownloadItemImpl::ReleaseDownloadFile() {
   // Complete the download and release the DownloadFile.
-  DCHECK(!is_save_package_download_);
   DCHECK(download_file_.get());
   BrowserThread::PostTask(
       BrowserThread::FILE, FROM_HERE,
-      base::Bind(&DownloadFileDetach, base::Passed(download_file_.Pass()),
-                 base::Bind(&DownloadItemImpl::OnDownloadFileReleased,
-                            weak_ptr_factory_.GetWeakPtr())));
+      base::Bind(&DownloadFileDetach, base::Passed(download_file_.Pass())));
 
   // We're not completely done with the download item yet, but at this
   // point we're committed to complete the download.  Cancels (or Interrupts,
   // though it's not clear how they could happen) after this point will be
   // ignored.
   TransitionTo(COMPLETING_INTERNAL);
-}
 
-void DownloadItemImpl::OnDownloadFileReleased(DownloadInterruptReason reason) {
-  if (DOWNLOAD_INTERRUPT_REASON_NONE != reason) {
-    Interrupt(reason);
-    return;
-  }
   if (delegate_->ShouldOpenDownload(this))
     Completed();
   else
@@ -1330,11 +1317,6 @@ bool DownloadItemImpl::IsDownloadReadyForCompletion() {
   return true;
 }
 
-bool DownloadItemImpl::NeedsRename() const {
-  DCHECK(target_path_.DirName() == current_path_.DirName());
-  return target_path_ != current_path_;
-}
-
 void DownloadItemImpl::TransitionTo(DownloadInternalState new_state) {
   if (state_ == new_state)
     return;