[Webkit-unassigned] [Bug 109021] [css3-text] Parsing -webkit-each-line value for text-indent from css3-text
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Mar 8 07:53:08 PST 2013
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.
More information about the webkit-unassigned
mailing list