RESOLVED FIXED 192950
Implement imagesrcset and imagesizes attributes on link rel=preload
https://bugs.webkit.org/show_bug.cgi?id=192950
Summary Implement imagesrcset and imagesizes attributes on link rel=preload
Dominic Farolino
Reported 2018-12-20 12:46:57 PST
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))
Attachments
Patch (190.96 KB, patch)
2019-03-27 08:38 PDT, Rob Buis
no flags
Archive of layout-test-results from ews122 for ios-simulator-wk2 (2.59 MB, application/zip)
2019-03-27 10:47 PDT, EWS Watchlist
no flags
Patch (192.19 KB, patch)
2019-03-29 09:43 PDT, Rob Buis
no flags
Archive of layout-test-results from ews100 for mac-highsierra (2.44 MB, application/zip)
2019-03-29 10:55 PDT, EWS Watchlist
no flags
Archive of layout-test-results from ews104 for mac-highsierra-wk2 (2.73 MB, application/zip)
2019-03-29 11:10 PDT, EWS Watchlist
no flags
Archive of layout-test-results from ews112 for mac-highsierra (2.26 MB, application/zip)
2019-03-29 11:38 PDT, EWS Watchlist
no flags
Patch (192.19 KB, patch)
2019-03-29 15:04 PDT, Rob Buis
no flags
Patch (150.63 KB, patch)
2019-04-18 03:38 PDT, Rob Buis
no flags
Archive of layout-test-results from ews126 for ios-simulator-wk2 (2.64 MB, application/zip)
2019-04-18 05:42 PDT, EWS Watchlist
no flags
Patch (152.81 KB, patch)
2019-05-14 12:29 PDT, Rob Buis
no flags
Archive of layout-test-results from ews122 for ios-simulator-wk2 (8.08 MB, application/zip)
2019-05-14 14:37 PDT, EWS Watchlist
no flags
Patch (173.62 KB, patch)
2019-05-15 05:46 PDT, Rob Buis
no flags
Archive of layout-test-results from ews126 for ios-simulator-wk2 (2.43 MB, application/zip)
2019-05-15 07:54 PDT, EWS Watchlist
no flags
Patch (175.15 KB, patch)
2019-05-15 11:50 PDT, Rob Buis
no flags
Archive of layout-test-results from ews213 for win-future (13.83 MB, application/zip)
2019-05-15 13:49 PDT, EWS Watchlist
no flags
Patch (173.81 KB, patch)
2019-05-15 23:54 PDT, Rob Buis
no flags
Archive of layout-test-results from ews210 for win-future (13.35 MB, application/zip)
2019-05-16 01:53 PDT, EWS Watchlist
no flags
Archive of layout-test-results from ews126 for ios-simulator-wk2 (2.58 MB, application/zip)
2019-05-16 02:00 PDT, EWS Watchlist
no flags
Patch (175.34 KB, patch)
2019-05-16 02:50 PDT, Rob Buis
no flags
Patch (177.05 KB, patch)
2019-05-16 12:10 PDT, Rob Buis
no flags
Archive of layout-test-results from ews126 for ios-simulator-wk2 (8.22 MB, application/zip)
2019-05-16 14:13 PDT, EWS Watchlist
no flags
Patch (177.89 KB, patch)
2019-05-17 01:09 PDT, Rob Buis
no flags
Patch (173.01 KB, patch)
2019-05-17 08:47 PDT, Rob Buis
no flags
Archive of layout-test-results from ews100 for mac-highsierra (3.08 MB, application/zip)
2019-05-17 10:01 PDT, EWS Watchlist
no flags
Patch (171.08 KB, patch)
2019-05-17 10:04 PDT, Rob Buis
no flags
Patch (173.81 KB, patch)
2019-05-17 11:12 PDT, Rob Buis
no flags
Patch (171.88 KB, patch)
2019-05-17 11:33 PDT, Rob Buis
no flags
Patch (180.99 KB, patch)
2019-05-31 01:51 PDT, Rob Buis
no flags
Patch (177.15 KB, patch)
2019-05-31 08:34 PDT, Rob Buis
no flags
Patch (174.97 KB, patch)
2019-05-31 10:59 PDT, Rob Buis
no flags
Patch (177.17 KB, patch)
2019-05-31 12:09 PDT, Rob Buis
no flags
Patch (176.83 KB, patch)
2019-06-02 05:54 PDT, Rob Buis
no flags
Archive of layout-test-results from ews102 for mac-highsierra (3.07 MB, application/zip)
2019-06-02 07:10 PDT, EWS Watchlist
no flags
Patch (176.83 KB, patch)
2019-06-02 07:44 PDT, Rob Buis
no flags
Archive of layout-test-results from ews104 for mac-highsierra-wk2 (2.79 MB, application/zip)
2019-06-02 09:08 PDT, EWS Watchlist
no flags
Patch (176.83 KB, patch)
2019-06-02 09:53 PDT, Rob Buis
no flags
Archive of layout-test-results from ews101 for mac-highsierra (3.09 MB, application/zip)
2019-06-02 11:09 PDT, EWS Watchlist
no flags
Patch (176.83 KB, patch)
2019-06-02 12:20 PDT, Rob Buis
no flags
Rob Buis
Comment 1 2019-03-27 08:38:59 PDT
EWS Watchlist
Comment 2 2019-03-27 10:47:43 PDT
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
EWS Watchlist
Comment 3 2019-03-27 10:47:45 PDT
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
Rob Buis
Comment 4 2019-03-29 09:43:30 PDT
EWS Watchlist
Comment 5 2019-03-29 10:55:09 PDT
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
EWS Watchlist
Comment 6 2019-03-29 10:55:11 PDT
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
EWS Watchlist
Comment 7 2019-03-29 11:10:18 PDT
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
EWS Watchlist
Comment 8 2019-03-29 11:10:20 PDT
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
EWS Watchlist
Comment 9 2019-03-29 11:38:48 PDT
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
EWS Watchlist
Comment 10 2019-03-29 11:38:50 PDT
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
Rob Buis
Comment 11 2019-03-29 15:04:31 PDT
Rob Buis
Comment 12 2019-04-02 07:52:45 PDT
(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.
Rob Buis
Comment 13 2019-04-18 03:38:57 PDT
EWS Watchlist
Comment 14 2019-04-18 05:42:28 PDT
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
EWS Watchlist
Comment 15 2019-04-18 05:42:30 PDT
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
Rob Buis
Comment 16 2019-05-09 08:26:43 PDT
Would it be better to have a runtime flag for this patch?
Rob Buis
Comment 17 2019-05-13 10:46:10 PDT
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.
youenn fablet
Comment 18 2019-05-13 13:55:32 PDT
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.
Rob Buis
Comment 19 2019-05-14 12:29:42 PDT
Rob Buis
Comment 20 2019-05-14 13:34:07 PDT
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.
Rob Buis
Comment 21 2019-05-14 13:39:36 PDT
(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
EWS Watchlist
Comment 22 2019-05-14 14:37:47 PDT
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
EWS Watchlist
Comment 23 2019-05-14 14:37:49 PDT
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
Frédéric Wang (:fredw)
Comment 24 2019-05-15 00:18:50 PDT
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
Rob Buis
Comment 25 2019-05-15 05:46:01 PDT
EWS Watchlist
Comment 26 2019-05-15 07:53:59 PDT
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
EWS Watchlist
Comment 27 2019-05-15 07:54:01 PDT
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
Rob Buis
Comment 28 2019-05-15 11:50:22 PDT
EWS Watchlist
Comment 29 2019-05-15 13:49:09 PDT
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
EWS Watchlist
Comment 30 2019-05-15 13:49:15 PDT
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
youenn fablet
Comment 31 2019-05-15 14:12:07 PDT
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?
Rob Buis
Comment 32 2019-05-15 23:54:12 PDT
EWS Watchlist
Comment 33 2019-05-16 01:53:14 PDT
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
EWS Watchlist
Comment 34 2019-05-16 01:53:16 PDT
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
EWS Watchlist
Comment 35 2019-05-16 02:00:51 PDT
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
EWS Watchlist
Comment 36 2019-05-16 02:00:53 PDT
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
Rob Buis
Comment 37 2019-05-16 02:50:24 PDT
Rob Buis
Comment 38 2019-05-16 12:10:38 PDT
EWS Watchlist
Comment 39 2019-05-16 14:13:28 PDT
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
EWS Watchlist
Comment 40 2019-05-16 14:13:30 PDT
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
Rob Buis
Comment 41 2019-05-17 01:09:51 PDT
Rob Buis
Comment 42 2019-05-17 08:47:18 PDT
EWS Watchlist
Comment 43 2019-05-17 10:01:04 PDT
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
EWS Watchlist
Comment 44 2019-05-17 10:01:06 PDT
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
Rob Buis
Comment 45 2019-05-17 10:04:30 PDT
Rob Buis
Comment 46 2019-05-17 11:12:59 PDT
Rob Buis
Comment 47 2019-05-17 11:33:19 PDT
Rob Buis
Comment 48 2019-05-17 13:05:35 PDT
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.
WebKit Commit Bot
Comment 49 2019-05-17 13:34:49 PDT
Comment on attachment 370130 [details] Patch Clearing flags on attachment: 370130 Committed r245475: <https://trac.webkit.org/changeset/245475>
WebKit Commit Bot
Comment 50 2019-05-17 13:34:51 PDT
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 51 2019-05-17 13:40:14 PDT
Truitt Savell
Comment 52 2019-05-21 09:58:08 PDT
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
Rob Buis
Comment 53 2019-05-21 10:15:49 PDT
(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.
Jon Lee
Comment 54 2019-05-28 13:52:24 PDT
(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?
Alexey Proskuryakov
Comment 55 2019-05-28 14:01:12 PDT
Let's roll back at this point. The test is still flaky, and yes, features need flags.
Shawn Roberts
Comment 56 2019-05-28 14:10:53 PDT
Reverted r245475 for reason: Newly imported test is flaky. Features need flags. Committed r245825: <https://trac.webkit.org/changeset/245825>
Rob Buis
Comment 57 2019-05-31 01:51:21 PDT
Rob Buis
Comment 58 2019-05-31 08:34:21 PDT
Rob Buis
Comment 59 2019-05-31 10:59:43 PDT
Rob Buis
Comment 60 2019-05-31 12:09:55 PDT
Rob Buis
Comment 61 2019-05-31 13:58:34 PDT
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.
youenn fablet
Comment 62 2019-05-31 15:34:52 PDT
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?
Rob Buis
Comment 63 2019-06-02 05:54:34 PDT
EWS Watchlist
Comment 64 2019-06-02 07:10:34 PDT
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
EWS Watchlist
Comment 65 2019-06-02 07:10:37 PDT
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
Rob Buis
Comment 66 2019-06-02 07:44:43 PDT
EWS Watchlist
Comment 67 2019-06-02 09:08:12 PDT
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
EWS Watchlist
Comment 68 2019-06-02 09:08:15 PDT
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
Rob Buis
Comment 69 2019-06-02 09:53:13 PDT
EWS Watchlist
Comment 70 2019-06-02 11:09:40 PDT
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
EWS Watchlist
Comment 71 2019-06-02 11:09:43 PDT
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
Rob Buis
Comment 72 2019-06-02 12:18:38 PDT
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.
Rob Buis
Comment 73 2019-06-02 12:20:01 PDT
WebKit Commit Bot
Comment 74 2019-06-03 13:50:00 PDT
Comment on attachment 371161 [details] Patch Clearing flags on attachment: 371161 Committed r246045: <https://trac.webkit.org/changeset/246045>
WebKit Commit Bot
Comment 75 2019-06-03 13:50:03 PDT
All reviewed patches have been landed. Closing bug.
youenn fablet
Comment 76 2019-06-03 13:50:20 PDT
> >> 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.
Rob Buis
Comment 77 2019-06-03 13:53:21 PDT
(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.
Rob Buis
Comment 78 2019-06-05 02:26:23 PDT
(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
Note You need to log in before you can comment on or make changes to this bug.