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

content/browser/download/download_request_core.h

Index: content/browser/download/download_request_core.h
diff --git a/content/browser/download/download_request_core.h b/content/browser/download/download_request_core.h
index 22ca862ef85fae62508046d270652178b625d67d..7766760de8417418617838c92c359f47689bf15a 100644
--- a/content/browser/download/download_request_core.h
+++ b/content/browser/download/download_request_core.h
@@ -20,7 +20,9 @@
 #include "content/public/browser/download_url_parameters.h"
 
 namespace net {
+class HttpResponseHeaders;
 class URLRequest;
+class URLRequestStatus;
 }  // namespace net
 
 namespace content {
@@ -38,29 +40,23 @@ struct DownloadCreateInfo;
 class CONTENT_EXPORT DownloadRequestCore
     : public base::SupportsWeakPtr<DownloadRequestCore> {
  public:
-  // Size of the buffer used between the DownloadRequestCore and the
-  // downstream receiver of its output.
-  static const int kDownloadByteStreamSize;
-
-  // |request| *must* outlive the DownloadRequestCore. |save_info| must be
-  // valid.
-  //
-  // Invokes |on_ready_to_read_callback| if a previous call to OnReadCompleted()
-  // resulted in |defer| being set to true, and DownloadRequestCore is now ready
-  // to commence reading.
-  DownloadRequestCore(net::URLRequest* request,
-                      scoped_ptr<DownloadSaveInfo> save_info,
-                      const base::Closure& on_ready_to_read_callback);
+  class Delegate {
+   public:
+    virtual void OnReadyToRead() = 0;
+    virtual void OnStart(

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

Suggestion/musing: I'm chewing on naming here.  There are a couple of different
meanings of Start at different points in this code; starting the request, the
response starting, and OnStart() being called (which can happen from
OnResponseCompleted() if OnResponseStarted() was never called), so part of me
wants a different name that's more specific; OnDownloadStart?  OnDownloadCommit
(to indicate that the download may be stillborn)?  What are your thoughts?

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

I threw one out a bit earlier which was OnResponseReady(). If that's clear, then
I can rename it along the whole stack so that it's consistent.

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

I think I like it better, so I vote in favor, but if you have a preference for
OnStart(), that's ok by me.

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

Presuming not changing this was a conscious choice, which is fine, but wanting
to make sure since you didn't respond.

[asanka, 2016/02/12 18:31:46]

Oops. I missed this in my previous pass. Shall I do the rename in a follow up?
It involves some churn since we are also trying to get rid of the
DownloadManager::StartDownload() interface (which is part of the rename stack).

+        scoped_ptr<DownloadCreateInfo> download_create_info,
+        scoped_ptr<ByteStreamReader> stream_reader,
+        const DownloadUrlParameters::OnStartedCallback& callback) = 0;
+  };
+
+  // All parameters are required. |request| and |delegate| must outlive
+  // DownloadRequestCore.
+  DownloadRequestCore(net::URLRequest* request, Delegate* delegate);
   ~DownloadRequestCore();
 
   // Should be called when the URLRequest::Delegate receives OnResponseStarted.
-  // Constructs a DownloadCreateInfo and a ByteStreamReader that should be
-  // passed into DownloadManagerImpl::StartDownload().
-  //
-  // Only populates the response derived fields of DownloadCreateInfo, with the
-  // exception of |save_info|.
-  void OnResponseStarted(scoped_ptr<DownloadCreateInfo>* info,
-                         scoped_ptr<ByteStreamReader>* stream_reader);
+  // Invokes Delegate::OnStart() with download start parameters.
+  bool OnResponseStarted();
 
   // Starts a read cycle. Creates a new IOBuffer which can be passed into
   // URLRequest::Read(). Call OnReadCompleted() when the Read operation
@@ -74,15 +70,19 @@ class CONTENT_EXPORT DownloadRequestCore
   // can be read, invokes the |on_ready_to_read_callback|.
   bool OnReadCompleted(int bytes_read, bool* defer);
 
-  // Called to signal that the response is complete. If the return value is
-  // something other than DOWNLOAD_INTERRUPT_REASON_NONE, then the download
-  // should be considered interrupted.
+  // Called to signal that the response is complete.
   //
   // It is expected that once this method is invoked, the DownloadRequestCore
   // object will be destroyed in short order without invoking any other methods
   // other than the destructor.
-  DownloadInterruptReason OnResponseCompleted(
-      const net::URLRequestStatus& status);
+  void OnResponseCompleted(const net::URLRequestStatus& status);
+
+  // Used to notify DownloadRequestCore that the caller is about to abort the
+  // outer request. |reason| will be used as the final interrupt reason when
+  // OnResponseCompleted() is called.
+  void OnWillAbort(DownloadInterruptReason reason);
+
+  void OnAbortedBeforeStart(DownloadInterruptReason reason);
 
   // Called if the request should suspend reading. A subsequent
   // OnReadCompleted() will result in |defer| being set to true.
@@ -98,13 +98,35 @@ class CONTENT_EXPORT DownloadRequestCore
 
   std::string DebugString() const;
 
+  static scoped_ptr<net::URLRequest> CreateRequestOnIOThread(
+      uint32_t download_id,
+      DownloadUrlParameters* params);
+
+  // Size of the buffer used between the DownloadRequestCore and the
+  // downstream receiver of its output.
+  static const int kDownloadByteStreamSize;
+
  protected:
   net::URLRequest* request() const { return request_; }
 
  private:
-  base::Closure on_ready_to_read_callback_;
+  static DownloadInterruptReason HandleRequestStatus(
+      const net::URLRequestStatus& status);
+
+  static DownloadInterruptReason HandleSuccessfulServerResponse(
+      const net::HttpResponseHeaders& http_headers,
+      DownloadSaveInfo* save_info);
+
+  scoped_ptr<DownloadCreateInfo> CreateDownloadCreateInfo(
+      DownloadInterruptReason result);
+
+  Delegate* delegate_;
   net::URLRequest* request_;
+
+  // Passthrough

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

What does "Passthrough" mean in this context?

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

That these fields aren't used by DownloadRequestCore. They are just here so that
they can be passed along in the DownloadCreateInfo. These are some of the things
that will go away when we can create the DownloadItem first.

I've updated the comment to make it clearer.

   scoped_ptr<DownloadSaveInfo> save_info_;
+  uint32_t download_id_;
+  DownloadUrlParameters::OnStartedCallback on_started_callback_;
 
   // Data flow
   scoped_refptr<net::IOBuffer> read_buffer_;    // From URLRequest.
@@ -125,6 +147,15 @@ class CONTENT_EXPORT DownloadRequestCore
 
   int pause_count_;
   bool was_deferred_;
+  bool is_resumption_request_;
+  bool started_;
+
+  // When DownloadRequestCore initiates an abort (by blocking a redirect, for
+  // example) it expects to eventually receive a OnResponseCompleted() with a
+  // status indicating that the request was aborted. When this happens, the
+  // interrupt reason in |abort_reason_| will be used instead of USER_CANCELED
+  // which is vague.
+  DownloadInterruptReason abort_reason_;
 
   // Each successful OnWillRead will yield a buffer of this size.
   static const int kReadBufSize = 32768;   // bytes