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

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has granted 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 175778: Patch
https://bugs.webkit.org/attachment.cgi?id=175778&action=review

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


> Source/WebCore/ChangeLog:36
> +	   * css/CSSParser.h:

It would be nice to fill those entries.

> Source/WebCore/css/CSSParser.cpp:3793
> +	       // We should only accept to match CSS grammar: [ center | left |
right | bottom | top ] [ left | right | top | bottom ] [ <percentage> |
<length> ]

This comment is one of the stuff that could have been improved upon:

// To match CSS grammar, we should only accept: ...

> Source/WebCore/css/CSSParser.cpp:3803
> +	       // Per CSS, we should only accept: [ right | left | top | bottom
] [ <percentage> | <length> ] [ center | left | right | bottom | top ] ]

There is an extra ']' at the end.

> Source/WebCore/css/CSSParser.cpp:3854
> +    // Parse the first value. We're just making sure that it is one of the
valid keywords or a percentage/length.

This comment is wrong: the first value has to be a keyword. You could probably
simplify more by moving the isBackgroundPositionKeyword check below here and
not call parseFillPositionComponent AFAICT.

> Source/WebCore/css/CSSParser.cpp:3865
> +    // In case we are parsing more than two values, relax the check inside
of parseFillPositionComponent. top 20px is
> +    // a valid start for a four values case.

This comment is stale too as top 20px is also a valid start for a 3 values
case.

> Source/WebCore/css/CSSParser.cpp:3878
> +    // Let's reset the return values.

Unneeded comment.

> Source/WebCore/css/CSSParser.cpp:3882
> +    // Per CSS3 syntax, <position> can't have the second keyword being
'center'.

'being' sounds wrong here (though I am a non-native English speaker so I could
be wrong). How about:

// Per CSS3 syntax, <position> can't have 'center' as its second keyword as we
have more arguments to follow.

(Adding a bit on the 'why' side).