[webkit-reviews] review granted: [Bug 81809] percentage height/width values in quirks mode are incorrectly resolved in flexbox children : [Attachment 157004] Patch

[bugzilla-daemon at webkit.org]

Tony Chang <tony at chromium.org> has granted Ojan Vafai <ojan at chromium.org>'s
request for review:
Bug 81809: percentage height/width values in quirks mode are incorrectly
resolved in flexbox children
https://bugs.webkit.org/show_bug.cgi?id=81809

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

------- Additional Comments from Tony Chang <tony at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=157004&action=review


> Source/WebCore/rendering/RenderFlexibleBox.cpp:407
> +    if (isColumnFlow())
> +	   return child->computeLogicalClientHeight(sizeType, size);
> +    return child->computeContentBoxLogicalWidth(valueForLength(size,
availableSize, view()));

This looks wrong for orthogonal flows, but I guess it's what we had before.

>>> Source/WebCore/rendering/RenderFlexibleBox.cpp:817
>>> +		 return minSize;
>> 
>> The -1 check seems bad. Negative min/max height are illegal, so it shouldn't
even be isSpecified() if it's negative. Is that a bug?
> 
> computePercentageLogicalHeight can return -1 in some cases depending on the
state of the containingBlock (e.g. in standards mode if the containing block is
height:auto).

I wonder if we can remove the isSpecified() check.  I don't have a strong
preference one way or another.

> Source/WebCore/rendering/RenderFlexibleBox.h:88
> +    LayoutUnit computeMainAxisSizeForChild(RenderBox* child, SizeType, const
Length& size, LayoutUnit availableSize);

Nit: I think we normally use Extent instead of Size for lengths.  I don't feel
strongly about this.

> LayoutTests/css3/flexbox/percentage-sizes-quirks.html:37
> +<script>
> +if (window.testRunner)
> +    testRunner.dumpAsText();
> +</script>

Nit: I would probably print document.compatMode somewhere just to be sure.