[CSS Grid Layout] Flex tracks sizing alg must handle 0fr values

[CSS Grid Layout] Flex tracks sizing alg must handle 0fr values

We don't allow 0 as flexible size value, which is not following current
specs; it just states that it must be a non-negative value. This patch
adds such change in the parser but some additional logic must be added
as well to handle 0 values during the flex tracks sizing algorithm.

The old algorithm didn't take 0 values into account, so there is the risk
of division by zero. Additionally, it was not handling fraction values
in the best way. The last versions of the spec changed this algorithm in
order to handle fraction values so that they don't cause exponential
grow of tracks using values bigger than 1.

This patch implements also such new algorithm, so we can deal not only
with 0 values, but managing fraction values properly.

BUG=520477
Committed: https://crrev.com/993d4f780cb06a25b21613fead8fceca12dcdf6a
Cr-Commit-Position: refs/heads/master@{#351301}

[jfernandez, 2015-09-07 10:23:53]

jfernandez@igalia.com changed reviewers:
+ dsinclair@chromium.org, esprehn@chromium.org, rego@igalia.com,
svillar@igalia.com

[svillar, 2015-09-08 13:27:07]

Really nice patch. It simplifies a lot the fr resolution code and makes it far
more understandable. I was tempted to give it a go but I'm a bit reluctant due
to the use of a vector to pass the list of inflexible tracks. First of all a
vector is not the best structure to perform searches (we do a contains()) and
secondly in theory we could call the method recursively forever as there is no
restriction that we can only call it once. 

Perhaps we need an auxiliary method that would:

1) create the proper data structure for inflexible tracks
2) call the recursive one
3) check the condition
4) call it again if the condition is false
5) return the fr value

What do you think?

PS: fix the typos in the description

https://codereview.chromium.org/1323053004/diff/40001/LayoutTests/fast/css-gr...
File
LayoutTests/fast/css-grid-layout/flex-and-minmax-content-resolution-rows.html
(right):

https://codereview.chromium.org/1323053004/diff/40001/LayoutTests/fast/css-gr...
LayoutTests/fast/css-grid-layout/flex-and-minmax-content-resolution-rows.html:133:
<div class="sizedToGridArea secondRowFirstColumn" data-expected-width="50"
data-expected-height="80"></div>
Shouldn't it be x4 the one in first row?

https://codereview.chromium.org/1323053004/diff/40001/LayoutTests/fast/css-gr...
File LayoutTests/fast/css-grid-layout/flex-content-resolution-columns.html
(right):

https://codereview.chromium.org/1323053004/diff/40001/LayoutTests/fast/css-gr...
LayoutTests/fast/css-grid-layout/flex-content-resolution-columns.html:181: <!--
Not enough space to repsect proportions, because minTrackBreadh it's a harder
requirement -->
Nit: respect

https://codereview.chromium.org/1323053004/diff/40001/LayoutTests/fast/css-gr...
File LayoutTests/fast/css-grid-layout/flex-content-resolution-rows.html (right):

https://codereview.chromium.org/1323053004/diff/40001/LayoutTests/fast/css-gr...
LayoutTests/fast/css-grid-layout/flex-content-resolution-rows.html:174: <div
class="sizedToGridArea secondRowFirstColumn" data-expected-width="50"
data-expected-height="20"></div>
Shouldn't it be x4 the one in first row?

https://codereview.chromium.org/1323053004/diff/40001/LayoutTests/fast/css-gr...
LayoutTests/fast/css-grid-layout/flex-content-resolution-rows.html:189: <div
class="sizedToGridArea secondRowFirstColumn" data-expected-width="50"
data-expected-height="20"></div>
Ditto.

https://codereview.chromium.org/1323053004/diff/40001/LayoutTests/fast/css-gr...
LayoutTests/fast/css-grid-layout/flex-content-resolution-rows.html:203: <div
class="sizedToGridArea secondRowFirstColumn" data-expected-width="50"
data-expected-height="20"></div>
Ditto.

https://codereview.chromium.org/1323053004/diff/40001/LayoutTests/fast/css-gr...
LayoutTests/fast/css-grid-layout/flex-content-resolution-rows.html:256: <!-- Not
enough space to repsect proportions, because minTrackBreadh it's a harder
requirement -->
Nit: respect

[jfernandez, 2015-09-09 09:22:31]

