[Webkit-unassigned] [Bug 113259] [css3-text] Rendering -webkit-each-line value for text-indent from css3-text

[bugzilla-daemon at webkit.org]

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





--- Comment #4 from Julien Chaffraix <jchaffraix at webkit.org>  2013-03-27 14:36:05 PST ---
(From update of attachment 194989)
View in context: https://bugs.webkit.org/attachment.cgi?id=194989&action=review

The change looks OK to me but some layout guru (looking at you levi!) should definitely do a sanity check.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:106
> +        , m_needsIndent(needsIndent)

I am really not a huge fan of the name |needsIndent| (and again one more boolean arguments). I think shouldIndentText (or equivalent) would be better. Also I would advise to use an enum as we are touching this code anyway

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:951
> +static bool requiresIndent(bool firstLine, bool forcedLineBreak, RenderStyle* style)

Per our coding style, boolean should start with a verb (the style mandates the use of "is" or "did" but really starting with a verb is the intent -> http://www.webkit.org/coding/coding-style.html)

While we are touching this code, I would prefer if we could fix some of that or at least not add more violations.

> LayoutTests/platform/win/TestExpectations:2492
> +fast/css3-text/css3-text-indent/each-line.html

Why only windows and not mac / chromium / qt / gtk? Can we just skip the whole css3-text-indent directory?

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