Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2642733002:
Prerender: Disable prefetch if there's an appcache. (Closed)
|
Created:
3 years, 11 months ago by mattcary Modified:
3 years, 8 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, tburkard+watch_chromium.org, blink-reviews-html_chromium.org, Yoav Weiss, loading-reviews+parser_chromium.org, gavinp+prer_chromium.org, dglazkov+blink, blink-reviews, kinuko+watch Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionPrerender: Disable prefetch if there's an appcache.
Stops the preload scanner if an appcache manifest is detected and the document
is prefetching.
BUG=632368
Review-Url: https://codereview.chromium.org/2642733002
Cr-Commit-Position: refs/heads/master@{#461081}
Committed: https://chromium.googlesource.com/chromium/src/+/d741ab419a18a996643c568434e0a756eaf7bee6
Patch Set 1 #Patch Set 2 : clarifying comment #
Total comments: 3
Patch Set 3 : comments #Patch Set 4 : Use appcache id rather than existence of tags in main resource #
Total comments: 4
Patch Set 5 : comments #
Total comments: 2
Patch Set 6 : comments #
Total comments: 2
Patch Set 7 : Use document loader main resource appcache id to detect presence of appcache #Patch Set 8 : Prerender: Disable prefetch if there's an appcache. #
Messages
Total messages: 48 (6 generated)
mattcary@chromium.org changed reviewers: + csharrison@chromium.org, droger@chromium.org
Disable prefetching if an AppCache is detected. Charlie, you're on here because you re-enable AppCache for preload scanning, and most of this CL is undoing your patch :) See crrev.com/2638383002 for how this would look if we plumbed the reason for stopping the prefetch back up to the browser. Since AppCache is deprecated anyway, it seems unnecessary. But LMK, we can do it that way if it seems better.
The chrome side looks generally good. Waiting for Charlie to comment on the blink side, and maybe give some more insights into how the AppCache works. https://codereview.chromium.org/2642733002/diff/20001/chrome/browser/prerende... File chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc (right): https://codereview.chromium.org/2642733002/diff/20001/chrome/browser/prerende... chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc:623: // continue. The page should be fetched from the appcache. I'm a bit confused, mostly because I am not familiar with AppCache. What I see here is that the AppCache here is used even though the renderer skips the AppCache initialization. So it seems the initialization is not necessary here. However, in the case from previous test, the AppCache would not be used because of the missing initialization. What is not quite clear to me, is in which cases AppCache works without being initialized, and if we can really rely on this kind of behavior. There may be other events that make the initialization necessary (such as maybe restarting Chrome for exemple). Maybe it's ok if the implementation is a bit hacky though, since AppCache is deprecated. So if there is no easy way to get clear answers or no alternative, I guess this could be good enough.
https://codereview.chromium.org/2642733002/diff/20001/chrome/browser/prerende... File chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc (right): https://codereview.chromium.org/2642733002/diff/20001/chrome/browser/prerende... chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc:623: // continue. The page should be fetched from the appcache. On 2017/01/18 13:59:49, droger wrote: > I'm a bit confused, mostly because I am not familiar with AppCache. > > What I see here is that the AppCache here is used even though the renderer skips > the AppCache initialization. So it seems the initialization is not necessary > here. > > However, in the case from previous test, the AppCache would not be used because > of the missing initialization. > > What is not quite clear to me, is in which cases AppCache works without being > initialized, and if we can really rely on this kind of behavior. There may be > other events that make the initialization necessary (such as maybe restarting > Chrome for exemple). > > > Maybe it's ok if the implementation is a bit hacky though, since AppCache is > deprecated. So if there is no easy way to get clear answers or no alternative, I > guess this could be good enough. Discussed somewhat offline. AFAIU the reason for the AppCacheHtml is because the manifest present in the file isn't processed by the AppCache (because the render side part of that isn't initialized yet). In AppCacheRegistered, the AppCache is already initialized by the page that loaded the manifest in the first place. Offline you raised a good point about whether this still works if the manifest is loaded and everything cached, then chrome is quit and restarted, and the prefetch is the first thing that happens. I'm digging into that now.
csharrison@chromium.org changed reviewers: + michaeln@chromium.org
Have you established that this isn't better to do browser side, which was discussed in the linked bug? The conversation got a little muddy, but I still think that might be the best route? In any case, if the renderer is the place, it might make sense to mimic the isCSPMeta logic. Since NoState prefetch just uses an HTMLPreloadScanner, it doesn't need to worry about checkpoints or any of that complex stuff. +michaeln
On 2017/01/18 14:21:47, Charlie Harrison wrote: > Have you established that this isn't better to do browser side, which was > discussed in the linked bug? The conversation got a little muddy, but I still > think that might be the best route? > > In any case, if the renderer is the place, it might make sense to mimic the > isCSPMeta logic. Since NoState prefetch just uses an HTMLPreloadScanner, it > doesn't need to worry about checkpoints or any of that complex stuff. > > +michaeln I don't understand how this could be done on the browser side, because the issue is parsing a main resource with a manifest attribute. I'm probably missing something, though. Thanks for the isCPSMeta logic pointer. It's true I don't understand the checkpoint stuff and just put back in what you took out :)
On 2017/01/18 14:26:59, mattcary wrote: > On 2017/01/18 14:21:47, Charlie Harrison wrote: > > Have you established that this isn't better to do browser side, which was > > discussed in the linked bug? The conversation got a little muddy, but I still > > think that might be the best route? > > > > In any case, if the renderer is the place, it might make sense to mimic the > > isCSPMeta logic. Since NoState prefetch just uses an HTMLPreloadScanner, it > > doesn't need to worry about checkpoints or any of that complex stuff. > > > > +michaeln > > I don't understand how this could be done on the browser side, because the issue > is parsing a main resource with a manifest attribute. I'm probably missing > something, though. > > Thanks for the isCPSMeta logic pointer. It's true I don't understand the > checkpoint stuff and just put back in what you took out :) I think the manifest is a signal that AppCache will be used for some resources, but it is not a necessary condition. i.e. we could load a main resource without a manifest declared in markup that still uses AppCache.
> I think the manifest is a signal that AppCache will be used for some resources, > but it is not a necessary condition. i.e. we could load a main resource without > a manifest declared in markup that still uses AppCache. Yes, that's true. That's what the second test checks: it appears to work correctly in that case.
On 2017/01/18 15:28:58, mattcary wrote: > > I think the manifest is a signal that AppCache will be used for some > resources, > > but it is not a necessary condition. i.e. we could load a main resource > without > > a manifest declared in markup that still uses AppCache. > > Yes, that's true. That's what the second test checks: it appears to work > correctly in that case. What do you mean by the "second test"? To me it looks like we are just aborting if we see the markup.
On 2017/01/18 15:38:37, Charlie Harrison wrote: > On 2017/01/18 15:28:58, mattcary wrote: > > > I think the manifest is a signal that AppCache will be used for some > > resources, > > > but it is not a necessary condition. i.e. we could load a main resource > > without > > > a manifest declared in markup that still uses AppCache. > > > > Yes, that's true. That's what the second test checks: it appears to work > > correctly in that case. > > What do you mean by the "second test"? To me it looks like we are just aborting > if we see the markup. AppCacheRegistered in prerender_nostate_browser_test.cc.
> In any case, if the renderer is the place, it might make sense to mimic the > isCSPMeta logic. Since NoState prefetch just uses an HTMLPreloadScanner, it > doesn't need to worry about checkpoints or any of that complex stuff. > Actually, it looks like using the same logic as the isCSPMeta is more complicated. The reason is that we only want to bail on the appcache during NoState prefetch and not all preload scanning. I put that prefetchOnly bit in the CachedDocumentParameters, which are pushed all the way into the TokenPreloadScanner and not easily visible to the HTMLPreloadScanner. Hence it makes sense for the TokenPreloadScanner to decide when to bail out, rather than having it just pass up a hasAppCacheManifest flag up to the HTMLPreloadScanner, which would then have to look back into the CachedDocuemntParameters in the TokenPreloadScanner for the prefetchOnly bit. Or, the prefetchOnly bit would have to be given to the HTMLPreloadScanner separately from the CachedDocumentParameters, which seems like an unnecessary widening of the API. LMK any thoughts you have.
I think I would just prefer the HTMLPreloadScanner pulling that bit out of the document params before they are sent to the TokenPreloadScanner in the constructor. That doesn't seem like an abuse of the cached document parameters to me.
On 2017/01/19 13:30:44, Charlie Harrison wrote: > I think I would just prefer the HTMLPreloadScanner pulling that bit out of the > document params before they are sent to the TokenPreloadScanner in the > constructor. That doesn't seem like an abuse of the cached document parameters > to me. OK, done.
parser LGTM
chrome/browser/prerender lgtm
On 2017/01/19 14:38:00, droger wrote: > chrome/browser/prerender lgtm Thanks. I'll give michaeln@ a chance today to respond.
On 2017/01/19 17:21:59, mattcary wrote: > On 2017/01/19 14:38:00, droger wrote: > > chrome/browser/prerender lgtm > > Thanks. I'll give michaeln@ a chance today to respond. michaeln@, any opinion on the interaction with AppCache?
> michaeln@, any opinion on the interaction with AppCache? I'm not sure what this is trying to accomplish? An "appcache manifest" being there or not is not a conclusive test about the appcached'ness of the page? Why do you want to bail out of the preload scanner if ((isAppCacheManifestTag && m_isPrefetchOnly)? Is the idea that if the page and its resources are stored locally in an appcache, there's no point prefetching? In the comment, what does "AppCache support has not yet been enabled" mean, and what is the "incorrect behavior" this change is trying to correct?
On 2017/01/25 02:07:39, michaeln wrote: > > michaeln@, any opinion on the interaction with AppCache? > > I'm not sure what this is trying to accomplish? An "appcache manifest" being > there or not is not a conclusive test about the appcached'ness of the page? > > Why do you want to bail out of the preload scanner if ((isAppCacheManifestTag && > m_isPrefetchOnly)? Is the idea that if the page and its resources are stored > locally in an appcache, there's no point prefetching? > > In the comment, what does "AppCache support has not yet been enabled" mean, and > what is the "incorrect behavior" this change is trying to correct? Because NoState prefetch runs the preload scanner synchronously, none of the appcache is initialized, and if a manifest is found in the html tag, it will be ignored. So there seem to be two choices: initialize the appcache if a manifest is found, or stop prefetching in this case. I chose the latter as it seemed most expedient. Current behavior will ignore the manifest and fetch from the network always, which seems incorrect as mentioned in the comment. By "not yet enabled" I meant that appcache is not inititialized due to the synchronous running of the preload scanner. At least this is all what I think I understand; Charlie wanted to add you to the CL to get some degree of authority and knowledge ;)
If i understand the purpose of the preload scanner properly, I don't think this change will do what you want, but I'll ask some stupid questions first to test if my understanding is right. I think the PreloadScanner is trying to preload subresources for the document being loaded, where the Document is committed but to its LocalFrame, but the DOM is not built. I think resource loads generated by this process are fetched thru the document's DocumentLoader. Is that right? And the problem your trying to solve is ResourceDispatcherHost receiving messages to ResourceHostMsg_RequestResource prior to AppCacheHostMsg_SelectCache being called. Is that right?
On 2017/01/26 00:20:26, michaeln wrote:
> If i understand the purpose of the preload scanner properly, I don't think
this
> change will do what you want, but I'll ask some stupid questions first to test
> if my understanding is right.
>
> I think the PreloadScanner is trying to preload subresources for the document
> being loaded, where the Document is committed but to its LocalFrame, but the
DOM
> is not built. I think resource loads generated by this process are fetched
thru
> the document's DocumentLoader. Is that right?
>
> And the problem your trying to solve is ResourceDispatcherHost receiving
> messages to ResourceHostMsg_RequestResource prior to
AppCacheHostMsg_SelectCache
> being called. Is that right?
If that is right, it might make sense to have the renderer to go ahead and
create the prefetch requests in advance of selectCache time, and to have the
browser process wait for selectCache to occur prior to starting the actual
URLRequest. The AppcacheRequestHandler defers actual URLRequest processing for a
variety of reasons, one of which is very similar to what I'm describing here.
The addition I have in mind would be to have the AppCacheRequestHandler defer
processing until was_select_cache_called_ is set to true in addition to what
it's current waiting on.
That might be a one line change.
if (!host->was_select_cache_called_ || host_->is_selection_pending()) {
// We have to wait until cache selection is complete and the
// selected cache is loaded.
is_waiting_for_cache_selection_ = true;
return CreateJob(request, network_delegate);
}
On 2017/01/26 00:42:31, michaeln wrote:
> On 2017/01/26 00:20:26, michaeln wrote:
> > If i understand the purpose of the preload scanner properly, I don't think
> this
> > change will do what you want, but I'll ask some stupid questions first to
test
> > if my understanding is right.
> >
> > I think the PreloadScanner is trying to preload subresources for the
document
> > being loaded, where the Document is committed but to its LocalFrame, but the
> DOM
> > is not built. I think resource loads generated by this process are fetched
> thru
> > the document's DocumentLoader. Is that right?
> >
> > And the problem your trying to solve is ResourceDispatcherHost receiving
> > messages to ResourceHostMsg_RequestResource prior to
> AppCacheHostMsg_SelectCache
> > being called. Is that right?
>
> If that is right, it might make sense to have the renderer to go ahead and
> create the prefetch requests in advance of selectCache time, and to have the
> browser process wait for selectCache to occur prior to starting the actual
> URLRequest. The AppcacheRequestHandler defers actual URLRequest processing for
a
> variety of reasons, one of which is very similar to what I'm describing here.
> The addition I have in mind would be to have the AppCacheRequestHandler defer
> processing until was_select_cache_called_ is set to true in addition to what
> it's current waiting on.
>
> That might be a one line change.
>
> if (!host->was_select_cache_called_ || host_->is_selection_pending()) {
> // We have to wait until cache selection is complete and the
> // selected cache is loaded.
> is_waiting_for_cache_selection_ = true;
> return CreateJob(request, network_delegate);
> }
I think your comment about requesting resources prior to
AppCacheHostMsg_SelectCache being called is exactly right, thanks for making my
vague understanding more specific.
The preload scanner is being run for prefetch: the document will not actually be
loaded with this navigation. Some subsequent navigation may load the page. As
soon as the preload scanner is done, the renderer process is killed. So I don't
know if was_select_cache_called_ will ever be called.
For example, HtmlHtmlElement::maybeSetUpApplicationCache only seems to be called
during parsing (?), which will never happen in our scenario.
Thanks for helping to clarify everything.
> I think your comment about requesting resources prior to > AppCacheHostMsg_SelectCache being called is exactly right, thanks for making my > vague understanding more specific. > > The preload scanner is being run for prefetch: the document will not actually be > loaded with this navigation. Some subsequent navigation may load the page. As > soon as the preload scanner is done, the renderer process is killed. So I don't > know if was_select_cache_called_ will ever be called. Ok, so in this case, it's not running ahead of a user visible navigation. Is this class also used in the way that I tried to describe earlier, to initiate subresource loads in advance of inserting the script tag with a src attribute into a live page. In the "live page" case, i think what I described would be a pretty good behavior (ala, if !host->was_select_cache_called_ wait). In this faceless disposable renderer prefetch case (where seeding blinks memcache is not wanted or needed), it might make sense to simply cancel requests that would hit the appcache, but to let request that will hit the network thru. We'd have to trigger the cacheSelection logic for this to work. Are these requests annotated in any way that would allow the browser process to determine "its for a live-page-preload" or "its for a faceless-prefect-preload"? > For example, HtmlHtmlElement::maybeSetUpApplicationCache only seems to be called > during parsing (?), which will never happen in our scenario. How can I invoke this behavior locally to see it in action? > Thanks for helping to clarify everything. Likewise
> Ok, so in this case, it's not running ahead of a user visible navigation. Is > this class also used in the way that I tried to describe earlier, to initiate > subresource loads in advance of inserting the script tag with a src attribute > into a live page. This is used as a replacement for prerender. We call it NoState prefetch. So, for example, it will be invoked via <link rel=prerender>, or mayLaunchURL from ChromeCustomTabs, or the omnibox. > > In this faceless disposable renderer prefetch case (where seeding blinks > memcache is not wanted or needed), it might make sense to simply cancel requests > Right, the memcache is not needed; the renderer is discarded as soon as preload scanning is done anyway, usually before any of these network requests have finished. > Are these requests annotated in any way that would allow the browser process to > determine "its for a live-page-preload" or "its for a faceless-prefect-preload"? > See PrerenderResourceThrottle [1] which can be used to intercept requests coming from a nostate prefetch renderer. This is done by keeping track of the WebContents in a so-called PrerenderManager (you can see here the extent to which NoState prefetch really is a prerender replacement). [1] https://cs.chromium.org/chromium/src/chrome/browser/prerender/prerender_resou... > > How can I invoke this behavior locally to see it in action? > The prerender_nostate_prefetch_browsertest.cc in this cl shows how to call it in a browsertest context. To play with it in a live context, use chrome://flags and look for "No-State" (M57 or later I think). This will be sticky. (for doing it via the command line you need to define a finch experiment, see https://cs.chromium.org/chromium/src/tools/android/customtabs_benchmark/scrip...). Is that what you meant by local-see-it-in-action? > > Thanks for helping to clarify everything. > > Likewise
On 2017/01/27 10:12:52, mattcary wrote: > > Ok, so in this case, it's not running ahead of a user visible navigation. Is > > this class also used in the way that I tried to describe earlier, to initiate > > subresource loads in advance of inserting the script tag with a src attribute > > into a live page. > > This is used as a replacement for prerender. We call it NoState prefetch. So, > for example, it will be invoked via <link rel=prerender>, or mayLaunchURL from > ChromeCustomTabs, or the omnibox. > > > > > In this faceless disposable renderer prefetch case (where seeding blinks > > memcache is not wanted or needed), it might make sense to simply cancel > requests > > > Right, the memcache is not needed; the renderer is discarded as soon as preload > scanning is done anyway, usually before any of these network requests have > finished. > > > > Are these requests annotated in any way that would allow the browser process > to > > determine "its for a live-page-preload" or "its for a > faceless-prefect-preload"? > > > See PrerenderResourceThrottle [1] which can be used to intercept requests coming > from a nostate prefetch renderer. This is done by keeping track of the > WebContents in a so-called PrerenderManager (you can see here the extent to > which NoState prefetch really is a prerender replacement). > > [1] > https://cs.chromium.org/chromium/src/chrome/browser/prerender/prerender_resou... > > > > > How can I invoke this behavior locally to see it in action? > > > The prerender_nostate_prefetch_browsertest.cc in this cl shows how to call it in > a browsertest context. > > To play with it in a live context, use chrome://flags and look for "No-State" > (M57 or later I think). This will be sticky. > > (for doing it via the command line you need to define a finch experiment, see > https://cs.chromium.org/chromium/src/tools/android/customtabs_benchmark/scrip...). > > Is that what you meant by local-see-it-in-action? Yes, thank you. I think it would be perfectly fine to bail out of NoState prefetch for appcached pages. The easiest way to do that is probably to test whether the page was loaded from an appcache prior to proceeding with preload scanning. The code in patchset #3 is not a valid way to test for that. See blink::ResourceResponse.appCacheID(), a non-zero value there means the resource was loaded from an appache. Also see content::ResourceResponseInfo.appacache_id which is the basis of the value reflected in blink. (kAppCacheNoHostId == 0) // The appcache this response was loaded from, or kAppCacheNoCacheId. int64_t appcache_id; To accomplish what your current CL is trying to accomplish, i think you'd be better off testing if (mainResourceResponse.appCacheID() != 0) bail out. If a page contains a manifest attribute but was not loaded from an appcache, that indicates the appcache for that page does not yet exist. It would probably make sense to continue with preloading because that page and its resources will be retrieved over the network.
Thanks, done (sorry for the delay, I was out at a summit in MTV for most of last week). Note I had to expose a previously private method in AppCacheInterceptor, please let me know if there's a better way to do it. I also used AppCacheService to expose the id to prerender (which is in chrome/). Let me know if there's a better way.
I can't really comment on the best way to detect AppCache, but other than that, LGTM with nits below. https://codereview.chromium.org/2642733002/diff/60001/chrome/browser/prerende... File chrome/browser/prerender/prerender_final_status.h (right): https://codereview.chromium.org/2642733002/diff/60001/chrome/browser/prerende... chrome/browser/prerender/prerender_final_status.h:73: FINAL_STATUS_CANCELLED_FOR_APPCACHE = 58, Add this to histograms.xml. https://codereview.chromium.org/2642733002/diff/60001/content/public/browser/... File content/public/browser/appcache_service.h (right): https://codereview.chromium.org/2642733002/diff/60001/content/public/browser/... content/public/browser/appcache_service.h:54: static bool URLRequestHasActiveAppCache(const net::URLRequest* request); Nit: the style guide recommends const reference. "In fact it is a very strong convention in Google code that input arguments are values or const references while output arguments are pointers." https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#...
https://codereview.chromium.org/2642733002/diff/60001/chrome/browser/prerende... File chrome/browser/prerender/prerender_final_status.h (right): https://codereview.chromium.org/2642733002/diff/60001/chrome/browser/prerende... chrome/browser/prerender/prerender_final_status.h:73: FINAL_STATUS_CANCELLED_FOR_APPCACHE = 58, On 2017/02/07 10:40:38, droger wrote: > Add this to histograms.xml. Done, thanks. https://codereview.chromium.org/2642733002/diff/60001/content/public/browser/... File content/public/browser/appcache_service.h (right): https://codereview.chromium.org/2642733002/diff/60001/content/public/browser/... content/public/browser/appcache_service.h:54: static bool URLRequestHasActiveAppCache(const net::URLRequest* request); On 2017/02/07 10:40:38, droger wrote: > Nit: the style guide recommends const reference. > > "In fact it is a very strong convention in Google code that input arguments are > values or const references while output arguments are pointers." > > https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#... Changed to nonconst * so that I don't have to change the AppCacheInterceptor types. Sorry for the churn, an earlier iteration ran into so const thrashing.
https://codereview.chromium.org/2642733002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2642733002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:102509: + <int value="58" label="FINAL_STATUS_CANCELLED_FOR_APPCACHE"/> remove "FINAL_STATUS_" for consistency
https://codereview.chromium.org/2642733002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2642733002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:102509: + <int value="58" label="FINAL_STATUS_CANCELLED_FOR_APPCACHE"/> On 2017/02/07 12:05:11, droger wrote: > remove "FINAL_STATUS_" for consistency Done.
I like that the new code isn't relying on manifest tags found by the parser. Thanks for the refactor!
+michaeln ping?
On 2017/02/16 09:36:50, mattcary wrote: > +michaeln > > ping? Reping? I know that AppCache changes aren't much of a priority, but it would be nice to make our implementation as correct as possible. Thanks Matt
On 2017/02/23 10:17:27, mattcary wrote: > On 2017/02/16 09:36:50, mattcary wrote: > > +michaeln > > > > ping? > > Reping? I know that AppCache changes aren't much of a priority, but it would be > nice to make our implementation as correct as possible. > > Thanks > > Matt Semi-weekly ping!
sorry, i was unexpectedly ooo for sometime in feb/mar, i'll take a look...
I'm wondering if it would make sense to modify the approach taken in snapshot 2? https://codereview.chromium.org/2642733002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/parser/HTMLPreloadScanner.cpp (right): https://codereview.chromium.org/2642733002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/parser/HTMLPreloadScanner.cpp:725: if (m_documentParameters->isPrefetchOnly && m_isAppCacheEnabled) Could this be done more directly and with less code in the renderer? Prior to HTMLPreloadScannerConstruction in the renderer, does the Document have it's mainResourceResponse? If so, would it make sense to replace the test in the firstg snapshot for appCached'ness with a different test based on the value of ResourceResponse.appCacheID(). The CachedDocumentParameters struct could be modified tell is if the doc was loaded from an appcache. CachedDocumentParameters::CachedDocumentParameters(Document* document) { DCHECK(isMainThread()); DCHECK(document); ... isLoadedFromAppCache = document->loader()->response().appCacheID() != 0; ... } Here we could test... if (m_documentParameters->isPrefetchOnly && m_documentParameters->isLoadedFromAppCache) { return; } https://codereview.chromium.org/2642733002/diff/100001/chrome/browser/prerend... File chrome/browser/prerender/prerender_resource_throttle.cc (right): https://codereview.chromium.org/2642733002/diff/100001/chrome/browser/prerend... chrome/browser/prerender/prerender_resource_throttle.cc:264: void PrerenderResourceThrottle::WillProcessResponseOnUI( Since this call on the UI thread is advisory only, the io thread and renderer will be racing ahead with (pre)loading while the task to invoke this method is in-flight. I think many preload requests could get processed prior to the Cancel() call making its way back to the IO thread (depending on how busy/scheduled various threads are relative to one another). https://codereview.chromium.org/2642733002/diff/100001/content/public/browser... File content/public/browser/appcache_service.h (right): https://codereview.chromium.org/2642733002/diff/100001/content/public/browser... content/public/browser/appcache_service.h:54: static bool URLRequestHasActiveAppCache(net::URLRequest* request); If we can avoid adding this to the content public api, we probably should.
On 2017/03/27 23:27:45, michaeln wrote: > I'm wondering if it would make sense to modify the approach taken in snapshot 2? > > https://codereview.chromium.org/2642733002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/html/parser/HTMLPreloadScanner.cpp (right): > > https://codereview.chromium.org/2642733002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/html/parser/HTMLPreloadScanner.cpp:725: if > (m_documentParameters->isPrefetchOnly && m_isAppCacheEnabled) > Could this be done more directly and with less code in the renderer? > > Prior to HTMLPreloadScannerConstruction in the renderer, does the Document have > it's mainResourceResponse? If so, would it make sense to replace the test in the > firstg snapshot for appCached'ness with a different test based on the value of > ResourceResponse.appCacheID(). The CachedDocumentParameters struct could be > modified tell is if the doc was loaded from an appcache. > > > CachedDocumentParameters::CachedDocumentParameters(Document* document) { > DCHECK(isMainThread()); > DCHECK(document); > ... > isLoadedFromAppCache = document->loader()->response().appCacheID() != 0; > ... > } > > Here we could test... > > if (m_documentParameters->isPrefetchOnly && > m_documentParameters->isLoadedFromAppCache) { > return; > } > > https://codereview.chromium.org/2642733002/diff/100001/chrome/browser/prerend... > File chrome/browser/prerender/prerender_resource_throttle.cc (right): > > https://codereview.chromium.org/2642733002/diff/100001/chrome/browser/prerend... > chrome/browser/prerender/prerender_resource_throttle.cc:264: void > PrerenderResourceThrottle::WillProcessResponseOnUI( > Since this call on the UI thread is advisory only, the io thread and renderer > will be racing ahead with (pre)loading while the task to invoke this method is > in-flight. I think many preload requests could get processed prior to the > Cancel() call making its way back to the IO thread (depending on how > busy/scheduled various threads are relative to one another). > > https://codereview.chromium.org/2642733002/diff/100001/content/public/browser... > File content/public/browser/appcache_service.h (right): > > https://codereview.chromium.org/2642733002/diff/100001/content/public/browser... > content/public/browser/appcache_service.h:54: static bool > URLRequestHasActiveAppCache(net::URLRequest* request); > If we can avoid adding this to the content public api, we probably should. All done, this seems to work. Is this what you had in mind? If so, it's substantially simpler than the old approach. Yay!
lgtm! > All done, this seems to work. Is this what you had in mind? If so, it's > substantially simpler than the old approach. Yay! hooray for simplicity :)
The CQ bit was checked by mattcary@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from csharrison@chromium.org, droger@chromium.org Link to the patchset: https://codereview.chromium.org/2642733002/#ps120001 (title: "Use document loader main resource appcache id to detect presence of appcache")
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": 120001, "attempt_start_ts": 1490944787712110,
"parent_rev": "f7e2cfea3cf9c671edd9c2dfc1feabc2d44671de", "commit_rev":
"d741ab419a18a996643c568434e0a756eaf7bee6"}
Message was sent while issue was closed.
Description was changed from ========== Prerender: Disable prefetch if there's an appcache. Stops the preload scanner if an appcache manifest is detected and the document is prefetching. BUG=632368 ========== to ========== Prerender: Disable prefetch if there's an appcache. Stops the preload scanner if an appcache manifest is detected and the document is prefetching. BUG=632368 Review-Url: https://codereview.chromium.org/2642733002 Cr-Commit-Position: refs/heads/master@{#461081} Committed: https://chromium.googlesource.com/chromium/src/+/d741ab419a18a996643c568434e0... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/d741ab419a18a996643c568434e0...
Message was sent while issue was closed.
On 2017/03/31 08:53:40, commit-bot: I haz the power wrote: > Committed patchset #7 (id:120001) as > https://chromium.googlesource.com/chromium/src/+/d741ab419a18a996643c568434e0... Parser still lgtm
Message was sent while issue was closed.
A revert of this CL (patchset #7 id:120001) has been created in https://codereview.chromium.org/2790323003/ by timloh@chromium.org. The reason for reverting is: Appears to make NoStatePrefetchBrowserTest.AppCacheHtmlInitialized and NoStatePrefetchBrowserTest.AppCacheRegistered flaky..
Message was sent while issue was closed.
Patch set 8 should be the unrevert cl/2819523002 and not in this one. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
