[webkit-reviews] review denied: [Bug 65627] [CSSRegions]RenderFlowThread should display its content using RenderRegion : [Attachment 102797] Patch

[bugzilla-daemon at webkit.org]

Dave Hyatt <hyatt at apple.com> has denied Mihnea Ovidenie <mihnea at adobe.com>'s
request for review:
Bug 65627: [CSSRegions]RenderFlowThread should display its content using
RenderRegion
https://bugs.webkit.org/show_bug.cgi?id=65627

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

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


> Source/WebCore/rendering/RenderFlowThread.cpp:188
> +void RenderFlowThread::computeLogicalWidth()

This seems odd to me. Does this mean that if you have one narrow region and one
wide region, the content will be flowed to the width of the widest region?
Shouldn't the width actually vary from region to region?

> Source/WebCore/rendering/RenderFlowThread.cpp:197
> +	   int regionWidth = region->contentWidth();
> +	   if (regionWidth > width)
> +	       width = regionWidth;

I'd just write this as width = max(width, region->contentWidth());

> Source/WebCore/rendering/RenderFlowThread.cpp:211
> +	   int regionHeight = region->contentHeight();
> +	   height += regionHeight;

Don't really understand the need for a local here. Just add
region->contentHeight() to height directly.

> Source/WebCore/rendering/RenderFlowThread.cpp:217
> +void RenderFlowThread::paintRegion(PaintInfo& paintInfo, const LayoutRect&
regionRect, const LayoutPoint& paintOffset)

It seems like you're propagating a mistake of columns here by clipping to the
region bounds. I don't think overflow should be clipped by default, should it?
That's actually a mistake columns make.

Also, what about RenderLayers?	I don't see how you can just do a RenderBlock
paint here, since that won't include descendants with layers.

> Source/WebCore/rendering/RenderRegion.cpp:90
> +    // Delegate painting of content in region to RenderFlowThread.
> +    adjustedPaintOffset.move(paddingLeft() + borderLeft(), paddingTop() +
borderTop());
> +    m_flowThread->paintRegion(paintInfo, regionRect(), adjustedPaintOffset);


Seems like you just want to paint the flow thread's RenderLayer (it will always
have one since you absolutely positioned it) with an offset and clip applied.

I'll give you a pass on clipping for now, since columns make the same mistake,
and I guess regions can make the same mistake as well.