[Webkit-unassigned] [Bug 109021] [css3-text] Support text-indent:each-line 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 #187774|review?                     |review-
               Flag|                            |




--- Comment #4 from Julien Chaffraix <jchaffraix at webkit.org>  2013-02-18 16:13:39 PST ---
(From update of attachment 187774)
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)

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