On 2015/09/08 13:27:07, svillar wrote:
> Really nice patch. It simplifies a lot the fr resolution code and makes it far
> more understandable. I was tempted to give it a go but I'm a bit reluctant due
> to the use of a vector to pass the list of inflexible tracks. First of all a
> vector is not the best structure to perform searches (we do a contains())
 
Yes, I agree. Perhaps I can use a Set using the track index as key.

> and secondly in theory we could call the method recursively forever as there
is no
> restriction that we can only call it once. 
> 

I don't think that's the case. First of all, the spec does not sate that we must
limit
recursion to 1 iteration. About your concern on infinite recursion, I don't
think it can
happen either, since we use the index vector to exclude tracks from being
processed,
ending the process when there is no track left or the ones remaining satisfy the
exit
condition.

> PS: fix the typos in the description
> 

Done.

[jfernandez, 2015-09-09 09:23:22]

Replied to your review comments and preparing a new patch with the suggested
changes.

https://codereview.chromium.org/1323053004/diff/40001/LayoutTests/fast/css-gr...
File
LayoutTests/fast/css-grid-layout/flex-and-minmax-content-resolution-rows.html
(right):

https://codereview.chromium.org/1323053004/diff/40001/LayoutTests/fast/css-gr...
LayoutTests/fast/css-grid-layout/flex-and-minmax-content-resolution-rows.html:133:
<div class="sizedToGridArea secondRowFirstColumn" data-expected-width="50"
data-expected-height="80"></div>
On 2015/09/08 13:27:07, svillar wrote:
> Shouldn't it be x4 the one in first row?

Actually not. Specs want fraction units to be normalized so they don't cause
exponential grow of tracks with values greater than 1.  However, I agree with
you this deserve a comment in the test because it's really confusing.

https://codereview.chromium.org/1323053004/diff/40001/LayoutTests/fast/css-gr...
File LayoutTests/fast/css-grid-layout/flex-content-resolution-columns.html
(right):

https://codereview.chromium.org/1323053004/diff/40001/LayoutTests/fast/css-gr...
LayoutTests/fast/css-grid-layout/flex-content-resolution-columns.html:181: <!--
Not enough space to repsect proportions, because minTrackBreadh it's a harder
requirement -->
On 2015/09/08 13:27:07, svillar wrote:
> Nit: respect

Done.

https://codereview.chromium.org/1323053004/diff/40001/LayoutTests/fast/css-gr...
File LayoutTests/fast/css-grid-layout/flex-content-resolution-rows.html (right):

https://codereview.chromium.org/1323053004/diff/40001/LayoutTests/fast/css-gr...
LayoutTests/fast/css-grid-layout/flex-content-resolution-rows.html:174: <div
class="sizedToGridArea secondRowFirstColumn" data-expected-width="50"
data-expected-height="20"></div>
On 2015/09/08 13:27:07, svillar wrote:
> Shouldn't it be x4 the one in first row?

Already replied in the other test.

https://codereview.chromium.org/1323053004/diff/40001/LayoutTests/fast/css-gr...
LayoutTests/fast/css-grid-layout/flex-content-resolution-rows.html:189: <div
class="sizedToGridArea secondRowFirstColumn" data-expected-width="50"
data-expected-height="20"></div>
On 2015/09/08 13:27:07, svillar wrote:
> Ditto.

Same answer.

https://codereview.chromium.org/1323053004/diff/40001/LayoutTests/fast/css-gr...
LayoutTests/fast/css-grid-layout/flex-content-resolution-rows.html:203: <div
class="sizedToGridArea secondRowFirstColumn" data-expected-width="50"
data-expected-height="20"></div>
On 2015/09/08 13:27:07, svillar wrote:
> Ditto.

Same answer.

https://codereview.chromium.org/1323053004/diff/40001/LayoutTests/fast/css-gr...
LayoutTests/fast/css-grid-layout/flex-content-resolution-rows.html:256: <!-- Not
enough space to repsect proportions, because minTrackBreadh it's a harder
requirement -->
On 2015/09/08 13:27:07, svillar wrote:
> Nit: respect

Done.

[jfernandez, 2015-09-09 11:15:51]

Added a new patch for review.

[svillar, 2015-09-09 16:50:50]

lgtm

[jfernandez, 2015-09-09 21:11:23]

The CQ bit was checked by jfernandez@igalia.com

