Bug 57842 – Add support for link rel type "subresource"

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Gavin Peters]

Link rel=prefetch is great for cache warming for resources on subsequent pages, but it launches requests at too low a priority to use for subresources of the current page.

Particularly after https://bugs.webkit.org/show_bug.cgi?id=51940 is implemented, a rel type that launches requests at an higher priority is going to be very useful.  This rel type is in the HTML5 spec at http://wiki.whatwg.org/wiki/RelExtensions .  An expected use case will be for servers to provide subresource hints in link headers, which will allow servers to help make the web faster.

 This feature continues my implementation of the Link header, which we've talked about in WebKit-dev in https://lists.webkit.org/pipermail/webkit-dev/2011-February/016034.html.

[Gavin Peters]

Created attachment 88228 [details]
Patch

[WebKit Review Bot]

Attachment 88228 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style']" exit_code: 1

Source/WebCore/loader/cache/CachedResourceLoader.h:72:  The parameter name "priority" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Gavin Peters]

Comment on attachment 88228 [details]
Patch

The http test isn't working as gloriously as it could, and I believe that's an instance of https://bugs.webkit.org/show_bug.cgi?id=49238 , which I will hack at next.

Meanwhile, this patch continues the development path of the Link header, from https://lists.webkit.org/pipermail/webkit-dev/2011-February/016034.html

[Gavin Peters]

A note on the style failure: the default argument requires it; see the same pattern a few lines up in the same header.

[Alexey Proskuryakov]

I still don't see the rel type registered at <http://paramsr.us/link-relation-types/>, which we should probably do before landing any code.

[Tony Gentilcore]

Can we add a test case that has a <link rel=subresource> which loads a non-cacheable resource then checks that it may be used as any other type on the same page without re-requesting it (e.g. as an img, script, style, iframe). I believe you can reuse some of the machinery introduced by http://trac.webkit.org/changeset/44350 .

Also, as an aside, we probably should teach the HTMLPreloadScanner about rel=subresource. That is fine to do as a follow-up as long as it is filed.

[Adam Barth]

