Network service: Implement URLLoader chaining for interceptors

content/browser/loader/url_loader_request_handler.h

Index: content/browser/loader/url_loader_request_handler.h
diff --git a/content/browser/loader/url_loader_request_handler.h b/content/browser/loader/url_loader_request_handler.h
new file mode 100644
index 0000000000000000000000000000000000000000..d0cf737021ce97a74a59485de0db519090ce2542
--- /dev/null
+++ b/content/browser/loader/url_loader_request_handler.h
@@ -0,0 +1,44 @@
+// Copyright 2017 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.
+
+#ifndef CONTENT_BROWSER_LOADER_URL_LOADER_REQUEST_HANDLER_H_
+#define CONTENT_BROWSER_LOADER_URL_LOADER_REQUEST_HANDLER_H_
+
+#include "base/macros.h"
+#include "content/common/url_loader.mojom.h"
+
+namespace content {
+
+class ResourceContext;
+struct ResourceRequest;
+
+// An instance of this class is a per-request object and kept around during
+// the lifetime of a request (including multiple redirect legs).

[scottmg, 2017/05/25 15:12:24]

Mention that this is IO thread.

[kinuko, 2017/05/26 14:30:05]

Done.

+class URLLoaderRequestHandler {

[michaeln, 2017/05/26 01:28:31]

This is looking good, I'm going to focus on this interface to start with. I have
a couple comments/questions.

1. The first is mostly stylistic. The handler's primary responsibility is to
optionally create a loader for the resource_request. We could make the api
reflect that more directly.

Handler {
  virtual void MaybeCreateLoader(Callback<void(Optional<Loader>)>);
}

That could eliminate the need for the 'Controller' interface at this level. The
calling code would work down the list of handlers_ and call
loader->Start(loaderReq, loaderClnt) on the first non-null return value. I think
that clarifies the api a little and it might make the testing of Handler
derivatives easier, either they return a properly constructed loader or not,
examine the return value to verify.

2. The second is about anticipating foreseen needs for appcache fallback
handling. The code here takes care of the MaybeInterceptRequest() cases, I'm
wondering about the MaybeInterceptResponse() and MaybeInterceptRedirect(). I
don't think we need to handling this in this CL, but I'd like to explore it a
little in this code review.

MaybeInterceptResponse() comes into play for main resource loads and subresource
loads
MaybeInterceptRedirect() comes into play for cross-origin redirects

Some ideas...
* the appcache knows at MaybeCreateLoader completion time whether it would
intercept cross-origin redirects or error respsonses
* hits and fallbacks are mutually exclusive
* plumbing the response head from the renderer to the main browser process just
to ask "do you want to intercept this" when 99.9% of the time the answer would
be "no", would be very ugly

Putting those together, I think the renderer side needs to know whether there's
a fallback and should ask for it when it sees the appropiate conditions. The
x-chromium-appcache-fallback-override custom header can be used to disable
fallback handling for an error response, the renderer side logic would have to
know about that header and about the conditions for which the fallback should be
used in general, those conditions are pretty straightforward, depends on few
things (is_main_resource, response_code, custom_header,
is_cross_origin_redirect). A renderer-side function to say whether to use a
fallback would be easy to write given those inputs.

Here's an idea to extend the browser-side api at least enough to let the
NavigationUrlLoaderNetworkService know about fallbacks.

Handler {
  virtual void MaybeCreateLoader(Callback<void(Optional<Loader>, bool
is_fallback)>);
}

Also, sometimes the appcache is supposed to return an ErrorResponse for a
cross-origin redirect instead of a cached resource, would could represent that
as as passing NULL for the Loader and true for is_fallback, but that's a little
sketchy.

[kinuko, 2017/05/26 02:34:20]

Sure.  To clarify when you say 'Loader' do you mean something like
URLLoaderFactory but as a new c++ interface?  So we'll have Handler and Loader
interfaces here, would that match what you thought?
Thanks, that was exactly the part I wasn't too sure about.
 
I think we probably need to handle similar situation in SW. (A bit hand-wavy for
now)

[kinuko, 2017/05/26 14:30:05]

Changed this to
MaybeCreateLoaderFactory(base::Callback<void(URLLoaderFactory*)>);

I'm assuming that each handler implementation doesn't really need to create a
new URLLoaderFactory but may just implement it by itself.  I also assume that
each handler would return URLLoaderFactory (or nullptr) as lazily as possible,
so most of the existing request handler code runs before it returns
URLLoaderFactory.

[michaeln, 2017/05/26 21:36:25]

I'm just noticing the handoff to the renderer doesn't happen until
OnStartLoadingResponseBody() so all the decision making about
MaybeInterceptRedirect() and MaybeInterceptResponse() can happen on this side,
coordinated by the NavigationURLLoaderNetworkService class.

+ public:
+  class Controller {
+   public:
+    // Forward the request to the next handler. Could be called when the
+    // calling request handler can't handle the given request.
+    virtual void Forward(mojom::URLLoaderAssociatedRequest request,
+                         mojom::URLLoaderClientPtr client_ptr) = 0;
+  };
+
+  URLLoaderRequestHandler() = default;
+  virtual ~URLLoaderRequestHandler() = default;
+
+  // Controller should outlive the request handler object.
+  virtual void Start(const ResourceRequest& resource_request,

[michaeln, 2017/05/26 01:43:21]

Also, the hand wavvy MaybeCreateLoader(request, context, completion_callback)
api, would let the Handler return a fallback loader without consuming the
loader_request and client_ptr.

+                     Controller* controller,
+                     ResourceContext* resource_context,
+                     mojom::URLLoaderAssociatedRequest loader_request,
+                     mojom::URLLoaderClientPtr loader_client_ptr) = 0;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(URLLoaderRequestHandler);

[jam, 2017/05/25 16:06:37]

nit: not needed on non-copyable interfaces

[kinuko, 2017/05/26 14:30:05]

Removed.

+};
+
+}  // namespace content
+
+#endif  // CONTENT_BROWSER_LOADER_URL_LOADER_REQUEST_HANDLER_H_