Bug 121107 – Move LineWidth out of RenderBlockLineLayout

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Zoltan Horvath]

Move LineWidth class and related code into LineWidth.{h,cpp}.

[Zoltan Horvath]

Created attachment 211218 [details]
proposed patch

[Sam Weinig]

Comment on attachment 211218 [details]
proposed patch

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.

> Source/WebCore/rendering/LineWidth.h:53
> +    LineWidth(RenderBlock* block, bool isFirstLine, IndentTextOrNot shouldIndentText)

Is it necessary for the constructor to be inline?

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

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

[Zoltan Horvath]

Created attachment 211252 [details]
proposed patch

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.

[WebKit Commit Bot]

Attachment 211252 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/CMakeLists.txt', u'Source/WebCore/ChangeLog', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/Target.pri', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/rendering/LineWidth.cpp', u'Source/WebCore/rendering/LineWidth.h', u'Source/WebCore/rendering/RenderBlockLineLayout.cpp']" exit_code: 1
Source/WebCore/rendering/LineWidth.h:53:  The parameter name "block" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Zoltan Horvath]

Created attachment 211253 [details]
proposed patch

[Sam Weinig]

Comment on attachment 211253 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=211253&action=review

> Source/WebCore/rendering/LineWidth.h:34
> +#include "RenderBlock.h"
> +#include "RenderRubyRun.h"

Do either of these really need to be in the header?

> Source/WebCore/rendering/LineWidth.h:48
> +    return max<LayoutUnit>(replacedHeight, block->lineHeight(isFirstLine, block->isHorizontalWritingMode() ? HorizontalLine : VerticalLine, PositionOfInteriorLineBoxes));

I am surprised this compiles without the std::.  I would add it though.

> Source/WebCore/rendering/LineWidth.h:79
> +    RenderBlock* m_block;

It looks like this is never null, can we make it a RenderBlock&?

[Zoltan Horvath]

Created attachment 211329 [details]
proposed patch

(In reply to comment #6)
> (From update of attachment 211253 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=211253&action=review
> 
> > Source/WebCore/rendering/LineWidth.h:34
> > +#include "RenderBlock.h"
> > +#include "RenderRubyRun.h"
> 
> Do either of these really need to be in the header?

For now, yes. I'd like to move logicalHeightForLine to RenderBlock in a separate patch, I'll remove the includes along with that patch.

> > Source/WebCore/rendering/LineWidth.h:48
> > +    return max<LayoutUnit>(replacedHeight, block->lineHeight(isFirstLine, block->isHorizontalWritingMode() ? HorizontalLine : VerticalLine, PositionOfInteriorLineBoxes));
> 
> I am surprised this compiles without the std::.  I would add it though.

Added.

> > Source/WebCore/rendering/LineWidth.h:79
> > +    RenderBlock* m_block;
> 
> It looks like this is never null, can we make it a RenderBlock&?

Done.

[Dave Hyatt]

Comment on attachment 211329 [details]
proposed patch

r=me

[WebKit Commit Bot]

Comment on attachment 211329 [details]
proposed patch

Clearing flags on attachment: 211329

Committed r155565: <http://trac.webkit.org/changeset/155565>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.