Bug 156334 – Initial Link preload support

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yoav Weiss]

Initial Link preload support

[Yoav Weiss]

Created attachment 275873 [details]
Patch

[Yoav Weiss]

As discussed (a while ago) on webkit-dev, I'm interested in implementing <link rel=preload> in WebKit. This patch contains initial support for the feature.

[Build Bot]

Comment on attachment 275873 [details]
Patch

Attachment 275873 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1113808

New failing tests:
imported/w3c/web-platform-tests/html/dom/interfaces.html
imported/w3c/web-platform-tests/html/dom/reflection-metadata.html

[Build Bot]

Created attachment 275875 [details]
Archive of layout-test-results from ews103 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 275873 [details]
Patch

Attachment 275873 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1113815

New failing tests:
imported/w3c/web-platform-tests/html/dom/interfaces.html
imported/w3c/web-platform-tests/html/dom/reflection-metadata.html

[Build Bot]

Created attachment 275876 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 275873 [details]
Patch

Attachment 275873 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1113814

New failing tests:
imported/w3c/web-platform-tests/html/dom/interfaces.html
imported/w3c/web-platform-tests/html/dom/reflection-metadata.html

[Build Bot]

Created attachment 275877 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 275873 [details]
Patch

Attachment 275873 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1113830

New failing tests:
http/tests/preload/download_resources.html

[Build Bot]

Created attachment 275878 [details]
Archive of layout-test-results from ews114 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Yoav Weiss]

Created attachment 275881 [details]
Patch

[Build Bot]

Comment on attachment 275881 [details]
Patch

Attachment 275881 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1114372

New failing tests:
imported/w3c/web-platform-tests/html/dom/interfaces.html

[Build Bot]

Created attachment 275887 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.10.5

[Yoav Weiss]

Created attachment 275888 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 275888 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        Initial Link preload support

I think that webkit-dev discussion resulted in us not willing to do this.

[Timothy Hatcher]

Comment on attachment 275888 [details]
Patch

We should review this patch, deciding to land it can be a separate discussion. I don't feel the discussion on webkit-dev resulted in "us not willing to do this".

[Alexey Proskuryakov]

Comment on attachment 275888 [details]
Patch

No, we don't r+ patches for features that we don't want. It's always ok to provide feedback to a patch even if it's not marked for review though.

I do not feel like a got a good faith response to my concerns on webkit-dev, which is why the discussion ended quickly. You should feel free to revive it if you think that the feature is so useful that it overweighs the downsides.

[Alexey Proskuryakov]

Comment on attachment 275888 [details]
Patch

Someone who chose not to speak publicly has provided somewhat more convincing arguments for trying out this feature. I still don't believe that it's a good idea, but not strongly enough to fight further.

[Chris Dumez]

Comment on attachment 275888 [details]
Patch

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

> Source/WebCore/loader/LinkLoader.cpp:88
> +bool LinkLoader::getResourceTypeFromAsAttribute(const String& as, CachedResource::Type& type)

Should probably return an Optional<CachedResource::Type> and return NullOpt for the error case.

> Source/WebCore/loader/LinkLoader.cpp:138
> +    document.cachedResourceLoader().preload(type, linkRequest, "");

"" -> emptyString()

[Timothy Hatcher]

We should consider putting this behind a runtime flag that is enabled for Safari Technology Preview and WebKit nightlies until there is more conscious around the feature. Especially in regards to the similar, but not mandatory, <link rel=prefetch>.

Also we should do as was mentioned in the webkit-dev thread and log a warning to the console when a preloaded resource is not used (likely after a short timeout from the load event or something).

[Timothy Hatcher]

