[Resumption 6/11] Add a RESUMING_INTERNAL state to DownloadItem.

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 fa530c8d98abbf5b7b8276963796b43a39d71396..b4a8e042d0f9246fbd9fcdf322882e6577e59a35 100644
--- a/content/browser/download/download_item_impl.cc
+++ b/content/browser/download/download_item_impl.cc
@@ -309,33 +309,39 @@ void DownloadItemImpl::Pause() {
 
 void DownloadItemImpl::Resume() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  switch (state_) {
+    case IN_PROGRESS_INTERNAL:
+      if (!is_paused_)
+        return;
+      request_handle_->ResumeRequest();
+      is_paused_ = false;
+      UpdateObservers();
+      return;
 
-  // Ignore irrelevant states.
-  if (state_ == COMPLETE_INTERNAL ||
-      state_ == COMPLETING_INTERNAL ||
-      state_ == CANCELLED_INTERNAL ||
-      (state_ == IN_PROGRESS_INTERNAL && !is_paused_))
-    return;
+    case COMPLETING_INTERNAL:
+    case COMPLETE_INTERNAL:
+    case CANCELLED_INTERNAL:
+    case RESUMING_INTERNAL:
+      return;
 
-  if (state_ == INTERRUPTED_INTERNAL) {
-    auto_resume_count_ = 0;  // User input resets the counter.
-    ResumeInterruptedDownload();
-    return;
-  }
-  DCHECK_EQ(IN_PROGRESS_INTERNAL, state_);
+    case INTERRUPTED_INTERNAL:
+      auto_resume_count_ = 0;  // User input resets the counter.
+      ResumeInterruptedDownload();
+      return;
 
-  request_handle_->ResumeRequest();
-  is_paused_ = false;
-  UpdateObservers();
+    case MAX_DOWNLOAD_INTERNAL_STATE:
+      NOTREACHED();
+  }
 }
 
 void DownloadItemImpl::Cancel(bool user_cancel) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   VLOG(20) << __FUNCTION__ << "() download = " << DebugString(true);
