[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


Julien Chaffraix <jchaffraix at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #191931|1                           |0
        is obsolete|                            |




--- Comment #16 from Julien Chaffraix <jchaffraix at webkit.org>  2013-03-08 07:55:32 PST ---
(From update of attachment 191931)
View in context: https://bugs.webkit.org/attachment.cgi?id=191931&action=review

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

> Source/WebCore/css/CSSParser.cpp:2188
> +        if (!parsedValue)
> +            return false;

This is unneeded and will be handled after the switch.

> Source/WebCore/css/CSSParser.cpp:9184
> +    // [ <length> | <percentage> ] && -webkit-each-line? | inherit
> +    RefPtr<CSSValueList> list = CSSValueList::createSpaceSeparated();

To answer your question, you are trying to keep the existing behavior of returning a numeric value here if !ENABLE(CSS3_TEXT). I think it's artificial as this representation is not exposed and you can do the massaging when we return it in CSSComputedStyleDeclaration.

Furthermore, you expect at most 2 values so you could actually use a Pair to get the values.

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

> Source/WebCore/css/CSSParser.cpp:9215
> +        list->append(cssValuePool().createIdentifierValue(CSSValueWebkitEachLine));
> +        list->append(createPrimitiveNumericValue(secondValue));

I would keep the ordering in which you have the values constant as it would simplify the StyleBuilder code (you can ASSERT it to catch offenders).

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

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