[Webkit-unassigned] [Bug 86703] [CSS3 Backgrounds and Borders] Refactor background-position parsing code for 2 values

[bugzilla-daemon at webkit.org]

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


Julien Chaffraix <jchaffraix at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #142412|review?, commit-queue?      |
               Flag|                            |




--- Comment #2 from Julien Chaffraix <jchaffraix at webkit.org>  2012-05-17 20:59:23 PST ---
(From update of attachment 142412)
View in context: https://bugs.webkit.org/attachment.cgi?id=142412&action=review

A global comment: it's really painful that you just forked the whole code here. Our current algorithm implements the CSS 2.1 version of it (see bug 47159). As CSS 3 should be backward compatible, there should be ways to share as much code as possible. If your refactoring is to duplicate the code in this way, I really doubt this is the right approach. It would be better to evaluate which blocks already exist, extract those that can be shared and try to build on top of them / extend them keeping the 2 code paths (old vs new grammars).

> Source/WebCore/ChangeLog:13
> +        Covered by existing test fast/backgrounds/background-position-parsing.html.

I am really not convinced the new code is equivalent even if you seem to be thinking it is.

> Source/WebCore/css/CSSParser.cpp:3267
> +static PassRefPtr<CSSPrimitiveValue> createBackgroundPosition(PassRefPtr<CSSPrimitiveValue> position)

This code already exists in parseFillPositionX, it also supported <length | percent>.

Note sure why I don't see the equivalent code for bottom / center / top.

> Source/WebCore/css/CSSParser.cpp:3302
> +        value = valueList->next();

You should just break if !value instead of NULL-checking all the way.

> Source/WebCore/css/CSSParser.cpp:3305
> +        if (value && value->unit == CSSParserValue::Operator && value->iValue == ',')

You can use isComma here.

> Source/WebCore/css/CSSParser.cpp:3312
> +    case 2:

I don't really understand why you picked up a switch - case. A very simple state machine (see the previous code) was a better design and for 4 values shouldn't be too complicated to implement.

Here you end up with some massive switch that will likely increase as you implement more of the syntax.

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