Split DownloadFile::Rename into RenameAndUniquify and RenameAndAnnotate.

Split DownloadFile::Rename into RenameAndUniquify and RenameAndAnnotate.

This matches much better with the semantics as expected by the DownloadItem.

R=asanka@chromium.org
BUG=159501
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=167028

[Randy Smith (Not in Mondays), 2012-11-06 23:38:23]

Asanka, PTAL?  There's a bit more context for this on the bug; basically, I want
to unify Interrupt & Cancel, and that requires me to make it illegal to
interrupt in the COMPLETING state.

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

LGTM with nits.

http://codereview.chromium.org/11366121/diff/1/content/browser/download/downl...
File content/browser/download/download_file.h (right):

http://codereview.chromium.org/11366121/diff/1/content/browser/download/downl...
content/browser/download/download_file.h:32: // Callback used with Rename().  On
a successful rename |reason| will be
Nit: Update comment.

http://codereview.chromium.org/11366121/diff/1/content/browser/download/downl...
File content/browser/download/download_file_impl.cc (right):

http://codereview.chromium.org/11366121/diff/1/content/browser/download/downl...
content/browser/download/download_file_impl.cc:147: // is pretty small (round
trip to the UI thread).
Nit: This comment is no longer accurate since we are doing the annotation
immediately after the rename. There's still a tiny window, but in practice it's
much smaller than the time it takes for AV to begin scanning the file.

http://codereview.chromium.org/11366121/diff/1/content/browser/download/downl...
File content/browser/download/download_item_impl.cc (right):

http://codereview.chromium.org/11366121/diff/1/content/browser/download/downl...
content/browser/download/download_item_impl.cc:1227:
delegate_->DownloadRenamedToFinalName(this);
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:35]

Thanks!  Can you take a quick look at what I did in case you think I need to do
more?  I only thoroughly addressed one of your comments :-}.

http://codereview.chromium.org/11366121/diff/1/content/browser/download/downl...
File content/browser/download/download_file.h (right):

http://codereview.chromium.org/11366121/diff/1/content/browser/download/downl...
content/browser/download/download_file.h:32: // Callback used with Rename().  On
a successful rename |reason| will be
On 2012/11/07 16:11:03, asanka wrote:
> Nit: Update comment.

Added "*"; let me know if you think I need more :-}.

http://codereview.chromium.org/11366121/diff/1/content/browser/download/downl...
File content/browser/download/download_file_impl.cc (right):

http://codereview.chromium.org/11366121/diff/1/content/browser/download/downl...
content/browser/download/download_file_impl.cc:147: // is pretty small (round
trip to the UI thread).
On 2012/11/07 16:11:03, asanka wrote:
> Nit: This comment is no longer accurate since we are doing the annotation
> immediately after the rename. There's still a tiny window, but in practice
it's
> much smaller than the time it takes for AV to begin scanning the file.

I rewrote rather than removed the comment, as your point about the relationship
with the time it takes for the AV to begin scanning the file points out that
there still is a window.  Let me know what you think.

http://codereview.chromium.org/11366121/diff/1/content/browser/download/downl...
File content/browser/download/download_item_impl.cc (right):

http://codereview.chromium.org/11366121/diff/1/content/browser/download/downl...
content/browser/download/download_item_impl.cc:1227:
delegate_->DownloadRenamedToFinalName(this);
On 2012/11/07 16:11:03, asanka wrote:
> Move the DCHECK(target_path_ == full_path) out of the conditional. It should
be
> true regardless.

Done.

[asanka, 2012-11-07 21:39:00]

Still LGTM :)

http://codereview.chromium.org/11366121/diff/1/content/browser/download/downl...
File content/browser/download/download_file.h (right):

http://codereview.chromium.org/11366121/diff/1/content/browser/download/downl...
content/browser/download/download_file.h:32: // Callback used with Rename().  On
a successful rename |reason| will be
On 2012/11/07 21:22:36, rdsmith wrote:
> On 2012/11/07 16:11:03, asanka wrote:
> > Nit: Update comment.
> 
> Added "*"; let me know if you think I need more :-}.

LOL. Yeah. That'll do.

