Bug 156334

Summary: Initial Link preload support
Product: WebKit Reporter: Yoav Weiss <yoav>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: aestes, ap, bfulgham, buildbot, cdumez, commit-queue, darin, esprehn+autocc, gyuyoung.kim, igrigorik, japhet, kondapallykalyan, manian, mkwst, orzage, rniwa, timothy, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ews103 for mac-yosemite
none
Archive of layout-test-results from ews105 for mac-yosemite-wk2
none
Archive of layout-test-results from ews125 for ios-simulator-wk2
none
Archive of layout-test-results from ews114 for mac-yosemite
none
Patch
none
Archive of layout-test-results from ews125 for ios-simulator-wk2
none
Patch
none
Patch
none
Patch
none
patch
none
Patch
none
Patch none

Description Yoav Weiss 2016-04-07 02:03:44 PDT
Initial Link preload support
Comment 1 Yoav Weiss 2016-04-07 02:25:19 PDT
Created attachment 275873 [details]
Patch
Comment 2 Yoav Weiss 2016-04-07 02:27:35 PDT
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.
Comment 3 Build Bot 2016-04-07 03:18:22 PDT
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
Comment 4 Build Bot 2016-04-07 03:18:25 PDT
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
Comment 5 Build Bot 2016-04-07 03:21:20 PDT
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
Comment 6 Build Bot 2016-04-07 03:21:23 PDT
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
Comment 7 Build Bot 2016-04-07 03:25:43 PDT
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
Comment 8 Build Bot 2016-04-07 03:25:46 PDT
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
Comment 9 Build Bot 2016-04-07 03:33:30 PDT
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
Comment 10 Build Bot 2016-04-07 03:33:35 PDT
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
Comment 11 Yoav Weiss 2016-04-07 05:01:38 PDT
Created attachment 275881 [details]
Patch
Comment 12 Build Bot 2016-04-07 06:02:28 PDT
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
Comment 13 Build Bot 2016-04-07 06:02:31 PDT
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
Comment 14 Yoav Weiss 2016-04-07 06:13:53 PDT
Created attachment 275888 [details]
Patch
Comment 15 Alexey Proskuryakov 2016-04-07 10:26:04 PDT
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.
Comment 16 Timothy Hatcher 2016-04-07 11:30:47 PDT
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".
Comment 17 Alexey Proskuryakov 2016-04-07 11:44:24 PDT
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.
Comment 18 Alexey Proskuryakov 2016-04-07 12:17:47 PDT
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.
Comment 19 Chris Dumez 2016-04-07 12:30:01 PDT
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()
Comment 20 Timothy Hatcher 2016-04-07 13:47:18 PDT
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).
Comment 21 Timothy Hatcher 2016-04-07 13:48:13 PDT
(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 …
Comment 22 Timothy Hatcher 2016-04-07 13:49:46 PDT
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.
Comment 23 Timothy Hatcher 2016-04-07 14:05:24 PDT
(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.
Comment 24 Yoav Weiss 2016-04-07 14:48:18 PDT
(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?
Comment 25 Yoav Weiss 2016-04-07 15:11:39 PDT
Created attachment 275944 [details]
Patch
Comment 26 Darin Adler 2016-04-10 17:39:30 PDT
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.
Comment 27 Darin Adler 2016-04-10 17:42:15 PDT
(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.
Comment 28 Yoav Weiss 2016-04-11 21:17:34 PDT
Created attachment 276213 [details]
Patch
Comment 29 Yoav Weiss 2016-04-12 11:40:32 PDT
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?
Comment 30 Darin Adler 2016-04-12 15:52:44 PDT
I mainly meant a test for each of the cached resource types.
Comment 31 Yoav Weiss 2016-04-13 06:43:50 PDT
Created attachment 276317 [details]
patch

Manually uploading the latest patch, as webkit-patch upload fails for me
Comment 32 Yoav Weiss 2016-04-13 13:24:23 PDT
Created attachment 276348 [details]
Patch
Comment 33 Yoav Weiss 2016-04-13 13:28:49 PDT
(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
Comment 34 Darin Adler 2016-04-13 14:02:30 PDT
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.
Comment 35 Darin Adler 2016-04-13 14:03:17 PDT
(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.
Comment 36 Yoav Weiss 2016-04-14 20:52:04 PDT
(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.
Comment 37 Yoav Weiss 2016-04-14 21:05:34 PDT
Created attachment 276456 [details]
Patch
Comment 38 Yoav Weiss 2016-04-17 22:17:27 PDT
Comment on attachment 276456 [details]
Patch

CQ+ing this. I'll add further tests in a followup patch.
Comment 39 WebKit Commit Bot 2016-04-17 23:06:28 PDT
Comment on attachment 276456 [details]
Patch

Clearing flags on attachment: 276456

Committed r199650: <http://trac.webkit.org/changeset/199650>
Comment 40 WebKit Commit Bot 2016-04-17 23:06:35 PDT
All reviewed patches have been landed.  Closing bug.
Comment 41 Radar WebKit Bug Importer 2017-05-02 16:26:40 PDT
<rdar://problem/31950983>