Bug 222376 – [css-grid] Replace the use of -1 with WTF::nullopt

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

It might make sense to replace the use of -1 with WTF::nullopt in grid item

Created attachment 421441 [details]
Patch

[Javier Fernandez]

Comment on attachment 421441 [details]
Patch

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

I bealive there are other functions using -1 as indefinite; I think this patch should address all of them. 

See GridTrackSizingAlgorithm::estimatedGridAreaBreadthForChild for instance.

> Source/WebCore/ChangeLog:7
> +

We should explain for each change why we need nullopt instead; those functions return an Optional type and we consider nullopt as indefinite, as we did before with the -1 value. 

It's also need to mention why this change doesn't need new tests, or changes on the already existing ones.
In this case, since we don't expect a behavior change, we don't need to provide tests.

https://bugs.webkit.org/show_bug.cgi?id=221439 has addressed and fixed issue mentioned here. Maybe we can close this one?

[Radar WebKit Bug Importer]

<rdar://problem/74997975>

Created attachment 422556 [details]
Patch

Created attachment 422559 [details]
Patch

[Sergio Villar Senin]

Comment on attachment 422559 [details]
Patch

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

Definitely a step in the right direction. There are some changes that need to be done though

> Source/WebCore/ChangeLog:6
> +        This change replaces -1 with nullopt in grid to indicate indefinte for funtions that

nit: indefinite

> Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:912
> +        overrideSize = WTF::nullopt;

Instead of doing this, change estimatedGridAreaBreadthForChild() so that it returns an Optional<LayoutUnit> and make it return nullopt instead of -1_lu

> Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:1064
> +            overridingSize = WTF::nullopt;

You can do it with a ternary operator:
auto overridingSize = direction() == childInlineDirection ? makeOptional(0_lu) : WTF::nullopt;

In that case maybe you don't even need the overridingSize variable and pass the value directly as argument to setOverriding...

> Source/WebCore/rendering/RenderGrid.cpp:928
>  }

If we change estimatedGridAreaBreadthForChild() as I suggested above then we don't need this change.

Created attachment 422597 [details]
Patch

(In reply to Sergio Villar Senin from comment #7)
> Comment on attachment 422559 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=422559&action=review
> 
> Definitely a step in the right direction. There are some changes that need
> to be done though
> 
> > Source/WebCore/ChangeLog:6
> > +        This change replaces -1 with nullopt in grid to indicate indefinte for funtions that
> 
> nit: indefinite
> 
Corrected.

> > Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:912
> > +        overrideSize = WTF::nullopt;
> 
> Instead of doing this, change estimatedGridAreaBreadthForChild() so that it
> returns an Optional<LayoutUnit> and make it return nullopt instead of -1_lu
> 
I tried this approach. The main issue is that LayoutUnit GridTrackSizingAlgorithm::estimatedGridAreaBreadthForChild(const RenderBox& child, GridTrackSizingDirection direction) is called by LayoutSize GridTrackSizingAlgorithm::estimatedGridAreaBreadthForChild(const RenderBox& child) const. LayoutSize doesn't like Optional<LayoutUnit>, especially when it's WTF::nullopt. I'm not sure how feasible this approach is. Any suggestions?

> > Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:1064
> > +            overridingSize = WTF::nullopt;
> 
> You can do it with a ternary operator:
> auto overridingSize = direction() == childInlineDirection ?
> makeOptional(0_lu) : WTF::nullopt;
> 
> In that case maybe you don't even need the overridingSize variable and pass
> the value directly as argument to setOverriding...
> 
Updated. Thanks very much for the suggestion!

> > Source/WebCore/rendering/RenderGrid.cpp:928
> >  }
> 
> If we change estimatedGridAreaBreadthForChild() as I suggested above then we
> don't need this change.

[Javier Fernandez]

Comment on attachment 422559 [details]
Patch

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

>>> Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:912
>>> +        overrideSize = WTF::nullopt;
>> 
>> Instead of doing this, change estimatedGridAreaBreadthForChild() so that it returns an Optional<LayoutUnit> and make it return nullopt instead of -1_lu
> 
> I tried this approach. The main issue is that LayoutUnit GridTrackSizingAlgorithm::estimatedGridAreaBreadthForChild(const RenderBox& child, GridTrackSizingDirection direction) is called by LayoutSize GridTrackSizingAlgorithm::estimatedGridAreaBreadthForChild(const RenderBox& child) const. LayoutSize doesn't like Optional<LayoutUnit>, especially when it's WTF::nullopt. I'm not sure how feasible this approach is. Any suggestions?

Umm, the only solution I see now is that we remove the estimatedGridAreaBreadthForChild(child), and the caller takes the responsibility of building the LayoutSize instance, based on the results of the function; this should also be the responsible of handling the nullopt. I'm not sure how big would be the change, though.

Created attachment 422684 [details]
Patch

[Javier Fernandez]

Comment on attachment 422684 [details]
Patch

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

> Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:554
> +Optional<LayoutUnit> GridTrackSizingAlgorithm::estimatedGridAreaBreadthForChildForColumns(const RenderBox& child) const

Perhaps we don't need this function; we can just calling the generic function with the appropriated argument when needed.

> Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:558
> +Optional<LayoutUnit> GridTrackSizingAlgorithm::estimatedGridAreaBreadthForChildForRows(const RenderBox& child) const

Ditto

> Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:839
> +    return minLogicalSizeForChild(child, childMinSize, gridAreaSize.valueOr(0_lu)) + baselineShim;

I'm not sure about this valurOr(0); it might be the root cause of the layout tests failures we have in the baseline alignment related tests.
I think this might be wrong when dealing with orthogonal grid items, so that we need nullopt here to detect indefinite size.

Created attachment 422930 [details]
Patch

[Sergio Villar Senin]

Comment on attachment 422930 [details]
Patch

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

Seems like you have to update the failed expectations of a test that is failing.

Looking great, I'm happy that we get rid of the -1's. It's a pitty that we cannot use LayoutSize and have to use width and height separatedely but it isn't a big deal.

> Source/WebCore/ChangeLog:7
> +        returns an Optional type. This change has exposed an issue when resolving repalced elemnt

Nit: replaced element

> Source/WebCore/ChangeLog:11
> +         

Extra line here. BTW this paragraph should go bellow the "Reviewed by" line

Created attachment 423023 [details]
Patch

(In reply to Javier Fernandez from comment #12)
> Comment on attachment 422684 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=422684&action=review
> 
> > Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:554
> > +Optional<LayoutUnit> GridTrackSizingAlgorithm::estimatedGridAreaBreadthForChildForColumns(const RenderBox& child) const
> 
> Perhaps we don't need this function; we can just calling the generic
> function with the appropriated argument when needed.
> 
> > Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:558
> > +Optional<LayoutUnit> GridTrackSizingAlgorithm::estimatedGridAreaBreadthForChildForRows(const RenderBox& child) const
> 
> Ditto
> 
> > Source/WebCore/rendering/GridTrackSizingAlgorithm.cpp:839
> > +    return minLogicalSizeForChild(child, childMinSize, gridAreaSize.valueOr(0_lu)) + baselineShim;
> 
> I'm not sure about this valurOr(0); it might be the root cause of the layout
> tests failures we have in the baseline alignment related tests.
> I think this might be wrong when dealing with orthogonal grid items, so that
> we need nullopt here to detect indefinite size.

Updated. Change the type to Optional.

(In reply to Sergio Villar Senin from comment #14)
> Comment on attachment 422930 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=422930&action=review
> 
> Seems like you have to update the failed expectations of a test that is
> failing.
> 
Updated. This test is fixing at https://bugs.webkit.org/show_bug.cgi?id=191627.

> Looking great, I'm happy that we get rid of the -1's. It's a pitty that we
> cannot use LayoutSize and have to use width and height separatedely but it
> isn't a big deal.
> 
> > Source/WebCore/ChangeLog:7
> > +        returns an Optional type. This change has exposed an issue when resolving repalced elemnt
> 
> Nit: replaced element
> 
> > Source/WebCore/ChangeLog:11
> > +         
> 
> Extra line here. BTW this paragraph should go bellow the "Reviewed by" line

Updated. Thank you!

Created attachment 423033 [details]
Patch

[Javier Fernandez]

Comment on attachment 423033 [details]
Patch

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

r=me

> Source/WebCore/ChangeLog:8
> +        This change replaces -1 with nullopt in grid to indicate indefinite for funtions that

s/funtions/functions

> Source/WebCore/ChangeLog:9
> +        returns an Optional type. This change has exposed an issue when resolving replaced elemnt

s/elemnt/element

> Source/WebCore/rendering/RenderReplaced.cpp:357
>          if (hasAutoHeightOrContainingBlockWithAutoHeight())

Nit: Maybe we can change this as well, something like this:

return !hasAutoHeightOrContainingBlockWithAutoHeight()

Created attachment 423222 [details]
Patch

(In reply to Javier Fernandez from comment #19)
> Comment on attachment 423033 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=423033&action=review
> 
> r=me
> 
> > Source/WebCore/ChangeLog:8
> > +        This change replaces -1 with nullopt in grid to indicate indefinite for funtions that
> 
> s/funtions/functions
> 
> > Source/WebCore/ChangeLog:9
> > +        returns an Optional type. This change has exposed an issue when resolving replaced elemnt
> 
> s/elemnt/element
> 
> > Source/WebCore/rendering/RenderReplaced.cpp:357
> >          if (hasAutoHeightOrContainingBlockWithAutoHeight())
> 
> Nit: Maybe we can change this as well, something like this:
> 
> return !hasAutoHeightOrContainingBlockWithAutoHeight()

Updated. Thanks!

[EWS]

Committed r274477: <https://commits.webkit.org/r274477>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 423222 [details].