Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2982363002:
Add support for fallback content for the frame. This includes main and subframes. (Closed)
DescriptionAdd support for fallback content for the frame. This includes main and subframes.
To achieve this the following changes were done.
1. The URLLoaderRequestHandler which is implemented by the navigation request handlers
like AppCache/ServiceWorker, etc now has the following method
MaybeCreateLoaderForResponse(). AppCache returns fallback content for the response
passed in if applicable.
2. The AppCacheRequestHandler class which implements the URLLoaderRequestHandler interface
implements the above method. It uses the MaybeLoadFallbackForResponse() function to potentially
return fallback content. The difference between fallback handling for subresources and the frame
is a new job is created for the latter. The MaybeCreateJobForFallback() function
has been updated with this check.
3. The URLLoaderRequestController class which handles top level navigations now provides
functionality to invoke the registered handlers when it receives a response. This is only done
if the request was sent to the default network loader. A boolean flag default_loader_used_ is used to
track this. If this is set we invoke the registered handlers and pass in the bound URLLoaderClient and
the request to the handlers which enables them to bind to the connection.
4. We don't use the throttling URL loader for this case. This is fine because the main URL would already have been
checked by the throtting URL loader when it was sent to the network service. Added a function DisconnectClient()
to the throttling URL loader which disconnects the client from the network service in case we are passing fallback
content to the client.
BUG=715632
Review-Url: https://codereview.chromium.org/2982363002
Cr-Commit-Position: refs/heads/master@{#490666}
Committed: https://chromium.googlesource.com/chromium/src/+/b9800e5823ff3358a4f90d1a9e72c26a3d742988
Patch Set 1 #Patch Set 2 : Cleanup #Patch Set 3 : More cleanup #
Total comments: 2
Patch Set 4 : Renamed the ThrottlingURLLoader's cancelled_by_throttle_ member to loader_cancelled_ and set it in … #
Total comments: 4
Patch Set 5 : Address review comments. #
Total comments: 23
Patch Set 6 : Address review comments. Add the fallback function as a parameter to LoaderCallback #
Total comments: 4
Patch Set 7 : Revert back to the previous patch set with the new method MaybeCreateLoaderForResponse in the URLLo… #Patch Set 8 : Rename stale methods #Patch Set 9 : Fix compile failures #Patch Set 10 : Fix other issues pointed in an earlier iteration #Patch Set 11 : Remove the MaybeGetFallbackForRedirect() method from AppCacheRequestHandler #Patch Set 12 : Changed the MaybeCreateLoaderForResponse function to return the URLLoaderPtr and URLLoaderClientReq… #
Total comments: 3
Patch Set 13 : Remove newline #
Total comments: 17
Patch Set 14 : Address review comments #Patch Set 15 : rebase to tip #Messages
Total messages: 75 (38 generated)
ananta@chromium.org changed reviewers: + jam@chromium.org, michaeln@chromium.org
Description was changed from
==========
Add support for fallback content for the frame. This includes main and
subframes.
To achieve this the following changes were done.
1. The URLLoaderRequestHandler which is implemented by the navigation request
handlers
like AppCache/ServiceWorker, etc now has two new methods.
A. MaybeGetFallbackForResponse(). This returns fallback content for the
response
passed in if applicable.
B. MaybeGetFallbackForRedirect(). This returns fallback content for the
redirect
response passed in if applicable.
2. The AppCacheRequestHandler class which implements the URLLoaderRequestHandler
interface
implements the above methods. It uses the MaybeLoadFallbackForResponse() and
MaybeLoadFallbackForRedirect()
functions to potentially return fallback content. The difference between
fallback handling
for subresources and the frame is a new job is created for the latter. The
MaybeCreateJobForFallback() function
has been updated with this check.
3. The URLLoaderRequestController class which handles top level navigations now
provides
functionality to invoke the registered handlers for fallback content. This is
only done
if the request was sent to the default network loader. A boolean flag
default_loader_used_ is used to
track this. If this is set we invoke the registered handlers and pass in the
bound URLLoaderClient and
the request to the handlers which enables them to bind to the connection.
4. We don't use the throttling URL loader for fallback. This is fine because the
main URL would already have been
checked by the throtting URL loader when it was sent to the network service.
Added a function DisconnectClient()
to the throttling URL loader which disconnects the client from the network
service in case we are passing fallback
content to the client.
BUG=715632
==========
to
==========
Add support for fallback content for the frame. This includes main and
subframes.
To achieve this the following changes were done.
1. The URLLoaderRequestHandler which is implemented by the navigation request
handlers
like AppCache/ServiceWorker, etc now has two new methods.
A. MaybeGetFallbackForResponse(). This returns fallback content for the
response
passed in if applicable.
B. MaybeGetFallbackForRedirect(). This returns fallback content for the
redirect
response passed in if applicable.
2. The AppCacheRequestHandler class which implements the URLLoaderRequestHandler
interface
implements the above methods. It uses the MaybeLoadFallbackForResponse() and
MaybeLoadFallbackForRedirect()
functions to potentially return fallback content. The difference between
fallback handling
for subresources and the frame is a new job is created for the latter. The
MaybeCreateJobForFallback() function
has been updated with this check.
3. The URLLoaderRequestController class which handles top level navigations now
provides
functionality to invoke the registered handlers for fallback content. This is
only done
if the request was sent to the default network loader. A boolean flag
default_loader_used_ is used to
track this. If this is set we invoke the registered handlers and pass in the
bound URLLoaderClient and
the request to the handlers which enables them to bind to the connection.
4. We don't use the throttling URL loader for fallback. This is fine because the
main URL would already have been
checked by the throtting URL loader when it was sent to the network service.
Added a function DisconnectClient()
to the throttling URL loader which disconnects the client from the network
service in case we are passing fallback
content to the client.
BUG=715632
==========
ananta@chromium.org changed reviewers: + yzshen@chromium.org
michaeln, please review everything.
jam, general content changes.
yzshen, please review changes to the ThrottlingURLLoader and
general mojo related code for correctness.
The CQ bit was checked by ananta@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
jam@chromium.org changed reviewers: + kinuko@chromium.org
+Kinuko who should take a look My main comment is whether we can hide these appcache specific methods from NavigationURLLoaderNetworkService and URLLoaderRequestHandler. As I understand it (please correct me if I'm wrong) fallback is when the appcache wants to serve the content only if the network has errors. Could we do this inside appcache handler instead? i.e. appcache URLLoader always handles URLs for which it has fallback content; internally it first calls the network service and if that fails, then it produces its own content. The benefit would be that we only have to complicate the appcache code and not the general networking code. WDYT? https://codereview.chromium.org/2982363002/diff/40001/content/common/throttli... File content/common/throttling_url_loader.h (right): https://codereview.chromium.org/2982363002/diff/40001/content/common/throttli... content/common/throttling_url_loader.h:72: void DisconnectClient(); please separate this from the other methods (which mirror URLLoader) and document it
On 2017/07/21 21:42:35, jam wrote: > +Kinuko who should take a look > > My main comment is whether we can hide these appcache specific methods from > NavigationURLLoaderNetworkService and URLLoaderRequestHandler. > > As I understand it (please correct me if I'm wrong) fallback is when the > appcache wants to serve the content only if the network has errors. Could we do > this inside appcache handler instead? i.e. appcache URLLoader always handles > URLs for which it has fallback content; internally it first calls the network > service and if that fails, then it produces its own content. The benefit would > be that we only have to complicate the appcache code and not the general > networking code. WDYT? > > https://codereview.chromium.org/2982363002/diff/40001/content/common/throttli... > File content/common/throttling_url_loader.h (right): > > https://codereview.chromium.org/2982363002/diff/40001/content/common/throttli... > content/common/throttling_url_loader.h:72: void DisconnectClient(); > please separate this from the other methods (which mirror URLLoader) and > document it AppCache can serve fallback content in other cases too. For e.g. if the response has specific headers, or during redirects, etc. For the frame for e.g the manifest could specify that any top level URL for that domain whether valid or not could have a fallback. It would be difficult to do this generically in the AppCache handler.
The CQ bit was checked by ananta@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/2982363002/diff/40001/content/common/throttli... File content/common/throttling_url_loader.h (right): https://codereview.chromium.org/2982363002/diff/40001/content/common/throttli... content/common/throttling_url_loader.h:72: void DisconnectClient(); On 2017/07/21 21:42:34, jam wrote: > please separate this from the other methods (which mirror URLLoader) and > document it Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/07/21 21:57:59, ananta wrote: > On 2017/07/21 21:42:35, jam wrote: > > +Kinuko who should take a look > > > > My main comment is whether we can hide these appcache specific methods from > > NavigationURLLoaderNetworkService and URLLoaderRequestHandler. > > > > As I understand it (please correct me if I'm wrong) fallback is when the > > appcache wants to serve the content only if the network has errors. Could we > do > > this inside appcache handler instead? i.e. appcache URLLoader always handles > > URLs for which it has fallback content; internally it first calls the network > > service and if that fails, then it produces its own content. The benefit would > > be that we only have to complicate the appcache code and not the general > > networking code. WDYT? > > > > > https://codereview.chromium.org/2982363002/diff/40001/content/common/throttli... > > File content/common/throttling_url_loader.h (right): > > > > > https://codereview.chromium.org/2982363002/diff/40001/content/common/throttli... > > content/common/throttling_url_loader.h:72: void DisconnectClient(); > > please separate this from the other methods (which mirror URLLoader) and > > document it > > AppCache can serve fallback content in other cases too. For e.g. if the response > has specific headers, or during > redirects, etc. If appcache handler is intercepting all requests when there's an appcache for a page, then it would be able to serve fallback when it sees these cases too. > For the frame for e.g the manifest could specify that any top > level URL for that domain whether valid or > not could have a fallback. It would be difficult to do this generically in the > AppCache handler. Can you expand more on why that'd be difficult?
Only a couple of nits for mojo stuff and throttling_url_loader. Other than that, I think I also like John's idea of isolating this fallback logic in appache's own handler if possible. https://codereview.chromium.org/2982363002/diff/60001/content/browser/appcach... File content/browser/appcache/appcache_url_loader_job.h (right): https://codereview.chromium.org/2982363002/diff/60001/content/browser/appcach... content/browser/appcache/appcache_url_loader_job.h:115: // Binds to the URLLoaderClient instance passed in via the |client| The naming and comment are a little confusing: this method actually binds |request|, not |client|. https://codereview.chromium.org/2982363002/diff/60001/content/common/throttli... File content/common/throttling_url_loader.cc (right): https://codereview.chromium.org/2982363002/diff/60001/content/common/throttli... content/common/throttling_url_loader.cc:297: loader_cancelled_ = true; nit: You could remove this line because DisconnectClient() would do it.
On 2017/07/24 15:48:49, jam wrote: > On 2017/07/21 21:57:59, ananta wrote: > > On 2017/07/21 21:42:35, jam wrote: > > > +Kinuko who should take a look > > > > > > My main comment is whether we can hide these appcache specific methods from > > > NavigationURLLoaderNetworkService and URLLoaderRequestHandler. > > > > > > As I understand it (please correct me if I'm wrong) fallback is when the > > > appcache wants to serve the content only if the network has errors. Could we > > do > > > this inside appcache handler instead? i.e. appcache URLLoader always handles > > > URLs for which it has fallback content; internally it first calls the > network > > > service and if that fails, then it produces its own content. The benefit > would > > > be that we only have to complicate the appcache code and not the general > > > networking code. WDYT? > > > > > > > > > https://codereview.chromium.org/2982363002/diff/40001/content/common/throttli... > > > File content/common/throttling_url_loader.h (right): > > > > > > > > > https://codereview.chromium.org/2982363002/diff/40001/content/common/throttli... > > > content/common/throttling_url_loader.h:72: void DisconnectClient(); > > > please separate this from the other methods (which mirror URLLoader) and > > > document it > > > > AppCache can serve fallback content in other cases too. For e.g. if the > response > > has specific headers, or during > > redirects, etc. > > If appcache handler is intercepting all requests when there's an appcache for a > page, then it would be able to serve fallback when it sees these cases too. > > > For the frame for e.g the manifest could specify that any top > > level URL for that domain whether valid or > > not could have a fallback. It would be difficult to do this generically in the > > AppCache handler. > > Can you expand more on why that'd be difficult? AppCache does not intercept all requests. For e.g. if foo.com has an appcache associated with it, foo.com/bar may not. If the manifest for foo.com indicates that it has fallback for all top level urls associated with that domain, then if the network request for foo.com/bar fails, then that fallback could be served out. AppCache cannot directly return a fallback without the network request for foo.com/bar actually going out. Same holds true for fallback during redirects.
https://codereview.chromium.org/2982363002/diff/60001/content/browser/appcach... File content/browser/appcache/appcache_url_loader_job.h (right): https://codereview.chromium.org/2982363002/diff/60001/content/browser/appcach... content/browser/appcache/appcache_url_loader_job.h:115: // Binds to the URLLoaderClient instance passed in via the |client| On 2017/07/24 17:48:54, yzshen1 wrote: > The naming and comment are a little confusing: this method actually binds > |request|, not |client|. Thanks. Renamed to BindRequest https://codereview.chromium.org/2982363002/diff/60001/content/common/throttli... File content/common/throttling_url_loader.cc (right): https://codereview.chromium.org/2982363002/diff/60001/content/common/throttli... content/common/throttling_url_loader.cc:297: loader_cancelled_ = true; On 2017/07/24 17:48:54, yzshen1 wrote: > nit: You could remove this line because DisconnectClient() would do it. Done.
The CQ bit was checked by ananta@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Sorry for chiming in late. https://codereview.chromium.org/2982363002/diff/80001/content/browser/loader/... File content/browser/loader/navigation_url_loader_network_service.cc (right): https://codereview.chromium.org/2982363002/diff/80001/content/browser/loader/... content/browser/loader/navigation_url_loader_network_service.cc:267: return; In my understanding we don't need this code at least for appcache, as we don't seem to intercept cross-origin redirects for main resource? https://codereview.chromium.org/2982363002/diff/80001/content/browser/loader/... content/browser/loader/navigation_url_loader_network_service.cc:316: response, &fallback_client, &fallback_request)) { Hmm. Sorry I think I'm repeating the same comment but I do like to avoid this plumbing if possible as well. One thing I heard from michael is that we should be able to tell whether we have fallback handler for a request or not by the time MaybeCreateLoader completes. Would that mean that we could still intercept the request handling if it has a fallback handler for the URL, just proxy it to the network and supply the fallback whenever necessary?
Sorry for chiming in late.
https://codereview.chromium.org/2982363002/diff/80001/content/browser/loader/... File content/browser/loader/navigation_url_loader_network_service.cc (right): https://codereview.chromium.org/2982363002/diff/80001/content/browser/loader/... content/browser/loader/navigation_url_loader_network_service.cc:316: response, &fallback_client, &fallback_request)) { On 2017/07/25 07:12:05, kinuko wrote: > Hmm. Sorry I think I'm repeating the same comment but I do like to avoid this > plumbing if possible as well. > > One thing I heard from michael is that we should be able to tell whether we have > fallback handler for a request or not by the time MaybeCreateLoader completes. > Would that mean that we could still intercept the request handling if it has a > fallback handler for the URL, just proxy it to the network and supply the > fallback whenever necessary? Or we could probably make MaybeCreateLoader take an additional LoaderCallback parameter, like response_loader_callback... but if we can just do this within appcache request handler I that'd feel much better imo.
https://codereview.chromium.org/2982363002/diff/80001/content/browser/loader/... File content/browser/loader/navigation_url_loader_network_service.cc (right): https://codereview.chromium.org/2982363002/diff/80001/content/browser/loader/... content/browser/loader/navigation_url_loader_network_service.cc:316: response, &fallback_client, &fallback_request)) { On 2017/07/25 07:12:05, kinuko wrote: > Hmm. Sorry I think I'm repeating the same comment but I do like to avoid this > plumbing if possible as well. > > One thing I heard from michael is that we should be able to tell whether we have > fallback handler for a request or not by the time MaybeCreateLoader completes. > Would that mean that we could still intercept the request handling if it has a > fallback handler for the URL, just proxy it to the network and supply the > fallback whenever necessary? Yep this is what I meant :) it's a change from the old code works, but that's irrelevant IMO. The appcache URLLoader would intercept all origins for which it could possibly serve fallback content.
https://codereview.chromium.org/2982363002/diff/80001/content/browser/loader/... File content/browser/loader/navigation_url_loader_network_service.cc (right): https://codereview.chromium.org/2982363002/diff/80001/content/browser/loader/... content/browser/loader/navigation_url_loader_network_service.cc:267: return; On 2017/07/25 07:12:05, kinuko wrote: > In my understanding we don't need this code at least for appcache, as we don't > seem to intercept cross-origin redirects for main resource? I think we need this code for same origin redirects? https://codereview.chromium.org/2982363002/diff/80001/content/browser/loader/... content/browser/loader/navigation_url_loader_network_service.cc:316: response, &fallback_client, &fallback_request)) { On 2017/07/25 07:12:05, kinuko wrote: > Hmm. Sorry I think I'm repeating the same comment but I do like to avoid this > plumbing if possible as well. > > One thing I heard from michael is that we should be able to tell whether we have > fallback handler for a request or not by the time MaybeCreateLoader completes. > Would that mean that we could still intercept the request handling if it has a > fallback handler for the URL, just proxy it to the network and supply the > fallback whenever necessary? Perhaps I am missing something here. Seems like it is desirable that the handlers don't directly call out to the network. We always want the other handlers later in the list to get control? https://codereview.chromium.org/2982363002/diff/80001/content/browser/loader/... content/browser/loader/navigation_url_loader_network_service.cc:316: response, &fallback_client, &fallback_request)) { On 2017/07/25 07:28:42, kinuko wrote: > On 2017/07/25 07:12:05, kinuko wrote: > > Hmm. Sorry I think I'm repeating the same comment but I do like to avoid this > > plumbing if possible as well. > > > > One thing I heard from michael is that we should be able to tell whether we > have > > fallback handler for a request or not by the time MaybeCreateLoader completes. > > > Would that mean that we could still intercept the request handling if it has a > > fallback handler for the URL, just proxy it to the network and supply the > > fallback whenever necessary? > > Or we could probably make MaybeCreateLoader take an additional LoaderCallback > parameter, like response_loader_callback... but if we can just do this within > appcache request handler I that'd feel much better imo. I am not sure if transparently proxying to the n/w from the appcache handler is a good idea. Perhaps we can return a signal from the handler that it has fallback potentially so the service can call out to it. That also means that the NavigationURLLoaderNetworkService has to have code to specifically request fallback. I am unsure if that is cleaner than adding the methods to the handler interface.
https://codereview.chromium.org/2982363002/diff/80001/content/browser/loader/... File content/browser/loader/navigation_url_loader_network_service.cc (right): https://codereview.chromium.org/2982363002/diff/80001/content/browser/loader/... content/browser/loader/navigation_url_loader_network_service.cc:267: return; On 2017/07/25 22:33:00, ananta wrote: > On 2017/07/25 07:12:05, kinuko wrote: > > In my understanding we don't need this code at least for appcache, as we don't > > seem to intercept cross-origin redirects for main resource? > > I think we need this code for same origin redirects? I was looking into this code where it seems we're returning nullptr if it's main resource: https://cs.chromium.org/chromium/src/content/browser/appcache/appcache_reques... but I could be reading the code wrongly. https://codereview.chromium.org/2982363002/diff/80001/content/browser/loader/... content/browser/loader/navigation_url_loader_network_service.cc:316: response, &fallback_client, &fallback_request)) { On 2017/07/25 22:33:00, ananta wrote: > On 2017/07/25 07:12:05, kinuko wrote: > > Hmm. Sorry I think I'm repeating the same comment but I do like to avoid this > > plumbing if possible as well. > > > > One thing I heard from michael is that we should be able to tell whether we > have > > fallback handler for a request or not by the time MaybeCreateLoader completes. > > > Would that mean that we could still intercept the request handling if it has a > > fallback handler for the URL, just proxy it to the network and supply the > > fallback whenever necessary? > > Perhaps I am missing something here. Seems like it is desirable that the > handlers don't directly call out to the network. We always want the other > handlers later in the list to get control? Hm, it sounds like a good point.
https://codereview.chromium.org/2982363002/diff/80001/content/browser/loader/... File content/browser/loader/navigation_url_loader_network_service.cc (right): https://codereview.chromium.org/2982363002/diff/80001/content/browser/loader/... content/browser/loader/navigation_url_loader_network_service.cc:316: response, &fallback_client, &fallback_request)) { On 2017/07/25 23:23:13, kinuko wrote: > On 2017/07/25 22:33:00, ananta wrote: > > On 2017/07/25 07:12:05, kinuko wrote: > > > Hmm. Sorry I think I'm repeating the same comment but I do like to avoid > this > > > plumbing if possible as well. > > > > > > One thing I heard from michael is that we should be able to tell whether we > > have > > > fallback handler for a request or not by the time MaybeCreateLoader > completes. > > > > > Would that mean that we could still intercept the request handling if it has > a > > > fallback handler for the URL, just proxy it to the network and supply the > > > fallback whenever necessary? > > > > Perhaps I am missing something here. Seems like it is desirable that the > > handlers don't directly call out to the network. We always want the other > > handlers later in the list to get control? > > Hm, it sounds like a good point. Can you expand on why it's desirable that the appcache handler doesn't call out to the network? Is that in case it redirects to service worker?
https://codereview.chromium.org/2982363002/diff/80001/content/browser/loader/... File content/browser/loader/navigation_url_loader_network_service.cc (right): https://codereview.chromium.org/2982363002/diff/80001/content/browser/loader/... content/browser/loader/navigation_url_loader_network_service.cc:316: response, &fallback_client, &fallback_request)) { On 2017/07/26 01:40:51, jam wrote: > On 2017/07/25 23:23:13, kinuko wrote: > > On 2017/07/25 22:33:00, ananta wrote: > > > On 2017/07/25 07:12:05, kinuko wrote: > > > > Hmm. Sorry I think I'm repeating the same comment but I do like to avoid > > this > > > > plumbing if possible as well. > > > > > > > > One thing I heard from michael is that we should be able to tell whether > we > > > have > > > > fallback handler for a request or not by the time MaybeCreateLoader > > completes. > > > > > > > Would that mean that we could still intercept the request handling if it > has > > a > > > > fallback handler for the URL, just proxy it to the network and supply the > > > > fallback whenever necessary? > > > > > > Perhaps I am missing something here. Seems like it is desirable that the > > > handlers don't directly call out to the network. We always want the other > > > handlers later in the list to get control? > > > > Hm, it sounds like a good point. > > Can you expand on why it's desirable that the appcache handler doesn't call out > to the network? Is that in case it redirects to service worker? One of the cases that come to my mind is like: - appcache has a fallback handler for a URL, but doesn't handle the request - SW is registered there, so that SW needs to serve the response whenever possible - While SW is not activated the appcache should still be able to give its fallback response I think some sites use this trick... https://codereview.chromium.org/2982363002/diff/80001/content/browser/loader/... File content/browser/loader/url_loader_request_handler.h (right): https://codereview.chromium.org/2982363002/diff/80001/content/browser/loader/... content/browser/loader/url_loader_request_handler.h:50: mojom::URLLoaderRequest* request); (If we end up having this kind of hooks) Could we try to make it look like a more generic response interception hook? I'd imagine something like: void MaybeCreateLoader(...); void MaybeCreateLoaderForResponse(...); It also feels a bit unfortunate to need to pre-create ClientPtr and Request pairs on every responses, could we avoid doing that?
I'd like to focus on the URLLoaderRequestHandler API and to come to an agreement on that before diving into the impl details too much. I think one reason it's good to have the fallback stuff exposed at the higher level (the navloader) is to explain it, otherwise it's too mysterious. https://codereview.chromium.org/2982363002/diff/80001/content/browser/appcach... File content/browser/appcache/appcache_request_handler.cc (right): https://codereview.chromium.org/2982363002/diff/80001/content/browser/appcach... content/browser/appcache/appcache_request_handler.cc:587: navigation_request_job_.release() if navigation_request_job_ is never retained beyond the end of this method, i'm not sure we need the instance ptr? https://codereview.chromium.org/2982363002/diff/80001/content/browser/appcach... File content/browser/appcache/appcache_url_loader_job.cc (right): https://codereview.chromium.org/2982363002/diff/80001/content/browser/appcach... content/browser/appcache/appcache_url_loader_job.cc:279: if (IsResourceTypeFrame(request_.resource_type) && !client_) { i don't understand the purpose of the !client_ check and why that affects whether we could invoke the loader_callback? https://codereview.chromium.org/2982363002/diff/80001/content/browser/loader/... File content/browser/loader/navigation_url_loader_network_service.cc (right): https://codereview.chromium.org/2982363002/diff/80001/content/browser/loader/... content/browser/loader/navigation_url_loader_network_service.cc:267: return; On 2017/07/25 23:23:14, kinuko wrote: > On 2017/07/25 22:33:00, ananta wrote: > > On 2017/07/25 07:12:05, kinuko wrote: > > > In my understanding we don't need this code at least for appcache, as we > don't > > > seem to intercept cross-origin redirects for main resource? > > > > I think we need this code for same origin redirects? > > I was looking into this code where it seems we're returning nullptr if it's main > resource: > https://cs.chromium.org/chromium/src/content/browser/appcache/appcache_reques... > > but I could be reading the code wrongly. Right! We don't need redirect fallback handling for the main resource. https://codereview.chromium.org/2982363002/diff/80001/content/browser/loader/... content/browser/loader/navigation_url_loader_network_service.cc:316: response, &fallback_client, &fallback_request)) { On 2017/07/26 01:59:14, kinuko wrote: > On 2017/07/26 01:40:51, jam wrote: > > On 2017/07/25 23:23:13, kinuko wrote: > > > On 2017/07/25 22:33:00, ananta wrote: > > > > On 2017/07/25 07:12:05, kinuko wrote: > > > > > Hmm. Sorry I think I'm repeating the same comment but I do like to avoid > > > this > > > > > plumbing if possible as well. > > > > > > > > > > One thing I heard from michael is that we should be able to tell whether > > we > > > > have > > > > > fallback handler for a request or not by the time MaybeCreateLoader > > > completes. > > > > > > > > > Would that mean that we could still intercept the request handling if it > > has > > > a > > > > > fallback handler for the URL, just proxy it to the network and supply > the > > > > > fallback whenever necessary? > > > > > > > > Perhaps I am missing something here. Seems like it is desirable that the > > > > handlers don't directly call out to the network. We always want the other > > > > handlers later in the list to get control? > > > > > > Hm, it sounds like a good point. > > > > Can you expand on why it's desirable that the appcache handler doesn't call > out > > to the network? Is that in case it redirects to service worker? > > One of the cases that come to my mind is like: > - appcache has a fallback handler for a URL, but doesn't handle the request > - SW is registered there, so that SW needs to serve the response whenever > possible > - While SW is not activated the appcache should still be able to give its > fallback response > > I think some sites use this trick... I think its easier to reason about too if the appcache loader just loads appcached resources. Also having fewer ways in which the network loader is used reduces code paths and the potential for different behavior depending on which one is used. https://codereview.chromium.org/2982363002/diff/80001/content/browser/loader/... File content/browser/loader/url_loader_request_handler.h (right): https://codereview.chromium.org/2982363002/diff/80001/content/browser/loader/... content/browser/loader/url_loader_request_handler.h:50: mojom::URLLoaderRequest* request); On 2017/07/26 01:59:14, kinuko wrote: > (If we end up having this kind of hooks) > > Could we try to make it look like a more generic response interception hook? > I'd imagine something like: > > void MaybeCreateLoader(...); > void MaybeCreateLoaderForResponse(...); > > It also feels a bit unfortunate to need to pre-create ClientPtr and Request > pairs on every responses, could we avoid doing that? I don't know how much it helps, but we could expose the fallback handling with a different API. The appcache system knows prior to returning from MaybeCreateLoader() whether or not it has a fallback. There are three possible return values from MaybeCreateLoader() - a loader that loads the main resouce - a loader that loads a fallback resource - no loader There's no case for the appcache providing a loader and a fallback_loader. Here's a MaybeCreateLoaderCallback function signature that captures the possiblities. void MaybeCreateLoaderCallback( StartLoaderCallback start_it, ShouldFallbackForResponseFunction should_fallback_function); bool ShouldFallbackForResponseFunction(response); If |should_fallback_function| is not empty, that means |start_it| should only be used for response fallback handling when should_fallback_function(response) returns true. The should_fallback conditions are pretty simple. https://cs.chromium.org/chromium/src/content/browser/appcache/appcache_reques... To simplify things, we could prohibit use of that api except for the appcache's URLLoaderRequestHandler so the NavigationLoader would not have to deal with a list of these potential fallbacks. It would need two new data members: a should_fallback_function_ and a start_fallback_loader_function_. Would that help?
https://codereview.chromium.org/2982363002/diff/80001/content/browser/appcach... File content/browser/appcache/appcache_request_handler.cc (right): https://codereview.chromium.org/2982363002/diff/80001/content/browser/appcach... content/browser/appcache/appcache_request_handler.cc:587: navigation_request_job_.release() On 2017/07/26 23:08:41, michaeln wrote: > if navigation_request_job_ is never retained beyond the end of this method, i'm > not sure we need the instance ptr? Done. https://codereview.chromium.org/2982363002/diff/80001/content/browser/appcach... File content/browser/appcache/appcache_url_loader_job.cc (right): https://codereview.chromium.org/2982363002/diff/80001/content/browser/appcach... content/browser/appcache/appcache_url_loader_job.cc:279: if (IsResourceTypeFrame(request_.resource_type) && !client_) { On 2017/07/26 23:08:41, michaeln wrote: > i don't understand the purpose of the !client_ check and why that affects > whether we could invoke the loader_callback? For fallback responses the callback would be null. I changed the check to validate if the callback is valid. https://codereview.chromium.org/2982363002/diff/80001/content/browser/loader/... File content/browser/loader/url_loader_request_handler.h (right): https://codereview.chromium.org/2982363002/diff/80001/content/browser/loader/... content/browser/loader/url_loader_request_handler.h:50: mojom::URLLoaderRequest* request); On 2017/07/26 23:08:41, michaeln wrote: > On 2017/07/26 01:59:14, kinuko wrote: > > (If we end up having this kind of hooks) > > > > Could we try to make it look like a more generic response interception hook? > > I'd imagine something like: > > > > void MaybeCreateLoader(...); > > void MaybeCreateLoaderForResponse(...); > > > > It also feels a bit unfortunate to need to pre-create ClientPtr and Request > > pairs on every responses, could we avoid doing that? > > I don't know how much it helps, but we could expose the fallback handling with a > different API. The appcache system knows prior to returning from > MaybeCreateLoader() whether or not it has a fallback. There are three possible > return values from MaybeCreateLoader() > - a loader that loads the main resouce > - a loader that loads a fallback resource > - no loader > > There's no case for the appcache providing a loader and a fallback_loader. > > Here's a MaybeCreateLoaderCallback function signature that captures the > possiblities. > > void MaybeCreateLoaderCallback( > StartLoaderCallback start_it, > ShouldFallbackForResponseFunction should_fallback_function); > > bool ShouldFallbackForResponseFunction(response); > > If |should_fallback_function| is not empty, that means |start_it| should only be > used for response fallback handling when should_fallback_function(response) > returns true. > > The should_fallback conditions are pretty simple. > https://cs.chromium.org/chromium/src/content/browser/appcache/appcache_reques... > > To simplify things, we could prohibit use of that api except for the appcache's > URLLoaderRequestHandler so the NavigationLoader would not have to deal with a > list of these potential fallbacks. It would need two new data members: a > should_fallback_function_ and a start_fallback_loader_function_. > > Would that help? Removed the functions added to the URLLoaderRequestHandler interface. As per michaeln's suggestion here and our discussion, passing a callback function in the LoaderCallback seems to be better. The LoaderCallback signature has been modified to take in an additional callback function ResponseFallback which is passed in by AppCache if it finds fallback for the main resource. This caused some changes to other places which invoke the LoaderCallback to pass in an empty ResponseFallback() https://codereview.chromium.org/2982363002/diff/80001/content/browser/loader/... content/browser/loader/url_loader_request_handler.h:50: mojom::URLLoaderRequest* request); On 2017/07/26 01:59:14, kinuko wrote: > (If we end up having this kind of hooks) > > Could we try to make it look like a more generic response interception hook? > I'd imagine something like: > > void MaybeCreateLoader(...); > void MaybeCreateLoaderForResponse(...); > > It also feels a bit unfortunate to need to pre-create ClientPtr and Request > pairs on every responses, could we avoid doing that? We decided to go with the additional callback function parameter to LoaderCallback here. This also ensures that we create the clientptr and request only if we have a valid fallback https://codereview.chromium.org/2982363002/diff/100001/content/browser/servic... File content/browser/service_worker/service_worker_url_loader_job.cc (right): https://codereview.chromium.org/2982363002/diff/100001/content/browser/servic... content/browser/service_worker/service_worker_url_loader_job.cc:254: if (main_script_http_info) Was hitting this DCHECK and the subsequent crash later on. Please check if this change is ok.
The CQ bit was checked by ananta@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...
Description was changed from
==========
Add support for fallback content for the frame. This includes main and
subframes.
To achieve this the following changes were done.
1. The URLLoaderRequestHandler which is implemented by the navigation request
handlers
like AppCache/ServiceWorker, etc now has two new methods.
A. MaybeGetFallbackForResponse(). This returns fallback content for the
response
passed in if applicable.
B. MaybeGetFallbackForRedirect(). This returns fallback content for the
redirect
response passed in if applicable.
2. The AppCacheRequestHandler class which implements the URLLoaderRequestHandler
interface
implements the above methods. It uses the MaybeLoadFallbackForResponse() and
MaybeLoadFallbackForRedirect()
functions to potentially return fallback content. The difference between
fallback handling
for subresources and the frame is a new job is created for the latter. The
MaybeCreateJobForFallback() function
has been updated with this check.
3. The URLLoaderRequestController class which handles top level navigations now
provides
functionality to invoke the registered handlers for fallback content. This is
only done
if the request was sent to the default network loader. A boolean flag
default_loader_used_ is used to
track this. If this is set we invoke the registered handlers and pass in the
bound URLLoaderClient and
the request to the handlers which enables them to bind to the connection.
4. We don't use the throttling URL loader for fallback. This is fine because the
main URL would already have been
checked by the throtting URL loader when it was sent to the network service.
Added a function DisconnectClient()
to the throttling URL loader which disconnects the client from the network
service in case we are passing fallback
content to the client.
BUG=715632
==========
to
==========
To achieve this the following changes were done.
1. The LoaderCallback function now takes an additional parameter which is the
callback
function to be invoked if the handler has fallback content available.
Currently
AppCache is the only handler which supports fallback content. It passes a
valid callback
if it has fallback content available for the main resource.
2. The AppCacheRequestHandler class which implements the URLLoaderRequestHandler
interface
passes the callback function for fallback content via the
AppCacheURLLoaderJob.
3. The URLLoaderRequestController class which handles top level navigations now
provides
functionality to invoke the registered handlers for fallback content. This is
only done
if the request was sent to the default network loader. A boolean flag
default_loader_used_ is used to
track this. If this is set and there is fallback content available we invoke
the fallback callback function
and pass in the bound URLLoaderClient and the request to it which enables the
job to bind to the connection.
4. We don't use the throttling URL loader for fallback. This is fine because the
main URL would already have been
checked by the throtting URL loader when it was sent to the network service.
Added a function DisconnectClient()
to the throttling URL loader which disconnects the client from the network
service in case we are passing fallback
content to the client.
BUG=715632
==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
https://codereview.chromium.org/2982363002/diff/80001/content/browser/loader/... File content/browser/loader/url_loader_request_handler.h (right): https://codereview.chromium.org/2982363002/diff/80001/content/browser/loader/... content/browser/loader/url_loader_request_handler.h:50: mojom::URLLoaderRequest* request); On 2017/07/27 02:40:25, ananta wrote: > On 2017/07/26 23:08:41, michaeln wrote: > > On 2017/07/26 01:59:14, kinuko wrote: > > > (If we end up having this kind of hooks) > > > > > > Could we try to make it look like a more generic response interception hook? > > > > I'd imagine something like: > > > > > > void MaybeCreateLoader(...); > > > void MaybeCreateLoaderForResponse(...); > > > > > > It also feels a bit unfortunate to need to pre-create ClientPtr and Request > > > pairs on every responses, could we avoid doing that? > > > > I don't know how much it helps, but we could expose the fallback handling with > a > > different API. The appcache system knows prior to returning from > > MaybeCreateLoader() whether or not it has a fallback. There are three possible > > return values from MaybeCreateLoader() > > - a loader that loads the main resouce > > - a loader that loads a fallback resource > > - no loader > > > > There's no case for the appcache providing a loader and a fallback_loader. > > > > Here's a MaybeCreateLoaderCallback function signature that captures the > > possiblities. > > > > void MaybeCreateLoaderCallback( > > StartLoaderCallback start_it, > > ShouldFallbackForResponseFunction should_fallback_function); > > > > bool ShouldFallbackForResponseFunction(response); > > > > If |should_fallback_function| is not empty, that means |start_it| should only > be > > used for response fallback handling when should_fallback_function(response) > > returns true. > > > > The should_fallback conditions are pretty simple. > > > https://cs.chromium.org/chromium/src/content/browser/appcache/appcache_reques... > > > > To simplify things, we could prohibit use of that api except for the > appcache's > > URLLoaderRequestHandler so the NavigationLoader would not have to deal with a > > list of these potential fallbacks. It would need two new data members: a > > should_fallback_function_ and a start_fallback_loader_function_. > > > > Would that help? > > Removed the functions added to the URLLoaderRequestHandler interface. As per > michaeln's suggestion here and our discussion, passing a callback function in > the LoaderCallback > seems to be better. The LoaderCallback signature has been modified to take in an > additional callback function ResponseFallback which is passed in by AppCache if > it finds fallback for the main resource. This caused some changes to other > places which invoke the LoaderCallback to pass in an empty ResponseFallback() Hm... sorry to say this, but I think I might have preferred the previous API, which looked a little more straightforward and generic. (Additional context: I noticed there's another project, namely Web Packaging project, that might also need response interception, and I'm trying to see if both can use common mechanism.) https://codereview.chromium.org/2982363002/diff/100001/content/browser/loader... File content/browser/loader/navigation_url_loader_network_service.cc (right): https://codereview.chromium.org/2982363002/diff/100001/content/browser/loader... content/browser/loader/navigation_url_loader_network_service.cc:322: .Run(response, std::move(fallback_client), std::move(fallback_request)); This fallback code works in a very specific way, which might be ok within this scope but it wouldn't be usable for other use cases. I wonder if something like following API would work... if (MaybeInterceptResponse(response, url_loader_)) return true; The implementation can internally call url_loader->DisconnectClient() if necessary. DisconnectClient() can possibly return its previous forwarding_client_ pointer (i.e. this) so that the implementation take the client over and call its OnFoo methods to provide response data. The implementation does not really need to use mojo binding for the fallback then. In this way handler implementation needs to know about ThrottlingURLLoader which is a little ugly, but I hope we can possibly fix it by introducing an additional wrapper layer later (something like what Yuzhu suggested in https://chromium-review.googlesource.com/c/567610/10/content/renderer/rendere...) Wdyt? https://codereview.chromium.org/2982363002/diff/100001/content/browser/servic... File content/browser/service_worker/service_worker_url_loader_job.cc (right): https://codereview.chromium.org/2982363002/diff/100001/content/browser/servic... content/browser/service_worker/service_worker_url_loader_job.cc:254: if (main_script_http_info) On 2017/07/27 02:40:25, ananta wrote: > Was hitting this DCHECK and the subsequent crash later on. Please check if this > change is ok. This should be fixed by my other change- https://chromium-review.googlesource.com/c/584551/ If this doesn't break any other tests I prefer having the DCHECK as is (otherwise could you put "TODO(kinuko): fix this" here)?
https://codereview.chromium.org/2982363002/diff/80001/content/browser/loader/... File content/browser/loader/url_loader_request_handler.h (right): https://codereview.chromium.org/2982363002/diff/80001/content/browser/loader/... content/browser/loader/url_loader_request_handler.h:50: mojom::URLLoaderRequest* request); On 2017/07/27 02:40:25, ananta wrote: > On 2017/07/26 23:08:41, michaeln wrote: > > On 2017/07/26 01:59:14, kinuko wrote: > > > (If we end up having this kind of hooks) > > > > > > Could we try to make it look like a more generic response interception hook? > > > > I'd imagine something like: > > > > > > void MaybeCreateLoader(...); > > > void MaybeCreateLoaderForResponse(...); > > > > > > It also feels a bit unfortunate to need to pre-create ClientPtr and Request > > > pairs on every responses, could we avoid doing that? > > > > I don't know how much it helps, but we could expose the fallback handling with > a > > different API. The appcache system knows prior to returning from > > MaybeCreateLoader() whether or not it has a fallback. There are three possible > > return values from MaybeCreateLoader() > > - a loader that loads the main resouce > > - a loader that loads a fallback resource > > - no loader > > > > There's no case for the appcache providing a loader and a fallback_loader. > > > > Here's a MaybeCreateLoaderCallback function signature that captures the > > possiblities. > > > > void MaybeCreateLoaderCallback( > > StartLoaderCallback start_it, > > ShouldFallbackForResponseFunction should_fallback_function); > > > > bool ShouldFallbackForResponseFunction(response); > > > > If |should_fallback_function| is not empty, that means |start_it| should only > be > > used for response fallback handling when should_fallback_function(response) > > returns true. > > > > The should_fallback conditions are pretty simple. > > > https://cs.chromium.org/chromium/src/content/browser/appcache/appcache_reques... > > > > To simplify things, we could prohibit use of that api except for the > appcache's > > URLLoaderRequestHandler so the NavigationLoader would not have to deal with a > > list of these potential fallbacks. It would need two new data members: a > > should_fallback_function_ and a start_fallback_loader_function_. > > > > Would that help? > > Removed the functions added to the URLLoaderRequestHandler interface. As per > michaeln's suggestion here and our discussion, passing a callback function in > the LoaderCallback > seems to be better. The LoaderCallback signature has been modified to take in an > additional callback function ResponseFallback which is passed in by AppCache if > it finds fallback for the main resource. This caused some changes to other > places which invoke the LoaderCallback to pass in an empty ResponseFallback() Hm... sorry to say this, but I think I might have preferred the previous API, which looked a little more straightforward and generic. (Additional context: I noticed there's another project, namely Web Packaging project, that might also need response interception, and I'm trying to see if both can use common mechanism.) https://codereview.chromium.org/2982363002/diff/100001/content/browser/loader... File content/browser/loader/navigation_url_loader_network_service.cc (right): https://codereview.chromium.org/2982363002/diff/100001/content/browser/loader... content/browser/loader/navigation_url_loader_network_service.cc:322: .Run(response, std::move(fallback_client), std::move(fallback_request)); This fallback code works in a very specific way, which might be ok within this scope but it wouldn't be usable for other use cases. I wonder if something like following API would work... if (MaybeInterceptResponse(response, url_loader_)) return true; The implementation can internally call url_loader->DisconnectClient() if necessary. DisconnectClient() can possibly return its previous forwarding_client_ pointer (i.e. this) so that the implementation take the client over and call its OnFoo methods to provide response data. The implementation does not really need to use mojo binding for the fallback then. In this way handler implementation needs to know about ThrottlingURLLoader which is a little ugly, but I hope we can possibly fix it by introducing an additional wrapper layer later (something like what Yuzhu suggested in https://chromium-review.googlesource.com/c/567610/10/content/renderer/rendere...) Wdyt? https://codereview.chromium.org/2982363002/diff/100001/content/browser/servic... File content/browser/service_worker/service_worker_url_loader_job.cc (right): https://codereview.chromium.org/2982363002/diff/100001/content/browser/servic... content/browser/service_worker/service_worker_url_loader_job.cc:254: if (main_script_http_info) On 2017/07/27 02:40:25, ananta wrote: > Was hitting this DCHECK and the subsequent crash later on. Please check if this > change is ok. This should be fixed by my other change- https://chromium-review.googlesource.com/c/584551/ If this doesn't break any other tests I prefer having the DCHECK as is (otherwise could you put "TODO(kinuko): fix this" here)?
I was working under the assumption that hooking the response was an overly appcache specific thing and that we were trying to demote the idea of response interception. If we anticipate other legitimate use cases, then I buy promoting it as a URLLoaderRequestHandler::MaybeInterceptResponse kind of first class method.
Description was changed from ========== To achieve this the following changes were done. 1. The LoaderCallback function now takes an additional parameter which is the callback function to be invoked if the handler has fallback content available. Currently AppCache is the only handler which supports fallback content. It passes a valid callback if it has fallback content available for the main resource. 2. The AppCacheRequestHandler class which implements the URLLoaderRequestHandler interface passes the callback function for fallback content via the AppCacheURLLoaderJob. 3. The URLLoaderRequestController class which handles top level navigations now provides functionality to invoke the registered handlers for fallback content. This is only done if the request was sent to the default network loader. A boolean flag default_loader_used_ is used to track this. If this is set and there is fallback content available we invoke the fallback callback function and pass in the bound URLLoaderClient and the request to it which enables the job to bind to the connection. 4. We don't use the throttling URL loader for fallback. This is fine because the main URL would already have been checked by the throtting URL loader when it was sent to the network service. Added a function DisconnectClient() to the throttling URL loader which disconnects the client from the network service in case we are passing fallback content to the client. BUG=715632 ========== to ========== Add support for fallback content for the frame. This includes main and subframes. To achieve this the following changes were done. 1. The URLLoaderRequestHandler which is implemented by the navigation request handlers like AppCache/ServiceWorker, etc now has the following method MaybeCreateLoaderForResponse(). AppCache returns fallback content for the response passed in if applicable. 2. The AppCacheRequestHandler class which implements the URLLoaderRequestHandler interface implements the above method. It uses the MaybeLoadFallbackForResponse() function to potentially return fallback content. The difference between fallback handling for subresources and the frame is a new job is created for the latter. The MaybeCreateJobForFallback() function has been updated with this check. 3. The URLLoaderRequestController class which handles top level navigations now provides functionality to invoke the registered handlers when it receives a response. This is only done if the request was sent to the default network loader. A boolean flag default_loader_used_ is used to track this. If this is set we invoke the registered handlers and pass in the bound URLLoaderClient and the request to the handlers which enables them to bind to the connection. 4. We don't use the throttling URL loader for this case. This is fine because the main URL would already have been checked by the throtting URL loader when it was sent to the network service. Added a function DisconnectClient() to the throttling URL loader which disconnects the client from the network service in case we are passing fallback content to the client. BUG=715632 ==========
On 2017/07/27 22:04:41, michaeln wrote: > I was working under the assumption that hooking the response was an overly > appcache specific thing and that we were trying to demote the idea of response > interception. If we anticipate other legitimate use cases, then I buy promoting > it as a URLLoaderRequestHandler::MaybeInterceptResponse kind of first class > method. I liked the earlier suggestion MaybeCreateLoaderforResponse()?. Would that be ok?
https://codereview.chromium.org/2982363002/diff/100001/content/browser/loader... File content/browser/loader/navigation_url_loader_network_service.cc (right): https://codereview.chromium.org/2982363002/diff/100001/content/browser/loader... content/browser/loader/navigation_url_loader_network_service.cc:322: .Run(response, std::move(fallback_client), std::move(fallback_request)); On 2017/07/27 13:46:26, kinuko wrote: > This fallback code works in a very specific way, which might be ok within this > scope but it wouldn't be usable for other use cases. I wonder if something like > following API would work... > > if (MaybeInterceptResponse(response, url_loader_)) > return true; > > The implementation can internally call url_loader->DisconnectClient() if > necessary. DisconnectClient() can possibly return its previous > forwarding_client_ pointer (i.e. this) so that the implementation take the > client over and call its OnFoo methods to provide response data. The > implementation does not really need to use mojo binding for the fallback then. > > In this way handler implementation needs to know about ThrottlingURLLoader which > is a little ugly, but I hope we can possibly fix it by introducing an additional > wrapper layer later (something like what Yuzhu suggested in > https://chromium-review.googlesource.com/c/567610/10/content/renderer/rendere...) > > Wdyt? It seems like the method name you suggested in an earlier iteration MaybeCreateLoaderForResponse is better. I changed the patch to reflect that. I left the other parts as is at the moment. I am not sure whether passing the ThrottlingURLLoader is better, because at the point where we are invoking the new method, the ThrottlingURLLoader has already done its job, i.e. it has verified the safe browsing database for the URL, etc. Perhaps we go with this approach for now and revisit in a future patchset.
On 2017/07/27 22:07:03, ananta wrote: > On 2017/07/27 22:04:41, michaeln wrote: > > I was working under the assumption that hooking the response was an overly > > appcache specific thing and that we were trying to demote the idea of response > > interception. If we anticipate other legitimate use cases, then I buy > promoting > > it as a URLLoaderRequestHandler::MaybeInterceptResponse kind of first class > > method. > > I liked the earlier suggestion MaybeCreateLoaderforResponse()?. Would that be > ok? Sure, i was just trying to agree with kinuko about expsoing it as a new seperate method on URLLoaderRequestHandler instead of coercing the MaybeCreateLoader method into service for it.
https://codereview.chromium.org/2982363002/diff/80001/content/browser/loader/... File content/browser/loader/url_loader_request_handler.h (right): https://codereview.chromium.org/2982363002/diff/80001/content/browser/loader/... content/browser/loader/url_loader_request_handler.h:50: mojom::URLLoaderRequest* request); On 2017/07/27 13:46:26, kinuko wrote: > On 2017/07/27 02:40:25, ananta wrote: > > On 2017/07/26 23:08:41, michaeln wrote: > > > On 2017/07/26 01:59:14, kinuko wrote: > > > > (If we end up having this kind of hooks) > > > > > > > > Could we try to make it look like a more generic response interception > hook? > > > > > > I'd imagine something like: > > > > > > > > void MaybeCreateLoader(...); > > > > void MaybeCreateLoaderForResponse(...); > > > > > > > > It also feels a bit unfortunate to need to pre-create ClientPtr and > Request > > > > pairs on every responses, could we avoid doing that? > > > > > > I don't know how much it helps, but we could expose the fallback handling > with > > a > > > different API. The appcache system knows prior to returning from > > > MaybeCreateLoader() whether or not it has a fallback. There are three > possible > > > return values from MaybeCreateLoader() > > > - a loader that loads the main resouce > > > - a loader that loads a fallback resource > > > - no loader > > > > > > There's no case for the appcache providing a loader and a fallback_loader. > > > > > > Here's a MaybeCreateLoaderCallback function signature that captures the > > > possiblities. > > > > > > void MaybeCreateLoaderCallback( > > > StartLoaderCallback start_it, > > > ShouldFallbackForResponseFunction should_fallback_function); > > > > > > bool ShouldFallbackForResponseFunction(response); > > > > > > If |should_fallback_function| is not empty, that means |start_it| should > only > > be > > > used for response fallback handling when should_fallback_function(response) > > > returns true. > > > > > > The should_fallback conditions are pretty simple. > > > > > > https://cs.chromium.org/chromium/src/content/browser/appcache/appcache_reques... > > > > > > To simplify things, we could prohibit use of that api except for the > > appcache's > > > URLLoaderRequestHandler so the NavigationLoader would not have to deal with > a > > > list of these potential fallbacks. It would need two new data members: a > > > should_fallback_function_ and a start_fallback_loader_function_. > > > > > > Would that help? > > > > Removed the functions added to the URLLoaderRequestHandler interface. As per > > michaeln's suggestion here and our discussion, passing a callback function in > > the LoaderCallback > > seems to be better. The LoaderCallback signature has been modified to take in > an > > additional callback function ResponseFallback which is passed in by AppCache > if > > it finds fallback for the main resource. This caused some changes to other > > places which invoke the LoaderCallback to pass in an empty ResponseFallback() > > > Hm... sorry to say this, but I think I might have preferred the previous API, > which looked a little more straightforward and generic. (Additional context: I > noticed there's another project, namely Web Packaging project, that might also > need response interception, and I'm trying to see if both can use common > mechanism.) If we don't want the appcache handler to make the network request and we have to put something in URLLoaderRequestHandler, then either this patch or the latter one is fine with me. I defer to others on which one is preferable. My main motivation was to see if we can avoid adding anything to this interface, but looks like that's not possible.
On 2017/07/27 22:32:03, michaeln wrote: > On 2017/07/27 22:07:03, ananta wrote: > > On 2017/07/27 22:04:41, michaeln wrote: > > > I was working under the assumption that hooking the response was an overly > > > appcache specific thing and that we were trying to demote the idea of > response > > > interception. If we anticipate other legitimate use cases, then I buy > > promoting > > > it as a URLLoaderRequestHandler::MaybeInterceptResponse kind of first class > > > method. Yeah I wanted to avoid response interception if it was possible / only for appcache cases, but it's getting unclear. I think it's probably good to try to put things as straightforward as possible now and clean them up later. > > I liked the earlier suggestion MaybeCreateLoaderforResponse()?. Would that be > > ok? > > Sure, i was just trying to agree with kinuko about expsoing it as a new seperate > method on URLLoaderRequestHandler instead of coercing the MaybeCreateLoader > method into service for it. Yes, works for me as well. (Will take another look soon later, need to go afk for now)
On 2017/07/27 23:31:43, kinuko wrote: > On 2017/07/27 22:32:03, michaeln wrote: > > On 2017/07/27 22:07:03, ananta wrote: > > > On 2017/07/27 22:04:41, michaeln wrote: > > > > I was working under the assumption that hooking the response was an overly > > > > appcache specific thing and that we were trying to demote the idea of > > response > > > > interception. If we anticipate other legitimate use cases, then I buy > > > promoting > > > > it as a URLLoaderRequestHandler::MaybeInterceptResponse kind of first > class > > > > method. > > Yeah I wanted to avoid response interception if it was possible / only for > appcache cases, but it's getting unclear. I think it's probably good to try to > put things as straightforward as possible now and clean them up later. > > > > I liked the earlier suggestion MaybeCreateLoaderforResponse()?. Would that > be > > > ok? > > > > Sure, i was just trying to agree with kinuko about expsoing it as a new > seperate > > method on URLLoaderRequestHandler instead of coercing the MaybeCreateLoader > > method into service for it. > > Yes, works for me as well. (Will take another look soon later, need to go afk > for now) Thanks to yzshen for his suggestion about returning the URLLoaderPtr and URLLoaderClientInfo pointers from the MaybeCreateLoaderForResponse() function. This ensures that we only create these interfaces when the handler truly has a loader to be created for the response and avoids the overhead of creating these interfaces for all responses. Additionally I restored the navigation_request_job_ member in the AppCacheRequestHandler class as there are legitimate reasons to hold on to the job and delete it ourselves. For e.g. if we are returning a network response. For error and appcache responses we can safely release the unique_ptr as the client disconnect will delete the job PTAL
The CQ bit was checked by ananta@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...
Thanks ananta, this is looking good! One quick question https://codereview.chromium.org/2982363002/diff/220001/content/browser/loader... File content/browser/loader/navigation_url_loader_network_service.cc (right): https://codereview.chromium.org/2982363002/diff/220001/content/browser/loader... content/browser/loader/navigation_url_loader_network_service.cc:308: return false; One question- do we need to keep this here or can appcache side do the minimum necessary check, and for the cases it can't be sure (e.g. the response might actually come from SW) it's still fine to supply fallback?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2982363002/diff/240001/content/browser/appcac... File content/browser/appcache/appcache_request_handler.cc (right): https://codereview.chromium.org/2982363002/diff/240001/content/browser/appcac... content/browser/appcache/appcache_request_handler.cc:306: // created for handling navigation requests. These instances will get nit: extra space https://codereview.chromium.org/2982363002/diff/240001/content/browser/appcac... content/browser/appcache/appcache_request_handler.cc:317: // created for handling navigation requests. These instances will get ditto https://codereview.chromium.org/2982363002/diff/240001/content/browser/appcac... content/browser/appcache/appcache_request_handler.cc:329: // created for handling navigation requests. ditto https://codereview.chromium.org/2982363002/diff/240001/content/browser/appcac... content/browser/appcache/appcache_request_handler.cc:374: // the existing job to deliver the fallback response. This comment doesn't explain why we have IsMainResourceType above, can you update the comment? https://codereview.chromium.org/2982363002/diff/240001/content/browser/loader... File content/browser/loader/navigation_url_loader_network_service.cc (right): https://codereview.chromium.org/2982363002/diff/240001/content/browser/loader... content/browser/loader/navigation_url_loader_network_service.cc:232: url_loader_->FollowRedirect(); It doesn't seem to happen in the current appcache code but actually FollowRedirect should call response_url_loader_->FollowRedirect() if there's the one, right? Should we at least DCHECK(!response_url_loader_) here? https://codereview.chromium.org/2982363002/diff/240001/content/browser/loader... content/browser/loader/navigation_url_loader_network_service.cc:369: mojom::URLLoaderClientRequest response_client_request_; Does this need to be kept in this field? Can we just stack-allocate this before calling MaybeCreateLoaderForResponse and pass it in OnResponseHandlerFound()?
This went together nicely, lgtm % some questions and a couple nits to consider. Also unittests would be good at some point. https://codereview.chromium.org/2982363002/diff/240001/content/browser/appcac... File content/browser/appcache/appcache_request_handler.cc (right): https://codereview.chromium.org/2982363002/diff/240001/content/browser/appcac... content/browser/appcache/appcache_request_handler.cc:598: // client connection is dropped. the lifetime is explained well enough in other comments, not sure this one is needed https://codereview.chromium.org/2982363002/diff/240001/content/browser/appcac... File content/browser/appcache/appcache_request_handler.h (right): https://codereview.chromium.org/2982363002/diff/240001/content/browser/appcac... content/browser/appcache/appcache_request_handler.h:163: // URLLoaderRequestHandler override nit: please make it more clear that these overrides aren't specific to sub-resource loading // URLLoaderRequestHandler overrides -------------------------- https://codereview.chromium.org/2982363002/diff/240001/content/browser/loader... File content/browser/loader/navigation_url_loader_network_service.cc (right): https://codereview.chromium.org/2982363002/diff/240001/content/browser/loader... content/browser/loader/navigation_url_loader_network_service.cc:294: if (MaybeCreateLoaderForResponse(response_)) I think we should only call MaybeCreateLoaderForResponse() once for each MaybeCreateLoader() call, but it looks like we can/will call it twice in most cases: first from OnReceivedResponse and again from OnComplete. If i understand properly, we're calling MaybeCreateLoaderForResponse() to cover the case where we did not receive a response at all. If that's right, I think we could make it more clear somehow... if (!recieved_response_) MaybeCreateLoaderForResposne(NORESPONSE); We'd have to reset recieved_response_ in Restart() for this to work. Also, I think the only case this applied to is when completion_status is !OK. if (completion_status != OK && !recieved_response_) MaybeCreateLoaderForResposne(NORESPONSE); ... at least i think?
https://codereview.chromium.org/2982363002/diff/220001/content/browser/loader... File content/browser/loader/navigation_url_loader_network_service.cc (right): https://codereview.chromium.org/2982363002/diff/220001/content/browser/loader... content/browser/loader/navigation_url_loader_network_service.cc:308: return false; On 2017/07/28 03:34:51, kinuko wrote: > One question- do we need to keep this here or can appcache side do the minimum > necessary check, and for the cases it can't be sure (e.g. the response might > actually come from SW) it's still fine to supply fallback? I always assumed that fallback was for network requests only. Will check with michael about this. https://codereview.chromium.org/2982363002/diff/240001/content/browser/appcac... File content/browser/appcache/appcache_request_handler.cc (right): https://codereview.chromium.org/2982363002/diff/240001/content/browser/appcac... content/browser/appcache/appcache_request_handler.cc:306: // created for handling navigation requests. These instances will get On 2017/07/28 09:50:28, kinuko wrote: > nit: extra space Done. https://codereview.chromium.org/2982363002/diff/240001/content/browser/appcac... content/browser/appcache/appcache_request_handler.cc:317: // created for handling navigation requests. These instances will get On 2017/07/28 09:50:28, kinuko wrote: > ditto Done. https://codereview.chromium.org/2982363002/diff/240001/content/browser/appcac... content/browser/appcache/appcache_request_handler.cc:329: // created for handling navigation requests. On 2017/07/28 09:50:28, kinuko wrote: > ditto Done. https://codereview.chromium.org/2982363002/diff/240001/content/browser/appcac... content/browser/appcache/appcache_request_handler.cc:598: // client connection is dropped. On 2017/07/28 19:11:02, michaeln wrote: > the lifetime is explained well enough in other comments, not sure this one is > needed Thanks. Removed https://codereview.chromium.org/2982363002/diff/240001/content/browser/appcac... File content/browser/appcache/appcache_request_handler.h (right): https://codereview.chromium.org/2982363002/diff/240001/content/browser/appcac... content/browser/appcache/appcache_request_handler.h:163: // URLLoaderRequestHandler override On 2017/07/28 19:11:02, michaeln wrote: > nit: please make it more clear that these overrides aren't specific to > sub-resource loading > > // URLLoaderRequestHandler overrides -------------------------- Done. https://codereview.chromium.org/2982363002/diff/240001/content/browser/loader... File content/browser/loader/navigation_url_loader_network_service.cc (right): https://codereview.chromium.org/2982363002/diff/240001/content/browser/loader... content/browser/loader/navigation_url_loader_network_service.cc:232: url_loader_->FollowRedirect(); On 2017/07/28 09:50:29, kinuko wrote: > It doesn't seem to happen in the current appcache code but actually > FollowRedirect should call response_url_loader_->FollowRedirect() if there's the > one, right? > > Should we at least DCHECK(!response_url_loader_) here? Thanks. Done https://codereview.chromium.org/2982363002/diff/240001/content/browser/loader... content/browser/loader/navigation_url_loader_network_service.cc:294: if (MaybeCreateLoaderForResponse(response_)) On 2017/07/28 19:11:02, michaeln wrote: > I think we should only call MaybeCreateLoaderForResponse() once for each > MaybeCreateLoader() call, but it looks like we can/will call it twice in most > cases: first from OnReceivedResponse and again from OnComplete. If i understand > properly, we're calling MaybeCreateLoaderForResponse() to cover the case where > we did not receive a response at all. If that's right, I think we could make it > more clear somehow... > > if (!recieved_response_) > MaybeCreateLoaderForResposne(NORESPONSE); > > We'd have to reset recieved_response_ in Restart() for this to work. > > Also, I think the only case this applied to is when completion_status is !OK. > > if (completion_status != OK && !recieved_response_) > MaybeCreateLoaderForResposne(NORESPONSE); > > ... at least i think? Thanks. I added a received_response_ flag which defaults to false and is set to true in OnReceiveResponse(). It resets to false in Restart(). https://codereview.chromium.org/2982363002/diff/240001/content/browser/loader... content/browser/loader/navigation_url_loader_network_service.cc:369: mojom::URLLoaderClientRequest response_client_request_; On 2017/07/28 09:50:29, kinuko wrote: > Does this need to be kept in this field? Can we just stack-allocate this before > calling MaybeCreateLoaderForResponse and pass it in OnResponseHandlerFound()? Done.
The CQ bit was checked by ananta@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by ananta@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/2982363002/diff/220001/content/browser/loader... File content/browser/loader/navigation_url_loader_network_service.cc (right): https://codereview.chromium.org/2982363002/diff/220001/content/browser/loader... content/browser/loader/navigation_url_loader_network_service.cc:308: return false; On 2017/07/28 22:59:21, ananta wrote: > On 2017/07/28 03:34:51, kinuko wrote: > > One question- do we need to keep this here or can appcache side do the minimum > > necessary check, and for the cases it can't be sure (e.g. the response might > > actually come from SW) it's still fine to supply fallback? > > I always assumed that fallback was for network requests only. Will check with > michael about this. We had talked about it couple weeks back. AppCache fallback only applies for network requests. michaeln, please correct me if I am wrong :)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by ananta@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from michaeln@chromium.org, kinuko@chromium.org Link to the patchset: https://codereview.chromium.org/2982363002/#ps270011 (title: "rebase to tip")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 270011, "attempt_start_ts": 1501351131806350,
"parent_rev": "ff86367a15a36e4bfddb326685b901006922242d", "commit_rev":
"b9800e5823ff3358a4f90d1a9e72c26a3d742988"}
Message was sent while issue was closed.
Description was changed from ========== Add support for fallback content for the frame. This includes main and subframes. To achieve this the following changes were done. 1. The URLLoaderRequestHandler which is implemented by the navigation request handlers like AppCache/ServiceWorker, etc now has the following method MaybeCreateLoaderForResponse(). AppCache returns fallback content for the response passed in if applicable. 2. The AppCacheRequestHandler class which implements the URLLoaderRequestHandler interface implements the above method. It uses the MaybeLoadFallbackForResponse() function to potentially return fallback content. The difference between fallback handling for subresources and the frame is a new job is created for the latter. The MaybeCreateJobForFallback() function has been updated with this check. 3. The URLLoaderRequestController class which handles top level navigations now provides functionality to invoke the registered handlers when it receives a response. This is only done if the request was sent to the default network loader. A boolean flag default_loader_used_ is used to track this. If this is set we invoke the registered handlers and pass in the bound URLLoaderClient and the request to the handlers which enables them to bind to the connection. 4. We don't use the throttling URL loader for this case. This is fine because the main URL would already have been checked by the throtting URL loader when it was sent to the network service. Added a function DisconnectClient() to the throttling URL loader which disconnects the client from the network service in case we are passing fallback content to the client. BUG=715632 ========== to ========== Add support for fallback content for the frame. This includes main and subframes. To achieve this the following changes were done. 1. The URLLoaderRequestHandler which is implemented by the navigation request handlers like AppCache/ServiceWorker, etc now has the following method MaybeCreateLoaderForResponse(). AppCache returns fallback content for the response passed in if applicable. 2. The AppCacheRequestHandler class which implements the URLLoaderRequestHandler interface implements the above method. It uses the MaybeLoadFallbackForResponse() function to potentially return fallback content. The difference between fallback handling for subresources and the frame is a new job is created for the latter. The MaybeCreateJobForFallback() function has been updated with this check. 3. The URLLoaderRequestController class which handles top level navigations now provides functionality to invoke the registered handlers when it receives a response. This is only done if the request was sent to the default network loader. A boolean flag default_loader_used_ is used to track this. If this is set we invoke the registered handlers and pass in the bound URLLoaderClient and the request to the handlers which enables them to bind to the connection. 4. We don't use the throttling URL loader for this case. This is fine because the main URL would already have been checked by the throtting URL loader when it was sent to the network service. Added a function DisconnectClient() to the throttling URL loader which disconnects the client from the network service in case we are passing fallback content to the client. BUG=715632 Review-Url: https://codereview.chromium.org/2982363002 Cr-Commit-Position: refs/heads/master@{#490666} Committed: https://chromium.googlesource.com/chromium/src/+/b9800e5823ff3358a4f90d1a9e72... ==========
Message was sent while issue was closed.
Committed patchset #15 (id:270011) as https://chromium.googlesource.com/chromium/src/+/b9800e5823ff3358a4f90d1a9e72...
Message was sent while issue was closed.
On 2017/07/29 18:04:48, commit-bot: I haz the power wrote: > Committed patchset #15 (id:270011) as > https://chromium.googlesource.com/chromium/src/+/b9800e5823ff3358a4f90d1a9e72... FYI this made http/tests/appcache/fallback.html start failing on Mojo Linux. Not sure if that was intended but I'll mark it as failing and file a bug.
Message was sent while issue was closed.
On 2017/07/31 01:29:17, falken wrote: > On 2017/07/29 18:04:48, commit-bot: I haz the power wrote: > > Committed patchset #15 (id:270011) as > > > https://chromium.googlesource.com/chromium/src/+/b9800e5823ff3358a4f90d1a9e72... > > FYI this made http/tests/appcache/fallback.html start failing on Mojo Linux. Not > sure if that was intended but I'll mark it as failing and file a bug. Ah, sorry, it didn't "start failing", it just went from Timeout to Failure. So I won't open a new bug as it was already failing. |