[commit-bot: I haz the power, 2015-09-09 21:11:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1323053004/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1323053004/60001

[commit-bot: I haz the power, 2015-09-09 21:21:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-09 21:21:42]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Timothy Loh, 2015-09-09 23:59:56]

On 2015/09/09 21:21:42, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

core/css lgtm

[esprehn, 2015-09-10 01:03:13]

esprehn@chromium.org changed reviewers:
+ timloh@chromium.org

[esprehn, 2015-09-10 01:03:14]

This creates tons of copies of the HashSet every time you call that function, do
you really want to do that?

https://codereview.chromium.org/1323053004/diff/60001/Source/core/layout/Layo...
File Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/1323053004/diff/60001/Source/core/layout/Layo...
Source/core/layout/LayoutGrid.cpp:545: return succeed ? hypotheticalFrSize :
findFrUnitSize(tracks, tracksSpan, direction, spaceToFill,
tracksToTreatAsInflexible);
This recursion keeps creating more copies of the set

https://codereview.chromium.org/1323053004/diff/60001/Source/core/layout/Layo...
File Source/core/layout/LayoutGrid.h (right):

https://codereview.chromium.org/1323053004/diff/60001/Source/core/layout/Layo...
Source/core/layout/LayoutGrid.h:139: double findFrUnitSize(Vector<GridTrack>&,
const GridSpan&, GridTrackSizingDirection, LayoutUnit spaceToFill, TrackIndexSet
tracksToTreatAsInflexible = TrackIndexSet()) const;
This is creating copies of the TrackIndexSet, you want a reference or a pointer.

[jfernandez, 2015-09-15 17:37:47]

New iterative approach to avoid recursion and using references to avoid copying
the HashSet.

[jfernandez, 2015-09-18 20:29:23]

@svillar didn't like the iterative version, so I've cooked a new patch, using
recursion only for computing the hypothetical flex factor unit size, updating
directly the flexFactorSum and leftoverSpace counters intead of iterating again
over the inflexible tracks. 

Additionally, this patch uses now pointers instead of references, so we can
create the track indexes HashSet on demand.

[svillar, 2015-09-23 14:57:16]

Nice job, I'm liking it again.

I was tempted to give it a go but I think we should fix the LayoutUnit/double
issue I mention in the comments. The rest are just nits.

https://codereview.chromium.org/1323053004/diff/120001/Source/core/layout/Lay...
File Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/1323053004/diff/120001/Source/core/layout/Lay...
Source/core/layout/LayoutGrid.cpp:512: double
LayoutGrid::computeFlexFactorUnitSize(const Vector<GridTrack>& tracks,
GridTrackSizingDirection direction, double flexFactorSum, double leftOverSpace,
const Vector<size_t, 8>& flexibleTracks, PassOwnPtr<TrackIndexSet>
tracksToTreatAsInflexible) const
I don't think you need to use specify the initial capacity for the vector here.

Also perhaps the name should be flexibleTracksIndexes

https://codereview.chromium.org/1323053004/diff/120001/Source/core/layout/Lay...
Source/core/layout/LayoutGrid.cpp:516: double hypotheticalFactorUnitSize =
flexFactorSum > 1 ? leftOverSpace / flexFactorSum : leftOverSpace;
Although perhaps slightly less efficient, I think we can remove the comment and
instead write this as follows:

double hypotheticalFactorUnitSize = leftOverSpace / std::max(1, leftOverSpace);

https://codereview.chromium.org/1323053004/diff/120001/Source/core/layout/Lay...
Source/core/layout/LayoutGrid.cpp:519: OwnPtr<TrackIndexSet>
additionalTacksToTreatAsInflexible = tracksToTreatAsInflexible;
nit: additionalTracks

https://codereview.chromium.org/1323053004/diff/120001/Source/core/layout/Lay...
Source/core/layout/LayoutGrid.cpp:536: return validFlexFactorUnit ?
hypotheticalFactorUnitSize : computeFlexFactorUnitSize(tracks, direction,
flexFactorSum, leftOverSpace, flexibleTracks,
additionalTacksToTreatAsInflexible.release());
I know this is nitpicking but I'd prefer this to be in different lines.
Something like

if (!valid)
    return computeFlexFactor();

return hypothetical;

https://codereview.chromium.org/1323053004/diff/120001/Source/core/layout/Lay...
Source/core/layout/LayoutGrid.cpp:545: double leftOverSpace = spaceToFill;
Why are you using doubles here? Although the flex factor is a double I think we
should still use LayoutUnit for all this size computations.

