Refactor ResourceHandler API.

content/browser/loader/resource_handler.h

Index: content/browser/loader/resource_handler.h
diff --git a/content/browser/loader/resource_handler.h b/content/browser/loader/resource_handler.h
index a6cc6822c691fc0ae706d25ae566b7bc65fb4655..c000b5b2bd5d2d5597d8e3687e5851d40f2eecfa 100644
--- a/content/browser/loader/resource_handler.h
+++ b/content/browser/loader/resource_handler.h
@@ -12,12 +12,14 @@
 #ifndef CONTENT_BROWSER_LOADER_RESOURCE_HANDLER_H_
 #define CONTENT_BROWSER_LOADER_RESOURCE_HANDLER_H_
 
+#include <memory>
 #include <string>
 
 #include "base/compiler_specific.h"
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
 #include "base/threading/non_thread_safe.h"
+#include "content/browser/loader/resource_controller.h"
 #include "content/common/content_export.h"
 
 class GURL;
@@ -30,7 +32,6 @@ struct RedirectInfo;
 }  // namespace net
 
 namespace content {
-class ResourceController;
 class ResourceMessageFilter;
 class ResourceRequestInfoImpl;
 struct ResourceResponse;
@@ -43,30 +44,50 @@ class CONTENT_EXPORT ResourceHandler
  public:
   virtual ~ResourceHandler();
 
-  // Sets the controller for this handler.
-  virtual void SetController(ResourceController* controller);
-
-  // The request was redirected to a new URL.  |*defer| has an initial value of
-  // false.  Set |*defer| to true to defer the redirect.  The redirect may be
-  // followed later on via ResourceDispatcherHost::FollowDeferredRedirect.  If
-  // the handler returns false, then the request is cancelled.
-  virtual bool OnRequestRedirected(const net::RedirectInfo& redirect_info,
-                                   ResourceResponse* response,
-                                   bool* defer) = 0;
-
-  // Response headers and meta data are available.  If the handler returns
-  // false, then the request is cancelled.  Set |*defer| to true to defer
-  // processing of the response.  Call ResourceDispatcherHostImpl::
-  // ResumeDeferredRequest to continue processing the response.
-  virtual bool OnResponseStarted(ResourceResponse* response, bool* defer) = 0;
+  class CONTENT_EXPORT Delegate {
+   protected:
+    Delegate();
+    virtual ~Delegate();
+
+   private:
+    friend class ResourceHandler;
+
+    // Cancels the request when the class does not currently have ownership of
+    // the
+    // ResourceController.
+    // |error_code| indicates the reason for the cancellation, and
+    // |tell_renderer| whether the renderer needs to be informed of the
+    // cancellation.
+    virtual void OutOfBandCancel(int error_code, bool tell_renderer) = 0;
+
+    DISALLOW_COPY_AND_ASSIGN(Delegate);
+  };
+
+  // Sets the ResourceHandler::Delegate, which handles out-of-bounds
+  // cancellation.
+  virtual void SetDelegate(Delegate* delegate);
+
+  // The request was redirected to a new URL.  The request will not continue
+  // until one of |controller|'s resume or cancellation methods is invoked, at
+  // which point the request will be resumed synchronously.

[Randy Smith (Not in Mondays), 2016/12/31 22:00:25]

I'm struggling with the actual behavior and the appropriate documentation around
the resumption.  The implementation of Resume in ResourceLoader::Controller
looks asynchronous to me (it does a PostTask).  But I'm not sure whether that's
something that should be documented at this level or not.  But if it shouldn't
be, I don't understand what describing the request as being resumed
synchronously means.  WDYT?

[mmenke, 2017/01/06 18:46:58]

There isn't a PostTask:  If we call Resume() immediately on the
ResourceLoader::Controller (Which is in state DEFERRED_SYNC), the controller
does nothing but update its state.  Then when we're back up the stack in
ResourceLoader, it synchronously continues the request.

If we call Resume() asynchronously, then ResourceLoader::Resume() will resume
the request synchronously in exactly the case where it's called from the
ResourceLoader.  However, we're careful not to call back into a ResourceHandler
in that case.

That having been said, that seems more an implementation detail that the RH
shouldn't care about.  All it should know is that it won't be called into
synchronously, so I've updated the comments.

+  virtual void OnRequestRedirected(
+      const net::RedirectInfo& redirect_info,
+      ResourceResponse* response,
+      std::unique_ptr<ResourceController> controller) = 0;
+
+  // Response headers and metadata are available.  The request will not continue
+  // until one of |controller|'s resume or cancellation methods is invoked, at
+  // which point the request will be resumed synchronously.
+  virtual void OnResponseStarted(
+      ResourceResponse* response,
+      std::unique_ptr<ResourceController> controller) = 0;
 
   // Called before the net::URLRequest (whose url is |url|) is to be started.
-  // If the handler returns false, then the request is cancelled.  Otherwise if
-  // the return value is true, the ResourceHandler can delay the request from
-  // starting by setting |*defer = true|.  A deferred request will not have
-  // called net::URLRequest::Start(), and will not resume until someone calls
-  // ResourceDispatcherHost::StartDeferredRequest().
-  virtual bool OnWillStart(const GURL& url, bool* defer) = 0;
+  // The request will not continue until one of |controller|'s resume or
+  // cancellation methods is invoked, at which point the request will be started
+  // synchronously.
+  virtual void OnWillStart(const GURL& url,
+                           std::unique_ptr<ResourceController> controller) = 0;
 
   // Data will be read for the response.  Upon success, this method places the
   // size and address of the buffer where the data is to be written in its
@@ -78,22 +99,27 @@ class CONTENT_EXPORT ResourceHandler
   //
   // If the handler returns false, then the request is cancelled.  Otherwise,
   // once data is available, OnReadCompleted will be called.
+  // TODO(mmenke):  Make this method use a ResourceController, and allow it to
+  // succeed asynchronously.
   virtual bool OnWillRead(scoped_refptr<net::IOBuffer>* buf,
                           int* buf_size,
                           int min_size) = 0;
 
   // Data (*bytes_read bytes) was written into the buffer provided by
-  // OnWillRead.  A return value of false cancels the request, true continues
-  // reading data.  Set |*defer| to true to defer reading more response data.
-  // Call controller()->Resume() to continue reading response data.  A zero
-  // |bytes_read| signals that no further data is available.
-  virtual bool OnReadCompleted(int bytes_read, bool* defer) = 0;
-
-  // The response is complete.  The final response status is given.  Set
-  // |*defer| to true to defer destruction to a later time.  Otherwise, the
-  // request will be destroyed upon return.
-  virtual void OnResponseCompleted(const net::URLRequestStatus& status,
-                                   bool* defer) = 0;
+  // OnWillRead.  The request will not continue until one of |controller|'s
+  // resume or cancellation methods is invoked, at which point the request will
+  // be resumed synchronously.  A zero |bytes_read| signals that no further data
+  // is available.
+  virtual void OnReadCompleted(
+      int bytes_read,
+      std::unique_ptr<ResourceController> controller) = 0;
+
+  // The response is complete.  The final response status is given.  The request
+  // will not be deleted until controller's Resume() method is invoked. It is
+  // illegal to use its cancellation methods.
+  virtual void OnResponseCompleted(
+      const net::URLRequestStatus& status,
+      std::unique_ptr<ResourceController> controller) = 0;
 
   // This notification is synthesized by the RedirectToFileResourceHandler
   // to indicate progress of 'download_to_file' requests. OnReadCompleted
@@ -102,9 +128,32 @@ class CONTENT_EXPORT ResourceHandler
   virtual void OnDataDownloaded(int bytes_downloaded) = 0;
 
  protected:
-  ResourceHandler(net::URLRequest* request);
+  explicit ResourceHandler(net::URLRequest* request);
+
+  //  Utility methods for managing a ResourceHandler's controller in the async
+  //  completion case. These ensure that the controller is nullptr after being
+  //  invoked, which allows for DCHECKing on it and better crashes on calling
+  //  into deleting objects.
+
+  void set_controller(std::unique_ptr<ResourceController> controller) {
+    controller_ = std::move(controller);
+  }
+
+  bool has_controller() const { return !!controller_; }
+
+  std::unique_ptr<ResourceController> TakeController();
+
+  // These call the corresponding methods on the previously set
+  // ResourceController, and then destroy the controller.
+  void Resume();
+  void Cancel();
+  void CancelAndIgnore();
+  void CancelWithError(int error_code);
+
+  // Cancels the request when the class does not currently have ownership of the
+  // ResourceController.
+  void OutOfBandCancel(int error_code, bool tell_renderer);
 
-  ResourceController* controller() const { return controller_; }
   net::URLRequest* request() const { return request_; }
 
   // Convenience functions.
@@ -112,9 +161,12 @@ class CONTENT_EXPORT ResourceHandler
   int GetRequestID() const;
   ResourceMessageFilter* GetFilter() const;
 
+  Delegate* delegate() { return delegate_; }
+
  private:
-  ResourceController* controller_;
   net::URLRequest* request_;
+  Delegate* delegate_;
+  std::unique_ptr<ResourceController> controller_;
 
   DISALLOW_COPY_AND_ASSIGN(ResourceHandler);
 };