[webkit-reviews] review granted: [Bug 127565] [New Multicolumn] Eliminate RenderMultiColumnBlock : [Attachment 222117] Patch

[bugzilla-daemon at webkit.org]

Antti Koivisto <koivisto at iki.fi> has granted Dave Hyatt <hyatt at apple.com>'s
request for review:
Bug 127565: [New Multicolumn] Eliminate RenderMultiColumnBlock
https://bugs.webkit.org/show_bug.cgi?id=127565

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

------- Additional Comments from Antti Koivisto <koivisto at iki.fi>
View in context: https://bugs.webkit.org/attachment.cgi?id=222117&action=review


> Source/WebCore/rendering/RenderBlockFlow.cpp:3214
> +	       if (minColumnCount >= desiredColumnCount) {
> +		   // The forced page breaks are in control of the balancing. 
Just set the column height to the
> +		   // maximum page break distance.
> +		   if (!pageLogicalHeight) {
> +		       LayoutUnit distanceBetweenBreaks =
std::max<LayoutUnit>(colInfo->maximumDistanceBetweenForcedBreaks(),
> +			   view().layoutState()->pageLogicalOffset(this,
borderAndPaddingBefore() + layoutOverflowLogicalBottom) -
colInfo->forcedBreakOffset());
> +		       columnHeight = std::max(colInfo->minimumColumnHeight(),
distanceBetweenBreaks);
> +		   }

This is getting deep, helper functions might be useful.

> Source/WebCore/rendering/RenderMultiColumnFlowThread.cpp:125
> -    RenderMultiColumnBlock* parentBlock =
toRenderMultiColumnBlock(parent());
> +    RenderBlockFlow* parentBlock = toRenderBlockFlow(parent());

i would just use auto, toRenderBlockFlow already names the type. 
parentFlow would be a more informative variable name.

> Source/WebCore/rendering/RenderMultiColumnSet.cpp:48
> -    RenderMultiColumnBlock* multicolBlock =
toRenderMultiColumnBlock(parent());
> +    RenderBlockFlow* multicolBlock = toRenderBlockFlow(parent());

same here, 'multicolumnFlow'

> Source/WebCore/rendering/RenderMultiColumnSet.cpp:202
> +    RenderBlockFlow* parentBlock = toRenderBlockFlow(parent());

same here

> Source/WebCore/rendering/RenderMultiColumnSet.cpp:224
> -    RenderMultiColumnBlock* multicolBlock =
toRenderMultiColumnBlock(parent());
> +    RenderBlockFlow* multicolBlock = toRenderBlockFlow(parent());

same here

> Source/WebCore/rendering/RenderMultiColumnSet.cpp:268
> +    RenderBlockFlow* parentBlock = toRenderBlockFlow(parent());

same here