-  if (state_ != IN_PROGRESS_INTERNAL && state_ != INTERRUPTED_INTERNAL) {
-    // Small downloads might be complete before this method has
-    // a chance to run.
+  if (state_ != IN_PROGRESS_INTERNAL &&
+      state_ != INTERRUPTED_INTERNAL &&
+      state_ != RESUMING_INTERNAL) {
+    // Small downloads might be complete before this method has a chance to run.
     return;
   }
 
@@ -352,7 +358,7 @@ void DownloadItemImpl::Cancel(bool user_cancel) {
   if (!is_save_package_download_ && download_file_)
     ReleaseDownloadFile(true);
 
-  if (state_ != INTERRUPTED_INTERNAL) {
+  if (state_ == IN_PROGRESS_INTERNAL) {
     // Cancel the originating URL request unless it's already been cancelled
     // by interrupt.
     request_handle_->CancelRequest();
@@ -1048,6 +1054,14 @@ void DownloadItemImpl::Start(
   download_file_ = file.Pass();
   request_handle_ = req_handle.Pass();
 
+  if (IsCancelled()) {
+    // The download was in the process of resuming when it was cancelled. Don't
+    // proceed.
+    ReleaseDownloadFile(true);
+    request_handle_->CancelRequest();

[Randy Smith (Not in Mondays), 2013/05/09 01:00:57]

Is there a reason to do this here, rather than above in Cancel()? I think
there's a virtue in having cancel imply that there isn't a download file; if
not, there's effectively one more emergent state to track.

I *think* you could make it fire-and-forget by revoking the weak pointers at
that point as well.

[asanka, 2013/05/09 16:54:33]

I may be misreading this comment.

The code is trying to handle a race between a URLRequest dispatched for a
resumption and a possibly user initiated Cancel. For example, if the following
sequence of events happen:

1. ::Interrupt().
2. ::ResumeInterruptedDownload(): Fires off a request.
3. ::Cancel().
4. Response received for request sent in #2.
5. ::Start() called in response to #4.

The request_handle and download_file being discarded are those that were created
in response to #4.

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

Whoops; the key thing I was spacing on was the last point.  We've started off
the request, but we don't have a handle for it yet.  Ok, I'm good.

+    return;
+  }
+
   TransitionTo(IN_PROGRESS_INTERNAL);
 
   last_reason_ = DOWNLOAD_INTERRUPT_REASON_NONE;
@@ -1331,6 +1345,25 @@ void DownloadItemImpl::Completed() {
   }
 }
 
+void DownloadItemImpl::OnResumeRequestStarted(DownloadItem* item,
+                                              net::Error error) {
+  // If |item| is not NULL, then Start() has been called already, and nothing
+  // more needs to be done here.
+  if (item) {
+    DCHECK_EQ(net::OK, error);
+    DCHECK_EQ(static_cast<DownloadItem*>(this), item);
+    return;
+  }
+  // Otherwise, the request failed without passing through
+  // DownloadResourceHandler::OnResponseStarted.
+  if (error == net::OK)
+    error = net::ERR_FAILED;
+  DownloadInterruptReason reason =
+      ConvertNetErrorToInterruptReason(error, DOWNLOAD_INTERRUPT_FROM_NETWORK);
+  DCHECK_NE(DOWNLOAD_INTERRUPT_REASON_NONE, reason);
+  Interrupt(reason);
+}
+
 // **** End of Download progression cascade
 
 // An error occurred somewhere.
@@ -1347,7 +1380,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)
+  if (state_ != IN_PROGRESS_INTERNAL && state_ != RESUMING_INTERNAL)

[Randy Smith (Not in Mondays), 2013/05/09 01:00:57]

Why should an interrupt fail in RESUMING_INTERNAL mode?  I can't offhand think
why one would happen, but that would mean a DCHECK(), not an ignore.  Cancels
work; if we're going to implement a particular behavior for this case, I would
think it would be parallel to Cancel().

[asanka, 2013/05/09 16:54:33]

RESUMING_INTERNAL -> INTERRUPTED_INTERNAL happens if the resumption request
fails. E.g.: internet disconnected while trying to resume a download.

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

Got it.  It was interrupts happening before we got a response to our request
dispatch that I was thinking of, but of course the request dispatch itself can
fail.

     return;
 
   last_reason_ = reason;
@@ -1560,7 +1593,7 @@ void DownloadItemImpl::ResumeInterruptedDownload() {
     return;
 
   // If we're not interrupted, ignore the request; our caller is drunk.
-  if (!IsInterrupted())
+  if (state_ != INTERRUPTED_INTERNAL)
     return;
 
   // If we can't get a web contents, we can't resume the download.
@@ -1589,11 +1622,15 @@ 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()));
 
   delegate_->ResumeInterruptedDownload(download_params.Pass(), GetGlobalId());
-
   // Just in case we were interrupted while paused.
   is_paused_ = false;
+
+  TransitionTo(RESUMING_INTERNAL);
 }
 
 // static
@@ -1610,29 +1647,32 @@ DownloadItem::DownloadState DownloadItemImpl::InternalToExternalState(
       return CANCELLED;
     case INTERRUPTED_INTERNAL:
       return INTERRUPTED;
-    default:
-      NOTREACHED();
+    case RESUMING_INTERNAL:
+      return INTERRUPTED;
+    case MAX_DOWNLOAD_INTERNAL_STATE:
+      break;
   }
+  NOTREACHED();
   return MAX_DOWNLOAD_STATE;
 }
 
 // static
 DownloadItemImpl::DownloadInternalState
 DownloadItemImpl::ExternalToInternalState(
-     DownloadState external_state) {
-   switch (external_state) {
-     case IN_PROGRESS:
-       return IN_PROGRESS_INTERNAL;
-     case COMPLETE:
-       return COMPLETE_INTERNAL;
-     case CANCELLED:
-       return CANCELLED_INTERNAL;
-     case INTERRUPTED:
-       return INTERRUPTED_INTERNAL;
-     default:
-       NOTREACHED();
-   }
-   return MAX_DOWNLOAD_INTERNAL_STATE;
+    DownloadState external_state) {
+  switch (external_state) {
+    case IN_PROGRESS:
+      return IN_PROGRESS_INTERNAL;
+    case COMPLETE:
+      return COMPLETE_INTERNAL;
+    case CANCELLED:
+      return CANCELLED_INTERNAL;
+    case INTERRUPTED:
+      return INTERRUPTED_INTERNAL;
+    default:
+      NOTREACHED();
+  }
+  return MAX_DOWNLOAD_INTERNAL_STATE;
  }

[benjhayden, 2013/05/09 14:49:51]

Thanks for fixing the above lines, please also dedent this line.

[asanka, 2013/05/09 16:54:33]

Ah thanks! Missed that one.

 
 const char* DownloadItemImpl::DebugDownloadStateString(
@@ -1648,10 +1688,13 @@ const char* DownloadItemImpl::DebugDownloadStateString(
       return "CANCELLED";
     case INTERRUPTED_INTERNAL:
       return "INTERRUPTED";
-    default:
-      NOTREACHED() << "Unknown download state " << state;
-      return "unknown";
+    case RESUMING_INTERNAL:
+      return "RESUMING";
+    case MAX_DOWNLOAD_INTERNAL_STATE:
+      break;
   };
+  NOTREACHED() << "Unknown download state " << state;
+  return "unknown";
 }
 
 const char* DownloadItemImpl::DebugResumeModeString(ResumeMode mode) {