[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 #20 from Yoav Weiss <yoav at yoav.ws> ---
(In reply to youenn fablet from comment #19)
> Comment on attachment 310652 [details]
> 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.

Only the viewport tests are skipped by default, because they rely on `<meta name=viewport>` which I failed to get working on non-ios builds.

> 
> 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.

We can by making MediaQuerySet's constructor public. This same pattern is used in many other places in the code.

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

Will add one

> 
> > Source/WebCore/html/HTMLImageElement.cpp:165
> > +            if (!type.isEmpty() && !MIMETypeRegistry::isSupportedImagePrefixedMIMEType(type))
> 
> Why do we need !type.isEmpty() here?

Good question. The spec doesn't seem to mandate it.
Avoiding it would mean that for `<picture><source srcset="foo" type><img src="bar"></picture>` "bar" would get picked rather than the current "foo".

If we want to change that behavior, I could do that as a separate patch.

> 
> > 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

Fair enough. Will do

> 
> > Source/WebCore/loader/LinkLoader.cpp:171
> > +{
> 
> Is there a spec defining which preloads we should do or is it browser
> dependent?

The supported mime types we should preload are not defined and are 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.

Would `isSupportedImageOrSVGMimeType` be better?

> 
> > 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.

OK.

> 
> > LayoutTests/TestExpectations:38
> > +http/tests/preload/viewport [ Skip ]
> 
> Why skipping the tests?

The viewport tests are skipped for most platforms and enabled for ios and ios-simluator, as I failed to get the viewport reacting to the `meta name=viewport` instructions elsewhere.

> 
> > 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.

I can move those tests there

-- 
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/20170520/fe66f441/attachment-0001.html>