[webkit-reviews] review granted: [Bug 94604] Fix cross-direction stretch for replaced elements in column flexbox : [Attachment 160237] Patch (with changes from comment 2)

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has granted 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 160237: Patch (with changes from comment 2)
https://bugs.webkit.org/attachment.cgi?id=160237&action=review

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


Looks great. Just a few nits.

> Source/WebCore/rendering/RenderBox.cpp:440
> +	   // Constrain by MaxSize.

I know the old code had these comments, but they're just stating what the code
obviously does. Mind deleting these while you're in here?

> Source/WebCore/rendering/RenderBox.cpp:443
> +    // Constrain by MinSize.

ditto

> Source/WebCore/rendering/RenderBox.cpp:1702
> +	   // Calculate LogicalWidth constrained by MaxLogicalWidth and
MinLogicalWidth

ditto

> LayoutTests/css3/flexbox/flexitem.html:130
> +    <img data-expected-display="block" data-expected-height="60"
style="min-height:60px" src="../images/resources/blue-100.png">

I think we have enough coverage of the expected-display. Mind remove this from
these new tests? It adds clutter that makes it harder to see what the test
actually cares about.

> LayoutTests/platform/chromium/TestExpectations:-3513
> -BUGWK94675 : css3/flexbox/flexitem.html = TEXT

TL;DR: I think we still need this line for now.

I think there's still the issue with the image cases that are flakily giving
the wrong results, so I think we need to leave this in until we figure out
what's going wrong there. I was hoping that once we got this checked in it
would filter out the cases that were due to the column-flow issue so we can see
where the flaky image failures happen across platforms and try to reproduce it
locally.