[Webkit-unassigned] [Bug 171720] [preload] Add media and type attribute support.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=171720

--- Comment #19 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 310652
  --> https://bugs.webkit.org/attachment.cgi?id=310652
Patch

Looks almost good to me.
Some questions and suggestions below.
I am not sure about the tests, why they are skipped by default.
I have not checked yet how precisely they are covering the changes but they looked ok.

View in context: https://bugs.webkit.org/attachment.cgi?id=310652&action=review

> Source/WebCore/css/MediaQueryEvaluator.cpp:741
> +    return MediaQueryEvaluator { "screen", document, &document.renderView()->style() }.evaluate(mediaQueries.get());

Do you know why we are refcounting MediaQuerySet? Can we make it not ref countable?
It seems wasteful to allocate memory that we will free just afterwards.

> Source/WebCore/css/MediaQueryEvaluator.h:73
> +    static bool mediaAttributeMatches(Document&, const String& attributeValue);

space here

> Source/WebCore/html/HTMLImageElement.cpp:165
> +            if (!type.isEmpty() && !MIMETypeRegistry::isSupportedImagePrefixedMIMEType(type))

Why do we need !type.isEmpty() here?

> Source/WebCore/loader/LinkLoader.cpp:89
> +void LinkLoader::loadLinksFromHeader(const String& headerValue, const URL& baseURL, Document& document, PreloadLinkType preloadLinkType)

This parameter was not very clear to me at first.
A suggestion to make things clearer:
Might be clearer to have loadLinksFromHeader with the 3 current parameters and loadMediaLinksIfAny() with no parameters.
loadLinksFromHeader would store any useful media header set for loadMediaLinksIfAny

> Source/WebCore/loader/LinkLoader.cpp:171
> +{

Is there a spec defining which preloads we should do or is it browser dependent?

> Source/WebCore/platform/MIMETypeRegistry.cpp:488
> +    return isSupportedImageMIMEType(mimeType) || equalLettersIgnoringASCIICase(mimeType, "image/svg+xml");

Sounds good to have this method. Seems also like this could be used in HTMLImageElement::bestFitSourceFromPictureElement.
I am not sure about its name and how it is expressing the fact that it allows svg in addition to other image types.

> Source/WebCore/platform/MIMETypeRegistry.cpp:522
> +    return equalLettersIgnoringASCIICase(mimeType, "text/css");

There are other similar checks in the code base, maybe related to the Content-Type which may have other parameters though.
Might want to consider consolidate this as some point.

> LayoutTests/TestExpectations:38
> +http/tests/preload/viewport [ Skip ]

Why skipping the tests?

> LayoutTests/http/tests/preload/media-attribute.html:4
> +<script>

I guess it is too late but this would be great tests to do in LayoutTests/http/wpt and be upstreamed to WPT in the future.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20170519/a95e3ebd/attachment.html>