[webkit-reviews] review denied: [Bug 94604] Fix cross-direction stretch for replaced elements in column flexbox : [Attachment 160020] 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 94604: Fix cross-direction stretch for replaced elements in column flexbox
https://bugs.webkit.org/show_bug.cgi?id=94604

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

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
Can you add test cases that set min-width/max-width to a pixel value, a percent
value, fit-content, min-content, max-content and fill-available? See
http://dev.w3.org/csswg/css3-sizing/ if you're unfamilar with the last four of
those (or just see the test-cases we have for those).

Also, sigh...as I look at this more closely, I think maybe our last patch was
not the best. Instead of logicalHeightConstrainedByMinMax, we should have added
constrainLogicalHeightByMinMax, which just takes a height value and constrains
it:
    LayoutUnit minH = computeLogicalHeightUsing(MinSize,
styleToUse->logicalMinHeight()); // Leave as -1 if unset.
    LayoutUnit maxH = styleToUse->logicalMaxHeight().isUndefined() ? result :
computeLogicalHeightUsing(MaxSize, styleToUse->logicalMaxHeight());
    if (maxH == -1)
	maxH = result;
    result = min(maxH, result);
    result = max(minH, result);
    return result;

Since, in this case, we know the height is auto, we don't need to do the first
few lines that are currently at the beginning of
logicalHeightConstrainedByMinMax. Also, we'll need this same stretching logic
for RenderGrid only if the height/width is auto. So, I'm thinking the above
change makes sense, WDYT?

Then we could get the logicalWidth case to do something similar so we can share
more code with RenderBox:
1. Add a constrainLogicalWidthByMinMax(availableWidth) that does the equivalent
of constrainLogicalHeightByMinMax.
2. Restructure RenderBox::computeLogicalWidthInRegion to use
constrainLogicalHeightByMinMax.
3. Use constrainLogicalHeightByMinMax here in RenderFlexibleBox.

Then you don't need computeStretchedLogicalWidthUsing and you'll handle the
above test-cases I suggested correctly.