Bug 113259 - [css3-text] Rendering -webkit-each-line value for text-indent from css3-text
Summary: [css3-text] Rendering -webkit-each-line value for text-indent from css3-text
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 112755
  Show dependency treegraph
 
Reported: 2013-03-25 18:20 PDT by Jaehun Lim
Modified: 2013-04-02 23:12 PDT (History)
11 users (show)

See Also:


Attachments
Patch (23.20 KB, patch)
2013-03-25 19:16 PDT, Jaehun Lim
no flags Details | Formatted Diff | Diff
Patch (23.26 KB, patch)
2013-03-25 20:10 PDT, Jaehun Lim
no flags Details | Formatted Diff | Diff
Patch (24.73 KB, patch)
2013-03-27 20:43 PDT, Jaehun Lim
no flags Details | Formatted Diff | Diff
Patch (24.72 KB, patch)
2013-04-02 17:21 PDT, Jaehun Lim
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jaehun Lim 2013-03-25 18:20:23 PDT
This is the rendering part for "-webkit-each-line" value from CSS3 text.
Comment 1 Jaehun Lim 2013-03-25 19:16:00 PDT
Created attachment 194979 [details]
Patch
Comment 2 Build Bot 2013-03-25 19:51:03 PDT
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
Comment 3 Jaehun Lim 2013-03-25 20:10:27 PDT
Created attachment 194989 [details]
Patch
Comment 4 Julien Chaffraix 2013-03-27 14:36:05 PDT
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?
Comment 5 Levi Weintraub 2013-03-27 17:11:48 PDT
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).
Comment 6 Jaehun Lim 2013-03-27 20:40:23 PDT
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.
Comment 7 Jaehun Lim 2013-03-27 20:43:17 PDT
Created attachment 195475 [details]
Patch
Comment 8 Levi Weintraub 2013-04-02 15:27:21 PDT
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.
Comment 9 Jaehun Lim 2013-04-02 17:17:58 PDT
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.
Comment 10 Jaehun Lim 2013-04-02 17:21:20 PDT
Created attachment 196258 [details]
Patch
Comment 11 Levi Weintraub 2013-04-02 17:44:53 PDT
Comment on attachment 196258 [details]
Patch

Okay.
Comment 12 Jaehun Lim 2013-04-02 17:46:43 PDT
(In reply to comment #11)
> (From update of attachment 196258 [details])
> Okay.

Thanks, Levi Weintraub.
Comment 13 WebKit Review Bot 2013-04-02 18:30:48 PDT
Comment on attachment 196258 [details]
Patch

Clearing flags on attachment: 196258

Committed r147513: <http://trac.webkit.org/changeset/147513>
Comment 14 WebKit Review Bot 2013-04-02 18:30:53 PDT
All reviewed patches have been landed.  Closing bug.