[Downloads] Always call DM::StartDownload() for explicit 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 7a66b4e5aea844a4f75247698b07d5c207110ad7..7fcfa20604f1b289177441ddcf054ed7778f053c 100644
--- a/content/browser/download/download_item_impl.cc
+++ b/content/browser/download/download_item_impl.cc
@@ -192,7 +192,7 @@ DownloadItemImpl::DownloadItemImpl(DownloadItemImplDelegate* delegate,
       bytes_per_sec_(0),
       last_modified_time_(info.last_modified),
       etag_(info.etag),
-      last_reason_(DOWNLOAD_INTERRUPT_REASON_NONE),
+      last_reason_(info.result),
       start_tick_(base::TimeTicks::Now()),
       state_(INITIAL_INTERNAL),
       danger_type_(DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS),
@@ -205,7 +205,7 @@ DownloadItemImpl::DownloadItemImpl(DownloadItemImplDelegate* delegate,
       auto_opened_(false),
       is_temporary_(!info.save_info->file_path.empty()),
       all_data_saved_(false),
-      destination_error_(content::DOWNLOAD_INTERRUPT_REASON_NONE),
+      destination_error_(DOWNLOAD_INTERRUPT_REASON_NONE),
       opened_(false),
       delegate_delayed_complete_(false),
       bound_net_log_(bound_net_log),
@@ -361,6 +361,7 @@ void DownloadItemImpl::Pause() {
       return;
 
     case TARGET_PENDING_INTERNAL:
+    case INTERRUPTED_TARGET_PENDING_INTERNAL:
     case TARGET_RESOLVED_INTERNAL:
     case IN_PROGRESS_INTERNAL:
       request_handle_->PauseRequest();
@@ -375,6 +376,7 @@ void DownloadItemImpl::Pause() {
 
 void DownloadItemImpl::Resume() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  DVLOG(20) << __FUNCTION__ << "() download = " << DebugString(true);
   switch (state_) {
     case INITIAL_INTERNAL:
     case COMPLETING_INTERNAL:
@@ -386,6 +388,7 @@ void DownloadItemImpl::Resume() {
       return;
 
     case TARGET_PENDING_INTERNAL:
+    case INTERRUPTED_TARGET_PENDING_INTERNAL:

[Randy Smith (Not in Mondays), 2016/02/11 22:08:08]

This also seems incompatible with mapping to an external INTERRUPTED state; the
external expectation is almost certainly that a Resume() call on the object will
actually result in an resumption.

[asanka, 2016/02/11 23:57:25]

It's worse than that, we can't trigger a resumption while a target determination
is pending. I've updated CanResume() to reflect this, but we should allow
resumption from this state, at least by resuming when target determination is
complete. But not for this CL since the state machine changes are complicated
enough as it is.

[asanka, 2016/02/12 16:06:49]

Fine! We'll go with mapping this to IN_PROGRESS :P

[Randy Smith (Not in Mondays), 2016/02/12 18:01:19]

:-}.

     case TARGET_RESOLVED_INTERNAL:
     case IN_PROGRESS_INTERNAL:
       if (!is_paused_)
@@ -398,6 +401,7 @@ void DownloadItemImpl::Resume() {
     case INTERRUPTED_INTERNAL:
       auto_resume_count_ = 0;  // User input resets the counter.
       ResumeInterruptedDownload();
+      UpdateObservers();

[Randy Smith (Not in Mondays), 2016/02/11 22:08:08]

See comment elsewhere about registered observers vs. polling consumers (though
I'm not certain what the right choice for this CL is, just calling out that it's
an issue I'm curious as to your thoughts about WRT this change).

[asanka, 2016/02/11 23:57:25]

Responded there. Basically, UpdateObservers is best called just before we exit
the state machine and yield to external signals. We need to be careful about the
state we leave the DII when we make async calls since then someone can examine
the DII. Even then, as long as the exposed state is consistent, I'm not
concerned.

       return;
 
     case MAX_DOWNLOAD_INTERNAL_STATE:
@@ -410,6 +414,7 @@ void DownloadItemImpl::Cancel(bool user_cancel) {
   DVLOG(20) << __FUNCTION__ << "() download = " << DebugString(true);
   Interrupt(user_cancel ? DOWNLOAD_INTERRUPT_REASON_USER_CANCELED
                         : DOWNLOAD_INTERRUPT_REASON_USER_SHUTDOWN);
+  UpdateObservers();
 }
 
 void DownloadItemImpl::Remove() {
@@ -487,6 +492,7 @@ bool DownloadItemImpl::CanResume() const {
       return false;
 
     case TARGET_PENDING_INTERNAL:
+    case INTERRUPTED_TARGET_PENDING_INTERNAL:
     case TARGET_RESOLVED_INTERNAL:
     case IN_PROGRESS_INTERNAL:
       return is_paused_;
@@ -512,6 +518,7 @@ bool DownloadItemImpl::IsDone() const {
     case COMPLETING_INTERNAL:
     case RESUMING_INTERNAL:
     case TARGET_PENDING_INTERNAL:
+    case INTERRUPTED_TARGET_PENDING_INTERNAL:

[Randy Smith (Not in Mondays), 2016/02/11 22:08:09]

I'm beginning to think that the mapping of this to INTERRUPTED rather than
IN_PROGRESS in InternalToExternalState was a typo .... :-}.

[asanka, 2016/02/11 23:57:25]

?

[asanka, 2016/02/12 16:06:49]

NM

     case TARGET_RESOLVED_INTERNAL:
     case IN_PROGRESS_INTERNAL:
       return false;
@@ -936,7 +943,7 @@ DownloadItemImpl::ResumeMode DownloadItemImpl::GetResumeMode() const {
   return mode;
 }
 
-void DownloadItemImpl::MergeOriginInfoOnResume(
+void DownloadItemImpl::UpdateValidatorsOnResumption(
     const DownloadCreateInfo& new_create_info) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   DCHECK_EQ(RESUMING_INTERNAL, state_);
@@ -1023,7 +1030,8 @@ void DownloadItemImpl::MarkAsComplete() {
 
   DCHECK(all_data_saved_);
   end_time_ = base::Time::Now();
-  TransitionTo(COMPLETE_INTERNAL, UPDATE_OBSERVERS);
+  TransitionTo(COMPLETE_INTERNAL);
+  UpdateObservers();
 }
 
 void DownloadItemImpl::DestinationUpdate(int64_t bytes_so_far,
@@ -1073,10 +1081,12 @@ void DownloadItemImpl::DestinationError(DownloadInterruptReason reason) {
   // Postpone recognition of this error until after file name determination
   // has completed and the intermediate file has been renamed to simplify
   // resumption conditions.
-  if (state_ != IN_PROGRESS_INTERNAL)
+  if (state_ != IN_PROGRESS_INTERNAL) {
     destination_error_ = reason;
-  else
-    Interrupt(reason);
+    return;
+  }
+  Interrupt(reason);
+  UpdateObservers();
 }
 
 void DownloadItemImpl::DestinationCompleted(const std::string& final_hash) {
@@ -1125,11 +1135,10 @@ void DownloadItemImpl::Init(bool active,
 // We're starting the download.
 void DownloadItemImpl::Start(
     scoped_ptr<DownloadFile> file,
-    scoped_ptr<DownloadRequestHandleInterface> req_handle) {
+    scoped_ptr<DownloadRequestHandleInterface> req_handle,
+    const DownloadCreateInfo& new_create_info) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   DCHECK(!download_file_.get());
-  DCHECK(file.get());
-  DCHECK(req_handle.get());
   DVLOG(20) << __FUNCTION__ << "() this=" << DebugString(true);
 
   download_file_ = std::move(file);
@@ -1139,11 +1148,49 @@ void DownloadItemImpl::Start(
     // The download was in the process of resuming when it was cancelled. Don't
     // proceed.
     ReleaseDownloadFile(true);
-    request_handle_->CancelRequest();
+    if (request_handle_)
+      request_handle_->CancelRequest();
     return;
   }
 
-  TransitionTo(TARGET_PENDING_INTERNAL, UPDATE_OBSERVERS);
+  // The state could be one of the following:
+  //
+  // INITIAL_INTERNAL: A normal download attempt.
+  //
+  // RESUMING_INTERNAL: A resumption attempt. May or may not have been
+  //     successful.
+  DCHECK(state_ == INITIAL_INTERNAL || state_ == RESUMING_INTERNAL);
+
+  // If a resumption attempted failed, or if the download was DOA, then the
+  // download should go back to being interrupted.
+  if (new_create_info.result != DOWNLOAD_INTERRUPT_REASON_NONE) {
+    DCHECK(!download_file_.get());
+
+    // For downloads that are interrupted on initiation, invoke target
+    // determination so that we'll end up with a usable filename.
+    if (state_ == INITIAL_INTERNAL) {
+      TransitionTo(INTERRUPTED_TARGET_PENDING_INTERNAL);
+      destination_error_ = new_create_info.result;
+      DetermineDownloadTarget();
+      return;
+    }
+
+    // Otherwise, this was a resumption attempt which ended with an
+    // interruption.
+    Interrupt(new_create_info.result);
+    UpdateObservers();
+    return;
+  }
+
+  // Successful download start.
+  DCHECK(download_file_.get());
+  DCHECK(request_handle_.get());
+
+  if (state_ == RESUMING_INTERNAL)
+    UpdateValidatorsOnResumption(new_create_info);
+
+  TransitionTo(TARGET_PENDING_INTERNAL);
+  UpdateObservers();
 
   BrowserThread::PostTask(
       BrowserThread::FILE, FROM_HERE,
@@ -1161,35 +1208,34 @@ void DownloadItemImpl::OnDownloadFileInitialized(
   DVLOG(20) << __FUNCTION__
             << "() result:" << DownloadInterruptReasonToString(result);
   if (result != DOWNLOAD_INTERRUPT_REASON_NONE) {
-    // Transition out to TARGET_RESOLVED_INTERNAL since this DownloadItem is
-    // skipping the download target determination process.
-    TransitionTo(TARGET_RESOLVED_INTERNAL, DONT_UPDATE_OBSERVERS);
-    Interrupt(result);
-    // TODO(rdsmith/asanka): Arguably we should show this in the UI, but
-    // 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;
+    // Whoops. That didn't work. Proceed as an interrupted download. Update the
+    // observers since this is an externally visible transition.
+    destination_error_ = result;
+    TransitionTo(INTERRUPTED_TARGET_PENDING_INTERNAL);
+    UpdateObservers();
   }
 
+  DetermineDownloadTarget();
+}
+
+void DownloadItemImpl::DetermineDownloadTarget() {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  DVLOG(20) << __FUNCTION__ << "() " << DebugString(true);
+
   delegate_->DetermineDownloadTarget(
       this, base::Bind(&DownloadItemImpl::OnDownloadTargetDetermined,
                        weak_ptr_factory_.GetWeakPtr()));
 }
 
-// Called by delegate_ when the download target path has been
-// determined.
+// Called by delegate_ when the download target path has been determined.
 void DownloadItemImpl::OnDownloadTargetDetermined(
     const base::FilePath& target_path,
     TargetDisposition disposition,
     DownloadDangerType danger_type,
     const base::FilePath& intermediate_path) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
-  DCHECK_EQ(state_, TARGET_PENDING_INTERNAL);
+  DCHECK(state_ == TARGET_PENDING_INTERNAL ||
+         state_ == INTERRUPTED_TARGET_PENDING_INTERNAL);
 
   // If the |target_path| is empty, then we consider this download to be
   // canceled.
@@ -1198,14 +1244,6 @@ void DownloadItemImpl::OnDownloadTargetDetermined(
     return;
   }
 
-  // TODO(rdsmith,asanka): We are ignoring the possibility that the download
-  // has been interrupted at this point until we finish the intermediate
-  // rename and set the full path.  That's dangerous, because we might race
-  // with resumption, either manual (because the interrupt is visible to the
-  // UI) or automatic.  If we keep the "ignore an error on download until file
-  // name determination complete" semantics, we need to make sure that the
-  // error is kept completely invisible until that point.
-
   DVLOG(20) << __FUNCTION__ << " " << target_path.value() << " " << disposition
             << " " << danger_type << " " << DebugString(true);
 
@@ -1213,6 +1251,14 @@ void DownloadItemImpl::OnDownloadTargetDetermined(
   target_disposition_ = disposition;
   SetDangerType(danger_type);
 
+  // This was an interrupted download that was looking for a filename. Now that
+  // it has one, transition to interrupted.
+  if (state_ == INTERRUPTED_TARGET_PENDING_INTERNAL) {
+    Interrupt(destination_error_);
+    UpdateObservers();
+    return;
+  }
+
   // 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.
@@ -1255,7 +1301,7 @@ void DownloadItemImpl::OnDownloadRenamedToIntermediateName(
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   DCHECK_EQ(state_, TARGET_PENDING_INTERNAL);
   DVLOG(20) << __FUNCTION__ << " download=" << DebugString(true);
-  TransitionTo(TARGET_RESOLVED_INTERNAL, DONT_UPDATE_OBSERVERS);
+  TransitionTo(TARGET_RESOLVED_INTERNAL);

[Randy Smith (Not in Mondays), 2016/02/11 22:08:08]

It looks to me as if this state isn't needed; the download item is never in it
when any other code can run on the UI thread.  Am I wrong?

[asanka, 2016/02/11 23:57:25]

I've tried to justify the existence of this state in download_item_impl.h. It's
needed since in its absence we'd be transitioning from TARGET_PENDING_INTERNAL
directly to INTERRUPTED. If that transition is allowed, then we lose the
advantage of having the PENDING state.

 
   if (DOWNLOAD_INTERRUPT_REASON_NONE != destination_error_) {
     // Process destination error.  If both |reason| and |destination_error_|
@@ -1273,9 +1319,10 @@ void DownloadItemImpl::OnDownloadRenamedToIntermediateName(
     DCHECK(current_path_.empty());
   } else {
     SetFullPath(full_path);
-    TransitionTo(IN_PROGRESS_INTERNAL, UPDATE_OBSERVERS);
+    TransitionTo(IN_PROGRESS_INTERNAL);
     MaybeCompleteDownload();
   }
+  UpdateObservers();
 }
 
 // When SavePackage downloads MHTML to GData (see
@@ -1321,9 +1368,8 @@ void DownloadItemImpl::OnDownloadCompleting() {
   // TODO(rdsmith/benjhayden): Remove as part of SavePackage integration.
   if (is_save_package_download_) {
     // Avoid doing anything on the file thread; there's nothing we control
-    // there.
-    // Strictly speaking, this skips giving the embedder a chance to open
-    // the download.  But on a save package download, there's no real
+    // there.  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;
@@ -1364,6 +1410,7 @@ void DownloadItemImpl::OnDownloadRenamedToFinalName(
     // All file errors should have resulted in in file deletion above. On
     // resumption we will need to re-do filename determination.
     DCHECK(current_path_.empty());
+    UpdateObservers();

[Randy Smith (Not in Mondays), 2016/02/11 22:08:08]

nit, suggestion: I'd rather have these right next to the state transition
functions (Interrupt in this case) so it's clear what's being broadcast to the
observers.

[asanka, 2016/02/11 23:57:25]

Iv'e tried to move the UpdateObservers() calls as close to an exit as possible
because the state of the DII after yielding to observers is undefined.

[Randy Smith (Not in Mondays), 2016/02/12 18:01:19]

SGTM.

     return;
   }
 
@@ -1383,7 +1430,7 @@ void DownloadItemImpl::OnDownloadRenamedToFinalName(
   // 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, DONT_UPDATE_OBSERVERS);
+  TransitionTo(COMPLETING_INTERNAL);
 
   if (delegate_->ShouldOpenDownload(
           this, base::Bind(&DownloadItemImpl::DelayedDownloadOpened,
@@ -1409,7 +1456,8 @@ void DownloadItemImpl::Completed() {
 
   DCHECK(all_data_saved_);
   end_time_ = base::Time::Now();
-  TransitionTo(COMPLETE_INTERNAL, UPDATE_OBSERVERS);
+  TransitionTo(COMPLETE_INTERNAL);
+  UpdateObservers();
   RecordDownloadCompleted(start_tick_, received_bytes_);
 
   if (auto_opened_) {
@@ -1428,22 +1476,6 @@ void DownloadItemImpl::Completed() {
   }
 }
 
-void DownloadItemImpl::OnResumeRequestStarted(
-    DownloadItem* item,
-    DownloadInterruptReason interrupt_reason) {
-  // If |item| is not NULL, then Start() has been called already, and nothing
-  // more needs to be done here.
-  if (item) {
-    DCHECK_EQ(DOWNLOAD_INTERRUPT_REASON_NONE, interrupt_reason);
-    DCHECK_EQ(static_cast<DownloadItem*>(this), item);
-    return;
-  }
-  // Otherwise, the request failed without passing through
-  // DownloadResourceHandler::OnResponseStarted.
-  DCHECK_NE(DOWNLOAD_INTERRUPT_REASON_NONE, interrupt_reason);
-  Interrupt(interrupt_reason);
-}
-
 // **** End of Download progression cascade
 
 // An error occurred somewhere.
@@ -1477,6 +1509,7 @@ void DownloadItemImpl::Interrupt(DownloadInterruptReason reason) {
       break;
 
     case RESUMING_INTERNAL:
+    case INTERRUPTED_TARGET_PENDING_INTERNAL:
     case INTERRUPTED_INTERNAL:
       // The first non-cancel interrupt reason wins in cases where multiple
       // things go wrong.
@@ -1531,16 +1564,16 @@ void DownloadItemImpl::Interrupt(DownloadInterruptReason reason) {
     }
 
     RecordDownloadCount(CANCELLED_COUNT);
-    TransitionTo(CANCELLED_INTERNAL, DONT_UPDATE_OBSERVERS);
-  } else {
-    RecordDownloadInterrupted(reason, received_bytes_, total_bytes_);
-    if (!GetWebContents())
-      RecordDownloadCount(INTERRUPTED_WITHOUT_WEBCONTENTS);
-    TransitionTo(INTERRUPTED_INTERNAL, DONT_UPDATE_OBSERVERS);
-    AutoResumeIfValid();
+    TransitionTo(CANCELLED_INTERNAL);
+    return;
   }
 
-  UpdateObservers();
+  RecordDownloadInterrupted(reason, received_bytes_, total_bytes_);
+  if (!GetWebContents())
+    RecordDownloadCount(INTERRUPTED_WITHOUT_WEBCONTENTS);
+
+  TransitionTo(INTERRUPTED_INTERNAL);
+  AutoResumeIfValid();
 }
 
 void DownloadItemImpl::ReleaseDownloadFile(bool destroy_file) {
@@ -1586,8 +1619,10 @@ bool DownloadItemImpl::IsDownloadReadyForCompletion(
   if (IsDangerous())
     return false;
 
-  // Invariants for the IN_PROGRESS state. DCHECKs here verify that the
-  // invariants are still true.
+  // Check for consistency before invoking delegate. Since there are no pending
+  // target determination calls and the download is in progress, both the target
+  // and current paths should be non-empty and they should point to the same
+  // directory.
   DCHECK(!target_path_.empty());
   DCHECK(!current_path_.empty());
   DCHECK(target_path_.DirName() == current_path_.DirName());
@@ -1600,8 +1635,7 @@ bool DownloadItemImpl::IsDownloadReadyForCompletion(
   return true;
 }
 
-void DownloadItemImpl::TransitionTo(DownloadInternalState new_state,
-                                    ShouldUpdateObservers notify_action) {
+void DownloadItemImpl::TransitionTo(DownloadInternalState new_state) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
   if (state_ == new_state)
@@ -1623,6 +1657,7 @@ void DownloadItemImpl::TransitionTo(DownloadInternalState new_state,
 
     case TARGET_PENDING_INTERNAL:
     case TARGET_RESOLVED_INTERNAL:
+    case INTERRUPTED_TARGET_PENDING_INTERNAL:
       break;
 
     case IN_PROGRESS_INTERNAL:
@@ -1705,9 +1740,6 @@ void DownloadItemImpl::TransitionTo(DownloadInternalState new_state,
                                          this, SRC_ACTIVE_DOWNLOAD,
                                          &file_name));
   }
-
-  if (notify_action == UPDATE_OBSERVERS)
-    UpdateObservers();
 }
 
 void DownloadItemImpl::SetDangerType(DownloadDangerType danger_type) {
@@ -1769,6 +1801,10 @@ void DownloadItemImpl::ResumeInterruptedDownload() {
   if (state_ != INTERRUPTED_INTERNAL)
     return;
 
+  // We are starting a new request. Shake off all pending operations.
+  DCHECK(!download_file_);
+  weak_ptr_factory_.InvalidateWeakPtrs();

[Randy Smith (Not in Mondays), 2016/02/11 22:08:09]

Why doesn't this solve the race that the earlier CL was trying to fix? 

[asanka, 2016/02/11 23:57:25]

The race was that a DestinationError() or DestinationCompleted() call could come
in after we start target determination. The destination observer and the target
determination can both be using weak pointers from the same session. However,
when the call comes in, we don't have a way to tell that there's a target
determination pending. The former logic was to examine the target path, but this
isn't reliable for a resumption.

[Randy Smith (Not in Mondays), 2016/02/12 18:01:19]

Right, sorry, still confused.  My image was that the other, smaller CL took care
of the race by making states finer grained and examining them.  But it looks to
me like this would also fix the race.  The DE() or DC() that comes in after
starting target determination would need to have been from a previous pre-resume
portion of the DIIs lifetime, wouldn't it?

[asanka, 2016/02/12 18:31:47]

It could be the same. The workflow is:

  - initialize download file.                   ----- [1]
  - initialize download file completes.

      [ download observer stuff happens ]

  - delegate starts download target determination

      [ download observer stuff happens ]

  - delegate determines download target
  - start intermediate rename

      [ download observer stuff happens ]

  - end intermediate rename                     ----- [2]

The destination observation calls can start coming through after [1]. [2] is
when we officially conclude target determination, and the first point where
successful new downloads have both a target and a current path.

[Randy Smith (Not in Mondays), 2016/02/12 18:58:10]

Ah, got it.  Thanks!

+
   // Reset the appropriate state if restarting.
   ResumeMode mode = GetResumeMode();
   if (mode == RESUME_MODE_IMMEDIATE_RESTART ||
@@ -1793,11 +1829,8 @@ void DownloadItemImpl::ResumeInterruptedDownload() {
   download_params->set_hash_state(GetHashState());
   download_params->set_last_modified(GetLastModifiedTime());
   download_params->set_etag(GetETag());
-  download_params->set_callback(
-      base::Bind(&DownloadItemImpl::OnResumeRequestStarted,
-                 weak_ptr_factory_.GetWeakPtr()));
 
-  TransitionTo(RESUMING_INTERNAL, DONT_UPDATE_OBSERVERS);
+  TransitionTo(RESUMING_INTERNAL);
   delegate_->ResumeInterruptedDownload(std::move(download_params), GetId());
   // Just in case we were interrupted while paused.
   is_paused_ = false;
@@ -1822,6 +1855,7 @@ DownloadItem::DownloadState DownloadItemImpl::InternalToExternalState(
       return COMPLETE;
     case CANCELLED_INTERNAL:
       return CANCELLED;
+    case INTERRUPTED_TARGET_PENDING_INTERNAL:

[Randy Smith (Not in Mondays), 2016/02/11 22:08:09]

Doesn't mapping this to interrupted leave you open to the consumer calling
ResumeInterruptedDownload() and being ignored?  That the behavior you want?

[asanka, 2016/02/11 23:57:25]

Resume() already doesn't work on an interrupted download that return false for
CanResume(). E.g. not HTTP, and in the future, not a supported HTTP verb. The
subtlety I'm introducing is that a download could sometimes change its
CanResume() value after entering an INTERRUPTED state. Currently this is
correctly handled by the UI because we update the observers when this happens
and the UI only looks at the CanResume() flag.

[asanka, 2016/02/12 16:06:49]

Retracted.

     case INTERRUPTED_INTERNAL:
       return INTERRUPTED;
     case RESUMING_INTERNAL:
@@ -1860,6 +1894,7 @@ bool DownloadItemImpl::IsValidSavePackageStateTransition(
   switch (from) {
     case INITIAL_INTERNAL:
     case TARGET_PENDING_INTERNAL:
+    case INTERRUPTED_TARGET_PENDING_INTERNAL:
     case TARGET_RESOLVED_INTERNAL:
     case COMPLETING_INTERNAL:
     case COMPLETE_INTERNAL:
@@ -1882,10 +1917,15 @@ bool DownloadItemImpl::IsValidStateTransition(DownloadInternalState from,
                                               DownloadInternalState to) {
   switch (from) {
     case INITIAL_INTERNAL:
-      return to == TARGET_PENDING_INTERNAL || to == INTERRUPTED_INTERNAL;
+      return to == TARGET_PENDING_INTERNAL ||
+             to == INTERRUPTED_TARGET_PENDING_INTERNAL;
 
     case TARGET_PENDING_INTERNAL:
-      return to == TARGET_RESOLVED_INTERNAL || to == CANCELLED_INTERNAL;
+      return to == INTERRUPTED_TARGET_PENDING_INTERNAL ||
+             to == TARGET_RESOLVED_INTERNAL || to == CANCELLED_INTERNAL;
+
+    case INTERRUPTED_TARGET_PENDING_INTERNAL:
+      return to == INTERRUPTED_INTERNAL || to == CANCELLED_INTERNAL;
 
     case TARGET_RESOLVED_INTERNAL:
       return to == IN_PROGRESS_INTERNAL || to == INTERRUPTED_INTERNAL ||
@@ -1905,7 +1945,9 @@ bool DownloadItemImpl::IsValidStateTransition(DownloadInternalState from,
       return to == RESUMING_INTERNAL || to == CANCELLED_INTERNAL;
 
     case RESUMING_INTERNAL:
-      return to == TARGET_PENDING_INTERNAL || to == CANCELLED_INTERNAL;
+      return to == TARGET_PENDING_INTERNAL ||
+             to == INTERRUPTED_TARGET_PENDING_INTERNAL ||
+             to == CANCELLED_INTERNAL;
 
     case CANCELLED_INTERNAL:
       return false;
@@ -1924,6 +1966,8 @@ const char* DownloadItemImpl::DebugDownloadStateString(
       return "INITIAL";
     case TARGET_PENDING_INTERNAL:
       return "TARGET_PENDING";
+    case INTERRUPTED_TARGET_PENDING_INTERNAL:
+      return "INTERRUPTED_TARGET_PENDING";
     case TARGET_RESOLVED_INTERNAL:
       return "TARGET_RESOLVED";
     case IN_PROGRESS_INTERNAL: