[webkit-reviews] review granted: [Bug 192950] Implement imagesrcset and imagesizes attributes on link rel=preload : [Attachment 371073] Patch

[bugzilla-daemon at webkit.org]

youenn fablet <youennf at gmail.com> has granted Rob Buis <rbuis at igalia.com>'s
request for review:
Bug 192950: Implement imagesrcset and imagesizes attributes on link rel=preload
https://bugs.webkit.org/show_bug.cgi?id=192950

Attachment 371073: Patch

https://bugs.webkit.org/attachment.cgi?id=371073&action=review




--- Comment #62 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 371073
  --> https://bugs.webkit.org/attachment.cgi?id=371073
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/prelo
ad/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?