[webkit-reviews] review granted: [Bug 219679] Support aspect-ratio on flexbox items : [Attachment 420958] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Feb 19 07:44:14 PST 2021
Sergio Villar Senin <svillar at igalia.com> has granted Rob Buis
<rbuis at igalia.com>'s request for review:
Bug 219679: Support aspect-ratio on flexbox items
https://bugs.webkit.org/show_bug.cgi?id=219679
Attachment 420958: Patch
https://bugs.webkit.org/attachment.cgi?id=420958&action=review
--- Comment #20 from Sergio Villar Senin <svillar at igalia.com> ---
Comment on attachment 420958
--> https://bugs.webkit.org/attachment.cgi?id=420958
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=420958&action=review
Awesome addition! Just one nit.
> Source/WebCore/rendering/RenderBox.cpp:2947
> + if (computedValues.m_extent != LayoutUnit::max())
Mind adding a comment here explaining that using max() is the way callers have
to tell this method to use a nullopt intrinsicHeight? Maybe even pointing to
the RenderFlexibleBox code.
Code should be self-explanatory but in this case the context is not enough and
I think it pays off to add a comment and ease the job of future reviewers :)
> Source/WebCore/rendering/RenderFlexibleBox.cpp:550
> +}
I guess it's OK to have this here now. I hope we could eventually add the
style().hasAspectRatio() check to the RenderBox::hasAspectRatio() in the
future, but that's a far more intrusive change indeed.
More information about the webkit-reviews
mailing list