[webkit-reviews] review denied: [Bug 26559] When a block's height is determined by min-height/max-height, children with percentage heights are sized incorrectly : [Attachment 173722] Updated fix

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has denied Bem Jones-Bey <bjonesbe at adobe.com>'s
request for review:
Bug 26559: When a block's height is determined by min-height/max-height,
children with percentage heights are sized incorrectly
https://bugs.webkit.org/show_bug.cgi?id=26559

Attachment 173722: Updated fix
https://bugs.webkit.org/attachment.cgi?id=173722&action=review

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=173722&action=review


Sorry it took so long to get you a review on this.

Instead of a reftest the test a check-layout.js test? They much easier to read
than this test I think. For example, instead of 
<div id="simple-min"></div>

you'd have:
<div id="simple-min" class="parent" style="width: 50px; height; 50px;
max-height: 25px">
    <div class="child" data-expected-width=50 data-expected-height=25></div>
</div>

It's much easier to see what's going on here than needing to coordinate between
three different files.

> Source/WebCore/rendering/RenderBox.cpp:2216
> +	   availableHeight = max<LayoutUnit>(0,
cb->constrainContentBoxLogicalHeightByMinMax(contentBoxHeight -
cb->scrollbarLogicalHeight()));

I think we need to call this on contentBoxHeight and then subtract
scrollbarLogicalHeight from the result of
constrainContentBoxLogicalHeightByMinMax, but I'm not sure. Can you add some
test cases with overflow:scroll on the containingBlock.

> Source/WebCore/rendering/RenderBox.cpp:2226
> +	       LayoutUnit contentBoxHeight =
cb->constrainContentBoxLogicalHeightByMinMax(contentBoxHeightWithScrollbar -
cb->scrollbarLogicalHeight());

Ditto.

Also, it's still not clear to me why you need to call this here. It seems like
computePercentageLogicalHeight should already constrain it with your change
above. I'm worried that this is masking a bug. Which test cases break if you
don't modify this line? It's very possible I'm just missing something obvious.
:)