[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





--- Comment #3 from Uday Kiran <udaykiran at motorola.com>  2012-05-18 00:52:14 PST ---
(In reply to comment #2)
> (From update of attachment 142412 [details])
> 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).

Thank you for reviewing.

Considering that my patches should be small for easier review, I thought of doing this in following approach.

1. Existing parsing function parseFillPosition is shared by background-position, -webkit-mask-position, -transform-origin, -prespective-origin and -radial-gradient. So consider one property at time.
2. Change code path just for background-position without affecting other properties. Start with background-position for two values in a patch.
3. Extend background-position to four values in another bug.
4. Make other properties -webkit-mask-position, -transform-origin, -prespective-origin and -radial-gradient to use newly implemented parsing code as to do code sharing.
   Each property will be in a separate bug and tested one at time.
5. Remove the duplicate code after all properties are parsed using newly implemented function.

Rendering code for two values already exists and <position> property with four values can be reduced to two values. One special case for which rendering support needs to be added is when offset in pixels from right or bottom is specified which only is case when three or four values are specified.

The patch uploaded is step2.

I couldn't think of alternate approach where it could be done without touching multiple CSS properties at same time.

I had given a thought of implementing the parsing code as a state machine but I thought code readability by using switch case for four values would be simpler to read and easier to understand and relate to specification. 

Also -webkit-position, -transform-origin and -perspective-origin take only two values in position and not four. So it would just be one of cases of switch statement.
I doubt if code size with state machine approach for four values will be much smaller than switch. I will give it another try. :)

I will upload patch addressing review comments in patch if the above approach is fine.

> 
> > 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.

This test covers the rendering part.
Sorry, I should have mentioned other testcases where getComputedStyle is covered.
fast/css/getComputedStyle/getComputedStyle-background-position.html
fast/backgrounds/multiple-backgrounds-computed-style.html
and from CSS2.1 test suite
css2.1/t140201-c536-bgpos-00-b-ag.html
css3/calc/background-position-parsing.html which tests calc values in position.
I will add these also in Changelog. Am I missing any other test? I would be glad to write those tests.

If you think new parsing code is different from existing parsing code, can you please elaborate where I might have gone wrong?

> 
> > 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.

parseFillPositionX only handles values on X coordinate. equivalent code for bottom / center / top is in parseFillPositionY.
To use these function first we need to determine on which axis in position and then call parseFillPositionX/Y 
which again checks for identifier. I don't think that would be good.

> 
> > Source/WebCore/css/CSSParser.cpp:3302
> > +        value = valueList->next();
> 
> You should just break if !value instead of NULL-checking all the way.
> 
Fixed.

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

> > 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.

See global comment.

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