Bug 192950 - Implement imagesrcset and imagesizes attributes on link rel=preload
Summary: Implement imagesrcset and imagesizes attributes on link rel=preload
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: Safari Technology Preview
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Rob Buis
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-12-20 12:46 PST by Dominic Farolino
Modified: 2019-06-05 02:26 PDT (History)
21 users (show)

See Also:


Attachments
Patch (190.96 KB, patch)
2019-03-27 08:38 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews122 for ios-simulator-wk2 (2.59 MB, application/zip)
2019-03-27 10:47 PDT, Build Bot
no flags Details
Patch (192.19 KB, patch)
2019-03-29 09:43 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-highsierra (2.44 MB, application/zip)
2019-03-29 10:55 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews104 for mac-highsierra-wk2 (2.73 MB, application/zip)
2019-03-29 11:10 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews112 for mac-highsierra (2.26 MB, application/zip)
2019-03-29 11:38 PDT, Build Bot
no flags Details
Patch (192.19 KB, patch)
2019-03-29 15:04 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (150.63 KB, patch)
2019-04-18 03:38 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews126 for ios-simulator-wk2 (2.64 MB, application/zip)
2019-04-18 05:42 PDT, Build Bot
no flags Details
Patch (152.81 KB, patch)
2019-05-14 12:29 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews122 for ios-simulator-wk2 (8.08 MB, application/zip)
2019-05-14 14:37 PDT, Build Bot
no flags Details
Patch (173.62 KB, patch)
2019-05-15 05:46 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews126 for ios-simulator-wk2 (2.43 MB, application/zip)
2019-05-15 07:54 PDT, Build Bot
no flags Details
Patch (175.15 KB, patch)
2019-05-15 11:50 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews213 for win-future (13.83 MB, application/zip)
2019-05-15 13:49 PDT, Build Bot
no flags Details
Patch (173.81 KB, patch)
2019-05-15 23:54 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews210 for win-future (13.35 MB, application/zip)
2019-05-16 01:53 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews126 for ios-simulator-wk2 (2.58 MB, application/zip)
2019-05-16 02:00 PDT, Build Bot
no flags Details
Patch (175.34 KB, patch)
2019-05-16 02:50 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (177.05 KB, patch)
2019-05-16 12:10 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews126 for ios-simulator-wk2 (8.22 MB, application/zip)
2019-05-16 14:13 PDT, Build Bot
no flags Details
Patch (177.89 KB, patch)
2019-05-17 01:09 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (173.01 KB, patch)
2019-05-17 08:47 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-highsierra (3.08 MB, application/zip)
2019-05-17 10:01 PDT, Build Bot
no flags Details
Patch (171.08 KB, patch)
2019-05-17 10:04 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (173.81 KB, patch)
2019-05-17 11:12 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (171.88 KB, patch)
2019-05-17 11:33 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (180.99 KB, patch)
2019-05-31 01:51 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (177.15 KB, patch)
2019-05-31 08:34 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (174.97 KB, patch)
2019-05-31 10:59 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (177.17 KB, patch)
2019-05-31 12:09 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (176.83 KB, patch)
2019-06-02 05:54 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-highsierra (3.07 MB, application/zip)
2019-06-02 07:10 PDT, Build Bot
no flags Details
Patch (176.83 KB, patch)
2019-06-02 07:44 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews104 for mac-highsierra-wk2 (2.79 MB, application/zip)
2019-06-02 09:08 PDT, Build Bot
no flags Details
Patch (176.83 KB, patch)
2019-06-02 09:53 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-highsierra (3.09 MB, application/zip)
2019-06-02 11:09 PDT, Build Bot
no flags Details
Patch (176.83 KB, patch)
2019-06-02 12:20 PDT, Rob Buis
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dominic Farolino 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))
Comment 1 Rob Buis 2019-03-27 08:38:59 PDT
Created attachment 366072 [details]
Patch
Comment 2 Build Bot 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
Comment 3 Build Bot 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
Comment 4 Rob Buis 2019-03-29 09:43:30 PDT
Created attachment 366276 [details]
Patch
Comment 5 Build Bot 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
Comment 6 Build Bot 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
Comment 7 Build Bot 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
Comment 8 Build Bot 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
Comment 9 Build Bot 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
Comment 10 Build Bot 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
Comment 11 Rob Buis 2019-03-29 15:04:31 PDT
Created attachment 366308 [details]
Patch
Comment 12 Rob Buis 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.
Comment 13 Rob Buis 2019-04-18 03:38:57 PDT
Created attachment 367724 [details]
Patch
Comment 14 Build Bot 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
Comment 15 Build Bot 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
Comment 16 Rob Buis 2019-05-09 08:26:43 PDT
Would it be better to have a runtime flag for this patch?
Comment 17 Rob Buis 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.
Comment 18 youenn fablet 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.
Comment 19 Rob Buis 2019-05-14 12:29:42 PDT
Created attachment 369880 [details]
Patch
Comment 20 Rob Buis 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.
Comment 21 Rob Buis 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
Comment 22 Build Bot 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
Comment 23 Build Bot 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
Comment 24 Frédéric Wang (:fredw) 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
Comment 25 Rob Buis 2019-05-15 05:46:01 PDT
Created attachment 369947 [details]
Patch
Comment 26 Build Bot 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
Comment 27 Build Bot 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
Comment 28 Rob Buis 2019-05-15 11:50:22 PDT
Created attachment 369976 [details]
Patch
Comment 29 Build Bot 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
Comment 30 Build Bot 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
Comment 31 youenn fablet 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?
Comment 32 Rob Buis 2019-05-15 23:54:12 PDT
Created attachment 370026 [details]
Patch
Comment 33 Build Bot 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
Comment 34 Build Bot 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
Comment 35 Build Bot 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
Comment 36 Build Bot 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
Comment 37 Rob Buis 2019-05-16 02:50:24 PDT
Created attachment 370034 [details]
Patch
Comment 38 Rob Buis 2019-05-16 12:10:38 PDT
Created attachment 370053 [details]
Patch
Comment 39 Build Bot 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
Comment 40 Build Bot 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
Comment 41 Rob Buis 2019-05-17 01:09:51 PDT
Created attachment 370109 [details]
Patch
Comment 42 Rob Buis 2019-05-17 08:47:18 PDT
Created attachment 370119 [details]
Patch
Comment 43 Build Bot 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
Comment 44 Build Bot 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
Comment 45 Rob Buis 2019-05-17 10:04:30 PDT
Created attachment 370124 [details]
Patch
Comment 46 Rob Buis 2019-05-17 11:12:59 PDT
Created attachment 370127 [details]
Patch
Comment 47 Rob Buis 2019-05-17 11:33:19 PDT
Created attachment 370130 [details]
Patch
Comment 48 Rob Buis 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.
Comment 49 WebKit Commit Bot 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>
Comment 50 WebKit Commit Bot 2019-05-17 13:34:51 PDT
All reviewed patches have been landed.  Closing bug.
Comment 51 Radar WebKit Bug Importer 2019-05-17 13:40:14 PDT
<rdar://problem/50904042>
Comment 52 Truitt Savell 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
Comment 53 Rob Buis 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.
Comment 54 Jon Lee 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?
Comment 55 Alexey Proskuryakov 2019-05-28 14:01:12 PDT
Let's roll back at this point. The test is still flaky, and yes, features need flags.
Comment 56 Shawn Roberts 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>
Comment 57 Rob Buis 2019-05-31 01:51:21 PDT
Created attachment 371044 [details]
Patch
Comment 58 Rob Buis 2019-05-31 08:34:21 PDT
Created attachment 371059 [details]
Patch
Comment 59 Rob Buis 2019-05-31 10:59:43 PDT
Created attachment 371070 [details]
Patch
Comment 60 Rob Buis 2019-05-31 12:09:55 PDT
Created attachment 371073 [details]
Patch
Comment 61 Rob Buis 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.
Comment 62 youenn fablet 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?
Comment 63 Rob Buis 2019-06-02 05:54:34 PDT
Created attachment 371145 [details]
Patch
Comment 64 Build Bot 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
Comment 65 Build Bot 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
Comment 66 Rob Buis 2019-06-02 07:44:43 PDT
Created attachment 371149 [details]
Patch
Comment 67 Build Bot 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
Comment 68 Build Bot 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
Comment 69 Rob Buis 2019-06-02 09:53:13 PDT
Created attachment 371157 [details]
Patch
Comment 70 Build Bot 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
Comment 71 Build Bot 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
Comment 72 Rob Buis 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.
Comment 73 Rob Buis 2019-06-02 12:20:01 PDT
Created attachment 371161 [details]
Patch
Comment 74 WebKit Commit Bot 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>
Comment 75 WebKit Commit Bot 2019-06-03 13:50:03 PDT
All reviewed patches have been landed.  Closing bug.
Comment 76 youenn fablet 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.
Comment 77 Rob Buis 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.
Comment 78 Rob Buis 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