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

• Component: CSS
• Status: RESOLVED INVALID

[Uday Kiran]

rewrite the background-position parsing code for two values in such a way as to later extend it to support three and four values.

[Uday Kiran]

Created attachment 142412 [details]
Patch

[Julien Chaffraix]

Comment on attachment 142412 [details]
Patch

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.

[Uday Kiran]

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

[Uday Kiran]

Comment on attachment 142412 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=142412&action=review

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

If you see a draft patch from bug 37514 (https://bugs.webkit.org/attachment.cgi?id=138085&action=review) which I had uploaded, it implements case values 3 and 4.
It is not much code since we need to consider only few cases as offset must be preceded by keyword.
1. keyword offset keyword offset for four values. 
2. keyword keyword offset, keyword offset keyword for three values.
We just need to validate this order. 
It is a little bit different with two values. '50% left' or 'top 50%' are invalid and hence case 2: code is little bigger.

One doubt about Vector<RefPtr<CSSPrimitiveValue> > values, is values.clear() call required?

[Alexis Menard (darktears)]

*** Bug 87673 has been marked as a duplicate of this bug. ***

[Uday Kiran]

Hi Julien,

I tried implementing background-position with 3-4 values with state machine logic like existing code but there are too many ambiguous states while parsing position values since possible ways of specifying position is much higher. We can see that by constructing state machine.
It becomes more complex as background-position, radial-gradient take maximum of 4 values and -webkit-mask-position, -transform-origin, -prespective-origin take only two values.

Implementing using switch case is much simpler and easier to read code.
If you think switch case logic doesn't cover some code path or if missing some tests, can you please elaborate? I will be glad to fix it.

Main concern here seems to be step 2 in comment #3, without that I am not sure how we can implement this feature in small patches as any modifications to existing parsing function needs testing for all properties, background-position, -webkit-mask-position, -transform-origin, -prespective-origin and -radial-gradient, making the patch larger.

[Alexis Menard (darktears)]

I'm closing it since the patch of https://bugs.webkit.org/show_bug.cgi?id=102104 implements the new feature on top of the old two values parsing code.