Bug 102104 – [CSS3 Backgrounds and Borders] Implement new CSS3 background-position parsing.

• Component: CSS
• Status: RESOLVED FIXED

[Alexis Menard (darktears)]

[CSS3 Backgrounds and Borders] Implement new CSS3 background-position parsing.

[Alexis Menard (darktears)]

Created attachment 173922 [details]
Patch

[Alexis Menard (darktears)]

Created attachment 173934 [details]
Patch

[Alexis Menard (darktears)]

Created attachment 174120 [details]
Fix GTK build

[kov's GTK+ EWS bot]

Comment on attachment 174120 [details]
Fix GTK build

Attachment 174120 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/14838294

[Alexis Menard (darktears)]

Created attachment 174171 [details]
Fix GTK build (2nd try)

[Kenneth Rohde Christiansen]

Comment on attachment 174120 [details]
Fix GTK build

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

> Source/WebCore/GNUmakefile.features.am:9
> +	ENABLE_CSS3_BACKGROUND_POSITION=1 \

why is it ok to enable it here for GTK etc

> LayoutTests/fast/backgrounds/background-position-parsing-2.html:368
> +//shouldBe("computedStyle.backgroundPosition", "'right 0% top 15px, right 20px bottom 0%'");

why are these commented out?

[Alexis Menard (darktears)]

(In reply to comment #6)
> (From update of attachment 174120 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=174120&action=review
> 
> > Source/WebCore/GNUmakefile.features.am:9
> > +	ENABLE_CSS3_BACKGROUND_POSITION=1 \
> 
> why is it ok to enable it here for GTK etc

Because I think it's a very nice feature, it ships on some other vendors for a while. And the spec is already a candidate recommendation. I really think it should be on by default. Right after this patch i'll propose the rendering making the feature complete.

> 
> > LayoutTests/fast/backgrounds/background-position-parsing-2.html:368
> > +//shouldBe("computedStyle.backgroundPosition", "'right 0% top 15px, right 20px bottom 0%'");
> 
> why are these commented out?

It's explained in the LayoutTests/Changelog. I wrote the tests thinking about the computed style also and I used Opera to double check the results. Now to get the computed style i need FillLayer to basically implement the new bits and pieces for the rendering of the feature. For the sake of keeping the review more simple i decided to split rendering and parsing. As I wrote the tests first before the implemetation, computed style checks included, i thought better to keep them. They will be uncommented in the next patch.

[Julien Chaffraix]

(In reply to comment #7)
> (In reply to comment #6)
> > (From update of attachment 174120 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=174120&action=review
> > 
> > > Source/WebCore/GNUmakefile.features.am:9
> > > +	ENABLE_CSS3_BACKGROUND_POSITION=1 \
> > 
> > why is it ok to enable it here for GTK etc
> 
> Because I think it's a very nice feature, it ships on some other vendors for a while. And the spec is already a candidate recommendation. I really think it should be on by default. Right after this patch i'll propose the rendering making the feature complete.

I strongly disagree with this statement on the basis that you are breaking feature detection. While all the pieces have not landed, you shouldn't make enabled the default.

Also enabling the feature on all ports requires some announcement as different ports have different rules and that hasn't been the case here.

[Alexis Menard (darktears)]

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > (From update of attachment 174120 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=174120&action=review
> > > 
> > > > Source/WebCore/GNUmakefile.features.am:9
> > > > +	ENABLE_CSS3_BACKGROUND_POSITION=1 \
> > > 
> > > why is it ok to enable it here for GTK etc
> > 
> > Because I think it's a very nice feature, it ships on some other vendors for a while. And the spec is already a candidate recommendation. I really think it should be on by default. Right after this patch i'll propose the rendering making the feature complete.
> 
> I strongly disagree with this statement on the basis that you are breaking feature detection. While all the pieces have not landed, you shouldn't make enabled the default.
> 
> Also enabling the feature on all ports requires some announcement as different ports have different rules and that hasn't been the case here.

Fair I will reupload the patch with the feature deactivated by default. I have no strong opinion here. I'll make an announcement and each port enable it if they want the feature.

Still I'd be happy to have feedback on the implementation of the feature.

[Julien Chaffraix]

Comment on attachment 174171 [details]
Fix GTK build (2nd try)

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

I haven't had time to digest the grammar change so it will have to wait a follow-up review.

> Source/WebCore/GNUmakefile.am:381
> +if !ENABLE_UNSTABLE_FEATURES
> +feature_defines_unstable += \
> +	ENABLE_CSP_NEXT=0 \
> +	ENABLE_CSS3_TEXT=0 \
> +	ENABLE_CSS_STICKY_POSITION=0 \
> +	ENABLE_LINK_PREFETCH=0 \
> +	ENABLE_MICRODATA=0 \
> +	ENABLE_MUTATION_OBSERVERS=0 \
> +	ENABLE_STYLE_SCOPED=0 \
> +	ENABLE_VIDEO_TRACK=0 \
> +	ENABLE_WEB_TIMING=0
> +endif

How is that related to this patch?

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

This looks really close to parseFillPositionX / parseFillPositionY, except that they convert 'left', 'top', 'right' and 'bottom' to percent values. Wouldn't it be possible to share some code with them?

> Source/WebCore/css/CSSParser.cpp:3682
> +static PassRefPtr<CSSPrimitiveValue> createPrimitiveValuePair(PassRefPtr<CSSPrimitiveValue> position, PassRefPtr<CSSPrimitiveValue> offset)
> +{
> +    return cssValuePool().createValue(Pair::create(position, offset));
> +}

This should be in a preparatory refactoring as there are some other call sites that would benefit from the function.

> Source/WebCore/css/CSSParser.cpp:3687
> +static bool valueIsKeyword(int value)
> +{
> +    return value == CSSValueInvalid ? false : true;
> +}

I am dubious about this function. First it could be written:

return value != CSSValueInvalid;

Also I find your naming confusing. You are checking that the value is a known keyword so it could be named isKnownKeyword(int)

It's also probably too broad as you should know what to expect (I am not sure accepting 'solid' would help for example)

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

Coding style violation: "Each statement should get its own line."

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

Ditto.

> Source/WebCore/css/CSSParser.cpp:3760
> +

Shouldn't you bail out here if propId is not CSSPropertyBackgroundPosition to avoid relaxing the parsing for other properties?

> Source/WebCore/css/CSSParser.cpp:3762
> +    value1 = value2 = 0;

Another coding style violation (I won't repeat it).

> Source/WebCore/css/CSSParser.cpp:3777
> +    if (value1Flag == XFillPosition && (parserValue2->id == CSSValueLeft || parserValue2->id == CSSValueRight))
> +        return;
> +
> +    if (value1Flag == YFillPosition && (parserValue2->id == CSSValueTop || parserValue2->id == CSSValueBottom))
> +        return;

Those checks should be moved to a helper function as you do them twice. It would be also easy to squash the 2 if's and make them right below the comment, thus increasing readability.

> Source/WebCore/css/CSSParser.cpp:3791
> +    // value3 is not a expected value, we return.

s/a /an /

> Source/WebCore/css/CSSParser.cpp:3841
> +        bool swapNeeded = false;
> +
> +        if (valueIsKeyword(parserValue2->id) || parserValue1->id == CSSValueCenter) {
> +            if (parserValue1->id == CSSValueCenter) {
> +                CSSValueID firstKeyword = CSSValueLeft;
> +                if (value2Flag == XFillPosition) {
> +                    firstKeyword = CSSValueTop;
> +                    swapNeeded = true;
> +                }
> +                value1 = createPrimitiveValuePair(cssValuePool().createIdentifierValue(firstKeyword), cssValuePool().createValue(50, CSSPrimitiveValue::CSS_PERCENTAGE));
> +            } else
> +                value1 = createPrimitiveValuePair(parsedFirstValue, cssValuePool().createValue(0, CSSPrimitiveValue::CSS_PERCENTAGE));
> +
> +            value2 = createPrimitiveValuePair(parsedSecondValue, value3);
> +        } else {
> +            value1 = createPrimitiveValuePair(parsedFirstValue, parsedSecondValue);
> +            if (value3Id == CSSValueCenter) {
> +                CSSValueID thirdKeyword = CSSValueTop;
> +                if (value1Flag != XFillPosition) {
> +                    thirdKeyword = CSSValueLeft;
> +                    swapNeeded = true;
> +                }
> +                value2 = createPrimitiveValuePair(cssValuePool().createIdentifierValue(thirdKeyword), cssValuePool().createValue(50, CSSPrimitiveValue::CSS_PERCENTAGE));
> +            } else
> +                value2 = createPrimitiveValuePair(value3, cssValuePool().createValue(0, CSSPrimitiveValue::CSS_PERCENTAGE));
> +        }
> +
> +        if (value1Flag == YFillPosition || swapNeeded)
> +            value1.swap(value2);

This should go a new method as it is fairly self-contained. It would also make the code more readable.

> Source/WebCore/css/CSSParser.h:119
> -    void parseFillPosition(CSSParserValueList*, RefPtr<CSSValue>&, RefPtr<CSSValue>&);
> +    void parseFillPosition(CSSParserValueList*, RefPtr<CSSValue>&, RefPtr<CSSValue>&, CSSPropertyID propId = CSSPropertyInvalid);

I don't think it's a good idea to let people call parseFillPosition without |propId|.

> Tools/Scripts/webkitperl/FeatureList.pm:179
> +    { option => "css3-background-position", desc => "Toggle CSS3 Background Position support",
> +      define => "ENABLE_CSS3_BACKGROUND_POSITION", default => 1, value => \$css3BackgroundPositionSupport },

High level comment: I really prefer a feature flag per spec rather than per property to avoid a feature flag hell. This is all the more true as there are other properties that will need to be updated if we are to support the spec fully.

[Alexis Menard (darktears)]

Created attachment 174956 [details]
Patch

[Alexis Menard (darktears)]

(In reply to comment #10)
> (From update of attachment 174171 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=174171&action=review
> 
> I haven't had time to digest the grammar change so it will have to wait a follow-up review.
> 
> > Source/WebCore/GNUmakefile.am:381
> > +if !ENABLE_UNSTABLE_FEATURES
> > +feature_defines_unstable += \
> > +	ENABLE_CSP_NEXT=0 \
> > +	ENABLE_CSS3_TEXT=0 \
> > +	ENABLE_CSS_STICKY_POSITION=0 \
> > +	ENABLE_LINK_PREFETCH=0 \
> > +	ENABLE_MICRODATA=0 \
> > +	ENABLE_MUTATION_OBSERVERS=0 \
> > +	ENABLE_STYLE_SCOPED=0 \
> > +	ENABLE_VIDEO_TRACK=0 \
> > +	ENABLE_WEB_TIMING=0
> > +endif
> 
> How is that related to this patch?

Fixed.

> 
> > Source/WebCore/css/CSSParser.cpp:3677
> > +}
> 
> This looks really close to parseFillPositionX / parseFillPositionY, except that they convert 'left', 'top', 'right' and 'bottom' to percent values. Wouldn't it be possible to share some code with them?
> 
> > Source/WebCore/css/CSSParser.cpp:3682
> > +static PassRefPtr<CSSPrimitiveValue> createPrimitiveValuePair(PassRefPtr<CSSPrimitiveValue> position, PassRefPtr<CSSPrimitiveValue> offset)
> > +{
> > +    return cssValuePool().createValue(Pair::create(position, offset));
> > +}
> 
> This should be in a preparatory refactoring as there are some other call sites that would benefit from the function.

Done and landed as a separate patch.

> 
> > Source/WebCore/css/CSSParser.cpp:3687
> > +static bool valueIsKeyword(int value)
> > +{
> > +    return value == CSSValueInvalid ? false : true;
> > +}
> 
> I am dubious about this function. First it could be written:
> 
> return value != CSSValueInvalid;
> 
> Also I find your naming confusing. You are checking that the value is a known keyword so it could be named isKnownKeyword(int)

I renamed it, and changed its purpose.

> 
> It's also probably too broad as you should know what to expect (I am not sure accepting 'solid' would help for example)
> 
> > Source/WebCore/css/CSSParser.cpp:3707
> > +    CSSParserValue* value = parserValue1 = valueList->current();
> 
> Coding style violation: "Each statement should get its own line."
> 

Fixed

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

Fixed.

> > Source/WebCore/css/CSSParser.cpp:3760
> > +
> 
> Shouldn't you bail out here if propId is not CSSPropertyBackgroundPosition to avoid relaxing the parsing for other properties?
> 

It's already the case, the for loop checking whether we have multiple keywords move the condition only for background-position.

> > Source/WebCore/css/CSSParser.cpp:3762
> > +    value1 = value2 = 0;
> 
> Another coding style violation (I won't repeat it).

Sorry :(. Fixed :) :)

> 
> > Source/WebCore/css/CSSParser.cpp:3777
> > +    if (value1Flag == XFillPosition && (parserValue2->id == CSSValueLeft || parserValue2->id == CSSValueRight))
> > +        return;
> > +
> > +    if (value1Flag == YFillPosition && (parserValue2->id == CSSValueTop || parserValue2->id == CSSValueBottom))
> > +        return;
> 
> Those checks should be moved to a helper function as you do them twice. It would be also easy to squash the 2 if's and make them right below the comment, thus increasing readability.

In a separate function now.

> 
> > Source/WebCore/css/CSSParser.cpp:3791
> > +    // value3 is not a expected value, we return.
> 
> s/a /an /
> 
> > Source/WebCore/css/CSSParser.cpp:3841
> > +        bool swapNeeded = false;
> > +
> > +        if (valueIsKeyword(parserValue2->id) || parserValue1->id == CSSValueCenter) {
> > +            if (parserValue1->id == CSSValueCenter) {
> > +                CSSValueID firstKeyword = CSSValueLeft;
> > +                if (value2Flag == XFillPosition) {
> > +                    firstKeyword = CSSValueTop;
> > +                    swapNeeded = true;
> > +                }
> > +                value1 = createPrimitiveValuePair(cssValuePool().createIdentifierValue(firstKeyword), cssValuePool().createValue(50, CSSPrimitiveValue::CSS_PERCENTAGE));
> > +            } else
> > +                value1 = createPrimitiveValuePair(parsedFirstValue, cssValuePool().createValue(0, CSSPrimitiveValue::CSS_PERCENTAGE));
> > +
> > +            value2 = createPrimitiveValuePair(parsedSecondValue, value3);
> > +        } else {
> > +            value1 = createPrimitiveValuePair(parsedFirstValue, parsedSecondValue);
> > +            if (value3Id == CSSValueCenter) {
> > +                CSSValueID thirdKeyword = CSSValueTop;
> > +                if (value1Flag != XFillPosition) {
> > +                    thirdKeyword = CSSValueLeft;
> > +                    swapNeeded = true;
> > +                }
> > +                value2 = createPrimitiveValuePair(cssValuePool().createIdentifierValue(thirdKeyword), cssValuePool().createValue(50, CSSPrimitiveValue::CSS_PERCENTAGE));
> > +            } else
> > +                value2 = createPrimitiveValuePair(value3, cssValuePool().createValue(0, CSSPrimitiveValue::CSS_PERCENTAGE));
> > +        }
> > +
> > +        if (value1Flag == YFillPosition || swapNeeded)
> > +            value1.swap(value2);
> 
> This should go a new method as it is fairly self-contained. It would also make the code more readable.

In a method.

> 
> > Source/WebCore/css/CSSParser.h:119
> > -    void parseFillPosition(CSSParserValueList*, RefPtr<CSSValue>&, RefPtr<CSSValue>&);
> > +    void parseFillPosition(CSSParserValueList*, RefPtr<CSSValue>&, RefPtr<CSSValue>&, CSSPropertyID propId = CSSPropertyInvalid);
> 
> I don't think it's a good idea to let people call parseFillPosition without |propId|.

Well there are some call sites where with the state of today it's hard to get which propId this function is called and now easy way to get it (e.g. parseRadialGradient). It would require a bigger refactoring (that I could do in a separate patch for sure). It's a compromise I found.

> 
> > Tools/Scripts/webkitperl/FeatureList.pm:179
> > +    { option => "css3-background-position", desc => "Toggle CSS3 Background Position support",
> > +      define => "ENABLE_CSS3_BACKGROUND_POSITION", default => 1, value => \$css3BackgroundPositionSupport },
> 
> High level comment: I really prefer a feature flag per spec rather than per property to avoid a feature flag hell. This is all the more true as there are other properties that will need to be updated if we are to support the spec fully.

Fixed.

[Alexis Menard (darktears)]

Created attachment 174994 [details]
Patch

[Julien Chaffraix]

Comment on attachment 174994 [details]
Patch

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

After talking about this on IRC, I was misguided as other properties won't use the new syntax until CSS level 4 and it's not even guaranteed. That said, it's probably better to have a dedicated function for background-position, that falls back to the (legacy) 2 values parsing. It would curb the complexity.

I have digested the grammar and I can't find a flaw for now. Nice testing btw!

> Source/WebCore/ChangeLog:18
> +        Opera is already implementing this feature.

Opera has already implemented this feature (as they already have it, they are not in the process of adding support for it).

> Source/WebCore/css/CSSParser.cpp:3647
> +        if (useValueAsKeyword)

This variable name didn't really speak to me. I spend some time thinking about an alternative proposition (avoidKeywordResolution) but it doesn't seem any better.

> Source/WebCore/css/CSSParser.cpp:3728
> +    if (propId == CSSPropertyBackgroundPosition && !inShorthand()) {

I would totally push the inShorthand() check to the caller.

> Source/WebCore/css/CSSParser.cpp:3796
> +

>> Shouldn't you bail out here if propId is not CSSPropertyBackgroundPosition to avoid relaxing the parsing for other properties?

> It's already the case, the for loop checking whether we have multiple keywords move the condition only for background-position.

OK, please add an ASSERT here. The legacy behavior should never reach here.

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

I usually advise people not to use boolean parameters as they make the code less readable.

> Source/WebCore/css/CSSParser.h:118
> +    void parseFillPosition(CSSParserValueList*, RefPtr<CSSValue>&, RefPtr<CSSValue>&, CSSPropertyID propId = CSSPropertyInvalid);

Probably cleaner would be to add an enum to opt into the new behavior as you use propId for that:

enum ParsingRuleUsed {
    UseCSS21ParsingRule,
    UseCSS3ParsingRule
};
void parseFillPosition(CSSParserValueList*, RefPtr<CSSValue>&, RefPtr<CSSValue>&, ParsingRuleUsed = UseCSS21ParsingRule);

That would give your intent a lot more.

> LayoutTests/fast/backgrounds/background-position-parsing-2.html:245
> +// Bunch of invalid new CSS3 values

Worth dumping that in the output (using debug("")) as you are expecting that the value doesn't change starting here.

> LayoutTests/fast/backgrounds/background-position-parsing-2.html:302
> +style.backgroundPosition = "top 0px bottom";

I would add a test for "left 0px right" as I couldn't see a 3 values left / right test.

> LayoutTests/fast/backgrounds/background-position-parsing-2.html:346
> +// With the comma.

Again, better to dump that in the test output!

> LayoutTests/fast/backgrounds/background-position-parsing-2.html:369
> +// Some invalid combinations.

Ditto.

[Alexis Menard (darktears)]

Created attachment 175015 [details]
Patch

[Alexis Menard (darktears)]

(In reply to comment #14)
> (From update of attachment 174994 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=174994&action=review
> 
> After talking about this on IRC, I was misguided as other properties won't use the new syntax until CSS level 4 and it's not even guaranteed. That said, it's probably better to have a dedicated function for background-position, that falls back to the (legacy) 2 values parsing. It would curb the complexity.

In the last patch I kept the old function as before and only add new code when we encounter more than two values.

> 
> I have digested the grammar and I can't find a flaw for now. Nice testing btw!

Thanks.

> 
> > Source/WebCore/ChangeLog:18
> > +        Opera is already implementing this feature.
> 
> Opera has already implemented this feature (as they already have it, they are not in the process of adding support for it).

Fixed

> 
> > Source/WebCore/css/CSSParser.cpp:3647
> > +        if (useValueAsKeyword)
> 
> This variable name didn't really speak to me. I spend some time thinking about an alternative proposition (avoidKeywordResolution) but it doesn't seem any better.

me neither.

> 
> > Source/WebCore/css/CSSParser.cpp:3728
> > +    if (propId == CSSPropertyBackgroundPosition && !inShorthand()) {
> 
> I would totally push the inShorthand() check to the caller.

Done.

> 
> > Source/WebCore/css/CSSParser.cpp:3796
> > +
> 
> >> Shouldn't you bail out here if propId is not CSSPropertyBackgroundPosition to avoid relaxing the parsing for other properties?
> 
> > It's already the case, the for loop checking whether we have multiple keywords move the condition only for background-position.
> 
> OK, please add an ASSERT here. The legacy behavior should never reach here.
> 
> > Source/WebCore/css/CSSParser.h:115
> > +    PassRefPtr<CSSPrimitiveValue> parseFillPositionComponent(CSSParserValueList*, unsigned& cumulativeFlags, FillPositionFlag& individualFlag, bool useValueAsKeyword = false);
> 
> I usually advise people not to use boolean parameters as they make the code less readable.
> 
> > Source/WebCore/css/CSSParser.h:118
> > +    void parseFillPosition(CSSParserValueList*, RefPtr<CSSValue>&, RefPtr<CSSValue>&, CSSPropertyID propId = CSSPropertyInvalid);
> 
> Probably cleaner would be to add an enum to opt into the new behavior as you use propId for that:
> 
> enum ParsingRuleUsed {
>     UseCSS21ParsingRule,
>     UseCSS3ParsingRule
> };
> void parseFillPosition(CSSParserValueList*, RefPtr<CSSValue>&, RefPtr<CSSValue>&, ParsingRuleUsed = UseCSS21ParsingRule);
> 
> That would give your intent a lot more.
> 
> > LayoutTests/fast/backgrounds/background-position-parsing-2.html:245
> > +// Bunch of invalid new CSS3 values
> 
> Worth dumping that in the output (using debug("")) as you are expecting that the value doesn't change starting here.

Good idea, done.

> 
> > LayoutTests/fast/backgrounds/background-position-parsing-2.html:302
> > +style.backgroundPosition = "top 0px bottom";
> 
> I would add a test for "left 0px right" as I couldn't see a 3 values left / right test.

Added.

> 
> > LayoutTests/fast/backgrounds/background-position-parsing-2.html:346
> > +// With the comma.
> 
> Again, better to dump that in the test output!
> 
> > LayoutTests/fast/backgrounds/background-position-parsing-2.html:369
> > +// Some invalid combinations.
> 
> Ditto.

[Alexis Menard (darktears)]

Created attachment 175030 [details]
Patch

[Alexis Menard (darktears)]

(In reply to comment #17)
> Created an attachment (id=175030) [details]
> Patch

It should apply now.

[Julien Chaffraix]

Comment on attachment 175030 [details]
Patch

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

[Alexis Menard (darktears)]

Created attachment 175706 [details]
Patch

[Julien Chaffraix]

Comment on attachment 175706 [details]
Patch

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

> Source/WebCore/css/CSSParser.cpp:3694
> +{

I would put what you are parsing here as it is simple:

// [ left | right ] [ <percentage] | <length> ] && [ top | bottom ] [ <percentage> | <length> ]

(Note that this underlines the assumptions you make in this function: that parseValue1 is left | right | top | bottom but not center (for which you have already checked before)).

> Source/WebCore/css/CSSParser.cpp:3740
> +    ASSERT(parsedValue1);
> +    ASSERT(parsedValue2);

Nit: those ASSERTs are not super useful and could be probably dropped.

> Source/WebCore/css/CSSParser.cpp:3791
> +        // At this point the second value should be a keyword but from on opposite edge of the first.
> +        if (isValueConflictingWithCurrentEdge(ident1, ident2))
> +            return;
> +
> +        // At this point the third value should be a keyword but from on opposite edge of the first.
> +        if (isValueConflictingWithCurrentEdge(ident1, ident3))
> +            return;

This is quite contradictory as all 3 values can't be keywords (as checked below). It would probably be clearer just to isolate the keywords and values, that would help match the syntax in a better way.

CSSValueID firstPositionKeyword = ident1;
CSSValueID secondPositionKeyword;
RefPtr<CSSPrimitiveValue> firstPositionValue;
RefPtr<CSSPrimitiveValue> secondPositionValue;
if (isBackgroundPositionKeyword(ident2)) {
    // We should only accept to match CSS grammar: [ center | left | right | bottom | top ] [ left | right | top | bottom ] [ <percentage> | <length> ]
    ASSERT(ident2 != CSSValueCenter);
    if (!validUnit(value3, FLength | FPercent))
        return;

    secondPositionValue = value3;
    secondPositionKeyword = ident2;
    firstPositionValue = cssValuePool().createValue(0, CSSPrimitiveValue::CSS_PERCENTAGE));
} else {
    // Per CSS, we should only accept: [ right | left | top | bottom ] [ <percentage> | <length> ] [ center | left | right | bottom | top ] ]
    if (!isBackgroundPositionKeyword(ident3))
        return;

    ...
}

if (isValueConflictingWithCurrentEdge(firstKeyword, secondKeyword))
    return;
...

I am not totally satisfied with this code though (variable naming, the comments could be improved...). Also I used validUnit which may be an overkill (though probably more readable than parseBackgroundFillComponent & !isBackgroundPositionKeyword).

> Source/WebCore/css/CSSParser.cpp:3880
> +        parse4ValuesBackgroundPosition(valueList, value1, value2, parsedValue1.release(), parsedValue2.release());

AFAICT this code would accept:

center 0px left 20%;

[Alexis Menard (darktears)]

Created attachment 175778 [details]
Patch

[Julien Chaffraix]

Comment on attachment 175778 [details]
Patch

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

[Alexis Menard (darktears)]

Created attachment 175833 [details]
Patch for landing

[Alexis Menard (darktears)]

(In reply to comment #24)
> Created an attachment (id=175833) [details]
> Patch for landing

Damn webkit-patch.

[Alexis Menard (darktears)]

Created attachment 175834 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 175834 [details]
Patch for landing

Clearing flags on attachment: 175834

Committed r135632: <http://trac.webkit.org/changeset/135632>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.