[webkit-reviews] review denied: [Bug 117885] [CSS Shapes] Fixed shape size on an auto height container should determine the containing block's height : [Attachment 208741] updated 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 208741: updated patch
https://bugs.webkit.org/attachment.cgi?id=208741&action=review

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


r-

> Source/WebCore/rendering/RenderBlock.cpp:1491
> +    ShapeInsideInfo* shapeInsideInfo = this->shapeInsideInfo();
> +    // We don't modify the logicalHeight when the container isn't
auto-height or the shape's height isn't resolved.
> +    if (!style()->height().isAuto() || !shapeInsideInfo ||
!(shapeInsideInfo->shapeLogicalBottom() - borderAndPaddingBefore()))
> +	   return;
> +    // We don't modify the logicalHeight when we the max-height is defined
and it's smaller than the shape's containingBlock height
> +    if (style()->logicalMaxHeight().isPositive() &&
style()->logicalMaxHeight().intValue() <= logicalHeight())
> +	   return;
> +    setLogicalHeight(shapeInsideInfo->shapeLogicalBottom() +
borderAndPaddingAfter());

It seems like you're making this too complicated. Remember that prior to
computeLogicalHeight(), we really are just computing the block's intrinsic
height. Then it gets adjusted.

In other words, I think you should just always increase the block and stop
checking for !style()->height().isAuto() and for max height. These checks are
wrong anyway, because of percentages that can end up being auto, etc.

You should be able to completely remove the style()->height() and
logicalMaxHeight() checks and all the tests should still pass. You should
probably also add percentage height and max-height tests, both where the
percentage is honored (by being inside a container whose size is fixed) and
where it is ignored (by being inside a container with no specified size).

Also, that style()->height() (if we do end up keeping it) should be a
logicalHeight(), which means i'd like some writing-mode vertical-rl tests too.