[Webkit-unassigned] [Bug 109021] [css3-text] Parsing each-line value for text-indent from css3-text

[bugzilla-daemon at webkit.org]

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


Julien Chaffraix <jchaffraix at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #191150|review?                     |review-
               Flag|                            |




--- Comment #11 from Julien Chaffraix <jchaffraix at webkit.org>  2013-03-04 17:28:40 PST ---
(From update of attachment 191150)
View in context: https://bugs.webkit.org/attachment.cgi?id=191150&action=review

This will need another round, preferably let's try to keep one code path, adding #if'defs around the new syntax.

> Source/WebCore/css/CSSParser.cpp:2177
> +#if ENABLE(CSS3_TEXT)
> +    case CSSPropertyTextIndent: // [ <length> | <percentage> ] && each-line? | inherit
> +        parsedValue = parseTextIndent();
> +        if (!parsedValue)
> +            return false;
> +        break;
> +#else
>      case CSSPropertyTextIndent:          // <length> | <percentage> | inherit
>          validPrimitive = (!id && validUnit(value, FLength | FPercent));
>          break;
> +#endif

This is not a good way of handling the new value. Ideally we want the code path to be as shared as possible instead of splitting the code. Here it's a lot easier to do:

case CSSPropertyTextIndent:
    parsedValue = parseTextIndent();
    break;

PassRefPtr<CSSValueList> CSSParser::parseTextIndent()
{
    CSSParserValue* value = m_valueList->current();
    if (!value->id || validUnit(value, FLength | FPercent))
        return createPrimitiveNumericValue(value);

#if ENABLE(CSS3_TEXT)
    // Handle the extended grammar, namely [ each-line && <length> | <percentage> ]
#else
    return 0;
#endif
}

> Source/WebCore/css/CSSParser.cpp:9162
> +    for (CSSParserValue* value = m_valueList->current(); value; value = m_valueList->next()) {

I don't think having a for loop for 2 values is really good. I would rather see 2 checks as:
* they really depend on the condition in which you parse (did I see CSSValueEachLine? A <length> | <percentage>?)
* you are allowing more than 2 values (see below)

> Source/WebCore/css/CSSParser.cpp:9165
> +        else if (!hasParsedTextIndent && !value->id && validUnit(value, FLength | FPercent)) {

Do you know if the !value->id is required? It seems like an overkill to me.

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

Your parsing allows the following:
* text-indent: each-line each-line;
* text-indent: each-line 10px each-line;

> Source/WebCore/css/CSSValueKeywords.in:1004
> +each-line

You are not prefixing text-indent (which is fine as it is an existing property). However this is new thus it should be prefixed.

> Source/WebCore/css/StyleBuilder.cpp:1819
> +        for (size_t i = 0; i < valueList->length(); i++) {

Again think about what you are parsing: you don't expect length() to be 15 so you should probably ASSERT it and have your code following the fact that we are probably get at most 2 values.

> Source/WebCore/css/StyleBuilder.cpp:1834
> +            else if (primitiveValue->isCalculatedPercentageWithLength())
> +                textIndent = Length(primitiveValue->cssCalcValue()->toCalcValue(styleResolver->style(), styleResolver->rootElementStyle(), styleResolver->style()->effectiveZoom()));

calc() is not allowed during parsing so I wouldn't expect it to show up here.

> LayoutTests/fast/css3-text/css3-text-indent/getComputedStyle/getComputedStyle-text-indent-expected.txt:27
> +

Missing some cases here or at least you should try different <length> values (how about viewport as you do have custom code for it?). Also how about <percentage>?

> LayoutTests/fast/css3-text/css3-text-indent/getComputedStyle/getComputedStyle-text-indent-expected.txt:39
> +PASS computedStyle.getPropertyValue('text-indent') is '0px'

Good test that you check invalid values but you should be a bit more extensive: how about 2 values? 3 values (including and not including each-line)? ...

> LayoutTests/fast/css3-text/css3-text-indent/getComputedStyle/getComputedStyle-text-indent-expected.txt:40
> +

How about inheritance which wasn't tested AFAICT?

> LayoutTests/platform/wincairo/TestExpectations:2989
> +fast/css3-text/css3-text-indent/getComputedStyle/getComputedStyle-text-indent.html

We should just skip the whole *directory* instead of touching every platforms everytime some new feature / test case is added.

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