(In reply to comment #20)
> We should consider putting this behind a runtime flag that is enabled for
> Safari Technology Preview and WebKit nightlies until there is more conscious
> around the feature. Especially in regards to the similar, but not mandatory,
> <link rel=prefetch>.
> 
> Also we should do as was mentioned in the webkit-dev thread and log a
> warning to the console when a preloaded resource is not used (likely after a
> short timeout from the load event or something).

… more consensus …

[Timothy Hatcher]

Comment on attachment 275888 [details]
Patch

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

> Source/WebCore/loader/LinkLoader.cpp:120
> +        document.addConsoleMessage(MessageSource::Other, MessageLevel::Warning, String("<link rel=preload> has an invalid `href` value"));

This should be an error, not a warning, since the preload is ignored without it.

> Source/WebCore/loader/LinkLoader.cpp:125
> +        document.addConsoleMessage(MessageSource::Other, MessageLevel::Warning, String("<link rel=preload> must have a valid `as` value"));

Ditto.

[Timothy Hatcher]

(In reply to comment #20)
> Especially in regards to the similar, but not mandatory, <link rel=prefetch>.

I was wrong about prefetch, that is about the next navigation, not the current page.

[Yoav Weiss]

(In reply to comment #20)
> We should consider putting this behind a runtime flag that is enabled for
> Safari Technology Preview and WebKit nightlies until there is more conscious
> around the feature. Especially in regards to the similar, but not mandatory,
> <link rel=prefetch>.

I have a runtime flag which prevents LinkRelAttribute::isLinkPreload from being true if the flag is false. That should be enough to turn off the feature.

I would have actually preferred that the flag would be false by default for WebKit nightly/Safari Technology Preview until I'd add support for the onload event (for Web compatibility reasons: things like http://filamentgroup.github.io/loadCSS/test/preload.html rely on the load event).

Is it possible to turn the flag on only for layout tests, but to keep it false for everything else?

Otherwise, I could add onload support to this patch, but it may increase its size.

> 
> Also we should do as was mentioned in the webkit-dev thread and log a
> warning to the console when a preloaded resource is not used (likely after a
> short timeout from the load event or something).

Sure. Should I add that mechanism as part of this patch? Or could it wait for a followup patch?

[Yoav Weiss]

Created attachment 275944 [details]
Patch

[Darin Adler]

Comment on attachment 275944 [details]
Patch

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

Looks OK. No test coverage for the various types of preloading, which seems like a problem to me.

> Source/WebCore/loader/LinkLoader.cpp:104
> +    String lowerAs = as.convertToASCIILowercase();
> +    if (lowerAs == "image")
> +        return CachedResource::ImageResource;
> +    if (lowerAs == "script")
> +        return CachedResource::Script;
> +    if (lowerAs == "style")
> +        return CachedResource::CSSStyleSheet;
> +    if (lowerAs == "media")
> +        return CachedResource::MediaResource;
> +    if (lowerAs == "font")
> +        return CachedResource::FontResource;
> +    if (lowerAs == "track")
> +        return CachedResource::TextTrackResource;

This should use equalLettersIgnoringASCIICase and not allocate a lowercased copy of the string.

> Source/WebCore/loader/LinkLoader.cpp:114
> +    if (!href.isValid() || href.isEmpty()) {

I found other code doing checks like this, but there is no need to check isEmpty after checking isValid. Empty URLs are always also invalid.

> Source/WebCore/loader/LinkLoader.cpp:118
> +    Optional<CachedResource::Type> type = LinkLoader::getResourceTypeFromAsAttribute(as);

I think this reads better with the type "auto" instead of writing out the Optional type.

[Darin Adler]

(In reply to comment #24)
> Is it possible to turn the flag on only for layout tests, but to keep it
> false for everything else?

You can add an internals function that turns the flag on.

[Yoav Weiss]

Created attachment 276213 [details]
Patch

[Yoav Weiss]

Comment on attachment 276213 [details]
Patch

Thanks for reviewing, Darin!

Could you elaborate on what more tests you'd like to see added to the feature?

[Darin Adler]

I mainly meant a test for each of the cached resource types.

[Yoav Weiss]

Created attachment 276317 [details]
patch

Manually uploading the latest patch, as webkit-patch upload fails for me

[Yoav Weiss]

Created attachment 276348 [details]
Patch

[Yoav Weiss]

(In reply to comment #30)
> I mainly meant a test for each of the cached resource types.

If I understand you correctly, that should be covered by http/tests/preload/download_resources.html

[Darin Adler]

Comment on attachment 275944 [details]
Patch

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

> Source/WebCore/loader/LinkLoader.h:54
> +    static Optional<CachedResource::Type> getResourceTypeFromAsAttribute(const String& as);

WebKit coding style would not use the word “get” in the name of this function.

[Darin Adler]

(In reply to comment #33)
> (In reply to comment #30)
> > I mainly meant a test for each of the cached resource types.
> 
> If I understand you correctly, that should be covered by
> http/tests/preload/download_resources.html

Not quite. Imagine for a second that getResourceTypeFromAsAttribute returned incorrect values; I just went in there and shuffled things around.

I am looking for a test that would fail in that case. That would detect that the loading isn’t being done correctly.

[Yoav Weiss]

(In reply to comment #35)
> (In reply to comment #33)
> > (In reply to comment #30)
> > > I mainly meant a test for each of the cached resource types.
> > 
> > If I understand you correctly, that should be covered by
> > http/tests/preload/download_resources.html
> 
> Not quite. Imagine for a second that getResourceTypeFromAsAttribute returned
> incorrect values; I just went in there and shuffled things around.
> 
> I am looking for a test that would fail in that case. That would detect that
> the loading isn’t being done correctly.

Oh, I now see what you mean and this is obviously needed. I think that adding such tests would require adding an internals function that indicates if a resource was preloaded and reused. I prefer to add that as a followup patch, if that's OK.

[Yoav Weiss]

Created attachment 276456 [details]
Patch

[Yoav Weiss]

Comment on attachment 276456 [details]
Patch

CQ+ing this. I'll add further tests in a followup patch.

[WebKit Commit Bot]

Comment on attachment 276456 [details]
Patch

Clearing flags on attachment: 276456

Committed r199650: <http://trac.webkit.org/changeset/199650>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/31950983>