[NoStatePrefetch] AppCache support |
|||||||||
Issue descriptionCurrently HTMLDocumentParser skips background parser chunks until AppCache is initialized for the document. We suspect that this is to avoid issuing network requests in cases when they can be served from appcache. The simplest way to support it would probably be to detect appcache manifest and give up prefetching the page. https://codesearch.chromium.org/chromium/src/third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp?q=notifyPendingTokenizedChunks&sq=package:chromium
,
Jul 28 2016
Ha, you mean something like this? https://codereview.chromium.org/2187193002/ +michaeln for appcache wisdom, but there are cases where we will use AppCache even if the manifest is found (and vice versa). I don't know the best approach here though.
,
Jul 28 2016
,
Jul 28 2016
,
Jul 30 2016
I think the logic in the renderer should be the same regardless of appcache vs not, just go thru all the motions unconditionally. > When it discovers a subresource, it asks the browser to prefetch it eventually with LOAD_PREFETCH flag in net/. Logic on the browser side can choose to drop the request on the floor if it would just be read from disk. And also choose to continue prefetching resources that will be retrieved over the net, even if an appcache is in use for other resources.
,
Jul 30 2016
How to deal with serviceworker'd pages is trickier given that onfetch can alter state.
,
Aug 1 2016
hoisting a discussion from a closed issue to here...
--- michaeln said
> It would not make sense to determine if a resource is fresh enough
> in the http cache and to avoid preloading it from within the renderer.
> Pretty much same thing for appcache.
--- charrison said
> Hm, so are you saying that we should just send the requests even before
> ApplicationCacheHost::selectCache{With,Without}Manifest is called?
> Sorry I'm very unfamiliar with the appcache, will it be intercepted in
> the browser process regardless?
I think that would be fine for the renderer to just send the requests w/o concern for the <html> tag or cache selection. Logic in the browser process can deal with how LOAD_PREFETCH requests are dealt with, including waiting for cache selection if thats desired.
It might make sense to wait from the <html> tag to be parsed from the point of view of the appcache system, but that is such a tiny miniority of cases. If there's real performance to be gained by not waiting for that... then don't. It'd probably be fine to not preload appcached pages at all, at least initially.
if (IsMainResourceLoad() && LOAD_PREFETCH)
cancel the prefetch request and don't load the page at all or even bother scanning it
We don't need to wait for the <html> tag in order to accomplish that.
If eventually we want to preload non-appcached resources that are loaded by appcached pages, that could be done most easily on the browser-side. It that world it might make sense to wait for the cache selection, but waiting for cache selection, that can more easily be done on the browser side. The renderer can't anticipate what cache is being used or what resources happen to be in that cache.
,
Aug 1 2016
So this issue seems to be grouping prefetches and preloads. Let's clarify that: Prefetches use LOAD_PREFETCH are used for link rel prefetch (and no state prefetch I believe). These requests are designed to just get into the HTTP cache for a subsequent navigation. Preloads, on the other hand, are designed to mimic Blink resources, and land in the MemoryCache. They do not send LOAD_PREFETCH to the browser. I don't think this is what NoState prefetch is targeting, whereas this *is* what I was targeting in the previous issue, which makes it a bit harder to do this filtering browser side. Let's keep this issue scoped to NoState Prefetch, which I agree should not worry too much about AppCache in the render process. michaeln@, can we move the preload discussion back to the other issue (can reopen if necessary)?
,
Aug 1 2016
ur, ooops, sry, i thought that bug and this bug were referring to the same thing?
,
Aug 1 2016
They are remarkably similar, in that they share the logic in the HTMLPreloadScanner. However, NoState Prefetch is designed to load a page the user will navigate to later, by stuffing the HTTP Cache (rather than keeping resources in memory for the current load). My current understanding is that NoState prefetch will load a main resource, run the HTMLPreloadScanner on it, and collect the scanned preloads and turn them into prefetches (attach LOAD_PREFETCH). Then just fire and forget them. So I think your comment in #7 applies to this issue, just not the other issue.
,
Aug 5 2016
,
Aug 5 2016
Maybe Blink>Loader instead of Blink>HTML>Parser? (it's all magic to me)
,
Jan 10 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/0aed258863772ab7ba983993a2e1ce934d1caf66 commit 0aed258863772ab7ba983993a2e1ce934d1caf66 Author: mattcary <mattcary@chromium.org> Date: Tue Jan 10 16:24:05 2017 Prerender: Confirm ServiceWorkers are invoked for NoState Prefetch. Confirm that a SW is invoked for files loaded from NoState prefetch, even if a registered SW does not have a process. The caveats discussed in crbug/465200 still hold regarding the origin for which the SW is called. BUG= 632368 Review-Url: https://codereview.chromium.org/2575523002 Cr-Commit-Position: refs/heads/master@{#442607} [modify] https://crrev.com/0aed258863772ab7ba983993a2e1ce934d1caf66/chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc [add] https://crrev.com/0aed258863772ab7ba983993a2e1ce934d1caf66/chrome/test/data/prerender/service_worker.html [add] https://crrev.com/0aed258863772ab7ba983993a2e1ce934d1caf66/chrome/test/data/prerender/service_worker.js [add] https://crrev.com/0aed258863772ab7ba983993a2e1ce934d1caf66/chrome/test/data/prerender/service_worker.js.mock-http-headers
,
Jan 10 2017
,
Mar 22 2017
Occasionally prefetching a resource that is in appcache seems unlikely to break any site. The usage of appcache is low to bother optimizing it. The logic to properly delay prefetches behind appcache may be complicated and slow. And this discussion looks stalled. Marking as wontfix as the above suggests to me that it is not a launch blocker.
,
Mar 31 2017
,
Mar 31 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d741ab419a18a996643c568434e0a756eaf7bee6 commit d741ab419a18a996643c568434e0a756eaf7bee6 Author: mattcary <mattcary@chromium.org> Date: Fri Mar 31 08:52:40 2017 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} [modify] https://crrev.com/d741ab419a18a996643c568434e0a756eaf7bee6/chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc [modify] https://crrev.com/d741ab419a18a996643c568434e0a756eaf7bee6/chrome/browser/prerender/prerender_test_utils.cc [modify] https://crrev.com/d741ab419a18a996643c568434e0a756eaf7bee6/chrome/browser/prerender/prerender_test_utils.h [add] https://crrev.com/d741ab419a18a996643c568434e0a756eaf7bee6/chrome/test/data/prerender/appcache.manifest [add] https://crrev.com/d741ab419a18a996643c568434e0a756eaf7bee6/chrome/test/data/prerender/appcache.manifest.mock-http-headers [add] https://crrev.com/d741ab419a18a996643c568434e0a756eaf7bee6/chrome/test/data/prerender/prefetch_appcache.html [modify] https://crrev.com/d741ab419a18a996643c568434e0a756eaf7bee6/third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp
,
Mar 31 2017
The change is in, we should now be reasonable for the appcache case.
,
Apr 4 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c17879886146bbb41cb4435f275ebdbc0f70903a commit c17879886146bbb41cb4435f275ebdbc0f70903a Author: timloh <timloh@chromium.org> Date: Tue Apr 04 08:40:32 2017 Revert of Prerender: Disable prefetch if there's an appcache. (patchset #7 id:120001 of https://codereview.chromium.org/2642733002/ ) Reason for revert: Appears to make NoStatePrefetchBrowserTest.AppCacheHtmlInitialized and NoStatePrefetchBrowserTest.AppCacheRegistered flaky. Original issue's description: > 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/+/d741ab419a18a996643c568434e0a756eaf7bee6 TBR=droger@chromium.org,csharrison@chromium.org,michaeln@chromium.org,mattcary@chromium.org # Not skipping CQ checks because original CL landed more than 1 days ago. BUG= 632368 , 707522 Review-Url: https://codereview.chromium.org/2790323003 Cr-Commit-Position: refs/heads/master@{#461662} [modify] https://crrev.com/c17879886146bbb41cb4435f275ebdbc0f70903a/chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc [modify] https://crrev.com/c17879886146bbb41cb4435f275ebdbc0f70903a/chrome/browser/prerender/prerender_test_utils.cc [modify] https://crrev.com/c17879886146bbb41cb4435f275ebdbc0f70903a/chrome/browser/prerender/prerender_test_utils.h [delete] https://crrev.com/b0c201c3dd37577f185f97bd397c4a8012ce17bc/chrome/test/data/prerender/appcache.manifest [delete] https://crrev.com/b0c201c3dd37577f185f97bd397c4a8012ce17bc/chrome/test/data/prerender/appcache.manifest.mock-http-headers [delete] https://crrev.com/b0c201c3dd37577f185f97bd397c4a8012ce17bc/chrome/test/data/prerender/prefetch_appcache.html [modify] https://crrev.com/c17879886146bbb41cb4435f275ebdbc0f70903a/third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp
,
Apr 14 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f0abffcea31f665a2b1cc1979a955cf308c4dcb4 commit f0abffcea31f665a2b1cc1979a955cf308c4dcb4 Author: mattcary <mattcary@chromium.org> Date: Fri Apr 14 10:44:37 2017 Prerender: Disable prefetch if there's an appcache (with nonflakey tests) Stops the preload scanner if an appcache manifest is detected and the document is prefetching. This un-reverts cl/2790323003. That test introduced by that CL was flakey due to a race between AppCache creation and prefetching. This CL fixes the race by blocking until the appropriate AppCache can be verified as existing. BUG= 632368 Review-Url: https://codereview.chromium.org/2819523002 Cr-Commit-Position: refs/heads/master@{#464713} [modify] https://crrev.com/f0abffcea31f665a2b1cc1979a955cf308c4dcb4/chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc [modify] https://crrev.com/f0abffcea31f665a2b1cc1979a955cf308c4dcb4/chrome/browser/prerender/prerender_test_utils.cc [modify] https://crrev.com/f0abffcea31f665a2b1cc1979a955cf308c4dcb4/chrome/browser/prerender/prerender_test_utils.h [add] https://crrev.com/f0abffcea31f665a2b1cc1979a955cf308c4dcb4/chrome/test/data/prerender/appcache.manifest [add] https://crrev.com/f0abffcea31f665a2b1cc1979a955cf308c4dcb4/chrome/test/data/prerender/appcache.manifest.mock-http-headers [add] https://crrev.com/f0abffcea31f665a2b1cc1979a955cf308c4dcb4/chrome/test/data/prerender/prefetch_appcache.html [modify] https://crrev.com/f0abffcea31f665a2b1cc1979a955cf308c4dcb4/third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp |
|||||||||
►
Sign in to add a comment |
|||||||||
Comment 1 by pasko@chromium.org
, Jul 28 2016