Bug 191358 – [css-grid] Refactoring to make more explicit the orthogonal items' pre-layout logic

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Javier Fernandez]

We need to adapt the code to integrate later the new Baseline Alignment logic, which in latest spec it was included as part of the Track Sizing algorithm. 

Orthogonal items must be laid out using a properly estimated grid area, since the result of this pre-layout may affect during the baseline offsets computation.

[Javier Fernandez]

Created attachment 354085 [details]
Patch

[Manuel Rego Casasnovas]

Comment on attachment 354085 [details]
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.

[Javier Fernandez]

Comment on attachment 354085 [details]
Patch

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

>> 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.

Well, this patch is just a refactoring needed to a follow-up patch to implement the new Baseline Alignment logic. In the next patches, I'll use this function again. 

I think it's useful to have this function, which is auto-explained and avoid the need of additional code comments.

>> 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);
>   }

I don't think this code improves current logic; also, as I commented above, I'd rather keep using the new function.

>> Source/WebCore/rendering/GridTrackSizingAlgorithm.h:172
>> +    // Data
> 
> Is this comment really useful? If you want to keep it add a "." at the end. :-)

Well, we have similar comments all over this header file, so I think is, at least, consistent with current codebase.

>> Source/WebCore/rendering/RenderGrid.cpp:205
>> +            // it.
> 
> Nit: You don't need to line wrap so hard :-)

ok

>> 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.

OK

>> Source/WebCore/rendering/RenderGrid.cpp:890
>> +    // the grid breadth to account for 'box-sizing'.
> 
> Nit: More line wrapping.

OK

[Javier Fernandez]

Created attachment 354514 [details]
Patch

[Manuel Rego Casasnovas]

Comment on attachment 354514 [details]
Patch

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

> Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:605
> +        return isRelativeGridLengthAsAuto(trackSize.minTrackBreadth(), direction);

I'm still not fully convinced of this. We know it's a percentage but we still check it again in isRelativeGridLengthAsAuto(), do we really need to do it twice?

[Javier Fernandez]

Created attachment 354539 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 354539 [details]
Patch

Clearing flags on attachment: 354539

Committed r238114: <https://trac.webkit.org/changeset/238114>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/46009086>