(In reply to comment #4)
> A note on the style failure: the default argument requires it; see the same pattern a few lines up in the same header.

You can omit argument names even with default arguments.

[Adam Barth]

(In reply to comment #5)
> I still don't see the rel type registered at <http://paramsr.us/link-relation-types/>, which we should probably do before landing any code.

This is <http://www.w3.org/html/wg/tracker/issues/27>.  I don't think we need to gate this patch on the resolution of that issue.  I'm sure Gavin will register the rel type in whatever registry the HTML WG decides to use.  At the moment, the spec says to use <http://wiki.whatwg.org/wiki/RelExtensions>, which is what he's done.

[Gavin Peters]

Thanks everyone for the comments!

Tony,

Re: the test

I can definitely write the test, but right now I don't think we'd pass it.  We'll make a second request, and as a result go back to our disk cache.  In the case of non-cacheable content, a 304 is the best case afaik.

There's two things worth considering for fixing this:

- the underlying disk cache might note subresource requests and make it available despite cacheability.  I believe we might have discussed this before.  :-)

- WebKit's cached resource hierarchy really ought to change so that CachedImage is just a view of the same underlying bytes in a CachedResource.

But even without either of those two changes, I think this CL is still useful; for the subresource rel type in particular is being used by people who we can hope are aware of the underlying cache issues.


Re: preload scanning

I'm not totally convinced here.  I don't know how useful the HTML rel=subresource type is, and as Alexey points out above, it's really most useful in the HTTP.  However, without a preload scanner, I suppose it is totally useless.  The other concern is that a preload scanner will hide the network prioritization bug https://bugs.webkit.org/show_bug.cgi?id=49238 , so I would at least want to hold off until we land that.

[Alexey Proskuryakov]

The nice thing about proper registration (and presumably the reason why it's not registered yet) is that you need a spec/documentation to file with registration. That's much preferable to adding unspecified and undiscussed features to WebKit.

[Adam Barth]

> The nice thing about proper registration (and presumably the reason why it's not registered yet) is that you need a spec/documentation to file with registration.

That's indeed one of the arguments folks have put forward for using the IANA registry.  There, of course, other arguments in favor of the other registry.  The proper forum for discussing which registry to use, however, isn't this bug.  It's the W3C HTML working group.

> That's much preferable to adding unspecified and undiscussed features to WebKit.

If you'd like a specification and/or discussion, you don't need to hide behind IANA.  You can just ask for those things.

[Tony Gentilcore]

> Re: the test
> 
> I can definitely write the test, but right now I don't think we'd pass it.  We'll make a second request, and as a result go back to our disk cache.  In the case of non-cacheable content, a 304 is the best case afaik.

I don't see the point of exposing rel=subresource without it. Perhaps I'm not understanding the use-case, but I thought the primary use case is for performance. With that bug in place, it is just a performance booby trap. Maybe I'm missing another use-case?

> Re: preload scanning
> 
> I'm not totally convinced here.  I don't know how useful the HTML rel=subresource type is, and as Alexey points out above, it's really most useful in the HTTP.  However, without a preload scanner, I suppose it is totally useless.  The other concern is that a preload scanner will hide the network prioritization bug https://bugs.webkit.org/show_bug.cgi?id=49238 , so I would at least want to hold off until we land that.

Actually, I think the HTML tag is very useful. Last week a web perf optimization guy emailed me about how to preload subresources in WebKit. The best technique he had found was to preload the subresources into <object> elements. Unfortunately, this is a pretty bad idea as the object actually acts like an iframe in that case, meaning that any textual content gets layout. The 200k compiled JS resource he was preloading took 500ms to perform line breaking on my workstation (see bug 56796). Unfortunately, I couldn't offer any reasonable approach other than to wait for something like rel=subresource.

On a similar note, a couple of months ago, there was a large thread on the whatwg about introducing a noexecute attribute to the <script> element so that a script could be downloaded without execution (see http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2011-February/030161.html). A problem with that proposal is that it only addresses preloading a subresource for scripts. With rel=subresource, a web dev could effectively preload any type of subresource needed by the page.

I think rel=subresource addresses these types of use cases in a really general way. Whether the preload scanner detects it is orthogonal to this patch, I just thought I'd bring it up.

[Alexey Proskuryakov]

> If you'd like a specification and/or discussion, you don't need to hide behind IANA.

You don't need to hide _from_ IANA either. Let's keep Bugzilla discussions polite and courteous.

[Adam Barth]

Sounds like the current status here is that we'd like to add support for this feature, but possible more registration and/or specification might be helpful.  IMHO, we should accept this patch now and then address any registration requirements that might arise out of Issue 27 in the HTML working group at such time as they actually become requirements.

[Adam Barth]

Comment on attachment 88228 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=88228&action=review

> Source/WebCore/loader/cache/CachedResourceRequest.cpp:64
> +    case CachedResource::LinkResource:
> +        if (priority == ResourceLoadPriorityLowest)
> +            return ResourceRequest::TargetIsPrefetch;
> +        return ResourceRequest::TargetIsSubresource;

This seems slightly like back-door dataflow, but it's probably ok.

[WebKit Commit Bot]

Comment on attachment 88228 [details]
Patch

Rejecting attachment 88228 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-7', 'build'..." exit_code: 2

Last 500 characters of output:
tenv YACC /Developer/usr/bin/yacc
    /bin/sh -c /mnt/git/webkit-commit-queue/WebKitBuild/WebCore.build/Debug/WebCore.build/Script-5DF50887116F3077005202AB.sh

** BUILD FAILED **


The following build commands failed:
WebCore:
	CompileC /mnt/git/webkit-commit-queue/WebKitBuild/WebCore.build/Debug/WebCore.build/Objects-normal/x86_64/CachedResourceRequest.o /mnt/git/webkit-commit-queue/Source/WebCore/loader/cache/CachedResourceRequest.cpp normal x86_64 c++ com.apple.compilers.gcc.4_2
(1 failure)


Full output: http://queues.webkit.org/results/8457705

[Adam Barth]

Created attachment 89955 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 89955 [details]
Patch for landing

Clearing flags on attachment: 89955

Committed r84110: <http://trac.webkit.org/changeset/84110>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Daniel Bates]

This patch broke the Chromium build because one instance of CachedResource::LinkPrefetch wasn't renamed to  CachedResource::LinkResource.

Committed build fix in changeset 84116<http://trac.webkit.org/changeset/84116>.

[Adam Barth]

Thanks Dan.  I wonder why it got a green on cr-linux....