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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Sep 10 15:15:52 PDT 2013

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

Attachment 211218: proposed patch

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


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

More information about the webkit-reviews mailing list