Refactor ResourceHandler API.

content/browser/loader/resource_handler.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.
 
 // This is the browser side of the resource dispatcher, it receives requests
 // from the RenderProcessHosts, and dispatches them to URLRequests. It then
 // fowards the messages from the URLRequests back to the correct process for
 // handling.
 //
 // See http://dev.chromium.org/developers/design-documents/multi-process-resource-loading
 
 #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;
 
 namespace net {
 class IOBuffer;
 class URLRequest;
 class URLRequestStatus;
 struct RedirectInfo;
 }  // namespace net
 
 namespace content {
-class ResourceController;
 class ResourceMessageFilter;
 class ResourceRequestInfoImpl;
 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.
 class CONTENT_EXPORT ResourceHandler
     : public NON_EXPORTED_BASE(base::NonThreadSafe) {
  public:
   virtual ~ResourceHandler();
 
-  // Sets the controller for this handler.
-  virtual void SetController(ResourceController* controller);
+  class CONTENT_EXPORT Delegate {
+   protected:
+    Delegate();
+    virtual ~Delegate();
 
-  // 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;
+   private:
+    friend class ResourceHandler;
 
-  // 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;
+    // 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.
+  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
   // out-params.  This call will be followed by either OnReadCompleted (on
   // successful read or EOF) or OnResponseCompleted (on error).  If
   // OnReadCompleted is called, the buffer may be recycled.  Otherwise, it may
   // not be recycled and may potentially outlive the handler.  If |min_size| is
   // not -1, it is the minimum size of the returned buffer.
   //
   // 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.

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

I could imagine that doing this change at the conceptual level (which I think is
primarily handling a defer response at the ResourceLoader level and in the other
ResourceControllers) might be a smaller change than this one.  Maybe worth doing
before this CL so that this CL can be a consistent API change?  Up to you.

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

I think it's less work to convert to the new API first, then convert this method
over.  It'll certainly be a smaller change than this one, but I'll have to go
over all implementations twice to do the change (Once to be able to defer, once
to use the new API - and write tests for the old API, and update them all to the
new one).

I'm also planning on switching the Mojo handler over to taking advantage of it
in the same CL I add the new API in, as well as add a bunch of tests, so it's
actually going to be a good sized CL.

Admittedly, I'm in a bit of a hurry to just finally land this CL, given how long
it's been since I started working on it.  If you feel strongly enough that this
method should be updated first, though, I can certainly do that.

   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;
+  // 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.  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;
+  // 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
   // calls are consumed by the RedirectToFileResourceHandler and replaced
   // with OnDataDownloaded calls.
   virtual void OnDataDownloaded(int bytes_downloaded) = 0;
 
  protected:
-  ResourceHandler(net::URLRequest* request);
+  explicit ResourceHandler(net::URLRequest* request);
 
-  ResourceController* controller() const { return controller_; }
+  //  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.

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

Suggestion: I was tripped up a little bit in my review by the lack of obvious
symmetry between set_controller() and TakeController().  Maybe worthwhile
renaming to SetController (or maybe HoldController/ReleaseController()?) to make
the pattern clearer?

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

Good idea.  I've gone with HoldController()/ReleaseController() (Good idea about
renaming "TakeController", too - it's a very ambiguous name).

+
+  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);
+
   net::URLRequest* request() const { return request_; }
 
   // Convenience functions.
   ResourceRequestInfoImpl* GetRequestInfo() const;
   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);
 };
 
 }  // namespace content
 
 #endif  // CONTENT_BROWSER_LOADER_RESOURCE_HANDLER_H_