[webkit-reviews] review granted: [Bug 133620] Add support for HTMLImageElement's sizes attribute : [Attachment 232884] Patch

[bugzilla-daemon at webkit.org]

Dean Jackson <dino at apple.com> has granted Yoav Weiss <yoav at yoav.ws>'s request
for review:
Bug 133620: Add support for HTMLImageElement's sizes attribute
https://bugs.webkit.org/show_bug.cgi?id=133620

Attachment 232884: Patch
https://bugs.webkit.org/attachment.cgi?id=232884&action=review

------- Additional Comments from Dean Jackson <dino at apple.com>
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 at 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 at 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.