[webkit-reviews] review granted: [Bug 94237] Fix cross-direction stretch for replaced elements in row flexbox : [Attachment 159707] Patch

[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 94237: Fix cross-direction stretch for replaced elements in row flexbox
https://bugs.webkit.org/show_bug.cgi?id=94237

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

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


Looks great! Please address my last couple comments then feel free to commit.

> Source/WebCore/rendering/RenderBox.cpp:2018
> +	       heightResult =
logicalHeightConstrainedByMinMax(logicalHeight());
>	   } else {

Nit: Single line if's shouldn't have curly braces, even if there is an else
clause.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:1241
> +	   if (logicalHeightAfter != logicalHeightBefore) {

Nit: lets get rid of logicalHeightBefore and just call child->logicalHeight()
here. Before this patch, we needed to save it out first because
computeLogicalHeight actually modifies it.

> LayoutTests/css3/flexbox/flexitem.html:111
> +<div class="flexbox column" style="height:210px">

Can we add this last test-case for the row flows as well? Just to make sure all
these cases work (and if not, having test coverage showing which cases don't).