Refactor ResourceHandler API.

content/browser/loader/throttling_resource_handler.cc

Index: content/browser/loader/throttling_resource_handler.cc
diff --git a/content/browser/loader/throttling_resource_handler.cc b/content/browser/loader/throttling_resource_handler.cc
index 6238ebb4d93319ffc0eabf51e79f9b13cd0c6fa8..32b12bb36966f4619563321a00305f5592532909 100644
--- a/content/browser/loader/throttling_resource_handler.cc
+++ b/content/browser/loader/throttling_resource_handler.cc
@@ -32,93 +32,110 @@ ThrottlingResourceHandler::ThrottlingResourceHandler(
 ThrottlingResourceHandler::~ThrottlingResourceHandler() {
 }
 
-bool ThrottlingResourceHandler::OnRequestRedirected(
+void ThrottlingResourceHandler::OnRequestRedirected(
     const net::RedirectInfo& redirect_info,
     ResourceResponse* response,
-    bool* defer) {
+    std::unique_ptr<ResourceController> controller) {
+  DCHECK(!has_controller());
   DCHECK(!cancelled_by_resource_throttle_);
-  DCHECK(!*defer);
 
+  set_controller(std::move(controller));
   while (next_index_ < throttles_.size()) {
     int index = next_index_;
-    throttles_[index]->WillRedirectRequest(redirect_info, defer);
+    bool defer = false;
+    throttles_[index]->WillRedirectRequest(redirect_info, &defer);

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

Is it your plan to push the API change into the throttles?  If so, is there a
reason to do RH->RT rather than the reverse?  (I don't have an opinion, just
wanted to make sure I understand the plan and its motivations.)

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

I'm not currently planning on changing the API here, though may want to either
ban OOB-cancels from them or do sync destruction in that case.  Since RH's are
mostly loader-implemented, and RT's are embedded-implemented, I think we want to
bend over backwards a bit to make the RT API dead simple, and separating out
cancel types seems to go against that.

I do want to clean up this class (And maybe the API as well), but I'm thinking
that should be done after OOB-cancellation becomes synchronous.  I've also kinda
forgotten just how I was thinking on cleaning this class up...

     next_index_++;
     if (cancelled_by_resource_throttle_)
-      return false;
-    if (*defer) {
+      return;
+
+    if (defer) {
       OnRequestDeferred(index);
       deferred_stage_ = DEFERRED_REDIRECT;
       deferred_redirect_ = redirect_info;
       deferred_response_ = response;
-      return true;  // Do not cancel.
+      return;
     }
   }
 
   next_index_ = 0;  // Reset for next time.
 
-  return next_handler_->OnRequestRedirected(redirect_info, response, defer);
+  next_handler_->OnRequestRedirected(redirect_info, response, TakeController());
 }
 
-bool ThrottlingResourceHandler::OnWillStart(const GURL& url, bool* defer) {
+void ThrottlingResourceHandler::OnWillStart(
+    const GURL& url,
+    std::unique_ptr<ResourceController> controller) {
   DCHECK(!cancelled_by_resource_throttle_);
-  DCHECK(!*defer);
+  DCHECK(!has_controller());
 
+  set_controller(std::move(controller));
   while (next_index_ < throttles_.size()) {
     int index = next_index_;
-    throttles_[index]->WillStartRequest(defer);
+    bool defer = false;
+    throttles_[index]->WillStartRequest(&defer);
     next_index_++;
     if (cancelled_by_resource_throttle_)
-      return false;
-    if (*defer) {
+      return;
+    if (defer) {
       OnRequestDeferred(index);
       deferred_stage_ = DEFERRED_START;
       deferred_url_ = url;
-      return true;  // Do not cancel.
+      return;
     }
   }
 
   next_index_ = 0;  // Reset for next time.
 
-  return next_handler_->OnWillStart(url, defer);
+  return next_handler_->OnWillStart(url, TakeController());
 }
 
-bool ThrottlingResourceHandler::OnResponseStarted(ResourceResponse* response,
-                                                  bool* defer) {
+void ThrottlingResourceHandler::OnResponseStarted(
+    ResourceResponse* response,
+    std::unique_ptr<ResourceController> controller) {
   DCHECK(!cancelled_by_resource_throttle_);
-  DCHECK(!*defer);
+  DCHECK(!has_controller());
 
+  set_controller(std::move(controller));
   while (next_index_ < throttles_.size()) {
     int index = next_index_;
-    throttles_[index]->WillProcessResponse(defer);
+    bool defer = false;
+    throttles_[index]->WillProcessResponse(&defer);
     next_index_++;
     if (cancelled_by_resource_throttle_)
-      return false;
-    if (*defer) {
+      return;
+    if (defer) {
       OnRequestDeferred(index);
       deferred_stage_ = DEFERRED_RESPONSE;
       deferred_response_ = response;
-      return true;  // Do not cancel.
+      return;
     }
   }
 
   next_index_ = 0;  // Reset for next time.
 
-  return next_handler_->OnResponseStarted(response, defer);
+  return next_handler_->OnResponseStarted(response, TakeController());
 }
 
 void ThrottlingResourceHandler::Cancel() {
-  cancelled_by_resource_throttle_ = true;
-  controller()->Cancel();
+  CancelWithError(net::ERR_ABORTED);
 }
 
 void ThrottlingResourceHandler::CancelAndIgnore() {
+  if (!has_controller()) {
+    OutOfBandCancel(net::ERR_ABORTED, false);
+    return;
+  }
   cancelled_by_resource_throttle_ = true;
-  controller()->CancelAndIgnore();
+  ResourceHandler::CancelAndIgnore();
 }
 
 void ThrottlingResourceHandler::CancelWithError(int error_code) {
+  if (!has_controller()) {
+    OutOfBandCancel(error_code, false);
+    return;
+  }
   cancelled_by_resource_throttle_ = true;
-  controller()->CancelWithError(error_code);
+  ResourceHandler::CancelWithError(error_code);
 }
 
 void ThrottlingResourceHandler::Resume() {
@@ -129,6 +146,8 @@ void ThrottlingResourceHandler::Resume() {
   if (cancelled_by_resource_throttle_)
     return;
 
+  DCHECK(has_controller());
+
   DeferredStage last_deferred_stage = deferred_stage_;
   deferred_stage_ = DEFERRED_NONE;
   // Clear information about the throttle that delayed the request.
@@ -151,46 +170,34 @@ void ThrottlingResourceHandler::Resume() {
 
 void ThrottlingResourceHandler::ResumeStart() {
   DCHECK(!cancelled_by_resource_throttle_);
+  DCHECK(has_controller());
 
   GURL url = deferred_url_;
   deferred_url_ = GURL();
 
-  bool defer = false;
-  if (!OnWillStart(url, &defer)) {
-    controller()->Cancel();
-  } else if (!defer) {
-    controller()->Resume();
-  }
+  OnWillStart(url, TakeController());
 }
 
 void ThrottlingResourceHandler::ResumeRedirect() {
   DCHECK(!cancelled_by_resource_throttle_);
+  DCHECK(has_controller());
 
   net::RedirectInfo redirect_info = deferred_redirect_;
   deferred_redirect_ = net::RedirectInfo();
   scoped_refptr<ResourceResponse> response;
   deferred_response_.swap(response);
 
-  bool defer = false;
-  if (!OnRequestRedirected(redirect_info, response.get(), &defer)) {
-    controller()->Cancel();
-  } else if (!defer) {
-    controller()->Resume();
-  }
+  OnRequestRedirected(redirect_info, response.get(), TakeController());
 }
 
 void ThrottlingResourceHandler::ResumeResponse() {
   DCHECK(!cancelled_by_resource_throttle_);
+  DCHECK(has_controller());
 
   scoped_refptr<ResourceResponse> response;
   deferred_response_.swap(response);
 
-  bool defer = false;
-  if (!OnResponseStarted(response.get(), &defer)) {
-    controller()->Cancel();
-  } else if (!defer) {
-    controller()->Resume();
-  }
+  OnResponseStarted(response.get(), TakeController());
 }
 
 void ThrottlingResourceHandler::OnRequestDeferred(int throttle_index) {