[webkit-reviews] review canceled: [Bug 121107] Move LineWidth out of RenderBlockLineLayout : [Attachment 211218] proposed patch

[bugzilla-daemon at webkit.org]

Zoltan Horvath <zoltan at webkit.org> has canceled Zoltan Horvath
<zoltan at webkit.org>'s request for review:
Bug 121107: Move LineWidth out of RenderBlockLineLayout
https://bugs.webkit.org/show_bug.cgi?id=121107

Attachment 211218: proposed patch
https://bugs.webkit.org/attachment.cgi?id=211218&action=review

------- Additional Comments from Zoltan Horvath <zoltan at webkit.org>
Please note that I was just moved out the code from RenderBlockLineLayout.cpp.
In this patch I addressed your comments.

(In reply to comment #2)
> (From update of attachment 211218 [details])
> View in context:
https://bugs.webkit.org/attachment.cgi?id=211218&action=review
> 
> > Source/WebCore/rendering/LineWidth.h:38
> > +static LayoutUnit logicalHeightForLine(const RenderBlock* block, bool
isFirstLine, LayoutUnit replacedHeight = 0)
> 
> Does this function really need to be in the header?  If so, you should not
mark it static, but rather inline.

It's used by both RenderBlockLineLayout.cpp and LineWidth.cpp. I made it
inline. I'm going to move that function to RenderBlock and make a member of it
in a separate patch.

> > Source/WebCore/rendering/LineWidth.h:53
> > +	 LineWidth(RenderBlock* block, bool isFirstLine, IndentTextOrNot
shouldIndentText)
> 
> Is it necessary for the constructor to be inline?

No. I moved it to the cpp.

> > Source/WebCore/rendering/LineWidth.h:108
> > +	 bool fitsOnLineExcludingTrailingCollapsedWhitespace() const { return
currentWidth() - m_trailingCollapsedWhitespaceWidth <= m_availableWidth; }
> 
> This should probably be on multiple lines if the one above it is.

Yeah, I moved these things to the cpp. 

> > Source/WebCore/rendering/LineWidth.h:110
> > +private:
> 
> This private: is redundant.

Removed.

> > Source/WebCore/rendering/LineWidth.h:144
> > +inline void
LineWidth::shrinkAvailableWidthForNewFloatIfNeeded(FloatingObject* newFloat)
> 
> This is an awfully big function to inline.  Are you sure it is worth it?

Probably it had been inline before that it has got big. I removed the moved to
the cpp. We can inline it later if the performance measurements suggest that
it's actually worth it.