[Downloads] Always call DM::StartDownload() for explicit 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 149 matching lines @@
       bound_net_log_(bound_net_log),
       weak_ptr_factory_(this) {
   delegate_->Attach();
   DCHECK_NE(IN_PROGRESS_INTERNAL, state_);
   Init(false /* not actively downloading */, SRC_HISTORY_IMPORT);
 }
 
 // Constructing for a regular download:
 DownloadItemImpl::DownloadItemImpl(DownloadItemImplDelegate* delegate,
                                    uint32_t download_id,
                                    const DownloadCreateInfo& info,

[Randy Smith (Not in Mondays), 2016/02/10 21:48:45]

Now that DII::Start() takes a DCI, is it possible to get rid of the one here? 
Conceptually it seems like it should be, but I haven't traced the code paths.

[asanka, 2016/02/11 03:43:07]

It is in fact possible since the state of the download item between construction
and having Start() called is only visible inside DownloadManagerImpl. And the
latter doesn't care about any of the state initialized here. It's also
conceptually cleaner since Start() would be the only place where we need to deal
with DownloadCreateInfo.

That said, the resulting refactor increases the size of this CL substantially
due to how tests work. I'd like to take that on as a follow up. I added a TODO()
which is just as good as cleanup. :-P

                                    const net::BoundNetLog& bound_net_log)
     : is_save_package_download_(false),
       download_id_(download_id),
       target_disposition_((info.save_info->prompt_for_save_location)
                               ? TARGET_DISPOSITION_PROMPT
                               : TARGET_DISPOSITION_OVERWRITE),
       url_chain_(info.url_chain),
       referrer_url_(info.referrer_url),
       tab_url_(info.tab_url),
       tab_referrer_url_(info.tab_referrer_url),
       suggested_filename_(base::UTF16ToUTF8(info.save_info->suggested_name)),
       forced_file_path_(info.save_info->file_path),
       transition_type_(info.transition_type),
       has_user_gesture_(info.has_user_gesture),
       content_disposition_(info.content_disposition),
       mime_type_(info.mime_type),
       original_mime_type_(info.original_mime_type),
       remote_address_(info.remote_address),
       total_bytes_(info.total_bytes),
       received_bytes_(0),
       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_(IN_PROGRESS_INTERNAL),
+      state_(info.result == DOWNLOAD_INTERRUPT_REASON_NONE
+                 ? IN_PROGRESS_INTERNAL
+                 : INTERRUPTED_INTERNAL),
       danger_type_(DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS),
       start_time_(info.start_time),
       delegate_(delegate),
       is_paused_(false),
       auto_resume_count_(0),
       open_when_complete_(false),
       file_externally_removed_(false),
       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),
       weak_ptr_factory_(this) {
   delegate_->Attach();
-  Init(true /* actively downloading */, SRC_ACTIVE_DOWNLOAD);
+  Init(state_ == IN_PROGRESS_INTERNAL /* actively downloading */,
+       SRC_ACTIVE_DOWNLOAD);
 
   // Link the event sources.
   bound_net_log_.AddEvent(
       net::NetLog::TYPE_DOWNLOAD_URL_REQUEST,
       info.request_bound_net_log.source().ToEventParametersCallback());
 
   info.request_bound_net_log.AddEvent(
       net::NetLog::TYPE_DOWNLOAD_STARTED,
       bound_net_log_.source().ToEventParametersCallback());
 }
@@ skipping 124 matching lines @@
   if (state_ != IN_PROGRESS_INTERNAL || is_paused_)
     return;
 
   request_handle_->PauseRequest();
   is_paused_ = true;
   UpdateObservers();
 }
 
 void DownloadItemImpl::Resume() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  DVLOG(20) << __FUNCTION__ << "() download = " << DebugString(true);
   switch (state_) {
     case IN_PROGRESS_INTERNAL:
       if (!is_paused_)
         return;
       request_handle_->ResumeRequest();
       is_paused_ = false;
       UpdateObservers();
       return;
 
     case COMPLETING_INTERNAL:
     case COMPLETE_INTERNAL:
     case CANCELLED_INTERNAL:
     case RESUMING_INTERNAL:
       return;
 
     case INTERRUPTED_INTERNAL:
       auto_resume_count_ = 0;  // User input resets the counter.
       ResumeInterruptedDownload();
+      UpdateObservers();
       return;
 
     case MAX_DOWNLOAD_INTERNAL_STATE:
       NOTREACHED();
   }
 }
 
 void DownloadItemImpl::Cancel(bool user_cancel) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
