[webkit-reviews] review denied: [Bug 117885] [CSS Shapes] Fixed shape size on an auto height container should determine the containing block's height : [Attachment 205223] proposed patch

[bugzilla-daemon at webkit.org]

Dave Hyatt <hyatt at apple.com> has denied Zoltan Horvath <zoltan at webkit.org>'s
request for review:
Bug 117885: [CSS Shapes] Fixed shape size on an auto height container should
determine the containing block's height
https://bugs.webkit.org/show_bug.cgi?id=117885

Attachment 205223: proposed patch
https://bugs.webkit.org/attachment.cgi?id=205223&action=review

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


r-

> Source/WebCore/rendering/RenderBox.cpp:2545
> +	   // If we have an auto sized block with a fixed size shape, the
height should be adjusted to the height of the shape
> +	   if (isRenderBlock() && h.isAuto()) {
> +	       ShapeInsideInfo* shapeInsideInfo =
toRenderBlock(this)->shapeInsideInfo();
> +	       if (shapeInsideInfo && shapeInsideInfo->shapeLogicalBottom())
> +		   heightResult = shapeInsideInfo->shapeLogicalBottom() +
paddingBefore() + paddingAfter();
> +	   }

This is misplaced. computeLogicalHeight is not about modifying intrinsic
heights. I think you should do this in RenderBlock's layout method, since it is
similar to the expansion we do for floats. I think it should go there instead
of in RenderBox, where it's clearly misplaced.