As part of the great RenderBlockFlow refactor, move m_lineBoxes from RenderBlock to RenderBlockFlow.
Created attachment 214467 [details] WIP It compiles and passes all the tests. But I need to fix a bunch of the FIXMEs I left for myself.
Comment on attachment 214467 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=214467&action=review Looks pretty good. A couple of comments: > Source/WebCore/rendering/RenderBlock.cpp:230 > if (!documentBeingDestroyed()) { > - if (firstLineBox()) { > - // We can't wait for RenderBox::destroy to clear the selection, > - // because by then we will have nuked the line boxes. > - // FIXME: The FrameSelection should be responsible for this when it > - // is notified of DOM mutations. > - if (isSelectionBorder()) > - view().clearSelection(); > - > - // If we are an anonymous block, then our line boxes might have children > - // that will outlast this block. In the non-anonymous block case those > - // children will be destroyed by the time we return from this function. > - if (isAnonymousBlock()) { > - for (auto box = firstLineBox(); box; box = box->nextLineBox()) { > - while (auto childBox = box->firstChild()) > - childBox->removeFromParent(); > - } > - } > - } else if (parent()) > + if (parent()) > parent()->dirtyLinesFromChangedChild(this); In my patch for this, I opted for the following: if (!documentBeingDestroyed() && parent() && !isRenderBlockFlow()) parent()->dirtyLinesFromChangedChild(this); That allows you to not duplicate all the code in RenderBlockFlow::willBeDestroyed. > Source/WebCore/rendering/RenderBlock.cpp:2274 > +// FIXME: This is partially duplicated in RenderBlockFlow::paintContents. Fix before landing. There is a specific FIXME I've been using for blockflow, i.e., FIXME-BLOCKFLOW. You can use that.
I'm a bit nervous about virtualizing the line box accessors too, but as long as it's temporary. I was trying to go further, but that's a clever way to make the patch way smaller. :)
(In reply to comment #3) > I'm a bit nervous about virtualizing the line box accessors too, but as long as it's temporary. I was trying to go further, but that's a clever way to make the patch way smaller. :) Yeah, it worries me too. I'm going to try to fix it.
Created attachment 214632 [details] Part 1
(In reply to comment #5) > Created an attachment (id=214632) [details] > Patch I decided to split it up into chunks. Here is the first one.
Comment on attachment 214632 [details] Part 1 Part 1 committed in revision 157662
Created attachment 214641 [details] Part 2
Attachment 214641 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/rendering/EllipsisBox.cpp', u'Source/WebCore/rendering/EllipsisBox.h', u'Source/WebCore/rendering/RenderBlock.cpp', u'Source/WebCore/rendering/RenderBlock.h', u'Source/WebCore/rendering/RenderBlockFlow.cpp', u'Source/WebCore/rendering/RenderBlockFlow.h', u'Source/WebCore/rendering/RenderDeprecatedFlexibleBox.cpp', u'Source/WebCore/rendering/RootInlineBox.cpp']" exit_code: 1 Source/WebCore/rendering/RenderBlockFlow.cpp:2476: l is incorrectly named. Don't use the single letter 'l' as an identifier name. [readability/naming] [4] Source/WebCore/rendering/RenderBlockFlow.cpp:2496: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Source/WebCore/rendering/RenderBlockFlow.cpp:2503: l is incorrectly named. Don't use the single letter 'l' as an identifier name. [readability/naming] [4] Total errors found: 3 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 214641 [details] Part 2 View in context: https://bugs.webkit.org/attachment.cgi?id=214641&action=review > Source/WebCore/rendering/RenderBlockFlow.cpp:2416 > +// Helper methods for obtaining the last line, computing line counts and heights for line counts (crawling into blocks). > +static bool shouldCheckLines(RenderObject* obj) > +{ The comment seems to be about the next function. This one could take reference instead of pointer. > Source/WebCore/rendering/RenderBlockFlow.cpp:2425 > + return 0; nullptr > Source/WebCore/rendering/RenderBlockFlow.cpp:2431 > + if (childrenInline()) { > + for (auto box = firstRootBox(); box; box = box->nextRootBox()) > + if (!i--) > + return box; > + } else { This could use early return. > Source/WebCore/rendering/RenderBlockFlow.cpp:2440 > + return 0; nullptr > Source/WebCore/rendering/RenderBlockFlow.cpp:2446 > + return 0; nullptr > Source/WebCore/rendering/RenderBlockFlow.cpp:2459 > + if (childrenInline()) { > + for (RootInlineBox* box = firstRootBox(); box; box = box->nextRootBox()) { > + count++; > + if (box == stopRootInlineBox) { > + if (found) > + *found = true; > + break; > + } > + } > + } else { Early return >> Source/WebCore/rendering/RenderBlockFlow.cpp:2476 >> +static int getHeightForLineCount(RenderBlockFlow* block, int l, bool includeBottom, int& count) > > l is incorrectly named. Don't use the single letter 'l' as an identifier name. [readability/naming] [4] const reference > Source/WebCore/rendering/RenderBlockFlow.cpp:2486 > + if (block->childrenInline()) { > + for (auto box = block->firstRootBox(); box; box = box->nextRootBox()) { > + if (++count == l) > + return box->lineBottom() + (includeBottom ? (block->borderBottom() + block->paddingBottom()) : LayoutUnit()); > + } > + } else { Early return > Source/WebCore/rendering/RenderBlockFlow.cpp:2518 > + if (childrenInline() && hasMarkupTruncation()) { > + setHasMarkupTruncation(false); > + for (RootInlineBox* box = firstRootBox(); box; box = box->nextRootBox()) > + box->clearTruncation(); > + } else { Early return > Source/WebCore/rendering/RenderBlockFlow.h:301 > + int lineCount(const RootInlineBox* = 0, bool* = 0) const; nullptrs
Comment on attachment 214641 [details] Part 2 Part 2 committed in revision 157674.
Created attachment 214662 [details] Part 3
Comment on attachment 214662 [details] Part 3 Part 3 committed in revision 157677.
Created attachment 214670 [details] Patch 4
Comment on attachment 214670 [details] Patch 4 Attachment 214670 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/6848036
Created attachment 214672 [details] Patch 4 (take 2)
Committed r157683: <http://trac.webkit.org/changeset/157683>
Did not mean to close. I'm not done yet.
Created attachment 214705 [details] Part 5
Attachment 214705 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/accessibility/AccessibilityRenderObject.cpp', u'Source/WebCore/rendering/HitTestResult.cpp', u'Source/WebCore/rendering/RenderBlock.cpp', u'Source/WebCore/rendering/RenderBlock.h', u'Source/WebCore/rendering/RenderBlockFlow.cpp', u'Source/WebCore/rendering/RenderBlockFlow.h', u'Source/WebCore/rendering/RenderBlockLineLayout.cpp', u'Source/WebCore/rendering/RootInlineBox.cpp']" exit_code: 1 Source/WebCore/rendering/RenderBlock.cpp:4646: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/rendering/RenderBlock.cpp:4647: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/rendering/RenderBlockFlow.cpp:2413: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] Source/WebCore/rendering/RenderBlockFlow.cpp:2521: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/rendering/RenderBlockFlow.cpp:2522: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/rendering/RenderBlockFlow.cpp:2560: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] Source/WebCore/rendering/RenderBlockFlow.cpp:2890: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] Source/WebCore/rendering/RenderBlockFlow.cpp:2898: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] Total errors found: 8 in 9 files If any of these errors are false positives, please file a bug against check-webkit-style.
Committed r157705: <http://trac.webkit.org/changeset/157705>