@@ skipping 532 matching lines @@
     case DOWNLOAD_INTERRUPT_REASON_SERVER_UNAUTHORIZED:
     case DOWNLOAD_INTERRUPT_REASON_SERVER_CERT_PROBLEM:
     case DOWNLOAD_INTERRUPT_REASON_SERVER_FORBIDDEN:
       mode = RESUME_MODE_INVALID;
       break;
   }
 
   return mode;
 }
 
-void DownloadItemImpl::MergeOriginInfoOnResume(
+void DownloadItemImpl::UpdateValidatorsOnResumption(
     const DownloadCreateInfo& new_create_info) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   DCHECK_EQ(RESUMING_INTERNAL, state_);
-  DCHECK(!new_create_info.url_chain.empty());
 
-  // We are going to tack on any new redirects to our list of redirects.
-  // When a download is resumed, the URL used for the resumption request is the
-  // one at the end of the previous redirect chain. Tacking additional redirects
-  // to the end of this chain ensures that:
-  // - If the download needs to be resumed again, the ETag/Last-Modified headers
-  //   will be used with the last server that sent them to us.
-  // - The redirect chain contains all the servers that were involved in this
-  //   download since the initial request, in order.
-  std::vector<GURL>::const_iterator chain_iter =
-      new_create_info.url_chain.begin();
-  if (*chain_iter == url_chain_.back())
-    ++chain_iter;
+  // Redirects are disallowed for a resumption request.
+  DCHECK_EQ(1u, new_create_info.url_chain.size());
+  DCHECK_EQ(url_chain_.back(), new_create_info.url_chain.back());

[Randy Smith (Not in Mondays), 2016/02/10 21:48:45]

So I could be misunderstand the code, but it seems like this assumption is
dependent on proper behavior of web servers, which I sorta think can't be a good
idea.  Specifically, IIUC, this invariant is believed true because we stored the
final URL in the histogram/DownloadItemImpl on the first attempt, and thus we
shouldn't get any redirects on the second attempt.  But I don't think there's
anything to stop the web server from sticking an extra redirect in between
attempt 1 & 2?  I could also imagine that we had, e.g., a network failure while
we were following the redirect chain, and thus only stored an intermediate URL. 
This might not be true now (i.e. it might be true that on an initial download
attempt any breakage during redirects won't result in a DownlodItemImpl) but if
we're aiming to eventually always create a DII for any download attempt, that
won't be something we can assume in the future.  Thoughts?

[asanka, 2016/02/11 03:43:07]

This is asserted by DownloadRequestCore which interrupts with .. oh wait, that's
https://codereview.chromium.org/1544603003/.

In reality, any validators we've presented only work for one hop since different
servers (or different URLs) don't necessarily honor the same validators. Also,
an unexpected redirect is very likely the result of a meddling middlebox rather
than the target server deciding not to honor the request this time around.

Rather than speculate, the CL mentioned above treats it as an error. But for
this round, I'm restoring the redirect handling since it should be dealt with on
the other CL.

 
   // Record some stats. If the precondition failed (the server returned
   // HTTP_PRECONDITION_FAILED), then the download will automatically retried as
   // a full request rather than a partial. Full restarts clobber validators.
   int origin_state = 0;
-  if (chain_iter != new_create_info.url_chain.end())
-    origin_state |= ORIGIN_STATE_ON_RESUMPTION_ADDITIONAL_REDIRECTS;
   if (etag_ != new_create_info.etag ||
       last_modified_time_ != new_create_info.last_modified)
     origin_state |= ORIGIN_STATE_ON_RESUMPTION_VALIDATORS_CHANGED;
   if (content_disposition_ != new_create_info.content_disposition)
     origin_state |= ORIGIN_STATE_ON_RESUMPTION_CONTENT_DISPOSITION_CHANGED;
   RecordOriginStateOnResumption(new_create_info.save_info->offset != 0,
                                 origin_state);
 
-  url_chain_.insert(
-      url_chain_.end(), chain_iter, new_create_info.url_chain.end());
   etag_ = new_create_info.etag;
   last_modified_time_ = new_create_info.last_modified;
   content_disposition_ = new_create_info.content_disposition;
 
   // Don't update observers. This method is expected to be called just before a
   // DownloadFile is created and Start() is called. The observers will be
   // notified when the download transitions to the IN_PROGRESS state.
 }
 
 void DownloadItemImpl::NotifyRemoved() {
@@ skipping 80 matching lines @@
   UpdateObservers();
 }
 
 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 (current_path_.empty() || target_path_.empty())
     destination_error_ = reason;
   else
-    Interrupt(reason);
+    Interrupt(reason, UPDATE_OBSERVERS);
 }
 
 void DownloadItemImpl::DestinationCompleted(const std::string& final_hash) {
   DVLOG(20) << __FUNCTION__ << " download=" << DebugString(true);
   if (GetState() != IN_PROGRESS)
     return;
   OnAllDataSaved(final_hash);
   MaybeCompleteDownload();
 }
 
