Bug 133620 – Add support for HTMLImageElement's sizes attribute

• Component: New Bugs
• Status: RESOLVED FIXED

[Yoav Weiss]

Add support for HTMlImageElement's sizes attribute

[Yoav Weiss]

Created attachment 232688 [details]
Patch

[Yoav Weiss]

This patch adds support for HTMLImageElement's sizes attribute and the related srcset extended syntax as defined in http://picture.responsiveimages.org/.
The sizes attribute syntax is added to the CSSGrammar and parsed by the CSSParser. 
The SourceSizeList class is generated by the parser, and used to get
the final source size.
HTMLImageElement and HTMLPreloadScanner send this value to HTMLSrcsetParser.
HTMLSrcsetParser uses this value in order to pick the right resource.

All these changes are behind a compile flag. I'm uploading this patch with the flag turned on by default, in order to make sure that the layout tests pass on the bots. Once they do, I intend to turn it off by default and add the test directories as skipped in TestExpectations.

[Yoav Weiss]

Comment on attachment 232688 [details]
Patch

Removing request for review, since the win bot fails to build.

[Build Bot]

Comment on attachment 232688 [details]
Patch

Attachment 232688 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4679770176487424

New failing tests:
http/tests/loading/sizes/preload-image-sizes-2x.html
fast/dom/HTMLImageElement/sizes/image-sizes-js-change.html

[Build Bot]

Created attachment 232692 [details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-01  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 232688 [details]
Patch

Attachment 232688 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6580826317783040

New failing tests:
http/tests/loading/sizes/preload-image-sizes-2x.html
fast/dom/HTMLImageElement/sizes/image-sizes-js-change.html

[Build Bot]

Created attachment 232693 [details]
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-02  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 232688 [details]
Patch

Attachment 232688 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4763885768802304

New failing tests:
http/tests/loading/sizes/preload-image-sizes-2x.html
fast/dom/HTMLImageElement/sizes/image-sizes-js-change.html

[Build Bot]

Created attachment 232697 [details]
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-15  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 232688 [details]
Patch

Attachment 232688 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4628900583833600

New failing tests:
media/W3C/video/networkState/networkState_during_loadstart.html
http/tests/loading/sizes/preload-image-sizes-2x.html
fast/dom/HTMLImageElement/sizes/image-sizes-js-change.html

[Build Bot]

Created attachment 232698 [details]
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-10  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Yoav Weiss]

Created attachment 232720 [details]
Patch

[Yoav Weiss]

Created attachment 232722 [details]
Patch

[Yoav Weiss]

Created attachment 232724 [details]
Patch

[Yoav Weiss]

Created attachment 232729 [details]
Patch

[Build Bot]

Comment on attachment 232729 [details]
Patch

Attachment 232729 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6529262282604544

New failing tests:
fast/dom/HTMLImageElement/sizes/image-sizes-js-change.html

[Build Bot]

Created attachment 232746 [details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-01  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 232729 [details]
Patch

Attachment 232729 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5980215138320384

New failing tests:
fast/dom/HTMLImageElement/sizes/image-sizes-js-change.html

[Build Bot]

Created attachment 232751 [details]
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-02  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 232729 [details]
Patch

Attachment 232729 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5483966832312320

New failing tests:
fast/dom/HTMLImageElement/sizes/image-sizes-js-change.html

[Build Bot]

Created attachment 232754 [details]
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-08  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Sam Weinig]

Comment on attachment 232729 [details]
Patch

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

> Source/WebCore/css/MediaList.cpp:126
> -    
> +

This change seems extraneous.

> Source/WebCore/css/MediaList.h:55
> -    
> +

The changes to this file seem extraneous.

> Source/WebCore/css/SourceSizeList.h:37
> +    SourceSize(MediaQueryExp* mediaExp, const CSSParserValue& length)

This should take a std::unique_ptr<MediaQueryExp> and move it into m_mediaExp.

> Source/WebCore/css/SourceSizeList.h:53
> +    void append(SourceSize* sourceSize)

This should take a std::unique_ptr<SourceSize> and move it onto the Vector.

> Source/WebCore/css/SourceSizeList.h:62
> +    Vector<std::unique_ptr<SourceSize> > m_list;

No need for the space between > and >

> Source/WebCore/html/HTMLImageElement.cpp:130
> +#if ENABLE_PICTURE_SIZES

We do ENABLE testing using the form #if ENABLE(PICTURE_SIZES). This is repeated a bunch.

> Source/WebCore/html/parser/HTMLSrcsetParser.cpp:235
> +            candidate.density = (float)candidate.resourceWidth / (float)sourceSize;

Please use c++ style casts.

[Yoav Weiss]

Created attachment 232780 [details]
Patch

[Yoav Weiss]

Thanks for reviewing :)
I've now fixed the review comments.

[Yoav Weiss]

Created attachment 232864 [details]
Patch

[Yoav Weiss]

Added missing calc() support

[Build Bot]

Comment on attachment 232864 [details]
Patch

