Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2219663005:
[NoStatePrefetch] Set LOAD_PREFETCH flag on all prefetch requests (Closed)
|
Created:
4 years, 4 months ago by droger Modified:
4 years, 4 months ago CC:
chromium-reviews, blink-reviews, dglazkov+blink, blink-reviews-html_chromium.org, kinuko+watch, loading-reviews+parser_chromium.org, pasko Base URL:
https://chromium.googlesource.com/chromium/src.git@prefetchProto Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPatch Set 1 #
Depends on Patchset: Messages
Total messages: 15 (4 generated)
pasko: FYI, if you want to play with this while I'm OOO.
Description was changed from ========== [NoStatePrefetch] Set LOAD_PREFETCH flag on all prefetch requests ========== to ========== [NoStatePrefetch] Set LOAD_PREFETCH flag on all prefetch requests BUG=632361 ==========
csharrison@chromium.org changed reviewers: + csharrison@chromium.org
drive-by: You want linkPrefetch not linkPreload.
On 2016/08/05 14:22:40, csharrison wrote: > drive-by: You want linkPrefetch not linkPreload. Ah right, thanks. I'm not sure this approach will work then :( I know you suggested to change the type of the resource to LinkPrefetch, but is this really OK to do? This will override the actual type of the resource potentially destroying information, and will lead to change of behavior, for example in DocumentLoader::startPreload: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/loader/Do... where there is a switch on the resource type.
csharrison@chromium.org changed reviewers: + yoav@yoav.ws
That's a good point, there needs to be functionality to preload scan a prefetch :) This doesn't seem to be an inherent problem, it just might require a slightly different approach. You probably want to end up calling this code: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/loader/Li... +Yoav for more input on this. I think it makes sense to support LinkRelPrefetch in preload scanner anyway, WDYT? For context: here is NoState prefetch design doc if you are unfamiliar: https://docs.google.com/document/d/16VCYGGWau483IMSxODpg5faZny1FJ6vNK2v-BuM5E...
On 2016/08/05 14:47:51, csharrison wrote: > You probably want to end up calling this code: > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/loader/Li... So, if the resource is an image for example, it is OK to call LinkFetchResource::fetch() instead of ImageResource::fetch()? There are no side effects to this?
On 2016/08/05 14:55:20, droger wrote: > On 2016/08/05 14:47:51, csharrison wrote: > > You probably want to end up calling this code: > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/loader/Li... > > So, if the resource is an image for example, it is OK to call > LinkFetchResource::fetch() instead of ImageResource::fetch()? There are no side > effects to this? No, that won't work. I looked into this recently (for <link rel=preload as=document> support) and I think we need to pipe a separate indication that the resource in question is a link preload (and kill the LinkPreload resource type while we're at it). Currently the `forPreload` flag is confusing link preload and preloadScanner based preloads. We need to split that up.
I think it should be okay (I'm not totally sure though). Blink shouldn't really care about the types of these resources. It looks like LinkFetchResource::fetch takes a resource type param so you might be able to keep this information.
On 2016/08/05 14:59:05, Yoav Weiss wrote: > On 2016/08/05 14:55:20, droger wrote: > > On 2016/08/05 14:47:51, csharrison wrote: > > > You probably want to end up calling this code: > > > > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/loader/Li... > > > > So, if the resource is an image for example, it is OK to call > > LinkFetchResource::fetch() instead of ImageResource::fetch()? There are no > side > > effects to this? > > No, that won't work. > > I looked into this recently (for <link rel=preload as=document> support) and I > think we need to pipe a separate indication that the resource in question is a > link preload (and kill the LinkPreload resource type while we're at it). > > Currently the `forPreload` flag is confusing link preload and preloadScanner > based preloads. We need to split that up. Disregard my last comment. I stand corrected :)
On 2016/08/05 14:59:05, Yoav Weiss wrote: > On 2016/08/05 14:55:20, droger wrote: > > On 2016/08/05 14:47:51, csharrison wrote: > > > You probably want to end up calling this code: > > > > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/loader/Li... > > > > So, if the resource is an image for example, it is OK to call > > LinkFetchResource::fetch() instead of ImageResource::fetch()? There are no > side > > effects to this? > > No, that won't work. > > I looked into this recently (for <link rel=preload as=document> support) and I > think we need to pipe a separate indication that the resource in question is a > link preload (and kill the LinkPreload resource type while we're at it). > > Currently the `forPreload` flag is confusing link preload and preloadScanner > based preloads. We need to split that up. Thanks! I'll dig into this direction then.
On 2016/08/05 14:59:05, Yoav Weiss wrote: > On 2016/08/05 14:55:20, droger wrote: > > On 2016/08/05 14:47:51, csharrison wrote: > > > You probably want to end up calling this code: > > > > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/loader/Li... > > > > So, if the resource is an image for example, it is OK to call > > LinkFetchResource::fetch() instead of ImageResource::fetch()? There are no > side > > effects to this? > > No, that won't work. > > I looked into this recently (for <link rel=preload as=document> support) and I > think we need to pipe a separate indication that the resource in question is a > link preload (and kill the LinkPreload resource type while we're at it). > > Currently the `forPreload` flag is confusing link preload and preloadScanner > based preloads. We need to split that up. I'm now confused. If I understand correctly, this comment applies to link preload, which is not why I'm looking for. I'm trying to add the LOAD_PREFETCH network flag to requests that are not <link rel=prefetch>. Can I go with Charlie's suggestion then?
Description was changed from ========== [NoStatePrefetch] Set LOAD_PREFETCH flag on all prefetch requests BUG=632361 ========== to ========== [NoStatePrefetch] Set LOAD_PREFETCH flag on all prefetch requests BUG=632361, 638155 ==========
Yoav, can you clarify your comment a bit? I'm not sure this is really necessary, considering that we don't even need the resource in Blink (we just need to populate the browser's HTTP cache). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
