[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 #6 from Jaehun Lim <ljaehun.lim at samsung.com>  2013-03-27 20:40:23 PST ---
(From update of attachment 194989)
View in context: https://bugs.webkit.org/attachment.cgi?id=194989&action=review

Thank you for your review, Julien Chaffraix and Levi Weintraub.

>>> 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
> 
> Yessir!

Add enum IndentTextOrNot and use bool shouldIndentText.

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

Use m_shouldIndentText and shouldIndentText().

>> Source/WebCore/rendering/RenderBlockLineLayout.cpp:983
>> +    bool forcedLineBreak = lineBox->prevRootBox() && lineBox->prevRootBox()->endsWithBreak();
> 
> We usually call this a hardLineBreak (see BidiResolver.h for instance).

Use isHardLineBreak.

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

Yes, other ports except windows skip the whole directory, fast/css3-text/css3-text-indent/.
I change this line to skip 'css3-text-indent' directory as other ports.

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