[webkit-reviews] review requested: [Bug 60044] Extract LineInfo class : [Attachment 92117] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has asked  for review:
Bug 60044: Extract LineInfo class
https://bugs.webkit.org/show_bug.cgi?id=60044

Attachment 92117: Patch
https://bugs.webkit.org/attachment.cgi?id=92117&action=review

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
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?