[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





--- Comment #12 from Jaehun Lim <ljaehun.lim at samsung.com>  2013-03-06 18:32:01 PST ---
(From update of attachment 191150)
View in context: https://bugs.webkit.org/attachment.cgi?id=191150&action=review

>> Source/WebCore/css/CSSParser.cpp:2177
>> +#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
> }

parseTextIndent() returns 2 types. "return createPrimitiveNumericValue(value);" return CSSValue and "// Handle the extended grammar, namely [ each-line && <length> | <percentage> ]" returns CSSValueList.
There is no problem in parseTextIndent() because CSSValueList is CSSValue, but other part like StyleBuilder needs to check whether CSSValue is list or not.
I think it's better that parseTextIndent() returns CSSvalueList when CSS3_TEXT in enabled and returns CSSValue when CSS3_TEXT is disabled.
How about ?

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

I remove for loop and make 2 if statements

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

Fixed.

>> Source/WebCore/css/CSSParser.cpp:9170
>> +    }
> 
> Your parsing allows the following:
> * text-indent: each-line each-line;
> * text-indent: each-line 10px each-line;

Fixed.

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

Use -webkit-each-line and CSSValueWebkitEachLine.

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

Fixed.

>> Source/WebCore/css/StyleBuilder.cpp:1834
>> +                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.

Remove this line.

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

Add 20ex and <Percentage>. viewport values are tested another testcase, css3/viewport-percentage-lengths/css3-viewport-percentage-lengths-getStyle.html.

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

Add 2 and 3 values cases with/without -webkit-each-line.

>> LayoutTests/fast/css3-text/css3-text-indent/getComputedStyle/getComputedStyle-text-indent-expected.txt:40
>> +
> 
> How about inheritance which wasn't tested AFAICT?

Inheritance is tested another test case, getComputedStyle-text-indent-inherited.js

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

CSS3_TEXT is enabled only for EFL, GTK ports. So I should edit each platform TestExpectations file.
And some other CSS3_TEXT test cases are registered like that. is it wrong ?

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