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

content/browser/download/download_item_impl.cc

 // Copyright (c) 2012 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.
 
 // File method ordering: Methods in this file are in the same order as
 // in download_item_impl.h, with the following exception: The public
 // interface Start is placed in chronological order with the other
 // (private) routines that together define a DownloadItem's state
 // transitions as the download progresses.  See "Download progression
 // cascade" later in this file.
@@ skipping 1109 matching lines @@
     // it's not at all clear what to show--we haven't done filename
     // determination, so we don't know what name to display.  OTOH,
     // the failure mode of not showing the DI if the file initialization
     // fails isn't a good one.  Can we hack up a name based on the
     // URLRequest?  We'll need to make sure that initialization happens
     // properly.  Possibly the right thing is to have the UI handle
     // this case specially.
     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()));
 }
 
 // Called by delegate_ when the download target path has been
 // determined.
 void DownloadItemImpl::OnDownloadTargetDetermined(
     const base::FilePath& target_path,
     TargetDisposition disposition,
@@ skipping 22 matching lines @@
   target_path_ = target_path;
   target_disposition_ = disposition;
   SetDangerType(danger_type);
   // TODO(asanka): SetDangerType() doesn't need to send a notification here.
 
   // 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() == 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
   //               filename. Unnecessary renames may cause bugs like
   //               http://crbug.com/74187.
   DCHECK(!is_save_package_download_);
   DCHECK(download_file_.get());
   DownloadFile::RenameCompletionCallback callback =
       base::Bind(&DownloadItemImpl::OnDownloadRenamedToIntermediateName,
                  weak_ptr_factory_.GetWeakPtr());
@@ skipping 543 matching lines @@
     case RESUME_MODE_USER_CONTINUE:
       return "USER_CONTINUE";
     case RESUME_MODE_USER_RESTART:
       return "USER_RESTART";
   }
   NOTREACHED() << "Unknown resume mode " << mode;
   return "unknown";
 }
 
 }  // namespace content