[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..bbc821f4cb0747d6aecebd75284dfe4dcb499a1d 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(
+        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);

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

Um ... this is probably my being clueless, but I can't find callers for either
of these routines in this CL (searching the raw diff).  Where are they called
from?

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

Ping?  git grep on a patched checkout seems to agree with the raw diff search.

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

Removed. There are for the dependent CL.

 
   // Called if the request should suspend reading. A subsequent
   // OnReadCompleted() will result in |defer| being set to true.
@@ -98,13 +98,36 @@ 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" fields. These are only kept here so that they can be used to
+  // populate the DownloadCreateInfo when the time comes.

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

nit, suggestion: Given that we have the URLRequest and these fields are the ones
we're putting in the UserData on the URLRequest, could we grab them from there
when populating the DCI?  This is a little wonky since the DRC makes more
conceptual sense than UserData on URLRequest, but I think that storing in one
place is better than storing in two.

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

Ping?

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

I started out doing that, but it ended up not being quite as clean as I wanted
because of requests that don't go through CreateRequestOnIOThread()

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

Ok with this not being in this CL, but: Aren't those exactly the requests where
these fields aren't needed (i.e. the ones in which we're getting a new request
from the renderers, and thus none of save_info_, id, or the callback is used)?

[asanka, 2016/02/12 20:52:28]

True. But there would also not be a UserData. So we'd end up having to check a
few places whether there's a UserData or not. The alternative is to use the
default (empty) values for these fields when there are no explicit values to
use. That's what we are doing in this CL.

   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 +148,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