Bug 60044 – Extract LineInfo class

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Levi Weintraub]

isFirstLine, isLastLine, isEmpty, and previousLineBrokeCleanly are identifiers used repeatedly through RenderBlock/RenderBlockLineLayout to track identifying line information. This could be packaged into a class that represents relevant information about a line.

[Ryosuke Niwa]

Like!

[Levi Weintraub]

Created attachment 92096 [details]
First patch

Removing some unused arguments that leak references to these boolean values to external code.

[Eric Seidel (no email)]

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?

[Ryosuke Niwa]

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

[Levi Weintraub]

Created attachment 92117 [details]
Patch

[Eric Seidel (no email)]

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. :)

[Levi Weintraub]

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

[Levi Weintraub]

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

[Ryosuke Niwa]

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?

[Ryosuke Niwa]

Oops, I cleared eric's r+.

[Daniel Bates]

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.