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

content/browser/download/download_item_impl.h

 // 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.
 
 #ifndef CONTENT_BROWSER_DOWNLOAD_DOWNLOAD_ITEM_IMPL_H_
 #define CONTENT_BROWSER_DOWNLOAD_DOWNLOAD_ITEM_IMPL_H_
 
 #include <stdint.h>
 
 #include <string>
@@ skipping 149 matching lines @@
   std::string DebugString(bool verbose) const override;
 
   // All remaining public interfaces virtual to allow for DownloadItemImpl
   // mocks.
 
   // Determines the resume mode for an interrupted download. Requires
   // last_reason_ to be set, but doesn't require the download to be in
   // 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 -------------------
 
   // TODO(rdsmith): Unwind DownloadManagerImpl and DownloadItemImpl,
   // removing these from the public interface.
 
   // Notify observers that this item is being removed by the user.
   virtual void NotifyRemoved();
 
   virtual void OnDownloadedFileRemoved();
@@ skipping 19 matching lines @@
 
   // DownloadDestinationObserver
   void DestinationUpdate(int64_t bytes_so_far,
                          int64_t bytes_per_sec,
                          const std::string& hash_state) override;
   void DestinationError(DownloadInterruptReason reason) override;
   void DestinationCompleted(const std::string& final_hash) override;
 
  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.
 
   enum DownloadInternalState {
     // Includes both before and after file name determination, and paused
     // downloads.
     // TODO(rdsmith): Put in state variable for file name determination.
     // Transitions from:
     //   <Initial creation>    Active downloads are created in this state.
     //   RESUMING_INTERNAL
@@ skipping 30 matching lines @@
     //   <Initial creation>    Canceleld persisted downloads.
     // Transitions to:
     //   <none>                Terminal state.
     CANCELLED_INTERNAL,
 
     // An error has interrupted the download.
     // Transitions from:
     //   IN_PROGRESS_INTERNAL
     //   RESUMING_INTERNAL
     //   <Initial creation>    Interrupted persisted downloads.
+    //   <Initial creation>    Failed new downloads.
     // Transitions to:
     //   RESUMING_INTERNAL     On resumption.
     INTERRUPTED_INTERNAL,
 
     // A request to resume this interrupted download is in progress.
     // Transitions from:
     //   INTERRUPTED_INTERNAL
     // Transitions to:
     //   IN_PROGRESS_INTERNAL  Once a server response is received from a
     //                         resumption.
@@ skipping 16 matching lines @@
   // These are listed in approximately chronological order.  There are also
   // public methods involved in normal download progression; see
   // the implementation ordering in download_item_impl.cc.
 
   // Construction common to all constructors. |active| should be true for new
   // downloads and false for downloads from the history.
   // |download_type| indicates to the net log system what kind of download
   // 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
   // |target_path| as determined by the caller. |intermediate_path| is the path
   // to use to store the download until OnDownloadCompleting() is called.
   virtual void OnDownloadTargetDetermined(
       const base::FilePath& target_path,
       TargetDisposition disposition,
       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);
 
   // If all pre-requisites have been met, complete download processing, i.e. do
   // internal cleanup, file rename, and potentially auto-open.  (Dangerous
   // downloads still may block on user acceptance after this point.)
   void MaybeCompleteDownload();
 
   // Called when the download is ready to complete.
   // This may perform final rename if necessary and will eventually call
   // DownloadItem::Completed().
   void OnDownloadCompleting();
 
   void OnDownloadRenamedToFinalName(DownloadInterruptReason reason,
                                     const base::FilePath& full_path);
 
   // Called if the embedder took over opening a download, to indicate that
   // the download has been opened.
   void DelayedDownloadOpened(bool auto_opened);
 
   // Called when the entire download operation (including renaming etc)
   // 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
   // object destruction to prevent file destruction. Destroying the file also
   // resets |current_path_|.
   void ReleaseDownloadFile(bool destroy_file);
 
   // Check if a download is ready for completion.  The callback provided
   // may be called at some point in the future if an external entity
   // state has change s.t. this routine should be checked again.
   bool IsDownloadReadyForCompletion(const base::Closure& state_change_notify);
 
   // Call to transition state; all state transitions should go through this.
   // |notify_action| specifies whether or not to call UpdateObservers() after
   // the state transition.
   void TransitionTo(DownloadInternalState new_state,
                     ShouldUpdateObservers notify_action);

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

This makes me architecturally happy but concerned that it's a big move with
potential gotchas for this CL; if you felt like this change was too risk in this
context, I'm fine with you not doing it.  (See above comment for context; it was
written before my eyes got here.  The meta-point is that I think you and I
should avoid letting me push you into making good architectural changes that
aren't directly relevant to this CL, given the time constraints.)

ETA: I now perceive you as having gone from this model to one in which any state
transition needs to be explicitly evaluated for whether it should have an
UpdateObservers() following it.  Looking only at TransitionTo() I like the old
situation better, as the argument list forced new uses of the function to think
about the issue, where now people may put in TransitionTo()'s without thinking
about it.  Coupled with the Interrupt() change above, I'm not sure what the
right thing is, but I'm a little tempted to go back to the state before I
started making noise and suggest that you/you&I re-address this question in a
future CL with an eye towards what the right overall architecture should be.

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

Hmm. I can revert if the change makes the CL harder to review. I'll wait for
your input on that.

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

I think we decided offline to let this be.

 
   // Set the |danger_type_| and invoke obserers if necessary.
   void SetDangerType(DownloadDangerType danger_type);
 
   void SetFullPath(const base::FilePath& new_path);
 
   void AutoResumeIfValid();
 
   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(
       DownloadState external_state);
 
   // Debugging routines --------------------------------------------------------
   static const char* DebugDownloadStateString(DownloadInternalState state);
   static const char* DebugResumeModeString(ResumeMode mode);
 
   // Will be false for save package downloads retrieved from the history.
@@ skipping 162 matching lines @@
   const net::BoundNetLog bound_net_log_;
 
   base::WeakPtrFactory<DownloadItemImpl> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(DownloadItemImpl);
 };
 
 }  // namespace content
 
 #endif  // CONTENT_BROWSER_DOWNLOAD_DOWNLOAD_ITEM_IMPL_H_