Use defer out-params instead of ResourceDispatcherHostImpl::PauseRequest(...true)

content/browser/renderer_host/buffered_resource_handler.cc

Index: content/browser/renderer_host/buffered_resource_handler.cc
===================================================================
--- content/browser/renderer_host/buffered_resource_handler.cc	(revision 136397)
+++ content/browser/renderer_host/buffered_resource_handler.cc	(working copy)
@@ -72,6 +72,7 @@
       bytes_read_(0),
       sniff_content_(false),
       wait_for_plugins_(false),
+      deferred_waiting_for_plugins_(false),
       buffering_(false),
       next_handler_needs_response_started_(false),
       next_handler_needs_will_read_(false),
@@ -80,10 +81,17 @@
 
 bool BufferedResourceHandler::OnResponseStarted(
     int request_id,
-    ResourceResponse* response) {
+    ResourceResponse* response,
+    bool* defer) {
   response_ = response;
+
   if (!DelayResponse())
-    return CompleteResponseStarted(request_id);
+    return CompleteResponseStarted(request_id, defer);
+
+  if (wait_for_plugins_) {
+    deferred_waiting_for_plugins_ = true;
+    *defer = true;
+  }
   return true;
 }
 
@@ -107,9 +115,9 @@
   if (finished_)
     return false;
 
-  if (!next_handler_->OnWillRead(request_id, buf, buf_size, min_size)) {
+  if (!next_handler_->OnWillRead(request_id, buf, buf_size, min_size))
     return false;
-  }
+
   read_buffer_ = *buf;
   read_buffer_size_ = *buf_size;
   DCHECK_GE(read_buffer_size_, net::kMaxBytesToSniff * 2);
@@ -117,35 +125,38 @@
   return true;
 }
 
-bool BufferedResourceHandler::OnReadCompleted(int request_id, int* bytes_read) {
-  ResourceRequestInfoImpl* info =
-      ResourceRequestInfoImpl::ForRequest(request_);
-
+bool BufferedResourceHandler::OnReadCompleted(int request_id, int* bytes_read,
+                                              bool* defer) {
   if (sniff_content_) {
-    if (KeepBuffering(*bytes_read))
+    if (KeepBuffering(*bytes_read)) {
+      if (wait_for_plugins_) {
+        deferred_waiting_for_plugins_ = true;
+        *defer = true;
+      }
       return true;
+    }
 
     *bytes_read = bytes_read_;
 
     // Done buffering, send the pending ResponseStarted event.
-    if (!CompleteResponseStarted(request_id))
+    if (!CompleteResponseStarted(request_id, defer))
       return false;
-
-    // The next handler might have paused the request in OnResponseStarted.
-    if (info->pause_count())
+    if (*defer)
       return true;
   } else if (wait_for_plugins_) {
+    deferred_waiting_for_plugins_ = true;
+    *defer = true;
     return true;
   }
 
-  if (!ForwardPendingEventsToNextHandler(request_id))
+  if (!ForwardPendingEventsToNextHandler(request_id, defer))
     return false;
-  if (info->pause_count())
+  if (*defer)
     return true;
 
   // Release the reference that we acquired at OnWillRead.
   read_buffer_ = NULL;
-  return next_handler_->OnReadCompleted(request_id, bytes_read);
+  return next_handler_->OnReadCompleted(request_id, bytes_read, defer);
 }
 
 BufferedResourceHandler::~BufferedResourceHandler() {}
@@ -243,7 +254,8 @@
   return false;
 }
 
