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

[bugzilla-daemon at webkit.org]

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





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

Thanks for your comments.

>> Source/WebCore/ChangeLog:17
>> +        It is implemented under ENABLE_CSS3_TEXT with "-webkit" prefix.
> 
> '"-webkit" prefix' is what is usually called 'prefixed' (see all the threads on prefixing / unprefixing on the internets).

Fixed.

>> Source/WebCore/css/CSSParser.cpp:2188
>> +            return false;
> 
> This is unneeded and will be handled after the switch.

if, return statements are removed.

>> Source/WebCore/css/CSSParser.cpp:9193
>> +            return list;
> 
> You need to call release on your RefPtr to convert it to a PassRefPtr. While this is semantically equivalent, it actually touches m_refCount twice (create a new PassRefPtr (+1) and destroying the RefPtr (-1)) instead of just transferring m_refCount untouched. See http://www.webkit.org/coding/RefPtr.html as a good intro on RefPtr / PassRefPtr.

Oops. I fixed.

>> Source/WebCore/css/CSSParser.cpp:9230
>> +#endif
> 
> As mentioned, forking the code is not nice as now we have code duplication and different code being stressed differently based on CSS3_TEXT.
> 
> Here you can easily parse the first value, store it into a variable and for the CSS3_TEXT case handle the extra grammar. A way to achieve that is to do:
> 
> PassRefPtr<CSSPrimitiveValue> textIndentLength = 0;
> PassRefPtr<CSSPrimitiveValue> eachLine = 0;
> if (!parseEachLine(textIndendLength, eachLine))
>    return 0;
> #if ENABLE(CSS3_TEXT)
> if (!parseEachLine(textIndendLength, eachLine))
>    return 0;
> 
> // We shouldn't accept more than 2 arguments.
> if (m_value->current)
>    return false;
> #endif
> return createPrimitiveValuePair(textIndendLength, eachLine);
> 
> with parseEachLine (you can find a better naming) taking the PassRefPtr as reference so that you can fill them and check if one was already set (to handle the duplicated case).

As I said on irc, Pair is not proper when text-indent has only one value.
I used CSSValueList, Please review again.

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