[css-grid] Use Optional to represent indefinite lengths

[css-grid] Use Optional to represent indefinite lengths

Blink has been using LayoutUnit(-1) to represent indefinite sizes or more in
general, sizes that cannot be resolved. That's unfortunate because it forces
us to use that magic number and also because negative LayoutUnits are valid
outcomes of some operations.

In our particular case, that was forcing us to check the SizingOperation
argument in the GridTrackSizingAlgorithm to know whether the passed
available and free sizes where actually indefinite or not. Actually that was
not totally correct as this was based on the assumption that we were doing
intrinsic size computations (preferred widths) whenever the sizes where
indefinite which is not correct for all the cases (it's true for widths but
a height:auto grid container would have indefinite height and require
indefinite size computations).

This does not require any additional tests as we are not changing any
behaviour. It's a step forward in the process of decoupling the concepts of
definite/indefinite sizes and intrinsicSize/layout operations.

Review-Url: https://codereview.chromium.org/2808453002
Cr-Commit-Position: refs/heads/master@{#462881}
Committed: https://chromium.googlesource.com/chromium/src/+/55b4817a2d8e7d13f51fd30b461c946671361d76

[svillar, 2017-04-07 10:01:37]

svillar@igalia.com changed reviewers:
+ jfernandez@igalia.com, rego@igalia.com

[jfernandez, 2017-04-07 11:48:57]

https://codereview.chromium.org/2808453002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/GridTrackSizingAlgorithm.cpp (right):

https://codereview.chromium.org/2808453002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/GridTrackSizingAlgorithm.cpp:1334: if
(this->freeSpace(m_direction)) {
Is possible that freeSpace(m_direction) is null ? If not, we might add an assert
here and avoid the conditional clause.

https://codereview.chromium.org/2808453002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/GridTrackSizingAlgorithm.cpp:1385: if
(availableSpace) {
Wouldn't be clearer this way ? 

DCHECK(!!availableSpace == !!freeSpace) or perhaps using an expression based on
the xor operator ?

Then we can simplify the conditional clause:

m_strategy = availableSpace 
    ? WTF::makeUnique<DefiniteSizeStrategy>(*this)
    : WTF::makeUnique<IndefiniteSizeStrategy>(*this)

https://codereview.chromium.org/2808453002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/GridTrackSizingAlgorithm.cpp:1396:
setFreeSpace(direction, freeSpace);
Do we want to set a new freeSpace if it's null ?

https://codereview.chromium.org/2808453002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/GridTrackSizingAlgorithm.cpp:1429:
m_strategy->maximizeTracks(tracks(m_direction), m_direction == ForColumns
Didn't we define a freeSpace(direction) ?

https://codereview.chromium.org/2808453002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/2808453002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1049: Optional<LayoutUnit>
freeSpace = m_trackSizingAlgorithm.freeSpace(direction);
Why are you changing the variable name here ? We were using both concepts in a
different way in other parts of the code. I think we should be consistent when
using these concepts.

https://codereview.chromium.org/2808453002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1074: LayoutUnit
sizeToIncrease = freeSpace.value() / numberOfAutoSizedTracks;
Are we sure freeSpace is non-null here ?

https://codereview.chromium.org/2808453002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1347: direction,
m_trackSizingAlgorithm.freeSpace(direction).value(),
ditto

[svillar, 2017-04-07 13:55:23]

Thanks for the review. I'll upload a modified version.

https://codereview.chromium.org/2808453002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/GridTrackSizingAlgorithm.cpp (right):

https://codereview.chromium.org/2808453002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/GridTrackSizingAlgorithm.cpp:1334: if
(this->freeSpace(m_direction)) {
On 2017/04/07 11:48:57, jfernandez wrote:
> Is possible that freeSpace(m_direction) is null ? If not, we might add an
assert
> here and avoid the conditional clause.

It's absolutely possible. Note that we have 2 code paths for the flex tracks
resolution, see above how we call different methods of the m_strategy.

It was not needed in the past because instead of passing an indefinite freespace
we were passing Layout(), so we didn't have to check it.

https://codereview.chromium.org/2808453002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/GridTrackSizingAlgorithm.cpp:1385: if
(availableSpace) {
On 2017/04/07 11:48:57, jfernandez wrote:
> Wouldn't be clearer this way ? 
> 
> DCHECK(!!availableSpace == !!freeSpace) or perhaps using an expression based
on
> the xor operator ?
> 
> Then we can simplify the conditional clause:
> 
> m_strategy = availableSpace 
>     ? WTF::makeUnique<DefiniteSizeStrategy>(*this)
>     : WTF::makeUnique<IndefiniteSizeStrategy>(*this)

Not sure if it's clearer. The if else clause is pretty simple to understand IMO.
I'll do it as you suggest as it is a bit more compact.

https://codereview.chromium.org/2808453002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/GridTrackSizingAlgorithm.cpp:1396:
setFreeSpace(direction, freeSpace);
On 2017/04/07 11:48:57, jfernandez wrote:
> Do we want to set a new freeSpace if it's null ?

Yes. Note that the same track sizing algorithm instance is used to size columns
and rows, so we need to overwritte the contents always to be safe.

https://codereview.chromium.org/2808453002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/GridTrackSizingAlgorithm.cpp:1429:
m_strategy->maximizeTracks(tracks(m_direction), m_direction == ForColumns
On 2017/04/07 11:48:57, jfernandez wrote:
> Didn't we define a freeSpace(direction) ? 

Right but that does not return a reference. What we want here is to pass a
reference to allow maximizeTracks to directly modify the attribute.

https://codereview.chromium.org/2808453002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/2808453002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1049: Optional<LayoutUnit>
freeSpace = m_trackSizingAlgorithm.freeSpace(direction);
On 2017/04/07 11:48:57, jfernandez wrote:
> Why are you changing the variable name here ? We were using both concepts in a
> different way in other parts of the code. I think we should be consistent when
> using these concepts.

Because what we really have here is the free space. The available space is a
different thing[1].

I agree we should be consistent but in this case it was wrong.

[1] https://drafts.csswg.org/css-sizing-3/#available

https://codereview.chromium.org/2808453002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1074: LayoutUnit
sizeToIncrease = freeSpace.value() / numberOfAutoSizedTracks;
On 2017/04/07 11:48:57, jfernandez wrote:
> Are we sure freeSpace is non-null here ?

Yes because there is a !freeSpace check at the very beginning. That's why I
didn't add a DCHECK here.

https://codereview.chromium.org/2808453002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1347: direction,
m_trackSizingAlgorithm.freeSpace(direction).value(),
On 2017/04/07 11:48:57, jfernandez wrote:
> ditto

We are sure there is a definite free space because this function is only called
as part of the layout process. At this point the track algorithm has already run
and the free space is definite.

[svillar, 2017-04-07 14:03:16]

https://codereview.chromium.org/2808453002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/GridTrackSizingAlgorithm.cpp (right):

https://codereview.chromium.org/2808453002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/GridTrackSizingAlgorithm.cpp:1385: if
(availableSpace) {
On 2017/04/07 11:48:57, jfernandez wrote:
> Then we can simplify the conditional clause:
> 
> m_strategy = availableSpace 
>     ? WTF::makeUnique<DefiniteSizeStrategy>(*this)
>     : WTF::makeUnique<IndefiniteSizeStrategy>(*this)

Hmm, aparently I cannot do it as the types of the branches are different. So I'm
going back to using an if-else clause.

[jfernandez, 2017-04-07 14:13:31]

lgtm

https://codereview.chromium.org/2808453002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/GridTrackSizingAlgorithm.cpp (right):

https://codereview.chromium.org/2808453002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/GridTrackSizingAlgorithm.cpp:1385: if
(availableSpace) {
On 2017/04/07 14:03:16, svillar wrote:
> On 2017/04/07 11:48:57, jfernandez wrote:
> > Then we can simplify the conditional clause:
> > 
> > m_strategy = availableSpace 
> >     ? WTF::makeUnique<DefiniteSizeStrategy>(*this)
> >     : WTF::makeUnique<IndefiniteSizeStrategy>(*this)
> 
> Hmm, aparently I cannot do it as the types of the branches are different. So
I'm
> going back to using an if-else clause.

In any case, I think it's clearer to have a single DCHECK outside the if-else
clause, but Im not against to the current code if you disagree.

[svillar, 2017-04-07 14:49:15]

The CQ bit was checked by svillar@igalia.com

[svillar, 2017-04-07 14:49:16]

The patchset sent to the CQ was uploaded after l-g-t-m from
jfernandez@igalia.com
Link to the patchset: https://codereview.chromium.org/2808453002/#ps20001
(title: "Changes after review")

[commit-bot: I haz the power, 2017-04-07 14:49:35]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[svillar, 2017-04-07 14:49:58]

On 2017/04/07 14:13:31, jfernandez wrote:
> lgtm
> 
>
https://codereview.chromium.org/2808453002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/layout/GridTrackSizingAlgorithm.cpp
(right):
> 
>
https://codereview.chromium.org/2808453002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/layout/GridTrackSizingAlgorithm.cpp:1385: if
> (availableSpace) {
> On 2017/04/07 14:03:16, svillar wrote:
> > On 2017/04/07 11:48:57, jfernandez wrote:
> > > Then we can simplify the conditional clause:
> > > 
> > > m_strategy = availableSpace 
> > >     ? WTF::makeUnique<DefiniteSizeStrategy>(*this)
> > >     : WTF::makeUnique<IndefiniteSizeStrategy>(*this)
> > 
> > Hmm, aparently I cannot do it as the types of the branches are different. So
> I'm
> > going back to using an if-else clause.
> 
> In any case, I think it's clearer to have a single DCHECK outside the if-else
> clause, but Im not against to the current code if you disagree.

I did the DCHECK change but not the ternary operator one.

[commit-bot: I haz the power, 2017-04-07 16:15:10]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1491576555541440,
"parent_rev": "e148e7ab0ffb1b5facad8c6c18a34af43f3bc1b3", "commit_rev":
"55b4817a2d8e7d13f51fd30b461c946671361d76"}

[commit-bot: I haz the power, 2017-04-07 16:16:00]

Description was changed from

==========
[css-grid] Use Optional to represent indefinite lengths

Blink has been using LayoutUnit(-1) to represent indefinite sizes or more in
general, sizes that cannot be resolved. That's unfortunate because it forces
us to use that magic number and also because negative LayoutUnits are valid
outcomes of some operations.

In our particular case, that was forcing us to check the SizingOperation
argument in the GridTrackSizingAlgorithm to know whether the passed
available and free sizes where actually indefinite or not. Actually that was
not totally correct as this was based on the assumption that we were doing
intrinsic size computations (preferred widths) whenever the sizes where
indefinite which is not correct for all the cases (it's true for widths but
a height:auto grid container would have indefinite height and require
indefinite size computations).

This does not require any additional tests as we are not changing any
behaviour. It's a step forward in the process of decoupling the concepts of
definite/indefinite sizes and intrinsicSize/layout operations.
==========

to

==========
[css-grid] Use Optional to represent indefinite lengths

Blink has been using LayoutUnit(-1) to represent indefinite sizes or more in
general, sizes that cannot be resolved. That's unfortunate because it forces
us to use that magic number and also because negative LayoutUnits are valid
outcomes of some operations.

In our particular case, that was forcing us to check the SizingOperation
argument in the GridTrackSizingAlgorithm to know whether the passed
available and free sizes where actually indefinite or not. Actually that was
not totally correct as this was based on the assumption that we were doing
intrinsic size computations (preferred widths) whenever the sizes where
indefinite which is not correct for all the cases (it's true for widths but
a height:auto grid container would have indefinite height and require
indefinite size computations).

This does not require any additional tests as we are not changing any
behaviour. It's a step forward in the process of decoupling the concepts of
definite/indefinite sizes and intrinsicSize/layout operations.

Review-Url: https://codereview.chromium.org/2808453002
Cr-Commit-Position: refs/heads/master@{#462881}
Committed:
https://chromium.googlesource.com/chromium/src/+/55b4817a2d8e7d13f51fd30b461c...
==========

[commit-bot: I haz the power, 2017-04-07 16:16:01]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/55b4817a2d8e7d13f51fd30b461c...