Initial Link preload support
Created attachment 275873 [details] Patch
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 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
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 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
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 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
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 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
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
Created attachment 275881 [details] Patch
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
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
Created attachment 275888 [details] Patch
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 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 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 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 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()
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).
(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 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.
(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.
(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?
Created attachment 275944 [details] Patch
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.
(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.
Created attachment 276213 [details] Patch
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?
I mainly meant a test for each of the cached resource types.
Created attachment 276317 [details] patch Manually uploading the latest patch, as webkit-patch upload fails for me
Created attachment 276348 [details] Patch
(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 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.
(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.
(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.
Created attachment 276456 [details] Patch
Comment on attachment 276456 [details] Patch CQ+ing this. I'll add further tests in a followup patch.
Comment on attachment 276456 [details] Patch Clearing flags on attachment: 276456 Committed r199650: <http://trac.webkit.org/changeset/199650>
All reviewed patches have been landed. Closing bug.
<rdar://problem/31950983>