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

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Jaehun Lim]

This is the rendering part for "-webkit-each-line" value from CSS3 text.

[Jaehun Lim]

Created attachment 194979 [details]
Patch

[Build Bot]

Comment on attachment 194979 [details]
Patch

Attachment 194979 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17328163

[Jaehun Lim]

Created attachment 194989 [details]
Patch

[Julien Chaffraix]

Comment on attachment 194989 [details]
Patch

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?

[Levi Weintraub]

Comment on attachment 194989 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=194989&action=review

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

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

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:983
> +    bool forcedLineBreak = lineBox->prevRootBox() && lineBox->prevRootBox()->endsWithBreak();

We usually call this a hardLineBreak (see BidiResolver.h for instance).

[Jaehun Lim]

Comment on attachment 194989 [details]
Patch

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.

[Jaehun Lim]

Created attachment 195475 [details]
Patch

[Levi Weintraub]

Comment on attachment 195475 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=195475&action=review

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:987
> +    bool isHardLineBreak = lineBox->prevRootBox() && lineBox->prevRootBox()->endsWithBreak();

This isn't exactly right either... this line box isn't a hard line break, it's *after* a hard line break.

[Jaehun Lim]

Comment on attachment 195475 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=195475&action=review

Thanks.

>> Source/WebCore/rendering/RenderBlockLineLayout.cpp:987
>> +    bool isHardLineBreak = lineBox->prevRootBox() && lineBox->prevRootBox()->endsWithBreak();
> 
> This isn't exactly right either... this line box isn't a hard line break, it's *after* a hard line break.

Fixed. Use isAfterHardLineBreak.

[Jaehun Lim]

Created attachment 196258 [details]
Patch

[Levi Weintraub]

Comment on attachment 196258 [details]
Patch

Okay.

[Jaehun Lim]

(In reply to comment #11)
> (From update of attachment 196258 [details])
> Okay.

Thanks, Levi Weintraub.

[WebKit Review Bot]

Comment on attachment 196258 [details]
Patch

Clearing flags on attachment: 196258

Committed r147513: <http://trac.webkit.org/changeset/147513>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

The new test is failing everywhere except Chromium:
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40ToT%20-%20webkit.org&tests=fast%2Fcss3-text%2Fcss3-text-indent%2Ftext-indent-each-line.html

[Jaehun Lim]

(In reply to comment #15)
> The new test is failing everywhere except Chromium:
> http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40ToT%20-%20webkit.org&tests=fast%2Fcss3-text%2Fcss3-text-indent%2Ftext-indent-each-line.html

I filed a bug. https://bugs.webkit.org/show_bug.cgi?id=113848