Bug 133620

Summary: Add support for HTMLImageElement's sizes attribute
Product: WebKit Reporter: Yoav Weiss <yoav>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, bunhere, cdumez, commit-queue, darin, dbates, dino, eric.carlson, esprehn+autocc, glenn, gyuyoung.kim, jer.noble, kling, kondapallykalyan, macpherson, menard, philipj, rakuco, rniwa, sergio, syoichi
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion
none
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion
none
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2
none
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2
none
Patch
none
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion
none
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion
none
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion
none
Patch
none
Patch
none
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion
none
Patch
none
Patch none

Description Yoav Weiss 2014-06-08 14:23:15 PDT
Add support for HTMlImageElement's sizes attribute
Comment 1 Yoav Weiss 2014-06-08 14:49:44 PDT
Created attachment 232688 [details]
Patch
Comment 2 Yoav Weiss 2014-06-08 14:56:07 PDT
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.
Comment 3 Yoav Weiss 2014-06-08 15:18:32 PDT
Comment on attachment 232688 [details]
Patch

Removing request for review, since the win bot fails to build.
Comment 4 Build Bot 2014-06-08 17:10:24 PDT
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
Comment 5 Build Bot 2014-06-08 17:10:30 PDT
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
Comment 6 Build Bot 2014-06-08 18:59:40 PDT
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
Comment 7 Build Bot 2014-06-08 18:59:45 PDT
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
Comment 8 Build Bot 2014-06-09 00:22:07 PDT
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
Comment 9 Build Bot 2014-06-09 00:22:12 PDT
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
Comment 10 Build Bot 2014-06-09 01:36:46 PDT
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
Comment 11 Build Bot 2014-06-09 01:36:53 PDT
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
Comment 12 Yoav Weiss 2014-06-09 11:56:42 PDT
Created attachment 232720 [details]
Patch
Comment 13 Yoav Weiss 2014-06-09 12:38:35 PDT
Created attachment 232722 [details]
Patch
Comment 14 Yoav Weiss 2014-06-09 13:01:33 PDT
Created attachment 232724 [details]
Patch
Comment 15 Yoav Weiss 2014-06-09 13:26:38 PDT
Created attachment 232729 [details]
Patch
Comment 16 Build Bot 2014-06-09 16:32:03 PDT
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
Comment 17 Build Bot 2014-06-09 16:32:09 PDT
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
Comment 18 Build Bot 2014-06-09 17:23:45 PDT
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
Comment 19 Build Bot 2014-06-09 17:23:52 PDT
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
Comment 20 Build Bot 2014-06-09 18:21:56 PDT
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
Comment 21 Build Bot 2014-06-09 18:22:03 PDT
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
Comment 22 Sam Weinig 2014-06-09 22:42:20 PDT
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.
Comment 23 Yoav Weiss 2014-06-10 00:40:51 PDT
Created attachment 232780 [details]
Patch
Comment 24 Yoav Weiss 2014-06-10 03:02:48 PDT
Thanks for reviewing :)
I've now fixed the review comments.
Comment 25 Yoav Weiss 2014-06-11 08:10:35 PDT
Created attachment 232864 [details]
Patch
Comment 26 Yoav Weiss 2014-06-11 08:11:20 PDT
Added missing calc() support
Comment 27 Build Bot 2014-06-11 11:26:57 PDT
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
Comment 28 Build Bot 2014-06-11 11:27:03 PDT
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
Comment 29 Yoav Weiss 2014-06-11 12:17:38 PDT
Created attachment 232884 [details]
Patch
Comment 30 Sam Weinig 2014-06-18 22:25:06 PDT
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.
Comment 31 Dean Jackson 2014-06-28 20:59:41 PDT
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 <yoav@yoav.ws>

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 <yoav@yoav.ws>

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.
Comment 32 Yoav Weiss 2014-06-29 15:04:16 PDT
Created attachment 234063 [details]
Patch
Comment 33 Yoav Weiss 2014-06-29 15:06:34 PDT
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.
Comment 34 WebKit Commit Bot 2014-06-29 20:32:37 PDT
Comment on attachment 234063 [details]
Patch

Rejecting attachment 234063 [details] from review queue.

yoav@yoav.ws 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.
Comment 35 WebKit Commit Bot 2014-06-29 21:07:36 PDT
Comment on attachment 234063 [details]
Patch

Clearing flags on attachment: 234063

Committed r170576: <http://trac.webkit.org/changeset/170576>
Comment 36 WebKit Commit Bot 2014-06-29 21:07:46 PDT
All reviewed patches have been landed.  Closing bug.
Comment 37 Darin Adler 2015-01-28 22:35:54 PST
This change caused leaks in the CSS grammar. The CSSGrammar.y.in part of this didn’t handle storage correctly.