It is useful for the UA to be able to preload responsive images. The idea is to allow this with the imagesrcset and imagesizes attributes on <link rel=preload as=image>. Spec PRs and discussions: - https://github.com/w3c/preload/issues/120 (Initial discussion) - https://github.com/whatwg/html/pull/4048 (HTML Standard PR) - https://github.com/w3c/preload/pull/134 (Preload Spec PR (minor))
Created attachment 366072 [details] Patch
Comment on attachment 366072 [details] Patch Attachment 366072 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/11684853 New failing tests: imported/w3c/web-platform-tests/preload/link-header-preload-srcset.tentative.html
Created attachment 366082 [details] Archive of layout-test-results from ews122 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews122 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.6
Created attachment 366276 [details] Patch
Comment on attachment 366276 [details] Patch Attachment 366276 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/11703692 New failing tests: imported/w3c/web-platform-tests/preload/link-header-preload-srcset.tentative.html
Created attachment 366282 [details] Archive of layout-test-results from ews100 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-highsierra Platform: Mac OS X 10.13.6
Comment on attachment 366276 [details] Patch Attachment 366276 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/11703728 New failing tests: imported/w3c/web-platform-tests/preload/link-header-preload-srcset.tentative.html
Created attachment 366283 [details] Archive of layout-test-results from ews104 for mac-highsierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-highsierra-wk2 Platform: Mac OS X 10.13.6
Comment on attachment 366276 [details] Patch Attachment 366276 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/11703770 New failing tests: imported/w3c/web-platform-tests/preload/link-header-preload-srcset.tentative.html
Created attachment 366285 [details] Archive of layout-test-results from ews112 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews112 Port: mac-highsierra Platform: Mac OS X 10.13.6
Created attachment 366308 [details] Patch
(In reply to Rob Buis from comment #11) > Created attachment 366308 [details] > Patch Note that the parameter list for loadLink is quite long, it may make sense to consolidate them in a single class, but I think it should be a separate patch.
Created attachment 367724 [details] Patch
Comment on attachment 367724 [details] Patch Attachment 367724 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/11913509 New failing tests: imported/w3c/web-platform-tests/preload/link-header-preload-srcset.tentative.html
Created attachment 367725 [details] Archive of layout-test-results from ews126 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.6
Would it be better to have a runtime flag for this patch?
Note that the ios-sim failure is likely because of different viewport sizes compared to the other platforms. I'll see if I can change the test or add correct expectation tomorrow.
Comment on attachment 367724 [details] Patch Patch looks good to me overall. Has this shipped in other browsers? View in context: https://bugs.webkit.org/attachment.cgi?id=367724&action=review > Source/WebCore/loader/LinkHeader.cpp:290 > case LinkParameterMedia: LinkHeader::setValue should probably be String&& > Source/WebCore/loader/LinkHeader.cpp:297 > + m_imageSizes = value; move value here and above > Source/WebCore/loader/LinkLoader.h:53 > + bool loadLink(const LinkRelAttribute&, const URL&, const String& as, const String& media, const String& type, const String& crossOrigin, const String&, const String&, Document&); Maybe give the names here for readability. > LayoutTests/imported/w3c/web-platform-tests/preload/link-header-preload-nonce.html:15 > + }, 5000); This test will take 5 seconds to load. Some will take 3 seconds or 5 seconds. Is there a way we can run the test faster? Like have a Link preload in the HTML and do these validation checks once its onload event handler is called. Or loop until all validation checks pass (and exit after a given number of iterations instead of timing out). > LayoutTests/imported/w3c/web-platform-tests/preload/link-header-preload-srcset.tentative-expected.txt:3 > +CONSOLE MESSAGE: The resource http://localhost:8800/preload/resources/square.png?from-header&300 was preloaded using link preload but not used within a few seconds from the window's load event. Please make sure it wasn't preloaded for nothing. Is there a possibility such messages will make the tests flaky? Should we use DumpJSConsoleLogInStdErr? Ditto for other tests.
Created attachment 369880 [details] Patch
Comment on attachment 367724 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=367724&action=review >> Source/WebCore/loader/LinkHeader.cpp:290 >> case LinkParameterMedia: > > LinkHeader::setValue should probably be String&& Done. >> Source/WebCore/loader/LinkHeader.cpp:297 >> + m_imageSizes = value; > > move value here and above Done. >> Source/WebCore/loader/LinkLoader.h:53 >> + bool loadLink(const LinkRelAttribute&, const URL&, const String& as, const String& media, const String& type, const String& crossOrigin, const String&, const String&, Document&); > > Maybe give the names here for readability. Done. >> LayoutTests/imported/w3c/web-platform-tests/preload/link-header-preload-nonce.html:15 >> + }, 5000); > > This test will take 5 seconds to load. Some will take 3 seconds or 5 seconds. > Is there a way we can run the test faster? > Like have a Link preload in the HTML and do these validation checks once its onload event handler is called. > Or loop until all validation checks pass (and exit after a given number of iterations instead of timing out). Isn't it the case that a Link preload in the HTML would not be guaranteed to finish after the Link headers? For now I am experimenting with iterations as you can see in the updated link-header-preload-nonce.html. However note that in the FAIL case this could still be a similar delay equal to the time out, i.e I do 10 loops of 500ms timeouts so in FAIL case it still takes 5s. link-header-preload-nonce.html is such a FAIL, I have not looked into why, but I think this patch is not the right one since it may be csp related. >> LayoutTests/imported/w3c/web-platform-tests/preload/link-header-preload-srcset.tentative-expected.txt:3 >> +CONSOLE MESSAGE: The resource http://localhost:8800/preload/resources/square.png?from-header&300 was preloaded using link preload but not used within a few seconds from the window's load event. Please make sure it wasn't preloaded for nothing. > > Is there a possibility such messages will make the tests flaky? > Should we use DumpJSConsoleLogInStdErr? > Ditto for other tests. It is possible with iterations/shorted timeout these messages will go away. FWIW I did not see DumpJSConsoleLogInStdErr being used in wpt so far.
(In reply to youenn fablet from comment #18) > Comment on attachment 367724 [details] > Patch > > Has this shipped in other browsers? It has shipped in Chrome: https://chromestatus.com/features/5164259990306816 It seems Firefox has no objections but needs to implement rel=preload first: https://github.com/mozilla/standards-positions/issues/130 https://bugzilla.mozilla.org/show_bug.cgi?id=1515760
Comment on attachment 369880 [details] Patch Attachment 369880 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/12190737 New failing tests: imported/w3c/web-platform-tests/preload/link-header-preload-srcset.tentative.html
Created attachment 369896 [details] Archive of layout-test-results from ews122 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews122 Port: ios-simulator-wk2 Platform: Mac OS X 10.14.4
Comment on attachment 369880 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=369880&action=review > Source/WebCore/html/HTMLLinkElement.cpp:265 > + if (!m_linkLoader.loadLink(m_relAttribute, url, attributeWithoutSynchronization(asAttr), attributeWithoutSynchronization(mediaAttr), attributeWithoutSynchronization(typeAttr), attributeWithoutSynchronization(crossoriginAttr), attributeWithoutSynchronization(imagesrcsetAttr), attributeWithoutSynchronization(imagesizesAttr), document())) The loadLink lines are becoming super long. I thought there was a coding style guideline to avoid that, but I only found https://webkit.org/code-style-guidelines/#indentation-wrap-bool-op
Created attachment 369947 [details] Patch
Comment on attachment 369947 [details] Patch Attachment 369947 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/12197067 New failing tests: imported/w3c/web-platform-tests/preload/link-header-preload-imagesrcset.html
Created attachment 369952 [details] Archive of layout-test-results from ews126 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.14.4
Created attachment 369976 [details] Patch
Comment on attachment 369976 [details] Patch Attachment 369976 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/12199967 New failing tests: js/dom/custom-constructors.html
Created attachment 369989 [details] Archive of layout-test-results from ews213 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews213 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Comment on attachment 369976 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=369976&action=review > Source/WebCore/loader/LinkLoader.cpp:250 > + url = href; s/url = href/url = document.completeURL(href)/ > Source/WebCore/loader/LinkLoader.cpp:253 > + document.addConsoleMessage(MessageSource::Other, MessageLevel::Error, "<link rel=preload> has an invalid `href` value"_s); This is a bit misleading since we are talking in the message of href, while it might be related to imageSrcSet value. Can we improve on this, either by adding the URL that is invalid or by making the message more specific. > Source/WebCore/loader/LinkLoader.cpp:262 > + auto linkRequest = createPotentialAccessControlRequest(document.completeURL(url), document, crossOriginMode, WTFMove(options)); s/document.completeURL(url)/url > LayoutTests/ChangeLog:11 > + * platform/ios-simulator-12-wk2/imported/w3c/web-platform-tests/preload/link-header-preload-imagesrcset-expected.txt: Added. It should probably be ios-simulator-wk2 instead of ios-simulator-12-wk2. Is there a way to modify the test to handle various dimensions?
Created attachment 370026 [details] Patch
Comment on attachment 370026 [details] Patch Attachment 370026 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/12205022 New failing tests: js/dom/custom-constructors.html
Created attachment 370030 [details] Archive of layout-test-results from ews210 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews210 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Comment on attachment 370026 [details] Patch Attachment 370026 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/12204984 New failing tests: imported/w3c/web-platform-tests/preload/link-header-preload-imagesrcset.html
Created attachment 370031 [details] Archive of layout-test-results from ews126 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.14.4
Created attachment 370034 [details] Patch
Created attachment 370053 [details] Patch
Comment on attachment 370053 [details] Patch Attachment 370053 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/12209105 New failing tests: imported/w3c/web-platform-tests/preload/dynamic-adding-preload-imagesrcset.html
Created attachment 370068 [details] Archive of layout-test-results from ews126 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.14.5
Created attachment 370109 [details] Patch
Created attachment 370119 [details] Patch
Comment on attachment 370119 [details] Patch Attachment 370119 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/12215917 New failing tests: imported/w3c/web-platform-tests/preload/single-download-preload.html
Created attachment 370123 [details] Archive of layout-test-results from ews100 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-highsierra Platform: Mac OS X 10.13.6
Created attachment 370124 [details] Patch
Created attachment 370127 [details] Patch
Created attachment 370130 [details] Patch
Comment on attachment 369976 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=369976&action=review >> Source/WebCore/loader/LinkLoader.cpp:250 >> + url = href; > > s/url = href/url = document.completeURL(href)/ Done. >> Source/WebCore/loader/LinkLoader.cpp:253 >> + document.addConsoleMessage(MessageSource::Other, MessageLevel::Error, "<link rel=preload> has an invalid `href` value"_s); > > This is a bit misleading since we are talking in the message of href, while it might be related to imageSrcSet value. > Can we improve on this, either by adding the URL that is invalid or by making the message more specific. Done. >> Source/WebCore/loader/LinkLoader.cpp:262 >> + auto linkRequest = createPotentialAccessControlRequest(document.completeURL(url), document, crossOriginMode, WTFMove(options)); > > s/document.completeURL(url)/url Done. >> LayoutTests/ChangeLog:11 >> + * platform/ios-simulator-12-wk2/imported/w3c/web-platform-tests/preload/link-header-preload-imagesrcset-expected.txt: Added. > > It should probably be ios-simulator-wk2 instead of ios-simulator-12-wk2. > Is there a way to modify the test to handle various dimensions? Unfortunately I did not see a way to modify the test.
Comment on attachment 370130 [details] Patch Clearing flags on attachment: 370130 Committed r245475: <https://trac.webkit.org/changeset/245475>
All reviewed patches have been landed. Closing bug.
<rdar://problem/50904042>
The newly imported imported/w3c/web-platform-tests/preload/link-header-preload-delay-onload.html from https://trac.webkit.org/changeset/245475/webkit is flakey History: https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=imported%2Fw3c%2Fweb-platform-tests%2Fpreload%2Flink-header-preload-delay-onload.html Diff: --- /Volumes/Data/slave/highsierra-release-tests-wk1/build/layout-test-results/imported/w3c/web-platform-tests/preload/link-header-preload-delay-onload-expected.txt +++ /Volumes/Data/slave/highsierra-release-tests-wk1/build/layout-test-results/imported/w3c/web-platform-tests/preload/link-header-preload-delay-onload-actual.txt @@ -1,4 +1,4 @@ -PASS Makes sure that Link headers preload resources and block window.onload after resource discovery +FAIL Makes sure that Link headers preload resources and block window.onload after resource discovery assert_true: expected true got false
(In reply to Truitt Savell from comment #52) > -PASS Makes sure that Link headers preload resources and block window.onload > after resource discovery > +FAIL Makes sure that Link headers preload resources and block window.onload > after resource discovery assert_true: expected true got false It is probably best to skip this test and make a bug to fix the test result. I should have some time to look into it end of the week.
(In reply to Rob Buis from comment #53) > (In reply to Truitt Savell from comment #52) > > -PASS Makes sure that Link headers preload resources and block window.onload > > after resource discovery > > +FAIL Makes sure that Link headers preload resources and block window.onload > > after resource discovery assert_true: expected true got false > > It is probably best to skip this test and make a bug to fix the test result. > I should have some time to look into it end of the week. Can we wrap this in an experimental feature flag?
Let's roll back at this point. The test is still flaky, and yes, features need flags.
Reverted r245475 for reason: Newly imported test is flaky. Features need flags. Committed r245825: <https://trac.webkit.org/changeset/245825>
Created attachment 371044 [details] Patch
Created attachment 371059 [details] Patch
Created attachment 371070 [details] Patch
Created attachment 371073 [details] Patch
Comment on attachment 371073 [details] Patch Sorry for not including the runtime flag in the patch that was reverted! I added it in the latest patch. I also removed the flaky test, since it is not actually testing imagesrcset/imagesizes.
Comment on attachment 371073 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=371073&action=review > Source/WebCore/loader/LinkLoader.cpp:245 > + if (type == CachedResource::Type::ImageResource && !imageSrcSet.isEmpty()) { We probably need to add a runtime check here to disable the behavior if the flag is not enabled. > Source/WebCore/loader/LinkLoader.h:66 > + static std::unique_ptr<LinkPreloadResourceClient> preloadIfNeeded(const LinkRelAttribute&, const URL& href, Document&, const String& as, const String& media, const String& type, const String& crossOriginMode, const String&, const String&, LinkLoader*); Could we add the names of the string variables to improve readability. > LayoutTests/imported/w3c/ChangeLog:66 > + Not needed. > LayoutTests/platform/ios-simulator-12-wk2/imported/w3c/web-platform-tests/preload/dynamic-adding-preload-imagesrcset-expected.txt:2 > +FAIL Makes sure that a dynamically added preload with imagesrcset works assert_equals: resources/square.png?400 expected 1 but got 0 Could we add specific variations of these tests for iOS?
Created attachment 371145 [details] Patch
Comment on attachment 371145 [details] Patch Attachment 371145 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/12355653 New failing tests: inspector/canvas/recording.html
Created attachment 371148 [details] Archive of layout-test-results from ews102 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-highsierra Platform: Mac OS X 10.13.6
Created attachment 371149 [details] Patch
Comment on attachment 371149 [details] Patch Attachment 371149 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/12356219 New failing tests: http/wpt/service-workers/service-worker-networkprocess-crash.html
Created attachment 371156 [details] Archive of layout-test-results from ews104 for mac-highsierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-highsierra-wk2 Platform: Mac OS X 10.13.6
Created attachment 371157 [details] Patch
Comment on attachment 371157 [details] Patch Attachment 371157 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/12356774 New failing tests: inspector/canvas/recording.html
Created attachment 371159 [details] Archive of layout-test-results from ews101 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-highsierra Platform: Mac OS X 10.13.6
Comment on attachment 371073 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=371073&action=review >> Source/WebCore/loader/LinkLoader.cpp:245 >> + if (type == CachedResource::Type::ImageResource && !imageSrcSet.isEmpty()) { > > We probably need to add a runtime check here to disable the behavior if the flag is not enabled. Done. >> Source/WebCore/loader/LinkLoader.h:66 >> + static std::unique_ptr<LinkPreloadResourceClient> preloadIfNeeded(const LinkRelAttribute&, const URL& href, Document&, const String& as, const String& media, const String& type, const String& crossOriginMode, const String&, const String&, LinkLoader*); > > Could we add the names of the string variables to improve readability. Done. >> LayoutTests/imported/w3c/ChangeLog:66 >> + > > Not needed. Removed. >> LayoutTests/platform/ios-simulator-12-wk2/imported/w3c/web-platform-tests/preload/dynamic-adding-preload-imagesrcset-expected.txt:2 >> +FAIL Makes sure that a dynamically added preload with imagesrcset works assert_equals: resources/square.png?400 expected 1 but got 0 > > Could we add specific variations of these tests for iOS? I still don't know how to do that. If anybody has an idea, I can do a follow-up patch.
Created attachment 371161 [details] Patch
Comment on attachment 371161 [details] Patch Clearing flags on attachment: 371161 Committed r246045: <https://trac.webkit.org/changeset/246045>
> >> LayoutTests/platform/ios-simulator-12-wk2/imported/w3c/web-platform-tests/preload/dynamic-adding-preload-imagesrcset-expected.txt:2 > >> +FAIL Makes sure that a dynamically added preload with imagesrcset works assert_equals: resources/square.png?400 expected 1 but got 0 > > > > Could we add specific variations of these tests for iOS? > > I still don't know how to do that. If anybody has an idea, I can do a > follow-up patch. IIUC the issue, the viewport for iOS simulator is different, which will trigger preloading of different resources than the test expects. I would copy the test in some LayoutTests iOS specific folder and update the test so that we validate preloading with the iOS simulator viewport works. Could the test get the viewport and from it, compute on its own which preload should be selected? Alternatively, the test could be copie-then-made-iOS-specific.
(In reply to youenn fablet from comment #76) > > >> LayoutTests/platform/ios-simulator-12-wk2/imported/w3c/web-platform-tests/preload/dynamic-adding-preload-imagesrcset-expected.txt:2 > > >> +FAIL Makes sure that a dynamically added preload with imagesrcset works assert_equals: resources/square.png?400 expected 1 but got 0 > > > > > > Could we add specific variations of these tests for iOS? > > > > I still don't know how to do that. If anybody has an idea, I can do a > > follow-up patch. > > IIUC the issue, the viewport for iOS simulator is different, which will > trigger preloading of different resources than the test expects. > I would copy the test in some LayoutTests iOS specific folder and update the > test so that we validate preloading with the iOS simulator viewport works. > > Could the test get the viewport and from it, compute on its own which > preload should be selected? > > Alternatively, the test could be copie-then-made-iOS-specific. Ok, I will have a look tomorrow.
(In reply to Rob Buis from comment #77) > (In reply to youenn fablet from comment #76) > > IIUC the issue, the viewport for iOS simulator is different, which will > > trigger preloading of different resources than the test expects. > > I would copy the test in some LayoutTests iOS specific folder and update the > > test so that we validate preloading with the iOS simulator viewport works. > > > > Could the test get the viewport and from it, compute on its own which > > preload should be selected? > > > > Alternatively, the test could be copie-then-made-iOS-specific. > > Ok, I will have a look tomorrow. I fixed the tests in the WPT repo and imported them here: https://bugs.webkit.org/show_bug.cgi?id=198533