[Downloads] Always call DM::StartDownload() for explicit downloads.

content/browser/download/download_item_impl.h

Index: content/browser/download/download_item_impl.h
diff --git a/content/browser/download/download_item_impl.h b/content/browser/download/download_item_impl.h
index c527861287f8acf546e5822f8ae31c54ebae8f3a..b5b9fed34e2190a722f345ab30f6c9d73118018d 100644
--- a/content/browser/download/download_item_impl.h
+++ b/content/browser/download/download_item_impl.h
@@ -167,18 +167,18 @@ class CONTENT_EXPORT DownloadItemImpl
   // INTERRUPTED state.
   virtual ResumeMode GetResumeMode() const;
 
-  // Notify the download item that new origin information is available due to a
-  // resumption request receiving a response.
-  virtual void MergeOriginInfoOnResume(
-      const DownloadCreateInfo& new_create_info);
-
   // State transition operations on regular downloads --------------------------
 
   // Start the download.
   // |download_file| is the associated file on the storage medium.
   // |req_handle| is the new request handle associated with the download.
+  // |new_create_info| is a DownloadCreateInfo containing the new response
+  // parameters. It may be different from the DownloadCreateInfo used to create
+  // the DownloadItem if Start() is being called in response for a download
+  // resumption request.
   virtual void Start(scoped_ptr<DownloadFile> download_file,
-                     scoped_ptr<DownloadRequestHandleInterface> req_handle);
+                     scoped_ptr<DownloadRequestHandleInterface> req_handle,
+                     const DownloadCreateInfo& new_create_info);
 
   // Needed because of intertwining with DownloadManagerImpl -------------------
 
@@ -218,7 +218,8 @@ class CONTENT_EXPORT DownloadItemImpl
 
  private:
   // Fine grained states of a download. Note that active downloads are created
-  // in IN_PROGRESS_INTERNAL state. However, downloads creates via history can
+  // in IN_PROGRESS_INTERNAL state. Failed downloads are created in
+  // INTERRUPTED_INTERNAL state. However, downloads creates via history can
   // be created in COMPLETE_INTERNAL, CANCELLED_INTERNAL and
   // INTERRUPTED_INTERNAL.
 
@@ -269,6 +270,7 @@ class CONTENT_EXPORT DownloadItemImpl
     //   IN_PROGRESS_INTERNAL
     //   RESUMING_INTERNAL
     //   <Initial creation>    Interrupted persisted downloads.
+    //   <Initial creation>    Failed new downloads.
     // Transitions to:
     //   RESUMING_INTERNAL     On resumption.
     INTERRUPTED_INTERNAL,
@@ -305,6 +307,13 @@ class CONTENT_EXPORT DownloadItemImpl
   // this is.
   void Init(bool active, DownloadType download_type);
 
+  // Callback from file thread when we initialize the DownloadFile.
+  void OnDownloadFileInitialized(DownloadInterruptReason result);
+
+  // Called to determine the target path. Will cause OnDownloadTargetDetermined
+  // to be called when the target information is available.
+  void DetermineDownloadTarget();
+
   // Called when the target path has been determined. |target_path| is the
   // suggested target path. |disposition| indicates how the target path should
   // be used (see TargetDisposition). |danger_type| is the danger level of
@@ -316,9 +325,6 @@ class CONTENT_EXPORT DownloadItemImpl
       DownloadDangerType danger_type,
       const base::FilePath& intermediate_path);
 
-  // Callback from file thread when we initialize the DownloadFile.
-  void OnDownloadFileInitialized(DownloadInterruptReason result);
-
   void OnDownloadRenamedToIntermediateName(
       DownloadInterruptReason reason, const base::FilePath& full_path);
 
@@ -343,14 +349,11 @@ class CONTENT_EXPORT DownloadItemImpl
   // is completed.
   void Completed();
 
-  // Callback invoked when the URLRequest for a download resumption has started.
-  void OnResumeRequestStarted(DownloadItem* item,
-                              DownloadInterruptReason interrupt_reason);
-
   // Helper routines -----------------------------------------------------------
 
   // Indicate that an error has occurred on the download.
-  void Interrupt(DownloadInterruptReason reason);
+  void Interrupt(DownloadInterruptReason reason,
+                 ShouldUpdateObservers should_update_observers);

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

I'll just note that I thought of the internal/external state split as solving
the same problem that this flag does (state transitions that shouldn't be
exposed to consumers are between internal states that map to the same external
states) and I wince a bit at having two mechanisms for the same purpose.  If you
still think this is needed after handling the race we talked about offline,
could you share with me your thoughts on the "two mechanisms to handle the same
problem" issue?

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

Good point. I removed the ShouldUpdateObservers logic and made explicit
UpdateObservers() calls where we are making externally visible changes.

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

*nod*  I had forgotten that TransitionTo() had that argument, which I have the
same concern about (though as I say elsewhere, I'm probably responsible for it
:-}); another way to think about it is that not updating observers hides you
from observers, but leaves you exposed to consumers who poll your state 
unexpectedly, which seems like the wrong choice.  In my (current?) ideal world,
we'd update observers whenever a TransitionTo() changed between internal states
that corresponded to different external states, and not otherwise, and make the
state diagram work with that logic.  But this is really solidly "not in this
CL"; you're getting these comments because this CL is <obiwan>making me engage
with the downloads architecture in a way I haven't in a long time</obiwan>.

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

I think the problem is that having things like TransitionTo() automatically
update observers make the DownloadItem state non-deterministic. Once we call
UpdateObservers, technically we don't even know whether the DownloadItem is
still around let alone in the state that we specified as an argument to
TransitionTo. The change I introduce in the next CL is to only invoke
UpdateObservers() at points where:

  a) the embedder would need to know the state transition.
  b) we are yielding the state machine to external signals anyway, and hence
there's no harm in an observer calling a method that changes state.

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

SGTM.   I'd like to be careful (at some point; I don't think we have to worry
about this now) that we're not relying on not calling UpdateObservers to save us
over some other yield point, but the fact that UpdateObservers() is a yield
point makes this unavoidably more complicated.

 
   // Destroy the DownloadFile object.  If |destroy_file| is true, the file is
   // destroyed with it.  Otherwise, DownloadFile::Detach() is called before
@@ -378,6 +381,13 @@ class CONTENT_EXPORT DownloadItemImpl
 
   void ResumeInterruptedDownload();
 
+  void InvokeDelegateForResumption();
+
+  // Update origin information based on the response to a download resumption
+  // request. Should only be called if the resumption request was successful.
+  virtual void UpdateValidatorsOnResumption(
+      const DownloadCreateInfo& new_create_info);
+
   static DownloadState InternalToExternalState(
       DownloadInternalState internal_state);
   static DownloadInternalState ExternalToInternalState(