[webkit-reviews] review granted: [Bug 191358] [css-grid] Refactoring to make more explicit the orthogonal items' pre-layout logic : [Attachment 354085] Patch

[bugzilla-daemon at webkit.org]

Manuel Rego Casasnovas <rego at igalia.com> has granted Javier Fernandez
<jfernandez at igalia.com>'s request for review:
Bug 191358: [css-grid] Refactoring to make more explicit the orthogonal items'
pre-layout logic
https://bugs.webkit.org/show_bug.cgi?id=191358

Attachment 354085: Patch

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




--- Comment #2 from Manuel Rego Casasnovas <rego at igalia.com> ---
Comment on attachment 354085
  --> https://bugs.webkit.org/attachment.cgi?id=354085
Patch

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

LGTM, but I added some minor comments inline.

> Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:602
> +bool GridTrackSizingAlgorithm::isRelativeSizedTrackAsAuto(const
GridTrackSize& trackSize, GridTrackSizingDirection direction) const

This method is only used in one place, do we really need it? We call it and
later we recheck the percentages things, I'm not sure if it's really adding any
value.

> Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:626
> +    if (isRelativeSizedTrackAsAuto(trackSize, direction)) {

I'd just modify this to something like:
  if (!availableSpace(direction)) {
    if (minTrackBreadth.isPercentage())
      minTrackBreadth = Length(Auto);
    if (maxTrackBreadth.isPercentage())
      maxTrackBreadth = Length(Auto);
  }

> Source/WebCore/rendering/GridTrackSizingAlgorithm.h:172
> +    // Data

Is this comment really useful? If you want to keep it add a "." at the end. :-)

> Source/WebCore/rendering/RenderGrid.cpp:205
> +	       // Grid's layout logic controls the grid item's override height,
hence
> +	       // we need to clear any override height set previously, so it
doesn't
> +	       // interfere in current layout execution.
> +	       // Grid never uses the override width, that's why we don't need
to clear
> +	       // it.

Nit: You don't need to line wrap so hard :-)

> Source/WebCore/rendering/RenderGrid.cpp:209
> +	       // We may need to repeat the track sizing in case of any grid
item was
> +	       // orthogonal.

Ditto about line wrapping.

> Source/WebCore/rendering/RenderGrid.cpp:648
> +    // FIXME(jfernandez): We need a way when we are calling this during
intrinsic size compuation before performing

Mmm, it seems some word is missing in this sentence.
Nit: Don't remember if this FIXME(jfernandez) is common in WebKit, or just a
simple FIXME.

> Source/WebCore/rendering/RenderGrid.cpp:890
> +    // Because the grid area cannot be styled, we don't need to adjust
> +    // the grid breadth to account for 'box-sizing'.

Nit: More line wrapping.