@@ skipping 30 matching lines @@
     bound_net_log_.AddEvent(
         net::NetLog::TYPE_DOWNLOAD_ITEM_ACTIVE, active_data);
   }
 
   DVLOG(20) << __FUNCTION__ << "() " << DebugString(true);
 }
 
 // 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());
 
   download_file_ = std::move(file);
   request_handle_ = std::move(req_handle);
 
   if (GetState() == CANCELLED) {
     // The download was in the process of resuming when it was cancelled. Don't
     // proceed.
     ReleaseDownloadFile(true);
     request_handle_->CancelRequest();
     return;
   }
 
+  // The state could be one of the following:
+  //
+  // IN_PROGRESS_INTERNAL: Download started normally.
+  //
+  // RESUMING_INTERNAL: A resumption attempt. May or may not have been
+  //     successful.
+  //
+  // INTERRUPTED_INTERNAL: Download was DOA. (E.g. a programmatic download that
+  //     encountered a network error and didn't progress to the point where a
+  //     server response was available, or the server responded with an error).
+  DCHECK(state_ == IN_PROGRESS_INTERNAL || state_ == RESUMING_INTERNAL ||
+         state_ == INTERRUPTED_INTERNAL);
+
+  // If a resumption attempted failed, then the download goes back to being
+  // interrupted.
+  if (state_ == RESUMING_INTERNAL &&
+      new_create_info.result != DOWNLOAD_INTERRUPT_REASON_NONE) {
+    DCHECK(!download_file_.get());
+
+    Interrupt(new_create_info.result, DONT_UPDATE_OBSERVERS);
+
+    // Transitioning to interrupted may trigger another resumption attempt. If
+    // so, Start() will be called again once a response is available for that
+    // request. Nothing more needs to be done now. Note that Start() is never
+    // called synchronously.
+    if (state_ == RESUMING_INTERNAL)
+      return;
+
+    // Otherwise, the download should now be interrupted.
+    DCHECK_EQ(INTERRUPTED_INTERNAL, state_);
+  }
+
+  if (state_ == INTERRUPTED_INTERNAL) {
+    DCHECK(!download_file_.get());
+    // If the download failed, then skip the download_file_ initialization. It
+    // isn't going to be used anyway.
+    DetermineDownloadTarget();
+    return;
+  }
+
+  // Successful download start.
+  DCHECK(download_file_.get());
+  DCHECK(request_handle_.get());
+
+  if (state_ == RESUMING_INTERNAL)
+    UpdateValidatorsOnResumption(new_create_info);
+
   TransitionTo(IN_PROGRESS_INTERNAL, UPDATE_OBSERVERS);
 
   BrowserThread::PostTask(
       BrowserThread::FILE, FROM_HERE,
       base::Bind(&DownloadFile::Initialize,
                  // Safe because we control download file lifetime.
                  base::Unretained(download_file_.get()),
                  base::Bind(&DownloadItemImpl::OnDownloadFileInitialized,
                             weak_ptr_factory_.GetWeakPtr())));
 }
 
 void DownloadItemImpl::OnDownloadFileInitialized(
     DownloadInterruptReason result) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   if (result != DOWNLOAD_INTERRUPT_REASON_NONE) {
-    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;
+    // The download file failed to initialize.
+    Interrupt(result, DONT_UPDATE_OBSERVERS);
+
+    // The download may transition to RESUMING_INTERNAL as a result of the
+    // Interrupt() call above if it was resumed automatically. If so, then
+    // Start() is expected to be called again and the download progression
+    // cascade will be redone. Therefore we are done for now. Start() is never
+    // called synchronously.
+    if (state_ == RESUMING_INTERNAL)
+      return;
   }
 
