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

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 2a2986e88bce8288a3932261c5629389d1f5fc4e..0cfa51bc40d6b29c76bf7ff10e37dc8917595966 100644
--- a/content/browser/download/download_item_impl.cc
+++ b/content/browser/download/download_item_impl.cc
@@ -1127,18 +1127,6 @@ void DownloadItemImpl::OnDownloadFileInitialized(
     return;
   }
 
-  // If we're resuming an interrupted download, we may already know the download
-  // target so we can skip target name determination. GetFullPath() is non-empty
-  // for interrupted downloads where the intermediate file is still present, and
-  // also for downloads with forced paths.
-  if (!GetTargetFilePath().empty() && !GetFullPath().empty()) {
-    // TODO(rdsmith/asanka): Check to confirm that the target path isn't
-    // present on disk; if it is, we should re-do filename determination to
-    // give the user a chance not to collide.
-    MaybeCompleteDownload();
-    return;
-  }
-
   delegate_->DetermineDownloadTarget(
       this, base::Bind(&DownloadItemImpl::OnDownloadTargetDetermined,
                        weak_ptr_factory_.GetWeakPtr()));
@@ -1181,6 +1169,15 @@ void DownloadItemImpl::OnDownloadTargetDetermined(
   // space/permission/availability constraints.
   DCHECK(intermediate_path.DirName() == target_path.DirName());
 
+  // During resumption, we may choose to proceed with the same intermediate
+  // file. No rename is necessary if our intermediate file already has the
+  // correct name.
+  if (intermediate_path == current_path_) {
+    OnDownloadRenamedToIntermediateName(DOWNLOAD_INTERRUPT_REASON_NONE,
+                                        intermediate_path);
+    return;
+  }
+

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

Why aren't we always using the current_path_ as our intermediate file in the
continuation case?  Is there not a clean way to code that?

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

At this point the delegate has requested an intermediate path, so we accommodate
it.

I wanted to reuse the current_path_ in DownloadTargetDeterminer, but that
involved detecting if current_path_ was appropriate for the danger type. So I
decided to re-generate one instead.

[Randy Smith (Not in Mondays), 2013/05/24 01:53:55]

It just seems a bit weird to have this code here.  I think you'll mystify future
generations with it (well, ok, maybe "confuse" is a better adjective :-}). 
What's the use case under which the appropriateness of the current_path_ changes
during resumption?  

[asanka, 2013/05/29 22:30:11]

Can you explain why skipping a rename if the new name is the same as the old one
is "mystifying"?

The intermediate path could change if the user (or the embedder) chooses a
different target path during resumption because the original target path was out
of space. This would make a rename necessary and would also make it necessary
for content/ to anticipate a name change.

The danger level could change if the user was prompted during resumption when
they weren't initially prompted.

As an option, we could re-generate an intermediate path in chrome/ only if the
target directory changes.

[Randy Smith (Not in Mondays), 2013/05/30 21:26:24]

Well, no, but I *did* back off from calling it "mystifying" :-}.

(The confusing part was that, if looked at across resumptions, that we're
repeatedly doing the intermediate file rename, when you wouldn't think we'd need
to do it more than once.)
Gotcha; that makes sense.  I just wanted some comment about why we might need a
new intermediate rename.
Presumably the prompt might occur if the failure error suggested that a new
filesystem would be best, as above.  Ok.
My goals were twofold: a) to understand the use cases (which I now do--thanks),
and b) to get a comment of some sort in here to reduce the potential future
confusion of people thinking the same thing I was.  Maybe "The intermediate name
may change from its original value during file name determination on resumption,
for example if the interruption reason resulting in a user prompt."?

[asanka, 2013/05/31 19:23:59]

Ah. Cool. I added a comment as suggested. I should have probably explained the
situation better.

I also added code to preserve a non-empty intermediate name if the download is
dangerous. This makes the assumption that downloads can become less dangerous on
resumption, but not the other way around. Also it avoid re-generating random
intermediate names for resumed dangerous downloads.

   // Rename to intermediate name.
   // TODO(asanka): Skip this rename if AllDataSaved() is true. This avoids a
   //               spurious rename when we can just rename to the final