[webkit-reviews] review denied: [Bug 116033] Column balancing support in the region based multicol implementation : [Attachment 201804] Patch

[bugzilla-daemon at webkit.org]

Dave Hyatt <hyatt at apple.com> has denied Morten Stenshorne
<mstensho at opera.com>'s request for review:
Bug 116033: Column balancing support in the region based multicol
implementation
https://bugs.webkit.org/show_bug.cgi?id=116033

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

------- Additional Comments from Dave Hyatt <hyatt at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=201804&action=review


This patch is awesome!

One comment on tests. You can in fact make reftests that compare the old
columns with the new columns. You can see examples of that in the newmulticol
subdirectory of fast/multicol. I understand your balancing algorithm may be
different, though, and it may lead to slightly different results, but maybe you
could take a look at a few balancing tests and see if you happen to match.

> Source/WebCore/rendering/RenderBlock.cpp:7525
> +    if (!style()->hasAutoOrphans() || !style()->hasAutoWidows()) {
> +	   // We require a certain minimum number of lines per page in order to
satisfy orphans and
> +	   // widows, so we may need to report a larger minimum page height
because of that.
> +	   int lineCount = max<int>(style()->hasAutoOrphans() ? 1 :
style()->orphans(), style()->hasAutoWidows() ? 1 : style()->widows());
> +	   RootInlineBox* line = lineBox;
> +	   for (int i = lineCount - 1; i > 0 && line->prevRootBox(); i--)
> +	       line = line->prevRootBox();
> +	   LayoutRect overflow =
line->logicalVisualOverflowRect(line->lineTop(), line->lineBottom());
> +	   minPageHeight = logicalBottom - min(line->lineTopWithLeading(),
overflow.y());
> +    }

Let's push this into a little helper function (even just a static would be
nice).

> Source/WebCore/rendering/RenderFlowThread.h:105
> +    virtual void setPageBreak(LayoutUnit /*offset*/, LayoutUnit
/*spaceShortage*/) { }
> +
> +    virtual void updateMinimumPageHeight(LayoutUnit /*offset*/, LayoutUnit
/*minHeight*/) { }

Add the OVERRIDE keyword to these functions please. I don't think you need a
blank line between them either.

> Source/WebCore/rendering/RenderMultiColumnBlock.cpp:44
>      , m_requiresBalancing(false)

BTW I added this variable on the assumption you might use it to set if a column
set needs balancing. If it turns out you don't need it, you could just take it
out.

> Source/WebCore/rendering/RenderMultiColumnSet.cpp:73
> +    unsigned columnIndex = columnIndexAtOffset(offset, true);

We dislike raw booleans, since what they mean is not discernible from the call
site. Please change this to use an enum instead of a boolean.

> Source/WebCore/rendering/RenderMultiColumnSet.cpp:244
> +unsigned RenderMultiColumnSet::columnIndexAtOffset(LayoutUnit offset, bool
buildingColumns) const

Use an enum instead of a boolean.

> Source/WebCore/rendering/RenderMultiColumnSet.h:85
> +    // Calculate the column height when contents is supposed to be balanced.
If 'initial' is set,

Should be "when the contents are supposed to be balanced."

> Source/WebCore/rendering/RenderMultiColumnSet.h:143
>      bool m_requiresBalancing; // Whether or not the columns in the column
set have to be balanced, i.e., made to be similar logical heights.

May as well ditch the member too, right? If you're not going to need it, dump
it.