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..e27e25c29c5cce8ad5a799b887af4eb2f4a3d5dd 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;
@@ -38,35 +39,61 @@ struct ResourceResponse;
 // The resource dispatcher host uses this interface to process network events
 // for an URLRequest instance.  A ResourceHandler's lifetime is bound to its
 // associated URLRequest.
+//
+// No ResourceHandler method will ever be called synchronously when it calls
+// into the ResourceController passed in to it, either to resume or cancel the
+// request.
 class CONTENT_EXPORT ResourceHandler
     : public NON_EXPORTED_BASE(base::NonThreadSafe) {
  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 {

[Charlie Harrison, 2017/01/25 20:23:00]

A class comment describing the delegate would be nice (essentially, to allow for
OOB cancel). Optionally, I would also support renaming this from the generic
"Delegate" to something a bit more specific.
"ResourceHandlerOutOfBandCanceller"? Might be too far, but my general point is
that scanning through a class that implements ResourceHandler::Delegate doesn't
say much without clicking through.

[mmenke, 2017/01/25 22:07:59]

Done.  Just added a comment.

+   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.
+  // |response| may be destroyed as soon as the method returns, so if the
+  // ResourceHandler wants to continue to use it, it must maintain a reference
+  // to it.
+  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.
+  // |response| may be destroyed as soon as the method returns, so if the
+  // ResourceHandler wants to continue to use it, it must maintain a reference
+  // to it.
+  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.
+  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 +105,26 @@ 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.  A zero |bytes_read| signals
+  // that no further data will be received.
+  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 +133,34 @@ class CONTENT_EXPORT ResourceHandler
   virtual void OnDataDownloaded(int bytes_downloaded) = 0;
 
  protected:
-  ResourceHandler(net::URLRequest* request);
+  explicit ResourceHandler(net::URLRequest* request);
+
+  // Convenience 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 deleted objects.
+
+  // Passes ownership of |controller| to |this|. Nothing is done with the
+  // |controller| automatically.
+  void HoldController(std::unique_ptr<ResourceController> controller);
+
+  // Returns ownership of the ResourceController previously passed in to
+  // HoldController.
+  std::unique_ptr<ResourceController> ReleaseController();
+
+  bool has_controller() const { return !!controller_; }
+
+  // These call the corresponding methods on the ResourceController previously
+  // oassed to HoldController and then destroy it.

[Charlie Harrison, 2017/01/25 20:23:00]

s/oassed/passed

[mmenke, 2017/01/25 22:07:59]

Done.

+  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 +168,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);
 };