Move LineWidth class and related code into LineWidth.{h,cpp}.
Created attachment 211218 [details] proposed patch
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?
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.
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.
Created attachment 211253 [details] proposed patch
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&?
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.
Comment on attachment 211329 [details] proposed patch r=me
Comment on attachment 211329 [details] proposed patch Clearing flags on attachment: 211329 Committed r155565: <http://trac.webkit.org/changeset/155565>
All reviewed patches have been landed. Closing bug.