[Downloads] Add more browsertests for resumption.

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 9b6b51637677c663957783b67abac46f46355138..5a8e413559ca0807df8a9ab552a0a2ef5d36a214 100644
--- a/content/browser/download/download_item_impl.cc
+++ b/content/browser/download/download_item_impl.cc
@@ -1036,8 +1036,6 @@ void DownloadItemImpl::Start(
 
   TransitionTo(IN_PROGRESS_INTERNAL);
 
-  last_reason_ = DOWNLOAD_INTERRUPT_REASON_NONE;
-
   BrowserThread::PostTask(
       BrowserThread::FILE, FROM_HERE,
       base::Bind(&DownloadFile::Initialize,
@@ -1359,9 +1357,11 @@ void DownloadItemImpl::Interrupt(DownloadInterruptReason reason) {
   // Cancel (delete file) if we're going to restart; no point in leaving
   // data around we aren't going to use.  Also cancel if resumption isn't
   // enabled for the same reason.
-  ReleaseDownloadFile(resume_mode == RESUME_MODE_IMMEDIATE_RESTART ||
-                      resume_mode == RESUME_MODE_USER_RESTART ||
-                      !IsDownloadResumptionEnabled());
+  if (download_file_.get()) {

[Randy Smith (Not in Mondays), 2013/06/19 21:46:21]

What are the circumstances that trigger this code (and the parallel
request_handle_ code below)?  This makes me worried that our state machine isn't
well-defined, as I think it's implying we can be interrupted while we're in
RESUMING.

[asanka, 2013/06/20 20:04:01]

That's exactly the transition that's being handled here (RESUMING ->
INTERRUPTED), which happens when the resumption request fails before a response
is received.

[Randy Smith (Not in Mondays), 2013/06/20 20:55:22]

Gotcha.  To the extent possible, I'd like to make state related conditionals
explicitly depend on the internal state, as part of trying to keep the
code-path-important changes in state to the state variable as much as we can. 
Willing to rewrite these to be conditionalized on state_, possibly with some
assertions?

[asanka, 2013/06/20 22:40:13]

Yup. That's a good suggestion.

+    ReleaseDownloadFile(resume_mode == RESUME_MODE_IMMEDIATE_RESTART ||
+                        resume_mode == RESUME_MODE_USER_RESTART ||
+                        !IsDownloadResumptionEnabled());
+  }
 
   // Reset all data saved, as even if we did save all the data we're going
   // to go through another round of downloading when we resume.
@@ -1376,12 +1376,18 @@ void DownloadItemImpl::Interrupt(DownloadInterruptReason reason) {
   all_data_saved_ = false;
 
   // Cancel the originating URL request.
-  request_handle_->CancelRequest();
+  if (request_handle_.get())
+    request_handle_->CancelRequest();
 
   TransitionTo(INTERRUPTED_INTERNAL);
   RecordDownloadInterrupted(reason, received_bytes_, total_bytes_);
 
   AutoResumeIfValid();
+
+  // Since the external state could be INTERRUPTED before and after the
+  // TransitionTo() calls, the observers may not be notified. Manually trigger
+  // an update since the value of CanResume() may have changed.
+  UpdateObservers();

[Randy Smith (Not in Mondays), 2013/06/19 21:46:21]

Making a note to myself to come back to this after I understand the state
diagram implications better.

 }
 
 void DownloadItemImpl::ReleaseDownloadFile(bool destroy_file) {