Attachment 232864 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5602100713095168

New failing tests:
fast/dom/HTMLImageElement/sizes/image-sizes-1x.html
http/tests/loading/sizes/preload-image-sizes.html

[Build Bot]

Created attachment 232876 [details]
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-06  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Yoav Weiss]

Created attachment 232884 [details]
Patch

[Sam Weinig]

Comment on attachment 232884 [details]
Patch

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

> Source/WebCore/css/SourceSizeList.cpp:42
> +    expList->append(m_mediaExp.release());
> +    m_mediaExp = nullptr;

Is this necessary, given that you just called release() on the m_mediaExp I would expect it to be null.

> Source/WebCore/css/SourceSizeList.cpp:46
> +    mediaQuerySet->addMediaQuery(std::make_unique<MediaQuery>(mediaQuery));

This seems odd. Why did you create MediaQuery on the stack, just to copy it into this unique_ptr.

[Dean Jackson]

Comment on attachment 232884 [details]
Patch

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

We should make a new image for image-set-4x.png. It's confusing that it looks like 2x.

> Source/WebCore/ChangeLog:28
> +        This patch adds support for HTMLImageElement's sizes attribute and the
> +        related srcset extended syntax as defined in
> +        http://picture.responsiveimages.org/.
> +        This sizes attribute syntax is added to the CSSGrammar and parsed by
> +        the CSSParser.
> +        The SourceSizeList class is generated by the parser, and used to get
> +        the final source size.
> +        HTMLImageElement and HTMLPreloadScanner send this value to
> +        HTMLSrcsetParser.
> +        HTMLSrcsetParser uses this value in order to pick the right resource.
> +
> +        * CMakeLists.txt:
> +        * Configurations/FeatureDefines.xcconfig:
> +        * WebCore.vcxproj/WebCore.vcxproj:

I really like per-file information :|

> Source/WebCore/css/CSSGrammar.y.in:85
> +#if ENABLE_PICTURE_SIZES
> +%expect 34
> +#else

Here's a good example of why per-file comments would be nice: I have no idea what that means! :)

> Source/WebCore/css/SourceSizeList.cpp:2
> + * Copyright (C) 2013 Yoav Weiss <[e-mail]>

2014

> Source/WebCore/css/SourceSizeList.cpp:93
> +    std::unique_ptr<SourceSizeList> sourceSizeList = parser.parseSizesAttribute(sizesAttribute);
> +    if (!sourceSizeList.get())

Just: if (!sourceSizeList)

> Source/WebCore/css/SourceSizeList.h:2
> + * Copyright (C) 2013 Yoav Weiss <[e-mail]>

2014

> Source/WebCore/css/SourceSizeList.h:29
> +#if ENABLE(PICTURE_SIZES)

Might as well put that earlier in the file.

> Source/WebCore/html/HTMLImageElement.cpp:149
> +#if ENABLE(PICTURE_SIZES)
> +            , SourceSizeList::parseSizesAttribute(fastGetAttribute(sizesAttr), document().renderView(), document().frame())
> +#endif
> +            );

Nit: Too much indent on that last line (or too little in HTMLDocumentParser.cpp)

> Source/WebCore/html/HTMLImageElement.cpp:352
> +#if ENABLE(PICTURE_SIZES)
> +const AtomicString& HTMLImageElement::currentSrc() const
> +{
> +    return m_currentSrc;
> +}
> +#endif

Maybe this could go in the .h?

> Source/WebCore/html/parser/HTMLDocumentParser.cpp:427
> -                m_preloadScanner->scan(m_preloader.get(), document()->baseElementURL());
> +                m_preloadScanner->scan(m_preloader.get(), document()->baseElementURL()
> +#if ENABLE(PICTURE_SIZES)
> +                    , document()->renderView(), document()->frame()
> +#endif
> +            );

Nit: Indentation is wrong on last line.

> Source/WebCore/html/parser/HTMLDocumentParser.cpp:569
> +    m_preloadScanner->scan(m_preloader.get(), document()->baseElementURL()
> +#if ENABLE(PICTURE_SIZES)
> +        , document()->renderView(), document()->frame()
> +#endif
> +        );

Nit: Another inconsistency in indentation.

[Yoav Weiss]

Created attachment 234063 [details]
Patch

[Yoav Weiss]

Thanks for reviewing! :)
I've fixed the review comments, added more info about the changes in the ChangeLog, and disabled the compile flag by default.

[WebKit Commit Bot]

Comment on attachment 234063 [details]
Patch

Rejecting attachment 234063 [details] from review queue.

[e-mail] does not have reviewer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/contributors.json.

- If you do not have reviewer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have reviewer rights please correct the error in Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your reviewer rights.

[WebKit Commit Bot]

Comment on attachment 234063 [details]
Patch

Clearing flags on attachment: 234063

Committed r170576: <http://trac.webkit.org/changeset/170576>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

This change caused leaks in the CSS grammar. The CSSGrammar.y.in part of this didn’t handle storage correctly.