Bug 36921 - Split RenderBlock::layoutInlineChildren into smaller functions
Summary: Split RenderBlock::layoutInlineChildren into smaller functions
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: James Robinson
URL:
Keywords:
Depends on: 37114 60052 60080
Blocks: 57779
  Show dependency treegraph
 
Reported: 2010-03-31 19:06 PDT by James Robinson
Modified: 2011-05-04 09:05 PDT (History)
7 users (show)

See Also:


Attachments
Patch (38.39 KB, patch)
2010-03-31 19:08 PDT, James Robinson
no flags Details | Formatted Diff | Diff
Patch (36.57 KB, patch)
2010-04-01 17:44 PDT, James Robinson
eric: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description James Robinson 2010-03-31 19:06:49 PDT
Split RenderBlock::layoutInlineChildren into smaller functions
Comment 1 James Robinson 2010-03-31 19:08:39 PDT
Created attachment 52237 [details]
Patch
Comment 2 James Robinson 2010-03-31 19:10:59 PDT
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.
Comment 3 WebKit Review Bot 2010-03-31 19:14:29 PDT
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.
Comment 4 Darin Adler 2010-04-01 14:04:51 PDT
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.
Comment 5 James Robinson 2010-04-01 14:14:48 PDT
Yes, making FloatWithRect public was from an earlier revision and not intentional.  Thank you for reviewing, I'll upload an updated patch shortly.
Comment 6 James Robinson 2010-04-01 17:44:32 PDT
Created attachment 52357 [details]
Patch
Comment 7 WebKit Review Bot 2010-04-01 17:46:49 PDT
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.
Comment 8 James Robinson 2010-04-01 17:47:52 PDT
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.
Comment 9 James Robinson 2010-04-02 16:21:56 PDT
Committed r57030: <http://trac.webkit.org/changeset/57030>
Comment 10 Adam Barth 2010-04-05 14:53:03 PDT
Rollout landed in r57096.
Comment 11 James Robinson 2010-04-05 14:55:52 PDT
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.
Comment 12 Eric Seidel (no email) 2010-04-06 23:46:53 PDT
Attachment 52357 [details] was posted by a committer and has review+, assigning to James Robinson for commit.
Comment 13 Eric Seidel (no email) 2010-05-17 00:35:21 PDT
Comment on attachment 52357 [details]
Patch

Marking r- since this was reverted.
Comment 14 Eric Seidel (no email) 2011-05-03 11:41:12 PDT
I'm re-writing James' patch in smaller pieces.  I'll attach patches to dependent bugs.
Comment 15 Eric Seidel (no email) 2011-05-03 14:04:09 PDT
Well, I found the cause of the regression, James.  See bug 60052
Comment 16 Ryosuke Niwa 2011-05-04 09:05:58 PDT
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