Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2897063002:
Network service: Implement URLLoader chaining for interceptors (Closed)
DescriptionNetwork service: Implement URLLoader chaining for interceptors
BUG=724322
Review-Url: https://codereview.chromium.org/2897063002
Cr-Commit-Position: refs/heads/master@{#475314}
Committed: https://chromium.googlesource.com/chromium/src/+/697329710bb599e46ae7dc72dfc5f88c4abc4545
Patch Set 1 #Patch Set 2 : . #Patch Set 3 : . #
Total comments: 20
Patch Set 4 : MaybeCreateLoaderFactory #
Total comments: 15
Patch Set 5 : . #
Total comments: 6
Patch Set 6 : documentation #Messages
Total messages: 38 (18 generated)
Patchset #2 (id:20001) has been deleted
Description was changed from ========== [not for review] urlloader chain BUG= ========== to ========== Network service: Implement URLLoader chaining for interceptors BUG=724322 ==========
kinuko@chromium.org changed reviewers: + ananta@chromium.org, jam@chromium.org, michaeln@chromium.org, scottmg@chromium.org
Hi, this is still a bit rough, but has the basic bits I wanted to have. I haven't worked on appcache-side code yet (so this patch just effectively removes the existing plumbing), but I think we can have something similar to the SW case code. What do you think?
Yeah, this looks great. :) It's nice that much of the SW code request handler can be shared. https://codereview.chromium.org/2897063002/diff/60001/content/browser/loader/... File content/browser/loader/navigation_url_loader_network_service.cc (right): https://codereview.chromium.org/2897063002/diff/60001/content/browser/loader/... content/browser/loader/navigation_url_loader_network_service.cc:140: // This could be called multiple times. nit; comment that this is URLLoaderRH::Controller. https://codereview.chromium.org/2897063002/diff/60001/content/browser/loader/... File content/browser/loader/url_loader_request_handler.h (right): https://codereview.chromium.org/2897063002/diff/60001/content/browser/loader/... content/browser/loader/url_loader_request_handler.h:17: // the lifetime of a request (including multiple redirect legs). Mention that this is IO thread. https://codereview.chromium.org/2897063002/diff/60001/content/browser/service... File content/browser/service_worker/service_worker_request_handler.cc (right): https://codereview.chromium.org/2897063002/diff/60001/content/browser/service... content/browser/service_worker/service_worker_request_handler.cc:246: NOTREACHED(); This is only for ServiceWorkerURLTrackingRequestHandler and ServiceWorkerContextRequestHandler right? (i.e. ServiceWorkerControlleeRequestHandler already works now)
(just looked in loader directory) https://codereview.chromium.org/2897063002/diff/60001/content/browser/loader/... File content/browser/loader/navigation_url_loader_network_service.cc (right): https://codereview.chromium.org/2897063002/diff/60001/content/browser/loader/... content/browser/loader/navigation_url_loader_network_service.cc:153: base::debug::StackTrace trace; nit: is this still needed? https://codereview.chromium.org/2897063002/diff/60001/content/browser/loader/... File content/browser/loader/url_loader_request_handler.h (right): https://codereview.chromium.org/2897063002/diff/60001/content/browser/loader/... content/browser/loader/url_loader_request_handler.h:39: DISALLOW_COPY_AND_ASSIGN(URLLoaderRequestHandler); nit: not needed on non-copyable interfaces
https://codereview.chromium.org/2897063002/diff/60001/content/browser/loader/... File content/browser/loader/navigation_url_loader_network_service.cc (right): https://codereview.chromium.org/2897063002/diff/60001/content/browser/loader/... content/browser/loader/navigation_url_loader_network_service.cc:94: // or be intercepted, so we just let it go here. nice clarifying change to rearrange the order https://codereview.chromium.org/2897063002/diff/60001/content/browser/loader/... content/browser/loader/navigation_url_loader_network_service.cc:101: i think blob scheme handling will goes here too, maybe a TODO fyi: jam has a lot of code for that in https://codereview.chromium.org/2906543002/ https://codereview.chromium.org/2897063002/diff/60001/content/browser/loader/... File content/browser/loader/url_loader_request_handler.h (right): https://codereview.chromium.org/2897063002/diff/60001/content/browser/loader/... content/browser/loader/url_loader_request_handler.h:18: class URLLoaderRequestHandler { 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. https://codereview.chromium.org/2897063002/diff/60001/content/browser/service... File content/browser/service_worker/service_worker_controllee_request_handler.h (right): https://codereview.chromium.org/2897063002/diff/60001/content/browser/service... content/browser/service_worker/service_worker_controllee_request_handler.h:64: // Used only for PlzNavigate and --enable-network-service cases. Maybe comment that this method replaces MaybeCreateJob() in the new NetworkService system
https://codereview.chromium.org/2897063002/diff/60001/content/browser/loader/... File content/browser/loader/url_loader_request_handler.h (right): https://codereview.chromium.org/2897063002/diff/60001/content/browser/loader/... content/browser/loader/url_loader_request_handler.h:32: virtual void Start(const ResourceRequest& resource_request, 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.
Thanks! Inline responses only, will update patch later too https://codereview.chromium.org/2897063002/diff/60001/content/browser/loader/... File content/browser/loader/navigation_url_loader_network_service.cc (right): https://codereview.chromium.org/2897063002/diff/60001/content/browser/loader/... content/browser/loader/navigation_url_loader_network_service.cc:153: base::debug::StackTrace trace; On 2017/05/25 16:06:37, jam wrote: > nit: is this still needed? Of course not :) https://codereview.chromium.org/2897063002/diff/60001/content/browser/loader/... File content/browser/loader/url_loader_request_handler.h (right): https://codereview.chromium.org/2897063002/diff/60001/content/browser/loader/... content/browser/loader/url_loader_request_handler.h:18: class URLLoaderRequestHandler { On 2017/05/26 01:28:31, michaeln wrote: > 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. 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? > 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. Thanks, that was exactly the part I wasn't too sure about. > 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. I think we probably need to handle similar situation in SW. (A bit hand-wavy for now) https://codereview.chromium.org/2897063002/diff/60001/content/browser/service... File content/browser/service_worker/service_worker_request_handler.cc (right): https://codereview.chromium.org/2897063002/diff/60001/content/browser/service... content/browser/service_worker/service_worker_request_handler.cc:246: NOTREACHED(); On 2017/05/25 15:12:24, scottmg wrote: > This is only for ServiceWorkerURLTrackingRequestHandler and > ServiceWorkerContextRequestHandler right? (i.e. > ServiceWorkerControlleeRequestHandler already works now) Yep. I could make only SWControlleeRequestHandler inherit from URLLoaderReqHandler but not this one, but it was a little easier
The CQ bit was checked by kinuko@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2897063002/diff/60001/content/browser/loader/... File content/browser/loader/navigation_url_loader_network_service.cc (right): https://codereview.chromium.org/2897063002/diff/60001/content/browser/loader/... content/browser/loader/navigation_url_loader_network_service.cc:140: // This could be called multiple times. On 2017/05/25 15:12:24, scottmg wrote: > nit; comment that this is URLLoaderRH::Controller. (Removed the Controller interface) https://codereview.chromium.org/2897063002/diff/60001/content/browser/loader/... content/browser/loader/navigation_url_loader_network_service.cc:153: base::debug::StackTrace trace; On 2017/05/25 16:06:37, jam wrote: > nit: is this still needed? Removed. https://codereview.chromium.org/2897063002/diff/60001/content/browser/loader/... File content/browser/loader/url_loader_request_handler.h (right): https://codereview.chromium.org/2897063002/diff/60001/content/browser/loader/... content/browser/loader/url_loader_request_handler.h:17: // the lifetime of a request (including multiple redirect legs). On 2017/05/25 15:12:24, scottmg wrote: > Mention that this is IO thread. Done. https://codereview.chromium.org/2897063002/diff/60001/content/browser/loader/... content/browser/loader/url_loader_request_handler.h:18: class URLLoaderRequestHandler { On 2017/05/26 02:34:20, kinuko wrote: > On 2017/05/26 01:28:31, michaeln wrote: > > 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. > > 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? 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. https://codereview.chromium.org/2897063002/diff/60001/content/browser/loader/... content/browser/loader/url_loader_request_handler.h:39: DISALLOW_COPY_AND_ASSIGN(URLLoaderRequestHandler); On 2017/05/25 16:06:37, jam wrote: > nit: not needed on non-copyable interfaces Removed. https://codereview.chromium.org/2897063002/diff/60001/content/browser/service... File content/browser/service_worker/service_worker_controllee_request_handler.h (right): https://codereview.chromium.org/2897063002/diff/60001/content/browser/service... content/browser/service_worker/service_worker_controllee_request_handler.h:64: // Used only for PlzNavigate and --enable-network-service cases. On 2017/05/26 01:28:31, michaeln wrote: > Maybe comment that this method replaces MaybeCreateJob() in the new > NetworkService system Done.
The CQ bit was checked by kinuko@chromium.org to run a CQ dry run
Patchset #4 (id:80001) has been deleted
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm, thanks this looks great.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
lgtm! i have a couple small comments to consider but this looks great https://codereview.chromium.org/2897063002/diff/100001/content/browser/loader... File content/browser/loader/navigation_url_loader_network_service.cc (right): https://codereview.chromium.org/2897063002/diff/100001/content/browser/loader... content/browser/loader/navigation_url_loader_network_service.cc:97: Restart(std::move(url_loader_request), std::move(url_loader_client_ptr)); Would it work to more directly invoke factory_for_webui_ptr->CreateLoaderAndStart() in this clause, we shouldn't see redirection (Restart) is this case and i thinjk wouldn't need the fallback_factory_ptr_ datamember at all, the bound factory_ptr could be local to this block. Also could make the straight line webui handling happens in more obvious. https://codereview.chromium.org/2897063002/diff/100001/content/browser/loader... content/browser/loader/navigation_url_loader_network_service.cc:126: fallback_factory_ = url_loader_factory_getter->GetNetworkFactory()->get(); consider calling this the network_factory, i think fallback_factory is confusing considering the what it means to retrieve a fallback from the appcache https://codereview.chromium.org/2897063002/diff/100001/content/browser/loader... content/browser/loader/navigation_url_loader_network_service.cc:235: mojom::URLLoaderFactoryPtrInfo factory_ptr_info; nit: maybe rename this factory_for_webui to line up with Start() method param naming https://codereview.chromium.org/2897063002/diff/100001/content/browser/loader... File content/browser/loader/url_loader_request_handler.h (right): https://codereview.chromium.org/2897063002/diff/100001/content/browser/loader... content/browser/loader/url_loader_request_handler.h:34: // to fallback to the network.) Can we specify that the returned URLLoaderFactory need only support a single call to CreateLoaderAndStart(), so it would be valid for subsequent calls to CreateLoaderAndStart() or any calls to the SyncLoad() to CHECK(false). class OnceURLLoaderFactory would probably be overkill, but you get the idea https://codereview.chromium.org/2897063002/diff/100001/content/browser/servic... File content/browser/service_worker/service_worker_request_handler.h (right): https://codereview.chromium.org/2897063002/diff/100001/content/browser/servic... content/browser/service_worker/service_worker_request_handler.h:48: public URLLoaderRequestHandler { i like this derivation :)
https://codereview.chromium.org/2897063002/diff/100001/content/browser/loader... File content/browser/loader/navigation_url_loader_network_service.cc (right): https://codereview.chromium.org/2897063002/diff/100001/content/browser/loader... content/browser/loader/navigation_url_loader_network_service.cc:66: resource_context_(resource_context) {} Don't forget to initialize fallback_factory_(nullptr) here, otherwise the DCHECK(!...) will trigger in debug builds.
https://codereview.chromium.org/2897063002/diff/60001/content/browser/loader/... File content/browser/loader/url_loader_request_handler.h (right): https://codereview.chromium.org/2897063002/diff/60001/content/browser/loader/... content/browser/loader/url_loader_request_handler.h:18: class URLLoaderRequestHandler { > I think we probably need to handle similar situation in SW. (A bit hand-wavy for > now) 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. https://codereview.chromium.org/2897063002/diff/100001/content/browser/loader... File content/browser/loader/navigation_url_loader_network_service.cc (right): https://codereview.chromium.org/2897063002/diff/100001/content/browser/loader... content/browser/loader/navigation_url_loader_network_service.cc:245: g_next_request_id--; is this global is being decremented on the UI thread and used on the IO thread? i'd vote to latch the value into a NavigationURLLoaderNetworkService instance member in the initialize list https://codereview.chromium.org/2897063002/diff/100001/content/browser/loader... content/browser/loader/navigation_url_loader_network_service.cc:334: GlobalRequestID(-1, g_next_request_id), i was assuming we're on the IO thread here, but maybe that's not the case, i suppose this is on the UI thread too
I reimplemented the browser-side loading path (the factory and its urlloader) here: https://codereview.chromium.org/2906163002/ One thing that I think might make sense here is to change where the URLLoaderClient is bound. If we're planning on having the chains/factories live on the IO thread, it'd be nice if the client methods could be called from the IO thread.
Thanks! https://codereview.chromium.org/2897063002/diff/100001/content/browser/loader... File content/browser/loader/navigation_url_loader_network_service.cc (right): https://codereview.chromium.org/2897063002/diff/100001/content/browser/loader... content/browser/loader/navigation_url_loader_network_service.cc:66: resource_context_(resource_context) {} On 2017/05/26 20:19:27, scottmg wrote: > Don't forget to initialize fallback_factory_(nullptr) here, otherwise the > DCHECK(!...) will trigger in debug builds. Done. https://codereview.chromium.org/2897063002/diff/100001/content/browser/loader... content/browser/loader/navigation_url_loader_network_service.cc:97: Restart(std::move(url_loader_request), std::move(url_loader_client_ptr)); On 2017/05/26 17:42:27, michaeln wrote: > Would it work to more directly invoke > factory_for_webui_ptr->CreateLoaderAndStart() in this clause, we shouldn't see > redirection (Restart) is this case and i thinjk wouldn't need the > fallback_factory_ptr_ datamember at all, the bound factory_ptr could be local to > this block. Also could make the straight line webui handling happens in more > obvious. Done. https://codereview.chromium.org/2897063002/diff/100001/content/browser/loader... content/browser/loader/navigation_url_loader_network_service.cc:126: fallback_factory_ = url_loader_factory_getter->GetNetworkFactory()->get(); On 2017/05/26 17:42:27, michaeln wrote: > consider calling this the network_factory, i think fallback_factory is confusing > considering the what it means to retrieve a fallback from the appcache Done. https://codereview.chromium.org/2897063002/diff/100001/content/browser/loader... content/browser/loader/navigation_url_loader_network_service.cc:235: mojom::URLLoaderFactoryPtrInfo factory_ptr_info; On 2017/05/26 17:42:27, michaeln wrote: > nit: maybe rename this factory_for_webui to line up with Start() method param > naming Done. https://codereview.chromium.org/2897063002/diff/100001/content/browser/loader... content/browser/loader/navigation_url_loader_network_service.cc:245: g_next_request_id--; On 2017/05/26 21:36:25, michaeln wrote: > is this global is being decremented on the UI thread and used on the IO thread? > i'd vote to latch the value into a NavigationURLLoaderNetworkService instance > member in the initialize list Currently this one's bound to UI thread, we only use this there. +1 to making this a bit saner, leaving it as is in this CL for now though https://codereview.chromium.org/2897063002/diff/100001/content/browser/loader... content/browser/loader/navigation_url_loader_network_service.cc:334: GlobalRequestID(-1, g_next_request_id), On 2017/05/26 21:36:25, michaeln wrote: > i was assuming we're on the IO thread here, but maybe that's not the case, i > suppose this is on the UI thread too Yep. https://codereview.chromium.org/2897063002/diff/100001/content/browser/loader... File content/browser/loader/url_loader_request_handler.h (right): https://codereview.chromium.org/2897063002/diff/100001/content/browser/loader... content/browser/loader/url_loader_request_handler.h:34: // to fallback to the network.) On 2017/05/26 17:42:27, michaeln wrote: > Can we specify that the returned URLLoaderFactory need only support a single > call to CreateLoaderAndStart(), so it would be valid for subsequent calls to > CreateLoaderAndStart() or any calls to the SyncLoad() to CHECK(false). > > class OnceURLLoaderFactory would probably be overkill, but you get the idea Done.
On 2017/05/27 00:46:18, scottmg wrote: > I reimplemented the browser-side loading path (the factory and its urlloader) > here: https://codereview.chromium.org/2906163002/ > > One thing that I think might make sense here is to change where the > URLLoaderClient is bound. If we're planning on having the chains/factories live > on the IO thread, it'd be nice if the client methods could be called from the IO > thread. Yeah, I found the current thread settings a bit odd, while all the NavigationURLLoaderDelegate methods are expected to be called on UI thread so we'll need thread forwarding stuff there if we do so. I'm putting off the decision to the follow-up CLs.
The CQ bit was checked by kinuko@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from michaeln@chromium.org, jam@chromium.org Link to the patchset: https://codereview.chromium.org/2897063002/#ps120001 (title: ".")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
falken@chromium.org changed reviewers: + falken@chromium.org
Just some nits. https://codereview.chromium.org/2897063002/diff/120001/content/browser/loader... File content/browser/loader/navigation_url_loader_network_service.cc (right): https://codereview.chromium.org/2897063002/diff/120001/content/browser/loader... content/browser/loader/navigation_url_loader_network_service.cc:9: #include "base/debug/stack_trace.h" nit: rm this? https://codereview.chromium.org/2897063002/diff/120001/content/browser/loader... content/browser/loader/navigation_url_loader_network_service.cc:60: Is it worth making a class comment? URLLoaderRequestController sends a ResourceRequest to the appropriate URLLoaderFactory. It builds a vector of URLLoaderRequestHandlers and successively calls MaybeCreateLoaderFactory on each until the request is successfully handled. https://codereview.chromium.org/2897063002/diff/120001/content/browser/loader... content/browser/loader/navigation_url_loader_network_service.cc:174: // The factory for doing a vanilla network request. (dunno if there's a better name here)
The CQ bit was unchecked by kinuko@chromium.org
Thanks! https://codereview.chromium.org/2897063002/diff/120001/content/browser/loader... File content/browser/loader/navigation_url_loader_network_service.cc (right): https://codereview.chromium.org/2897063002/diff/120001/content/browser/loader... content/browser/loader/navigation_url_loader_network_service.cc:9: #include "base/debug/stack_trace.h" On 2017/05/29 06:51:12, falken wrote: > nit: rm this? Oops, thanks, done. https://codereview.chromium.org/2897063002/diff/120001/content/browser/loader... content/browser/loader/navigation_url_loader_network_service.cc:60: On 2017/05/29 06:51:12, falken wrote: > Is it worth making a class comment? > > URLLoaderRequestController sends a ResourceRequest to the appropriate > URLLoaderFactory. It builds a vector of URLLoaderRequestHandlers and > successively calls MaybeCreateLoaderFactory on each until the request is > successfully handled. Done, with slight modifications. https://codereview.chromium.org/2897063002/diff/120001/content/browser/loader... content/browser/loader/navigation_url_loader_network_service.cc:174: On 2017/05/29 06:51:12, falken wrote: > // The factory for doing a vanilla network request. > (dunno if there's a better name here) Done.
The CQ bit was checked by kinuko@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from michaeln@chromium.org, jam@chromium.org Link to the patchset: https://codereview.chromium.org/2897063002/#ps140001 (title: "documentation")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
(ah, forgot to say lgtm too)
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1496041565886910,
"parent_rev": "535b6156d10f03d8da4f0115773120f7ba694f01", "commit_rev":
"697329710bb599e46ae7dc72dfc5f88c4abc4545"}
Message was sent while issue was closed.
Description was changed from ========== Network service: Implement URLLoader chaining for interceptors BUG=724322 ========== to ========== Network service: Implement URLLoader chaining for interceptors BUG=724322 Review-Url: https://codereview.chromium.org/2897063002 Cr-Commit-Position: refs/heads/master@{#475314} Committed: https://chromium.googlesource.com/chromium/src/+/697329710bb599e46ae7dc72dfc5... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:140001) as https://chromium.googlesource.com/chromium/src/+/697329710bb599e46ae7dc72dfc5... |
