[webkit-reviews] review denied: [Bug 102104] [CSS3 Backgrounds and Borders] Implement new CSS3 background-position parsing. : [Attachment 175030] Patch

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has denied Alexis Menard (darktears)
<alexis at webkit.org>'s request for review:
Bug 102104: [CSS3 Backgrounds and Borders] Implement new CSS3
background-position parsing.
https://bugs.webkit.org/show_bug.cgi?id=102104

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

------- Additional Comments from Julien Chaffraix <jchaffraix at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=175030&action=review


> Source/WebCore/css/CSSParser.cpp:3693
> +    bool swapNeeded = false;

Btw, you are assuming that this is a valid 3 values background-position. You
may want to add some ASSERTs here.

> Source/WebCore/css/CSSParser.cpp:3697
> +    if (isBackgroundPositionKeyword(ident2) || ident1 == CSSValueCenter) {

I would really separate those as it would make the code more readable and the
sharing is just one line.

> Source/WebCore/css/CSSParser.cpp:3698
> +	   if (parsedValue1->getIdent() == CSSValueCenter) {

ident1 == parsedValue1->getIndent()

> Source/WebCore/css/CSSParser.cpp:3724
> +}

You should probably ASSERT that value1 and value2 are set up there. They should
also contain the right value in order (value1 is left/right, value2 top/bottom)


> Source/WebCore/css/CSSParser.cpp:3729
> +    for (unsigned i = valueList->currentIndex() ; i < valueList->size();
++i, ++numberOfValues) {

Style violation: no space before ';'

> Source/WebCore/css/CSSParser.cpp:3746
> +    CSSParserValue* value = valueList->current();

I don't see the need for |value|. You only use it to do store valueList->next()
and don't seem to read it. If you do, you can use valueList->current() which
would be more readable.

> Source/WebCore/css/CSSParser.cpp:3778
> +    // If both value1 and value2 are not keywords, return, no further
processing for CSS3.

I would rephrase that as:

// Per CSS3 syntax, <position> requires one of the 2 values to be a background
keyword.

> Source/WebCore/css/CSSParser.cpp:3779
> +    if (!isBackgroundPositionKeyword(parsedValue1->getIdent()) &&
!isBackgroundPositionKeyword(parsedValue2->getIdent()))

I think this could be narrowed: we know we are parsing 3+ values (which
corresponds to	[ center | [ left | right ] [ <percentage> | <length> ]? ] &&
  [ center | [ top | bottom ] [ <percentage> | <length> ]? ]) so only the first
value must be a keyword, the second value can be either a keyword, a
<percentage> or a <length>.

> Source/WebCore/css/CSSParser.cpp:3789
> +    if (!isBackgroundPositionKeyword(parsedValue1->getIdent()) ||
parsedValue2->getIdent() == CSSValueCenter)

Per the above comment, the first part is already covered and can be removed.
You could also move the 'center' check above as you can't conflict if you are
invalid.

> Source/WebCore/css/CSSParser.cpp:3793
> +    if (isBackgroundPositionKeyword(parsedValue1->getIdent()) &&
isBackgroundPositionKeyword(parsedValue2->getIdent()) &&
isBackgroundPositionKeyword(valueList->current()->id))

Again per the above comment the first part is unneeded AFAICT.

> Source/WebCore/css/CSSParser.cpp:3816
> +	   parseFillPosition3Values(value1, value2, value3, parsedValue1,
parsedValue2);

You probably want to call release on your parsedValue as you are transferring
ownership.

> Source/WebCore/css/CSSParser.cpp:4059
> +#endif

Good to indicate the intend here:

// Fall through to CSS 2.1 parsing.

> Source/WebCore/css/CSSParser.h:115
> -    PassRefPtr<CSSValue> parseFillPositionComponent(CSSParserValueList*,
unsigned& cumulativeFlags, FillPositionFlag& individualFlag);
> +    PassRefPtr<CSSPrimitiveValue>
parseFillPositionComponent(CSSParserValueList*, unsigned& cumulativeFlags,
FillPositionFlag& individualFlag, bool useValueAsKeyword = false);

Please, no boolean parameters. You had to explicitly put the argument name to
make the caller readable which is working around the issue that it should be an
enum for readability.

> Source/WebCore/css/CSSParser.h:121
> +    void parseFillPosition3Values(RefPtr<CSSValue>&, RefPtr<CSSValue>&,
PassRefPtr<CSSPrimitiveValue>, PassRefPtr<CSSPrimitiveValue>,
PassRefPtr<CSSPrimitiveValue>);

This name could be improved. Alternatives:

* parse3ValueBackgroundPosition
* parseBackgroundPositionWith3Values

> LayoutTests/platform/chromium/TestExpectations:217
> +# CSS3 Background is not yet enabled (needs ENABLE_CSS3_BACKGROUND).
> +webkit.org/b/37514 fast/backgrounds/background-position-parsing-2.html

Not providing an expected value is equivalent to [ Skip ] but skipping a test
makes us lose more coverage as it is never run (ie you won't catch crashers). I
would advise you to use [ Text ] instead as the test will be run but the result
ignored. This applies to all ports.

> configure.ac:816
> +				[enable CSS3 background support [default=no]]),

> +		 [],[enable_css3_background="yes"])

You are putting the default to 'yes' here (which doesn't match the help).