+  DetermineDownloadTarget();

[Randy Smith (Not in Mondays), 2016/02/05 00:51:41]

Random thought: Reading through this code, I'm struck by its similarity to the
type of problem DoLoop was designed to solve (basically single threaded control
flow but with places where writing the code as a single function would require
sleeping waiting for async results).  I wonder if we should consider rewriting
this in the DoLoop() idiom.  Low priority, obviously; I'm not really sure it
would aid comprehension.  But there's a certain value to solving the same
problem the same way.

[asanka, 2016/02/11 03:43:07]

This is true. I'm also tempted to pull out the download job handling part out of
DownloadItem so that only active downloads have all the guts needed to drive a
download request. But yeah, that's a CL for another day.

+}
+
+void DownloadItemImpl::DetermineDownloadTarget() {
   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,
     DownloadDangerType danger_type,
     const base::FilePath& intermediate_path) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
   // If the |target_path| is empty, then we consider this download to be
   // canceled.
   if (target_path.empty()) {
     Cancel(true);
     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);
 
   target_path_ = target_path;
   target_disposition_ = disposition;
   SetDangerType(danger_type);
+  if (state_ != IN_PROGRESS_INTERNAL) {
+    DCHECK(!download_file_);
+    // Already interrupted or cancelled. Since there's now a filename for
+    // display purposes, observers can be safely notified of the new [failed]
+    // download. Nothing more needs to be done since there's no active request
+    // or in-use file.

[Randy Smith (Not in Mondays), 2016/02/05 00:51:41]

Possible alternative implementation: Create a new _INTERNAL state which maps to
IN_PROGRESS but the internal code knows means "fail when name is determined".

[asanka, 2016/02/11 03:43:07]

Yup. That's what I ended up doing, partly to deal with the race condition I
mentioned.

+    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.
   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.
   //
@@ skipping 29 matching lines @@
     const base::FilePath& full_path) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   DVLOG(20) << __FUNCTION__ << " download=" << DebugString(true);
 
   if (DOWNLOAD_INTERRUPT_REASON_NONE != destination_error_) {
     // Process destination error.  If both |reason| and |destination_error_|
     // refer to actual errors, we want to use the |destination_error_| as the
     // argument to the Interrupt() routine, as it happened first.
     if (reason == DOWNLOAD_INTERRUPT_REASON_NONE)
       SetFullPath(full_path);
-    Interrupt(destination_error_);
+    Interrupt(destination_error_, UPDATE_OBSERVERS);
     destination_error_ = DOWNLOAD_INTERRUPT_REASON_NONE;
   } else if (DOWNLOAD_INTERRUPT_REASON_NONE != reason) {
-    Interrupt(reason);
+    Interrupt(reason, UPDATE_OBSERVERS);
     // All file errors result in file deletion above; no need to cleanup.  The
     // current_path_ should be empty. Resuming this download will force a
     // restart and a re-doing of filename determination.
     DCHECK(current_path_.empty());
   } else {
     SetFullPath(full_path);
     UpdateObservers();
     MaybeCompleteDownload();
   }
 }
@@ skipping 76 matching lines @@
   // will result in deleting the file on the file thread.  So we don't
   // care about the name having been changed.
   if (state_ != IN_PROGRESS_INTERNAL)
     return;
 
   DVLOG(20) << __FUNCTION__ << "()"
             << " full_path = \"" << full_path.value() << "\""
             << " " << DebugString(false);
 
   if (DOWNLOAD_INTERRUPT_REASON_NONE != reason) {
-    Interrupt(reason);
+    Interrupt(reason, UPDATE_OBSERVERS);
 
     // 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());
     return;
   }
 
   DCHECK(target_path_ == full_path);
 
   if (full_path != current_path_) {
@@ skipping 48 matching lines @@
     // we still need to set it auto-opened so that it can be removed from the
     // download shelf.
     if (!IsTemporary())
       OpenDownload();
 
     auto_opened_ = true;
     UpdateObservers();
   }
 }
 
