Bug 122969 – Move m_lineBoxes from RenderBlock to RenderBlockFlow

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Sam Weinig]

As part of the great RenderBlockFlow refactor, move m_lineBoxes from RenderBlock to RenderBlockFlow.

[Sam Weinig]

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.

[Dave Hyatt]

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.

[Dave Hyatt]

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

[Sam Weinig]

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

[Sam Weinig]

Created attachment 214632 [details]
Part 1

[Sam Weinig]

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

[Sam Weinig]

Comment on attachment 214632 [details]
Part 1

Part 1 committed in revision 157662

[Sam Weinig]

Created attachment 214641 [details]
Part 2

[WebKit Commit Bot]

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.

[Antti Koivisto]

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

[Sam Weinig]

Comment on attachment 214641 [details]
Part 2

Part 2 committed in revision 157674.

[Sam Weinig]

Created attachment 214662 [details]
Part 3

[Sam Weinig]

Comment on attachment 214662 [details]
Part 3

Part 3 committed in revision 157677.

[Sam Weinig]

Created attachment 214670 [details]
Patch 4

[EFL EWS Bot]

Comment on attachment 214670 [details]
Patch 4

Attachment 214670 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/6848036

[Sam Weinig]

Created attachment 214672 [details]
Patch 4 (take 2)

[Sam Weinig]

Committed r157683: <http://trac.webkit.org/changeset/157683>

[Sam Weinig]

Did not mean to close. I'm not done yet.

[Sam Weinig]

Created attachment 214705 [details]
Part 5

[WebKit Commit Bot]

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.

[Sam Weinig]

Committed r157705: <http://trac.webkit.org/changeset/157705>