Bug 36921 – Split RenderBlock::layoutInlineChildren into smaller functions

• Status: RESOLVED
• Resolution: CONFIGURATION CHANGED
• Component: New Bugs

[James Robinson]

Split RenderBlock::layoutInlineChildren into smaller functions

[James Robinson]

Created attachment 52237 [details]
Patch

[James Robinson]

Hopefully this is slightly more readable and easy to edit.  I've been careful not to change any logic, just moved code into new functions, added comments, and renamed a few variables that were confusingly named.

[WebKit Review Bot]

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

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/rendering/RenderBlockLineLayout.cpp:817:  One line control clauses should not use braces.  [whitespace/braces] [4]
WebCore/rendering/RenderBlockLineLayout.cpp:867:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 2 in 3 files


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

[Darin Adler]

Comment on attachment 52237 [details]
Patch

Good idea.

> +    struct FloatWithRect {

Since all the helper functions are member functions, I don't see why we need to make the FloatWithRect struct public. Maybe this is left over from an earlier version of the patch?

> +    bool layoutReplacedElements(bool relayoutChildren, bool fullLayout, Vector<FloatWithRect>& floats);

We don't need the argument name "floats" here.

> +    RootInlineBox* createLineBoxesForResolver(const InlineBidiResolver& resolver, const InlineIterator& currentPosition,
> +                                              bool firstLine, bool previousLineBrokeCleanly, BidiRun* trailingSpaceRun);

We don't line up subsequent lines with the "(" from the first line in WebKit. We don't need the argument name "resolver" here and it's arguably we don't need the name "currentPosition" either.

And for the actual function definition I also think that "currentPosition" is possibly a too-long name. I suggest just "position", which although ambiguous is shorter and probably clearer despite the ambiguity.

> +    void layoutRunsAndFloats(bool fullLayout, Vector<FloatWithRect>& floats, int& repaintTop, int& repaintBottom);

We don't need the argument name "floats" here.

> +bool RenderBlock::layoutReplacedElements(bool relayoutChildren, bool fullLayout, Vector<FloatWithRect>& floats)

May want to consider marking this "used in only one place" function inline. Same with others. In some cases that results in smaller code size and slightly better performance.

> +    RenderObject* o = bidiFirst(this, 0, false);
> +    while (o) {

It's good that you just moved the code and kept the changes to a minimum. Two things I would change in this code in the future would be using a word instead of the letter "o" and use a for loop instead of a while loop.

> +        lineBox = constructLine(resolver.runCount(), resolver.firstRun(), resolver.lastRun(), firstLine,
> +                                !currentPosition.obj, currentPosition.obj && !currentPosition.pos ? currentPosition.obj : 0);

We don't line up second lines under the "(" like this in WebKit code. If the length of the line bothers you than I suggest we use local variables for some of these expressions.

One such expression would be "currentPosition.obj && !currentPosition.pos ? currentPosition.obj : 0". And I don't see why that needs "currentPosition.obj &&" in it at all.

> +    size_t floatCount = floats.size();
> +    // Floats that did not have layout did not repaint when we laid them out. They would have
> +    // painted by now if they had moved, but if they stayed at (0, 0), they still need to be
> +    // painted.
> +    for (size_t i = 0; i < floatCount; ++i) {

Not new, but it's quite change that this comment is tucked in between the floatCount line and the for line. I would move the comment up one line.

Since this is a refactoring patch, I'm going to say review- due to the fact that it needlessly makes FloatWithRect public.

[James Robinson]

Yes, making FloatWithRect public was from an earlier revision and not intentional.  Thank you for reviewing, I'll upload an updated patch shortly.

[James Robinson]

Created attachment 52357 [details]
Patch

[WebKit Review Bot]

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

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/rendering/RenderBlockLineLayout.cpp:817:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 1 in 3 files


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

[James Robinson]

I experimented with adding the 'inline' keyword to each of the called functions, but gcc still refused to inline any of them at -O3 citing "--param max-inline-insns-single limit reached" meaning it thought the function body was too large to consider inlining.  I can add the keyword back if you think it might help on other platforms.

[James Robinson]

Committed r57030: <http://trac.webkit.org/changeset/57030>

[Adam Barth]

Rollout landed in r57096.

[James Robinson]

This regressed fast/repaint/line-flow-with-floats-9 in pixel mode.  Reverted at
http://trac.webkit.org/changeset/57096, will update the patch once I figure out
how I broke stuff.

[Eric Seidel (no email)]

Attachment 52357 [details] was posted by a committer and has review+, assigning to James Robinson for commit.

[Eric Seidel (no email)]

Comment on attachment 52357 [details]
Patch

Marking r- since this was reverted.

[Eric Seidel (no email)]

I'm re-writing James' patch in smaller pieces.  I'll attach patches to dependent bugs.

[Eric Seidel (no email)]

Well, I found the cause of the regression, James.  See bug 60052

[Ryosuke Niwa]

Is this bug now fixed given that layoutInlineChildren has been broken down into smaller pieces?
http://trac.webkit.org/browser/trunk/Source/WebCore/rendering/RenderBlockLineLayout.cpp#L1103

[Ahmad Saleem]

(In reply to Ryosuke Niwa from comment #16)
> Is this bug now fixed given that layoutInlineChildren has been broken down
> into smaller pieces?
> http://trac.webkit.org/browser/trunk/Source/WebCore/rendering/
> RenderBlockLineLayout.cpp#L1103

Is this required anymore because as mentioned by Ryosuke, this refactoring / simplification happened already? Thanks!

[Alexey Proskuryakov]

We've been asking each other about whether this refactoring is done for 10+ years, I think it's time to stop.