[jfernandez, 2015-09-24 08:16:36]

https://codereview.chromium.org/1323053004/diff/120001/Source/core/layout/Lay...
File Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/1323053004/diff/120001/Source/core/layout/Lay...
Source/core/layout/LayoutGrid.cpp:516: double hypotheticalFactorUnitSize =
flexFactorSum > 1 ? leftOverSpace / flexFactorSum : leftOverSpace;
On 2015/09/23 14:57:15, svillar wrote:
> Although perhaps slightly less efficient, I think we can remove the comment
and
> instead write this as follows:
> 

I think I'll change the comment so it explains why we don't want flex factor sum
less than 1.

> double hypotheticalFactorUnitSize = leftOverSpace / std::max(1,
leftOverSpace);

I'm not sure abut this. I don't think the code is clearer and we are not only
computing the max, we also do an useless division by 1 operation.

https://codereview.chromium.org/1323053004/diff/120001/Source/core/layout/Lay...
Source/core/layout/LayoutGrid.cpp:519: OwnPtr<TrackIndexSet>
additionalTacksToTreatAsInflexible = tracksToTreatAsInflexible;
On 2015/09/23 14:57:15, svillar wrote:
> nit: additionalTracks

Done.

https://codereview.chromium.org/1323053004/diff/120001/Source/core/layout/Lay...
Source/core/layout/LayoutGrid.cpp:536: return validFlexFactorUnit ?
hypotheticalFactorUnitSize : computeFlexFactorUnitSize(tracks, direction,
flexFactorSum, leftOverSpace, flexibleTracks,
additionalTacksToTreatAsInflexible.release());
On 2015/09/23 14:57:15, svillar wrote:
> I know this is nitpicking but I'd prefer this to be in different lines.
> Something like
> 
> if (!valid)
>     return computeFlexFactor();
> 
> return hypothetical;

Done.

https://codereview.chromium.org/1323053004/diff/120001/Source/core/layout/Lay...
Source/core/layout/LayoutGrid.cpp:545: double leftOverSpace = spaceToFill;
On 2015/09/23 14:57:15, svillar wrote:
> Why are you using doubles here? Although the flex factor is a double I think
we
> should still use LayoutUnit for all this size computations.

Well, some operators doesn't allow to mix LayoutUnit and double, like "<", ">",
"-=" and "+="; we use those in the recursive function computeFlexFactorUnitSize.
 Also, the main operation to compute the flex factor unit size is "/", which
will result a double value, not to mention the returning type is double as well.


If there is no important reason to use use LayoutUnit in this computation I
think converting the 2 LayoutUnit values we have as input arguments,
'spaceToFill' and 'baseSize', just once is the best approach.

[jfernandez, 2015-09-25 11:42:20]

Added a new patch which uses LayoutUnit for availableSpace and baseSize, as
requested. 

I had to rebase it so it can land in the new repo, after the Blink+Chromiun
merge.

[jfernandez, 2015-09-25 11:46:17]

Patchset #9 (id:160001) has been deleted

[svillar, 2015-09-29 09:08:19]

Go go go!!! lgtm

https://codereview.chromium.org/1323053004/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/1323053004/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:514: double
hypotheticalFactorUnitSize = leftOverSpace / std::max(1.0, flexFactorSum);
Nit: std::max<double>(1, flexFactorSum) and you don't need the 1.0 trick

[jfernandez, 2015-09-29 11:14:41]

The CQ bit was checked by jfernandez@igalia.com

[jfernandez, 2015-09-29 11:14:42]

The patchset sent to the CQ was uploaded after l-g-t-m from timloh@chromium.org,
svillar@igalia.com
Link to the patchset: https://codereview.chromium.org/1323053004/#ps220001
(title: "Applied last suggested changes.")

[commit-bot: I haz the power, 2015-09-29 11:15:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1323053004/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1323053004/220001

[commit-bot: I haz the power, 2015-09-29 13:10:17]

Committed patchset #11 (id:220001)

[commit-bot: I haz the power, 2015-09-29 13:12:01]

Patchset 11 (id:??) landed as
https://crrev.com/993d4f780cb06a25b21613fead8fceca12dcdf6a
Cr-Commit-Position: refs/heads/master@{#351301}