Add support for HTMlImageElement's sizes attribute
Created attachment 232688 [details] Patch
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 on attachment 232688 [details] Patch Removing request for review, since the win bot fails to build.
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
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 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
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 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
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 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
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
Created attachment 232720 [details] Patch
Created attachment 232722 [details] Patch
Created attachment 232724 [details] Patch
Created attachment 232729 [details] Patch
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
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 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
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 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
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 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.
Created attachment 232780 [details] Patch
Thanks for reviewing :) I've now fixed the review comments.
Created attachment 232864 [details] Patch
Added missing calc() support
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
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
Created attachment 232884 [details] Patch
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 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.
Created attachment 234063 [details] Patch
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 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 on attachment 234063 [details] Patch Clearing flags on attachment: 234063 Committed r170576: <http://trac.webkit.org/changeset/170576>
All reviewed patches have been landed. Closing bug.
This change caused leaks in the CSS grammar. The CSSGrammar.y.in part of this didn’t handle storage correctly.