[webkit-reviews] review denied: [Bug 94237] iframe does not stretch in flexbox : [Attachment 159443] Patch

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has denied Shezan Baig <shezbaig.wk at gmail.com>'s
request for review:
Bug 94237: iframe does not stretch in flexbox
https://bugs.webkit.org/show_bug.cgi?id=94237

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

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


> Source/WebCore/rendering/RenderFlexibleBox.cpp:1240
> +	       child->setOverrideLogicalContentHeight(stretchedLogicalHeight -
child->borderAndPaddingLogicalHeight());

As I look more closely, I think this will do the wrong thing in some cases. At
the least, when the item has a fixed logical height (e.g. height:100px...it
shouldn't stretch) or a min/max logical height (it should respect min/max
constraints).

Sorry I didn't catch this earlier.

I think the right solution might be to move the following code out of
RenderBox::computeLogicalHeight into a helper function that RenderFlexibleBox
can call:
	    heightResult = computeLogicalHeightUsing(MainOrPreferredSize,
styleToUse->logicalHeight());
	    if (heightResult == -1)
		heightResult = logicalHeight();
	    LayoutUnit minH = computeLogicalHeightUsing(MinSize,
styleToUse->logicalMinHeight()); // Leave as -1 if unset.
	    LayoutUnit maxH = styleToUse->logicalMaxHeight().isUndefined() ?
heightResult : computeLogicalHeightUsing(MaxSize,
styleToUse->logicalMaxHeight());
	    if (maxH == -1)
		maxH = heightResult;
	    heightResult = min(maxH, heightResult);
	    heightResult = max(minH, heightResult);

The helper function (computeLogicalHeightCheckingMinMax?) would take the
"logicalHeight()" as an argument and return the heightResult. Then here in
RenderFlexibleBox, we'd call:
LayoutUnit logicalHeightAfter =
computeLogicalHeightCheckingMinMax(stretchedLogicalHeight);

Then we don't need the computeLogicalHeight call below and we can do the same
thing for replace and non-replaced items.

As a followup, we could also remove the essentially duplicate code in
RenderFlexibleBox::computeAvailableFreeSpace by having it call
computeLogicalHeightCheckingMinMax in the else clause and then adjusting
appropriately for box-sizing.

Tony, WDYT of this solution?