[webkit-reviews] review denied: [Bug 109021] [css3-text] Support text-indent:each-line from css3-text : [Attachment 187774] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Feb 18 16:11:17 PST 2013


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

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

------- Additional Comments from Julien Chaffraix <jchaffraix at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=187774&action=review


The change should be guarded by ENABLE(CSS3_TEXT).

As mentioned on IRC, the testing is inadequate and I would rather see the style
parsing, application and getComputedStyle part done first (with good testing)
*before* attacking the rendering. Good testing mean that you do validate that
we can set the style and get it back from getComputedStyle, including the wrong
cases (that obviously shouldn't update our style) and 'inherit'. Obviously if
this is already covered by existing tests, you should mention it in the
ChangeLog and only add the missing pieces.

> Source/WebCore/css/CSSParser.cpp:2103
> +	   // [ <length> | <percentage> ] && [ hanging || each-line ]?

This line is wrong and it confused me. You are adding parsing for a subset of
that and this should reflect what we *do* support. Also inherit is supported
and should still be.

> Source/WebCore/css/CSSParser.cpp:9019
> +    bool haveTextIndent = false;
> +    bool haveEachLine = false;

have (first it should be 'has') is a bit too generic here. Also I don't see why
you seperate text-indent from each-line as each-line *is part of text-indent*.
How about:

hasSeenFixedOrPercentage, hasParsedFixedPercentage,
hasEncounteredFixedOrPercentage

(slight preference for hasParsedTextIndent)


More information about the webkit-reviews mailing list