[webkit-reviews] review denied: [Bug 109021] [css3-text] Parsing each-line value for text-indent from css3-text : [Attachment 191150] Patch

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has denied Jaehun Lim
<ljaehun.lim at samsung.com>'s request for review:
Bug 109021: [css3-text] Parsing each-line value for text-indent from css3-text
https://bugs.webkit.org/show_bug.cgi?id=109021

Attachment 191150: Patch
https://bugs.webkit.org/attachment.cgi?id=191150&action=review

------- Additional Comments from Julien Chaffraix <jchaffraix at webkit.org>
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-t
ext-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-t
ext-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-t
ext-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.