[webkit-reviews] review granted: [Bug 214292] WPT test css/css-flexbox/flex-minimum-height-flex-items-023.html fails : [Attachment 421914] Patch

[bugzilla-daemon at webkit.org]

Javier Fernandez <jfernandez at igalia.com> has granted Sergio Villar Senin
<svillar at igalia.com>'s request for review:
Bug 214292: WPT test css/css-flexbox/flex-minimum-height-flex-items-023.html
fails
https://bugs.webkit.org/show_bug.cgi?id=214292

Attachment 421914: Patch

https://bugs.webkit.org/attachment.cgi?id=421914&action=review




--- Comment #9 from Javier Fernandez <jfernandez at igalia.com> ---
Comment on attachment 421914
  --> https://bugs.webkit.org/attachment.cgi?id=421914
Patch

View in context: https://b.webkit.org/attachment.cgi?id=421914&action=review

r=me

> Source/WebCore/rendering/RenderFlexibleBox.cpp:433
> +    if (!minSize.isAuto() && !childBlockSizeIsEquivalentToAutomaticSize)

Just a suggestion, not a strong opinion; what about this ways ?

return minSize.IsAuto() || childBlockSizeIsEquivalentToAutomaticSize() ?
mainAxisOverflowForChild(child) == Overflow::Visible : false;

I wouldn't mind to use an if instead, but using the an affirmative clause, like
this:

if (minSize.IsAuto() || childBlockSizeIsEquivalentToAutomaticSize())
  return mainAxisOverflowForChild(child) == Overflow::Visible;

return false;

Or even better, like this

return (minSize.IsAuto() || childBlockSizeIsEquivalentToAutomaticSize()) &&
mainAxisOverflowForChild(child) == Overflow::Visible;