[Bug 36921] Split RenderBlock::layoutInlineChildren into smaller functions

bugzill...@webkit.org

unread,

Mar 31, 2010, 10:14:30 PM3/31/10

to webkit-bo...@googlegroups.com

https://bugs.webkit.org/show_bug.cgi?id=36921

WebKit Review Bot <webkit.r...@gmail.com> changed:

--- Comment #3 from WebKit Review Bot <webkit.r...@gmail.com> 2010-03-31 19:14:29 PST ---
Attachment 52237 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.

--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.

bugzill...@webkit.org

unread,

Apr 1, 2010, 5:04:51 PM4/1/10

to webkit-bo...@googlegroups.com

https://bugs.webkit.org/show_bug.cgi?id=36921

Darin Adler <da...@apple.com> changed:

--- Comment #4 from Darin Adler <da...@apple.com> 2010-04-01 14:04:51 PST ---
(From update of attachment 52237)
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.

bugzill...@webkit.org

unread,

Apr 1, 2010, 5:14:48 PM4/1/10

to webkit-bo...@googlegroups.com

https://bugs.webkit.org/show_bug.cgi?id=36921

--- Comment #5 from James Robinson <jam...@chromium.org> 2010-04-01 14:14:48 PST ---
Yes, making FloatWithRect public was from an earlier revision and not
intentional. Thank you for reviewing, I'll upload an updated patch shortly.

bugzill...@webkit.org

unread,

Apr 1, 2010, 8:44:29 PM4/1/10

to webkit-bo...@googlegroups.com

https://bugs.webkit.org/show_bug.cgi?id=36921

James Robinson <jam...@chromium.org> changed:

bugzill...@webkit.org

unread,

Apr 1, 2010, 8:44:32 PM4/1/10

to webkit-bo...@googlegroups.com

https://bugs.webkit.org/show_bug.cgi?id=36921

James Robinson <jam...@chromium.org> changed:

--- Comment #6 from James Robinson <jam...@chromium.org> 2010-04-01 17:44:32 PST ---
Created an attachment (id=52357)
--> (https://bugs.webkit.org/attachment.cgi?id=52357)
Patch

bugzill...@webkit.org

unread,

Apr 1, 2010, 8:46:50 PM4/1/10

to webkit-bo...@googlegroups.com

https://bugs.webkit.org/show_bug.cgi?id=36921

--- Comment #7 from WebKit Review Bot <webkit.r...@gmail.com> 2010-04-01 17:46:49 PST ---
Attachment 52357 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.

--

bugzill...@webkit.org

unread,

Apr 1, 2010, 8:47:52 PM4/1/10

to webkit-bo...@googlegroups.com

https://bugs.webkit.org/show_bug.cgi?id=36921

--- Comment #8 from James Robinson <jam...@chromium.org> 2010-04-01 17:47:52 PST ---
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.

unread,

Apr 5, 2010, 5:55:53 PM4/5/10

to webkit-bo...@googlegroups.com

https://bugs.webkit.org/show_bug.cgi?id=36921

--- Comment #11 from James Robinson <jam...@chromium.org> 2010-04-05 14:55:52 PST ---
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.

bugzill...@webkit.org

unread,

May 17, 2010, 3:35:23 AM5/17/10

to webkit-bo...@googlegroups.com

https://bugs.webkit.org/show_bug.cgi?id=36921

Eric Seidel <er...@webkit.org> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #52357|review+ |review-
Flag| |

--- Comment #13 from Eric Seidel <er...@webkit.org> 2010-05-17 00:35:21 PST ---
(From update of attachment 52357)
Marking r- since this was reverted.

Reply all

Reply to author

Forward