-bool BufferedResourceHandler::CompleteResponseStarted(int request_id) {
+bool BufferedResourceHandler::CompleteResponseStarted(int request_id,
+                                                      bool* defer) {
   ResourceRequestInfoImpl* info =
       ResourceRequestInfoImpl::ForRequest(request_);
   std::string mime_type;
@@ -272,14 +284,13 @@
         new X509UserCertResourceHandler(request_,
                                         info->GetChildID(),
                                         info->GetRouteID());
-    if (!UseAlternateResourceHandler(request_id, x509_cert_handler))
+    if (!UseAlternateResourceHandler(request_id, x509_cert_handler, defer))
       return false;
   }
-
   // Check to see if we should forward the data from this request to the
   // download thread.
   // TODO(paulg): Only download if the context from the renderer allows it.
-  if (info->allow_download() && ShouldDownload(NULL)) {
+  else if (info->allow_download() && ShouldDownload(NULL)) {

[Randy Smith (Not in Mondays), 2012/05/18 20:17:02]

Suggestion (as I can't find any support for my position in the style guide): I
find the separation of the else from its previous curly brace jarring; any
chance that it could be put on that line and the comment moved underneath the if
without it being confusing?

[darin (slow to review), 2012/05/18 23:09:05]

Yes, that can probably be done.

     if (response_->headers &&  // Can be NULL if FTP.
         response_->headers->response_code() / 100 != 2) {
       // The response code indicates that this is an error page, but we don't
@@ -303,14 +314,14 @@
             DownloadSaveInfo(),
             DownloadResourceHandler::OnStartedCallback()));
 
-    if (!UseAlternateResourceHandler(request_id, handler))
+    if (!UseAlternateResourceHandler(request_id, handler, defer))
       return false;
   }
 
-  if (info->pause_count())
+  if (*defer)
     return true;
 
-  return next_handler_->OnResponseStarted(request_id, response_);
+  return next_handler_->OnResponseStarted(request_id, response_, defer);
 }
 
 bool BufferedResourceHandler::ShouldWaitForPlugins() {
@@ -318,14 +329,11 @@
   if (!ShouldDownload(&need_plugin_list) || !need_plugin_list)
     return false;
 
-  // We don't want to keep buffering as our buffer will fill up.
-  ResourceRequestInfoImpl* info =
-      ResourceRequestInfoImpl::ForRequest(request_);
-  host_->PauseRequest(info->GetChildID(), info->GetRequestID(), true);
-
   // Get the plugins asynchronously.
   PluginServiceImpl::GetInstance()->GetPlugins(
       base::Bind(&BufferedResourceHandler::OnPluginsLoaded, this));
+
+  // We don't want to keep buffering as our buffer will fill up.

[Randy Smith (Not in Mondays), 2012/05/18 20:17:02]

nit, suggestion: This comment feels a little strange to me, disassociated from
the pause.  I think it'd make more sense at the places where we translate the
return value from ShouldWaitForPlugins into a *defer = true;

[darin (slow to review), 2012/05/18 23:09:05]

Agreed.

   return true;
 }
 
@@ -378,11 +386,14 @@
 
 bool BufferedResourceHandler::UseAlternateResourceHandler(
     int request_id,
-    ResourceHandler* handler) {
+    ResourceHandler* handler,
+    bool* defer) {
   // Inform the original ResourceHandler that this will be handled entirely by
   // the new ResourceHandler.
   // TODO(darin): We should probably check the return values of these.
-  next_handler_->OnResponseStarted(request_id, response_);
+  bool defer_ignored = false;
+  next_handler_->OnResponseStarted(request_id, response_, &defer_ignored);
+  DCHECK(!defer_ignored);

[Randy Smith (Not in Mondays), 2012/05/18 20:17:02]

What's the rational for this?  

[darin (slow to review), 2012/05/18 23:09:05]

The old code fails to handle pausing done by the old handler.  I think it should
probably be fixed.  This DCHECK will complain if the handler does try to pause,
and then we can notice that and fix this code to handle it properly.

[Randy Smith (Not in Mondays), 2012/05/19 00:42:14]

SG.

   net::URLRequestStatus status(net::URLRequestStatus::HANDLED_EXTERNALLY, 0);
   next_handler_->OnResponseCompleted(request_id, status, std::string());
 
@@ -400,32 +411,24 @@
   next_handler_needs_response_started_ = true;
   next_handler_needs_will_read_ = true;
 
-  return ForwardPendingEventsToNextHandler(request_id);
+  return ForwardPendingEventsToNextHandler(request_id, defer);
 }
 
 bool BufferedResourceHandler::ForwardPendingEventsToNextHandler(
-    int request_id) {
-  ResourceRequestInfoImpl* info =
-      ResourceRequestInfoImpl::ForRequest(request_);
-  if (info->pause_count())
-    return true;
-
+    int request_id,
+    bool* defer) {

[Randy Smith (Not in Mondays), 2012/05/18 20:17:02]

nit: Why two lines?

[darin (slow to review), 2012/05/18 23:09:05]

Looks like the first parameter actually fits on the previous line.

   if (next_handler_needs_response_started_) {
-    if (!next_handler_->OnResponseStarted(request_id, response_))
+    if (!next_handler_->OnResponseStarted(request_id, response_, defer))
       return false;
-    // If the request was paused during OnResponseStarted, we need to avoid
+    // If the request was deferred during OnResponseStarted, we need to avoid
     // calling OnResponseStarted again.
     next_handler_needs_response_started_ = false;
-    if (info->pause_count())
+    if (*defer)
       return true;
   }
 
   if (next_handler_needs_will_read_) {
     CopyReadBufferToNextHandler(request_id);
-    // If the request was paused during OnWillRead, we need to be sure to try
-    // calling OnWillRead again.
-    if (info->pause_count())
-      return true;
     next_handler_needs_will_read_ = false;
   }
   return true;
@@ -445,6 +448,9 @@
 
 void BufferedResourceHandler::OnPluginsLoaded(
     const std::vector<webkit::WebPluginInfo>& plugins) {
+  bool needs_resume = deferred_waiting_for_plugins_;
+  deferred_waiting_for_plugins_ = false;
+
   wait_for_plugins_ = false;
   if (!request_)
     return;
@@ -454,9 +460,12 @@
   int child_id = info->GetChildID();
   int request_id = info->GetRequestID();
 
-  host_->PauseRequest(child_id, request_id, false);
-  if (!CompleteResponseStarted(request_id))
+  bool defer = false;
+  if (!CompleteResponseStarted(request_id, &defer)) {
     host_->CancelRequest(child_id, request_id, false);
+  } else if (!defer && needs_resume) {
+    host_->ResumeDeferredRequest(child_id, request_id);
+  }
 }
 
 }  // namespace content