Bug 60044

Like!

Created attachment 92096 [details] First patch Removing some unused arguments that leak references to these boolean values to external code.

Comment on attachment 92096 [details] First patch View in context: https://bugs.webkit.org/attachment.cgi?id=92096&action=review > Source/WebCore/rendering/RenderBlock.h:492 > + static bool requiresLineBox(const InlineIterator&, bool isLineEmpty = true, bool previousLineBrokeCleanly = true); I take it this is called other places with isLineEmpty and previousLineBrokeCleanly? This patch is just removing some unused plumbing?

(In reply to comment #3) > (From update of attachment 92096 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=92096&action=review > > > Source/WebCore/rendering/RenderBlock.h:492 > > + static bool requiresLineBox(const InlineIterator&, bool isLineEmpty = true, bool previousLineBrokeCleanly = true); > > I take it this is called other places with isLineEmpty and previousLineBrokeCleanly? This patch is just removing some unused plumbing? Yes, this patch only removes unused arguments from generatesLineBoxesForInlineChild.

Created attachment 92117 [details] Patch

Comment on attachment 92117 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=92117&action=review LGTM. > Source/WebCore/rendering/RenderBlockLineLayout.cpp:80 > + void setPreviousLineBrokeCleanly(bool previousLineBrokeCleanly) { m_previousLineBrokeCleanly = previousLineBrokeCleanly; } I used to think we normally put a \n before private. But since thsi is the 3rd time I've made ethis comment today, I think my sense may be off. :)

(In reply to comment #6) > (From update of attachment 92117 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=92117&action=review > > LGTM. > > > Source/WebCore/rendering/RenderBlockLineLayout.cpp:80 > > + void setPreviousLineBrokeCleanly(bool previousLineBrokeCleanly) { m_previousLineBrokeCleanly = previousLineBrokeCleanly; } > > I used to think we normally put a \n before private. But since thsi is the 3rd time I've made ethis comment today, I think my sense may be off. :) Thanks for the reviews!

Committed r85652: <http://trac.webkit.org/changeset/85652>

Comment on attachment 92117 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=92117&action=review > Source/WebCore/rendering/RenderBlock.h:46 > +class LineInfo; Nit: you should declare this right above LineWidth. > Source/WebCore/rendering/RenderBlockLineLayout.cpp:69 > + : m_isFirstLine(true) > + , m_isLastLine(false) > + , m_isEmpty(true) > + , m_previousLineBrokeCleanly(true) I feel like some of these member variables being true by default is arbitrary. Can we add an enum flags that the constructor takes? > Source/WebCore/rendering/RenderBlockLineLayout.cpp:1445 > -bool RenderBlock::requiresLineBox(const InlineIterator& it, bool isLineEmpty, bool previousLineBrokeCleanly) > +static bool requiresLineBox(const InlineIterator& it, const LineInfo& lineInfo = LineInfo()) Nice! > Source/WebCore/rendering/RenderBlockLineLayout.cpp:1458 > + && !skipNonBreakingSpace(it, lineInfo); Nit: You need one more indentation here. > Source/WebCore/rendering/RenderBlockLineLayout.cpp:1725 > + bool startingNewParagraph = lineInfo.previousLineBrokeCleanly(); Maybe previousLineBrokeCleanly is a bad name for the member function?

Oops, I cleared eric's r+.

Comment on attachment 92117 [details] Patch Clearing review flag to remove this patch from the review queue since it was already committed as per comment 8.