[Webkit-unassigned] [Bug 133620] Add support for HTMLImageElement's sizes attribute

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=133620


Dean Jackson <dino at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #232884|review?                     |review+
               Flag|                            |




--- Comment #31 from Dean Jackson <dino at apple.com>  2014-06-28 20:59:41 PST ---
(From update of attachment 232884)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.