-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.
-void DownloadItemImpl::Interrupt(DownloadInterruptReason reason) {
+void DownloadItemImpl::Interrupt(
+    DownloadInterruptReason reason,
+    ShouldUpdateObservers should_update_observers) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   DCHECK_NE(DOWNLOAD_INTERRUPT_REASON_NONE, reason);
+  DVLOG(20) << __FUNCTION__
+            << "() reason=" << DownloadInterruptReasonToString(reason);
 
   // Somewhat counter-intuitively, it is possible for us to receive an
   // interrupt after we've already been interrupted.  The generation of
   // interrupts from the file thread Renames and the generation of
   // interrupts from disk writes go through two different mechanisms (driven
   // by rename requests from UI thread and by write requests from IO thread,
   // respectively), and since we choose not to keep state on the File thread,
   // this is the place where the races collide.  It's also possible for
   // interrupts to race with cancels.
 
@@ skipping 34 matching lines @@
   // from the DownloadFile, which would behave properly with setting
   // all_data_saved_ to false here.
   all_data_saved_ = false;
 
   TransitionTo(INTERRUPTED_INTERNAL, DONT_UPDATE_OBSERVERS);
   RecordDownloadInterrupted(reason, received_bytes_, total_bytes_);
   if (!GetWebContents())
     RecordDownloadCount(INTERRUPTED_WITHOUT_WEBCONTENTS);
 
   AutoResumeIfValid();
-  UpdateObservers();
+  if (should_update_observers == UPDATE_OBSERVERS)
+    UpdateObservers();
 }
 
 void DownloadItemImpl::ReleaseDownloadFile(bool destroy_file) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
   if (destroy_file) {
     BrowserThread::PostTask(
         BrowserThread::FILE, FROM_HERE,
         // Will be deleted at end of task execution.
         base::Bind(&DownloadFileCancel, base::Passed(&download_file_)));
@@ skipping 171 matching lines @@
 
 void DownloadItemImpl::ResumeInterruptedDownload() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   if (!IsDownloadResumptionEnabled())
     return;
 
   // If we're not interrupted, ignore the request; our caller is drunk.
   if (state_ != INTERRUPTED_INTERNAL)
     return;
 
+  // We are starting a new request. Shake off all pending operations.
+  DCHECK(!download_file_);
+  weak_ptr_factory_.InvalidateWeakPtrs();
+
+  TransitionTo(RESUMING_INTERNAL, DONT_UPDATE_OBSERVERS);
+  BrowserThread::PostTask(
+      BrowserThread::UI, FROM_HERE,
+      base::Bind(&DownloadItemImpl::InvokeDelegateForResumption,
+                 weak_ptr_factory_.GetWeakPtr()));
+}
+
+void DownloadItemImpl::InvokeDelegateForResumption() {

[Randy Smith (Not in Mondays), 2016/02/05 00:51:41]

Why replacing the sync invocation with an async invocation?

[asanka, 2016/02/11 03:43:07]

This CL relies on Start() never being called synchronously. I added the extra
async hop so that we can enforce this from within DII rather than relying on
callers doing the right thing.

However, we are no longer relying on that due to the extra internal states that
I added for the new patchset. So I've removed the extra hop.

+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  // A state change happend while we were out. This could be because of a
+  // Cancel() call. Since we aren't resuming anymore, there's nothing else to
+  // do.
+  if (state_ != RESUMING_INTERNAL)
+    return;
+
   // Reset the appropriate state if restarting.
   ResumeMode mode = GetResumeMode();
   if (mode == RESUME_MODE_IMMEDIATE_RESTART ||
       mode == RESUME_MODE_USER_RESTART) {
     received_bytes_ = 0;
     hash_state_ = "";
     last_modified_time_ = "";
     etag_ = "";
   }
 
   scoped_ptr<DownloadUrlParameters> download_params;
   if (GetWebContents()) {
     download_params =
         DownloadUrlParameters::FromWebContents(GetWebContents(), GetURL());
   } else {
     download_params = make_scoped_ptr(new DownloadUrlParameters(
         GetURL(), -1, -1, -1, GetBrowserContext()->GetResourceContext()));
   }
 
   download_params->set_file_path(GetFullPath());
   download_params->set_offset(GetReceivedBytes());
   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(std::move(download_params), GetId());
   // Just in case we were interrupted while paused.
   is_paused_ = false;
-
-  TransitionTo(RESUMING_INTERNAL, DONT_UPDATE_OBSERVERS);
 }
 
 // static
 DownloadItem::DownloadState DownloadItemImpl::InternalToExternalState(
     DownloadInternalState internal_state) {
   switch (internal_state) {
     case IN_PROGRESS_INTERNAL:
       return IN_PROGRESS;
     case COMPLETING_INTERNAL:
       return IN_PROGRESS;
@@ skipping 64 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