PlzNavigate: make NavigationResourceHandler a LayeredResourceHandler

content/browser/loader/navigation_resource_handler.cc

 // Copyright 2014 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.
 
 #include "content/browser/loader/navigation_resource_handler.h"
 
 #include <memory>
 
 #include "base/bind.h"
 #include "base/logging.h"
@@ skipping 17 matching lines @@
 
 void NavigationResourceHandler::GetSSLStatusForRequest(
     const net::SSLInfo& ssl_info,
     SSLStatus* ssl_status) {
   DCHECK(ssl_info.cert);
   *ssl_status = SSLStatus(ssl_info);
 }
 
 NavigationResourceHandler::NavigationResourceHandler(
     net::URLRequest* request,
+    std::unique_ptr<ResourceHandler> next_handler,
     NavigationURLLoaderImplCore* core,
-    ResourceDispatcherHostDelegate* resource_dispatcher_host_delegate)
-    : ResourceHandler(request),
+    ResourceDispatcherHostDelegate* resource_dispatcher_host_delegate,
+    std::unique_ptr<StreamHandle> stream_handle)
+    : LayeredResourceHandler(request, std::move(next_handler)),
       core_(core),
+      stream_handle_(std::move(stream_handle)),
       resource_dispatcher_host_delegate_(resource_dispatcher_host_delegate) {
   core_->set_resource_handler(this);
-  writer_.set_immediate_mode(true);
 }
 
 NavigationResourceHandler::~NavigationResourceHandler() {
   if (core_) {
     core_->NotifyRequestFailed(false, net::ERR_ABORTED);
     DetachFromCore();
   }
 }
 
 void NavigationResourceHandler::Cancel() {
   if (core_) {
     core_ = nullptr;

[mmenke, 2017/04/28 16:02:20]

Why does this just set core_ to nullptr instead of calling DetachFromCore?

[clamy, 2017/05/02 15:05:44]

No idea. I have changed it in the latest patchset.

-    OutOfBandCancel(net::ERR_ABORTED, true /* tell_renderer */);
+    if (has_controller()) {
+      CancelAndIgnore();
+    } else {
+      OutOfBandCancel(net::ERR_ABORTED, true /* tell_renderer */);
+    }
   }
 }
 
 void NavigationResourceHandler::FollowRedirect() {
-  Resume();
+  DCHECK(response_);
+  DCHECK(redirect_info_);
+  DCHECK(has_controller());
+  next_handler_->OnRequestRedirected(*redirect_info_, response_.get(),
+                                     ReleaseController());
+  response_ = nullptr;
+  redirect_info_ = nullptr;
 }
 
 void NavigationResourceHandler::ProceedWithResponse() {
+  DCHECK(response_);
+  DCHECK(has_controller());
   // Detach from the loader; at this point, the request is now owned by the
   // StreamHandle sent in OnResponseStarted.
   DetachFromCore();
-  Resume();
+  next_handler_->OnResponseStarted(response_.get(), ReleaseController());
+  response_ = nullptr;
 }
 
 void NavigationResourceHandler::OnRequestRedirected(
     const net::RedirectInfo& redirect_info,
     ResourceResponse* response,
     std::unique_ptr<ResourceController> controller) {
   DCHECK(!has_controller());
 
   // The UI thread already cancelled the navigation. Do not proceed.
   if (!core_) {
     controller->CancelAndIgnore();
     return;
   }
 
   NetLogObserver::PopulateResponseInfo(request(), response);
   response->head.encoded_data_length = request()->GetTotalReceivedBytes();
   core_->NotifyRequestRedirected(redirect_info, response);
 
   HoldController(std::move(controller));
+  response_ = response;
+  redirect_info_ =
+      std::unique_ptr<net::RedirectInfo>(new net::RedirectInfo(redirect_info));

[mmenke, 2017/04/28 16:02:20]

base::MakeUnique<net::RedirectInfo>(redirect_info)...Or could just make it no
longer a unique_ptr.  Don't think we actually get anything from making it a
unique_ptr.

[clamy, 2017/05/02 15:05:44]

Done. I made it a unique_ptr in order not to initialize it if we don't have
redirects. I can make it a plain member variable if you prefer.

[mmenke, 2017/05/02 15:55:39]

I'd tend to just make it a plain member variable, since we make few enough
NavigationResourceHandlers that I don't think size or perf cost of a couple
extra initializations hurts us at all.  But if you prefer this approach, I think
that's fine.  Code should hopefully be going away relatively soon, anyways,
using the URLLoader API instead.

 }
 
 void NavigationResourceHandler::OnResponseStarted(
     ResourceResponse* response,
     std::unique_ptr<ResourceController> controller) {
   DCHECK(!has_controller());
 
   // The UI thread already cancelled the navigation. Do not proceed.
   if (!core_) {
     controller->CancelAndIgnore();
     return;
   }
 
   ResourceRequestInfoImpl* info = GetRequestInfo();
 
-  StreamContext* stream_context =
-      GetStreamContextForResourceContext(info->GetContext());
-  writer_.InitializeStream(
-      stream_context->registry(), request()->url().GetOrigin(),
-      base::Bind(&NavigationResourceHandler::OutOfBandCancel,
-                 base::Unretained(this), net::ERR_ABORTED,
-                 true /* tell_renderer */));
-
   NetLogObserver::PopulateResponseInfo(request(), response);
   response->head.encoded_data_length = request()->raw_header_size();
 
   std::unique_ptr<NavigationData> cloned_data;
   if (resource_dispatcher_host_delegate_) {
     // Ask the embedder for a NavigationData instance.
     NavigationData* navigation_data =
         resource_dispatcher_host_delegate_->GetNavigationData(request());
 
     // Clone the embedder's NavigationData before moving it to the UI thread.
     if (navigation_data)
       cloned_data = navigation_data->Clone();
   }
 
   SSLStatus ssl_status;
   if (request()->ssl_info().cert.get())
     GetSSLStatusForRequest(request()->ssl_info(), &ssl_status);
 
-  core_->NotifyResponseStarted(response, writer_.stream()->CreateHandle(),
-                               ssl_status, std::move(cloned_data),
-                               info->GetGlobalRequestID(), info->IsDownload(),
-                               info->is_stream());
-  // Don't defer stream based requests. This includes requests initiated via
-  // mime type sniffing, etc.
-  // TODO(ananta)
-  // Make sure that the requests go through the throttle checks. Currently this
-  // does not work as the InterceptingResourceHandler is above us and hence it
-  // does not expect the old handler to defer the request.
-  // TODO(clamy): We should also make the downloads wait on the
-  // NavigationThrottle checks be performed. Similarly to streams, it doesn't
-  // work because of the InterceptingResourceHandler.
-  // TODO(clamy): This NavigationResourceHandler should be split in two, with
-  // one part that wait on the NavigationThrottle to execute located between the
-  // MIME sniffing and the ResourceThrotlle, and one part that write the
-  // response to the stream being the leaf ResourceHandler.
-  if (info->is_stream() || info->IsDownload()) {
-    controller->Resume();
-  } else {
-    HoldController(std::move(controller));
-  }
-}
-
-void NavigationResourceHandler::OnWillStart(
-    const GURL& url,
-    std::unique_ptr<ResourceController> controller) {
-  DCHECK(!has_controller());
-  controller->Resume();
-}
-
-void NavigationResourceHandler::OnWillRead(
-    scoped_refptr<net::IOBuffer>* buf,
-    int* buf_size,
-    std::unique_ptr<ResourceController> controller) {
-  DCHECK(!has_controller());
-  writer_.OnWillRead(buf, buf_size, -1);
-  controller->Resume();
-}
-
-void NavigationResourceHandler::OnReadCompleted(
-    int bytes_read,
-    std::unique_ptr<ResourceController> controller) {
-  DCHECK(!has_controller());
-  writer_.OnReadCompleted(bytes_read,
-                          base::Bind(&ResourceController::Resume,
-                                     base::Passed(std::move(controller))));
+  core_->NotifyResponseStarted(
+      response, std::move(stream_handle_), ssl_status, std::move(cloned_data),
+      info->GetGlobalRequestID(), info->IsDownload(), info->is_stream());
+  HoldController(std::move(controller));
+  response_ = response;
 }
 
 void NavigationResourceHandler::OnResponseCompleted(
     const net::URLRequestStatus& status,
     std::unique_ptr<ResourceController> controller) {
-  // If the request has already committed, close the stream and leave it as-is.
-  if (writer_.stream()) {
-    writer_.Finalize(status.error());
-    controller->Resume();
-    return;
-  }
-
   if (core_) {
     DCHECK_NE(net::OK, status.error());
     core_->NotifyRequestFailed(request()->response_info().was_cached,
                                status.error());
     DetachFromCore();
   }
-  controller->Resume();
-}
-
-void NavigationResourceHandler::OnDataDownloaded(int bytes_downloaded) {
-  NOTREACHED();
+  next_handler_->OnResponseCompleted(status, std::move(controller));

[mmenke, 2017/04/28 16:02:20]

So I guess we now unconditionally create writer_.stream(), and finalize it, even
if there's nothing on the other end?  Does that work?  (I'm not at all familiar
with content/browser/streams)

[clamy, 2017/05/02 15:05:44]

Based on the tests, it seems to work. When we have a stream right now (like for
pdfs) we do create it before having something reading it on the other end. And
it's possible that we get the end of the stream before the renderer claims it
(eg the renderer was busy for some time before starting to access the stream).
So I think it should work.

 }
 
 void NavigationResourceHandler::DetachFromCore() {
   DCHECK(core_);
   core_->set_resource_handler(nullptr);
   core_ = nullptr;
 }
 
 }  // namespace content