[webkit-reviews] review granted: [Bug 210089] [css-flexbox] min-height: auto not applied to nested flexboxes. : [Attachment 407755] Patch

[bugzilla-daemon at webkit.org]

Daniel Bates <dbates at webkit.org> has granted Sergio Villar Senin
<svillar at igalia.com>'s request for review:
Bug 210089: [css-flexbox] min-height: auto not applied to nested flexboxes.
https://bugs.webkit.org/show_bug.cgi?id=210089

Attachment 407755: Patch

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




--- Comment #8 from Daniel Bates <dbates at webkit.org> ---
Comment on attachment 407755
  --> https://bugs.webkit.org/attachment.cgi?id=407755
Patch

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

Patch looks good. Optional, check the spelling of the ChangeLog and in comments
check for sentence capitalization.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:1581
> +    Length childMinSize = isHorizontalFlow() ? child.style().minWidth() :
child.style().minHeight();
> +    Length childMaxSize = isHorizontalFlow() ? child.style().maxWidth() :
child.style().maxHeight();

OK as-is. No need to change anything. 

Just for me: future cleanup idea; just noticed the same result can be achieved
using one branch. Also these locals could be moved after the
!mainAxisLengthIsDefinite branch since they aren't used if that evaluates to
true.

> Source/WebCore/rendering/RenderFlexibleBox.h:119
>      Length flexBasisForChild(const RenderBox& child) const;
> +    bool shouldApplyMinSizeAutoForChild(const RenderBox& child) const;
>      LayoutUnit crossAxisExtentForChild(const RenderBox& child) const;
>      LayoutUnit crossAxisIntrinsicExtentForChild(const RenderBox& child)
const;
>      LayoutUnit childIntrinsicLogicalHeight(const RenderBox& child) const;

OK as-is. No change needed.

Just for me: future cleanup idea; the parameter name can be removed because its
purpose is encoded in the names of these functions.