[Webkit-unassigned] [Bug 72584] Support Image Level 3's image() notation; it offers automated image flipping for RTL

[bugzilla-daemon at webkit.org]

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


Alexey Proskuryakov <ap at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #120084|commit-queue?               |commit-queue-
               Flag|                            |




--- Comment #11 from Alexey Proskuryakov <ap at webkit.org>  2011-12-20 20:59:09 PST ---
(From update of attachment 120084)
View in context: https://bugs.webkit.org/attachment.cgi?id=120084&action=review

Commenting on a few things I couldn't help but notice when skimming over the patch. I did't not attempt to verify the logic, or even to comprehensively check the style.

> LayoutTests/fast/css/getComputedStyle/computed-style-image.html:27
> +shouldBe('testImage("background-image: -webkit-image(\'example1.png\' ltr)", "background-image")', '"-webkit-image(\'example1.png\' ltr)"')
> +shouldBe('testImage("background-image: -webkit-image(\'example1.png\'    ltr)", "background-image")', '"-webkit-image(\'example1.png\' ltr)"')

Given that you're testing one aspect of this already, it would seem useful to test no space between URL and "ltr", and then tabs and other whitespace characters.

Test that "-webkit-image" is case insensitive?

> Source/WebCore/css/CSSImageFallbackValue.cpp:7
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.

Is it intentional that you are adding new code under LGPL? Generally, we prefer BSD style on new files.

> Source/WebCore/css/CSSImageFallbackValue.cpp:21
> +#include "config.h"
> +
> +#include "CSSImageFallbackValue.h"

No space between these.

> Source/WebCore/css/CSSImageFallbackValue.cpp:32
> +    size_t nImages = this->m_images.size();

Please don't abbreviate.

> Source/WebCore/css/CSSImageFallbackValue.h:34
> +
> +
> +

Too much whitespace.

> Source/WebCore/css/CSSImageFallbackValue.h:40
> +        NONE,
> +        LTR,
> +        RTL

We only use ALL_CAPS for macros and abbreviations. So, the first value should be None.

> Source/WebCore/css/CSSImageFallbackValue.h:61
> +        , m_images(Vector<ImageFallbackPair>())
> +        , m_color(Color())

Please don't spell out initializers that just perform the default action.

> Source/WebCore/css/CSSImageFallbackValue.h:70
> +    String customCssText() const;

s/Css/CSS/.

> Source/WebCore/css/CSSImageFallbackValue.h:71
> +    PassRefPtr<Image> image(RenderObject*, const IntSize&);

If this function just finds an image (as opposed to creating it), raw pointer seems preferable.

> Source/WebCore/css/CSSParser.cpp:35
>  #include "CSSCrossfadeValue.h"
> +#include "CSSImageFallbackValue.h"
>  #include "CSSCursorImageValue.h"

Please maintain alphabetic order. I'm surprised that style bot isn't yelling.

> Source/WebCore/css/CSSParser.cpp:176
> +static inline bool isComma(const CSSParserValue* a)

Why "a"? I'd suggest "value".

> Source/WebCore/css/CSSParser.cpp:6325
> +    const CSSParserString& funcName = val->function->name;

s/funcName/functionName/

> Source/WebCore/css/CSSParser.cpp:6344
> +    const CSSParserString& fname = val->function->name;

s/fname/functionName/

> Source/WebCore/css/CSSParser.cpp:6427
> +    CSSParserValueList* args = valueList->current()->function->args.get();

s/args/arguments/.

> Source/WebCore/css/CSSParser.cpp:6433
> +    CSSParserValue* arg = args->current();

s/arg/argument/

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