[webkit-reviews] review denied: [Bug 36921] Split RenderBlock::layoutInlineChildren into smaller functions : [Attachment 52237] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied James Robinson <jamesr at chromium.org>'s
request for review:
Bug 36921: Split RenderBlock::layoutInlineChildren into smaller functions
https://bugs.webkit.org/show_bug.cgi?id=36921

Attachment 52237: Patch
https://bugs.webkit.org/attachment.cgi?id=52237&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.