This is the rendering part for "-webkit-each-line" value from CSS3 text.
Created attachment 194979 [details] Patch
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
Created attachment 194989 [details] Patch
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 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 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.
Created attachment 195475 [details] Patch
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 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.
Created attachment 196258 [details] Patch
Comment on attachment 196258 [details] Patch Okay.
(In reply to comment #11) > (From update of attachment 196258 [details]) > Okay. Thanks, Levi Weintraub.
Comment on attachment 196258 [details] Patch Clearing flags on attachment: 196258 Committed r147513: <http://trac.webkit.org/changeset/147513>
All reviewed patches have been landed. Closing bug.
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
(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