http://codereview.chromium.org/11366121/diff/1/content/browser/download/downl...
File content/browser/download/download_file_impl.cc (right):

http://codereview.chromium.org/11366121/diff/1/content/browser/download/downl...
content/browser/download/download_file_impl.cc:147: // is pretty small (round
trip to the UI thread).
On 2012/11/07 21:22:36, rdsmith wrote:
> On 2012/11/07 16:11:03, asanka wrote:
> > Nit: This comment is no longer accurate since we are doing the annotation
> > immediately after the rename. There's still a tiny window, but in practice
> it's
> > much smaller than the time it takes for AV to begin scanning the file.
> 
> I rewrote rather than removed the comment, as your point about the
relationship
> with the time it takes for the AV to begin scanning the file points out that
> there still is a window.  Let me know what you think.

LG

[commit-bot: I haz the power, 2012-11-07 21:41:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rdsmith@chromium.org/11366121/7001

[commit-bot: I haz the power, 2012-11-07 21:41:57]

Presubmit check for 11366121-7001 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    content/shell
    content/public/test

Presubmit checks took 3.0s to calculate.

[Randy Smith (Not in Mondays), 2012-11-07 21:49:34]

Looking for stamps; PTAL?  
* content/public/test: phajdan.jr
* content/shell: jochen

[jochen (gone - plz use gerrit), 2012-11-07 21:58:47]

content/shell lgtm with a question to consider

https://chromiumcodereview.appspot.com/11366121/diff/7001/content/shell/shell...
File content/shell/shell_download_manager_delegate.h (right):

https://chromiumcodereview.appspot.com/11366121/diff/7001/content/shell/shell...
content/shell/shell_download_manager_delegate.h:36: virtual
~ShellDownloadManagerDelegate();
why not just friend the test?

[Paweł Hajdan Jr., 2012-11-07 22:09:44]

content/public/test LGTM

[Randy Smith (Not in Mondays), 2012-11-07 22:24:55]

https://chromiumcodereview.appspot.com/11366121/diff/7001/content/shell/shell...
File content/shell/shell_download_manager_delegate.h (right):

https://chromiumcodereview.appspot.com/11366121/diff/7001/content/shell/shell...
content/shell/shell_download_manager_delegate.h:36: virtual
~ShellDownloadManagerDelegate();
On 2012/11/07 21:58:48, jochen wrote:
> why not just friend the test?

Huh.  My inclination is against, partially because it doesn't scale (in theory,
several tests could want to inherit from SDMD) and partially because the test in
question doesn't really have anything to do with SDMS, so it isn't clear to me
that the SDMD should have to know about it.  This seems better.  But if you feel
strongly (or evenly moderately; I don't have any sense of ownership of the SDMD
code), I'll change it.

[commit-bot: I haz the power, 2012-11-09 16:56:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rdsmith@chromium.org/11366121/7001

[commit-bot: I haz the power, 2012-11-09 16:56:46]

Failed to apply patch for content/browser/download/download_item_impl.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file content/browser/download/download_item_impl.cc
  Hunk #2 succeeded at 1278 (offset 452 lines).
  Hunk #3 FAILED at 1099.
  Hunk #4 FAILED at 1130.
  Hunk #5 FAILED at 1169.
  Hunk #6 FAILED at 1178.
  Hunk #7 FAILED at 1210.
  Hunk #8 succeeded at 1331 (offset 3 lines).
  5 out of 8 hunks FAILED -- saving rejects to file
content/browser/download/download_item_impl.cc.rej

Patch:       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..35a1c9f630bb2562c613eb387e0598c250dcb4fd
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,34 @@ 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);
+  DCHECK(target_path_ == full_path);
 
-  ReleaseDownloadFile();
-}
+  if (full_path != current_path_) {
+    // full_path is now the current and target file path.
+    DCHECK(!full_path.empty());
+    SetFullPath(full_path);
+    delegate_->DownloadRenamedToFinalName(this);
+  }
 
-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 +1318,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;

[commit-bot: I haz the power, 2012-11-09 22:30:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rdsmith@chromium.org/11366121/4002

[commit-bot: I haz the power, 2012-11-10 01:22:53]

Change committed as 167028