Refactor ResourceHandler API.

content/browser/loader/detachable_resource_handler.cc

Index: content/browser/loader/detachable_resource_handler.cc
diff --git a/content/browser/loader/detachable_resource_handler.cc b/content/browser/loader/detachable_resource_handler.cc
index 2e1af5e7c6c396821e762203c1d9628012f1093b..8934bbccc47c6e46a9a87194f69eddbdf561e3b5 100644
--- a/content/browser/loader/detachable_resource_handler.cc
+++ b/content/browser/loader/detachable_resource_handler.cc
@@ -7,7 +7,10 @@
 #include <utility>
 
 #include "base/logging.h"
+#include "base/memory/ptr_util.h"
 #include "base/time/time.h"
+#include "content/browser/loader/null_resource_controller.h"
+#include "content/browser/loader/resource_controller.h"
 #include "content/browser/loader/resource_request_info_impl.h"
 #include "net/base/io_buffer.h"
 #include "net/base/net_errors.h"
@@ -21,6 +24,32 @@ const int kReadBufSize = 32 * 1024;
 
 namespace content {
 
+// ResourceController that, when invoked, runs the corresponding method on
+// ResourceHandler.
+class DetachableResourceHandler::Controller : public ResourceController {
+ public:
+  explicit Controller(DetachableResourceHandler* detachable_handler)
+      : detachable_handler_(detachable_handler){};
+
+  ~Controller() override {}
+
+  // ResourceController implementation:
+  void Resume() override { detachable_handler_->Resume(); }
+
+  void Cancel() override { detachable_handler_->Cancel(); }
+
+  void CancelAndIgnore() override { detachable_handler_->CancelAndIgnore(); }
+
+  void CancelWithError(int error_code) override {
+    detachable_handler_->CancelWithError(error_code);
+  }
+
+ private:
+  DetachableResourceHandler* detachable_handler_;
+
+  DISALLOW_COPY_AND_ASSIGN(Controller);
+};
+
 DetachableResourceHandler::DetachableResourceHandler(
     net::URLRequest* request,
     base::TimeDelta cancel_delay,
@@ -28,7 +57,6 @@ DetachableResourceHandler::DetachableResourceHandler(
     : ResourceHandler(request),
       next_handler_(std::move(next_handler)),
       cancel_delay_(cancel_delay),
-      is_deferred_(false),
       is_finished_(false) {
   GetRequestInfo()->set_detachable_handler(this);
 }
@@ -38,6 +66,12 @@ DetachableResourceHandler::~DetachableResourceHandler() {
   GetRequestInfo()->set_detachable_handler(NULL);
 }
 
+void DetachableResourceHandler::SetDelegate(Delegate* delegate) {
+  ResourceHandler::SetDelegate(delegate);
+  if (next_handler_)
+    next_handler_->SetDelegate(delegate);
+}
+
 void DetachableResourceHandler::Detach() {
   if (is_detached())
     return;
@@ -46,9 +80,10 @@ void DetachableResourceHandler::Detach() {
     // Simulate a cancel on the next handler before destroying it.
     net::URLRequestStatus status(net::URLRequestStatus::CANCELED,
                                  net::ERR_ABORTED);
-    bool defer_ignored = false;
-    next_handler_->OnResponseCompleted(status, &defer_ignored);
-    DCHECK(!defer_ignored);
+    bool was_resumed;
+    next_handler_->OnResponseCompleted(
+        status, base::MakeUnique<NullResourceController>(&was_resumed));
+    DCHECK(was_resumed);
     // If |next_handler_| were to defer its shutdown in OnResponseCompleted,
     // this would destroy it anyway. Fortunately, AsyncResourceHandler never
     // does this anyway, so DCHECK it. MimeTypeResourceHandler and RVH shutdown
@@ -66,66 +101,65 @@ void DetachableResourceHandler::Detach() {
 
   // Time the request out if it takes too long.
   detached_timer_.reset(new base::OneShotTimer());
-  detached_timer_->Start(
-      FROM_HERE, cancel_delay_, this, &DetachableResourceHandler::Cancel);
+  detached_timer_->Start(FROM_HERE, cancel_delay_, this,
+                         &DetachableResourceHandler::OnTimedOut);
 
   // Resume if necessary. The request may have been deferred, say, waiting on a
   // full buffer in AsyncResourceHandler. Now that it has been detached, resume
   // and drain it.
-  if (is_deferred_) {
+  if (has_controller()) {
     // The nested ResourceHandler may have logged that it's blocking the
     // request.  Log it as no longer doing so, to avoid a DCHECK on resume.
     request()->LogUnblocked();
+
+    // Resume.

[Charlie Harrison, 2017/01/25 20:22:59]

nit: remove redundant comment.

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

Done.

     Resume();
   }
 }
 
-void DetachableResourceHandler::SetController(ResourceController* controller) {
-  ResourceHandler::SetController(controller);
-
-  // Intercept the ResourceController for downstream handlers to keep track of
-  // whether the request is deferred.
-  if (next_handler_)
-    next_handler_->SetController(this);
-}
-
-bool DetachableResourceHandler::OnRequestRedirected(
+void DetachableResourceHandler::OnRequestRedirected(
     const net::RedirectInfo& redirect_info,
     ResourceResponse* response,
-    bool* defer) {
-  DCHECK(!is_deferred_);
+    std::unique_ptr<ResourceController> controller) {
+  DCHECK(!has_controller());
 
-  if (!next_handler_)
-    return true;
+  if (!next_handler_) {
+    controller->Resume();
+    return;
+  }
 
-  bool ret = next_handler_->OnRequestRedirected(
-      redirect_info, response, &is_deferred_);
-  *defer = is_deferred_;
-  return ret;
+  HoldController(std::move(controller));
+  next_handler_->OnRequestRedirected(redirect_info, response,
+                                     base::MakeUnique<Controller>(this));
 }
 
-bool DetachableResourceHandler::OnResponseStarted(ResourceResponse* response,
-                                                  bool* defer) {
-  DCHECK(!is_deferred_);
+void DetachableResourceHandler::OnResponseStarted(
+    ResourceResponse* response,
+    std::unique_ptr<ResourceController> controller) {
+  DCHECK(!has_controller());
 
-  if (!next_handler_)
-    return true;
+  if (!next_handler_) {
+    controller->Resume();
+    return;
+  }
 
-  bool ret =
-      next_handler_->OnResponseStarted(response, &is_deferred_);
-  *defer = is_deferred_;
-  return ret;
+  HoldController(std::move(controller));
+  next_handler_->OnResponseStarted(response,
+                                   base::MakeUnique<Controller>(this));
 }
 
-bool DetachableResourceHandler::OnWillStart(const GURL& url, bool* defer) {
-  DCHECK(!is_deferred_);
+void DetachableResourceHandler::OnWillStart(
+    const GURL& url,
+    std::unique_ptr<ResourceController> controller) {
+  DCHECK(!has_controller());
 
-  if (!next_handler_)
-    return true;
+  if (!next_handler_) {
+    controller->Resume();
+    return;
+  }
 
-  bool ret = next_handler_->OnWillStart(url, &is_deferred_);
-  *defer = is_deferred_;
-  return ret;
+  HoldController(std::move(controller));
+  next_handler_->OnWillStart(url, base::MakeUnique<Controller>(this));
 }
 
 bool DetachableResourceHandler::OnWillRead(scoped_refptr<net::IOBuffer>* buf,
@@ -143,31 +177,35 @@ bool DetachableResourceHandler::OnWillRead(scoped_refptr<net::IOBuffer>* buf,
   return next_handler_->OnWillRead(buf, buf_size, min_size);
 }
 
-bool DetachableResourceHandler::OnReadCompleted(int bytes_read, bool* defer) {
-  DCHECK(!is_deferred_);
+void DetachableResourceHandler::OnReadCompleted(
+    int bytes_read,
+    std::unique_ptr<ResourceController> controller) {
+  DCHECK(!has_controller());
 
-  if (!next_handler_)
-    return true;
+  if (!next_handler_) {
+    controller->Resume();
+    return;
+  }
 
-  bool ret =
-      next_handler_->OnReadCompleted(bytes_read, &is_deferred_);
-  *defer = is_deferred_;
-  return ret;
+  HoldController(std::move(controller));
+  next_handler_->OnReadCompleted(bytes_read,
+                                 base::MakeUnique<Controller>(this));
 }
 
 void DetachableResourceHandler::OnResponseCompleted(
     const net::URLRequestStatus& status,
-    bool* defer) {
+    std::unique_ptr<ResourceController> controller) {
   // No DCHECK(!is_deferred_) as the request may have been cancelled while
   // deferred.
 
-  if (!next_handler_)
+  if (!next_handler_) {
+    controller->Resume();
     return;
+  }
 
   is_finished_ = true;
 
-  next_handler_->OnResponseCompleted(status, &is_deferred_);
-  *defer = is_deferred_;
+  next_handler_->OnResponseCompleted(status, std::move(controller));
 }
 
 void DetachableResourceHandler::OnDataDownloaded(int bytes_downloaded) {
@@ -177,22 +215,13 @@ void DetachableResourceHandler::OnDataDownloaded(int bytes_downloaded) {
   next_handler_->OnDataDownloaded(bytes_downloaded);
 }
 
-void DetachableResourceHandler::Resume() {
-  DCHECK(is_deferred_);
-  is_deferred_ = false;
-  controller()->Resume();
-}
-
-void DetachableResourceHandler::Cancel() {
-  controller()->Cancel();
-}
-
-void DetachableResourceHandler::CancelAndIgnore() {
-  controller()->CancelAndIgnore();
-}
+void DetachableResourceHandler::OnTimedOut() {
+  // Requests are only timed out after being detached, and shouldn't be deferred
+  // once detached.
+  DCHECK(!next_handler_);
+  DCHECK(!has_controller());
 
-void DetachableResourceHandler::CancelWithError(int error_code) {
-  controller()->CancelWithError(error_code);
+  OutOfBandCancel(net::ERR_ABORTED, true);

[Charlie Harrison, 2017/01/25 20:22:59]

would appreciate a /* tell_renderer */ here.

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

Done, good idea.

 }
 
 }  // namespace content