[css-grid] Fix placement for unknown named grid lines

[css-grid] Fix placement for unknown named grid lines

The spec has changed and now all the implicit lines should be considered
when we're resolving named grid lines with an unknown name.

The relevant part of the spec is
(http://dev.w3.org/csswg/css-grid/#line-placement):
"If a name is given as a <custom-ident>, only lines with that name
 are counted. If not enough lines with that name exist,
 all implicit grid lines are assumed to have that name
 for the purpose of finding this position."

Modified the code to resolve named grid lines in GridResolvedPosition.
We need to keep the old behavior of considering "auto" unknown named
grid lines for the case of positioned grid items.

Updated current tests to the new expected behavior and created a new
test checking different cases explicitly.

BUG=442954
TEST=fast/css-grid-layout/grid-item-unknown-named-grid-line-resolution.html

Committed: https://crrev.com/4a86c9a04f09c3a0cbf741feb535a2680c1faa79
Cr-Commit-Position: refs/heads/master@{#371792}

[Manuel Rego, 2016-01-12 12:03:44]

rego@igalia.com changed reviewers:
+ cbiesinger@chromium.org, svillar@igalia.com, timloh@chromium.org

[Manuel Rego, 2016-01-20 11:37:06]

I asked some doubts to the CSS WG:
https://lists.w3.org/Archives/Public/www-style/2016Jan/0063.html

And they changed the spec, so I've uploaded an updated patch:
https://drafts.csswg.org/css-grid/#grid-placement-span-int

[svillar, 2016-01-26 09:29:37]

Looking good but not ready yet.

https://codereview.chromium.org/1576993003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/style/GridResolvedPosition.cpp (right):

https://codereview.chromium.org/1576993003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/GridResolvedPosition.cpp:72: start = 0;
Not sure I understand this condition.

https://codereview.chromium.org/1576993003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/GridResolvedPosition.cpp:80: }
This loop looks terribly confusing, we're using a variable to iterate and then
some other in the exit condition. What's more, the very same condition to stop
de loop is used to early return.

https://codereview.chromium.org/1576993003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/GridResolvedPosition.cpp:99: }
Same comments than in lookAhead

https://codereview.chromium.org/1576993003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/GridResolvedPosition.cpp:108: if (side ==
RowStartSide || side == ColumnStartSide) {
I think we had a isStartSide() or something like that doing exactly what this
condition does.

https://codereview.chromium.org/1576993003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/GridResolvedPosition.cpp:144: return
definiteGridSpanWithNamedSpanAgainstOpposite(resolvedOppositePosition, position,
side, it == gridLinesNames.end() ? Vector<size_t>() : it->value, lastLine);
Uhu, so each time it == gridLinesNames.end() then we create an empty vector. We
cannot do that. You should modify definiteGridSpanWithNamedSpanAgainstOpposite()
to accept a pointer to a vector instead of a reference and deal with that there
and inside the functions it calls.

https://codereview.chromium.org/1576993003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/GridResolvedPosition.cpp:198: const
Vector<size_t>& gridLines = it == gridLinesNames.end() ? Vector<size_t>() :
it->value;
Same thing about creating Vectors.

[Manuel Rego, 2016-01-26 11:07:35]

Thanks for the review!

I've applied the changes and tried to improve the loop.
Let's see if it looks better now.

https://codereview.chromium.org/1576993003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/style/GridResolvedPosition.cpp (right):

https://codereview.chromium.org/1576993003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/GridResolvedPosition.cpp:72: start = 0;
This is needed because of the last change on the spec.

See this example:
https://drafts.csswg.org/css-grid/#example-64f80282

I've added a comment to clarify it.

https://codereview.chromium.org/1576993003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/GridResolvedPosition.cpp:80: }
I've tried a different approach, I think it's a bit better.

https://codereview.chromium.org/1576993003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/GridResolvedPosition.cpp:99: }
On 2016/01/26 09:29:37, svillar wrote:
> Same comments than in lookAhead

Acknowledged.

https://codereview.chromium.org/1576993003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/GridResolvedPosition.cpp:108: if (side ==
RowStartSide || side == ColumnStartSide) {
Mmmm, I cannot find it.

We can add it if needed, but there're more parts of the code
with a condition like this one. We should change all of them.

https://codereview.chromium.org/1576993003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/GridResolvedPosition.cpp:144: return
definiteGridSpanWithNamedSpanAgainstOpposite(resolvedOppositePosition, position,
side, it == gridLinesNames.end() ? Vector<size_t>() : it->value, lastLine);
True, changed to use pointers.

https://codereview.chromium.org/1576993003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/GridResolvedPosition.cpp:198: const
Vector<size_t>& gridLines = it == gridLinesNames.end() ? Vector<size_t>() :
it->value;
On 2016/01/26 09:29:37, svillar wrote:
> Same thing about creating Vectors.

Done.

[svillar, 2016-01-27 09:52:33]

lgtm from the grid layout POV although you need a style/ owner

[Manuel Rego, 2016-01-27 09:55:39]

rego@igalia.com changed reviewers:
+ rune@opera.com

[Manuel Rego, 2016-01-27 09:55:40]

Thanks for the review!

@rune and/or @timloh could you please take a look?

[rune, 2016-01-27 10:27:54]

https://codereview.chromium.org/1576993003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/style/GridResolvedPosition.cpp (right):

https://codereview.chromium.org/1576993003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/GridResolvedPosition.cpp:68:
ASSERT(numberOfLines > 0);
ASSERT(numberOfLines);

https://codereview.chromium.org/1576993003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/GridResolvedPosition.cpp:76:
numberOfLines--;
Skip for-loop similar to lookBackForNamedGridLine?

https://codereview.chromium.org/1576993003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/GridResolvedPosition.cpp:79: ASSERT(end >
0);
ASSERT(end);

https://codereview.chromium.org/1576993003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/GridResolvedPosition.cpp:85:
ASSERT(numberOfLines > 0);
ASSERT(numberOfLines)

https://codereview.chromium.org/1576993003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/GridResolvedPosition.cpp:94: }
The for-loop looks unnecessary when namedGridLinesIndex is null.

Can you do something like:

if (!namedGridLinesIndexes)
    return std::min(1, numberOfLines - start + 1);

and skip the null check in the for loop?

[Manuel Rego, 2016-01-27 12:34:25]

@rune thanks for the quick review.

Uploaded a new patch applying your suggestions.

https://codereview.chromium.org/1576993003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/style/GridResolvedPosition.cpp (right):

https://codereview.chromium.org/1576993003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/GridResolvedPosition.cpp:68:
ASSERT(numberOfLines > 0);
On 2016/01/27 10:27:54, rune wrote:
> ASSERT(numberOfLines);

Done.

https://codereview.chromium.org/1576993003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/GridResolvedPosition.cpp:76:
numberOfLines--;
On 2016/01/27 10:27:53, rune wrote:
> Skip for-loop similar to lookBackForNamedGridLine?

Acknowledged.

https://codereview.chromium.org/1576993003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/GridResolvedPosition.cpp:79: ASSERT(end >
0);
On 2016/01/27 10:27:53, rune wrote:
> ASSERT(end);

Done.

https://codereview.chromium.org/1576993003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/GridResolvedPosition.cpp:85:
ASSERT(numberOfLines > 0);
On 2016/01/27 10:27:53, rune wrote:
> ASSERT(numberOfLines)

Done.

https://codereview.chromium.org/1576993003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/GridResolvedPosition.cpp:94: }
On 2016/01/27 10:27:53, rune wrote:
> The for-loop looks unnecessary when namedGridLinesIndex is null.

Yeah, we can skip the loop in that case.
As we'd be only using the implicit lines to find the position.

I've changed it and used a slightly different code for the if.

[rune, 2016-01-27 13:58:18]

lgtm

[Manuel Rego, 2016-01-27 14:31:25]

The CQ bit was checked by rego@igalia.com

[Manuel Rego, 2016-01-27 14:31:26]

The patchset sent to the CQ was uploaded after l-g-t-m from svillar@igalia.com
Link to the patchset: https://codereview.chromium.org/1576993003/#ps100001
(title: "Fixing last review comments")

[commit-bot: I haz the power, 2016-01-27 14:32:35]

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

[commit-bot: I haz the power, 2016-01-27 15:49:43]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-01-27 15:50:53]

Description was changed from

==========
[css-grid] Fix placement for unknown named grid lines

The spec has changed and now all the implicit lines should be considered
when we're resolving named grid lines with an unknown name.

The relevant part of the spec is
(http://dev.w3.org/csswg/css-grid/#line-placement):
"If a name is given as a <custom-ident>, only lines with that name
 are counted. If not enough lines with that name exist,
 all implicit grid lines are assumed to have that name
 for the purpose of finding this position."

Modified the code to resolve named grid lines in GridResolvedPosition.
We need to keep the old behavior of considering "auto" unknown named
grid lines for the case of positioned grid items.

Updated current tests to the new expected behavior and created a new
test checking different cases explicitly.

BUG=442954
TEST=fast/css-grid-layout/grid-item-unknown-named-grid-line-resolution.html
==========

to

==========
[css-grid] Fix placement for unknown named grid lines

The spec has changed and now all the implicit lines should be considered
when we're resolving named grid lines with an unknown name.

The relevant part of the spec is
(http://dev.w3.org/csswg/css-grid/#line-placement):
"If a name is given as a <custom-ident>, only lines with that name
 are counted. If not enough lines with that name exist,
 all implicit grid lines are assumed to have that name
 for the purpose of finding this position."

Modified the code to resolve named grid lines in GridResolvedPosition.
We need to keep the old behavior of considering "auto" unknown named
grid lines for the case of positioned grid items.

Updated current tests to the new expected behavior and created a new
test checking different cases explicitly.

BUG=442954
TEST=fast/css-grid-layout/grid-item-unknown-named-grid-line-resolution.html

Committed: https://crrev.com/4a86c9a04f09c3a0cbf741feb535a2680c1faa79
Cr-Commit-Position: refs/heads/master@{#371792}
==========

[commit-bot: I haz the power, 2016-01-27 15:50:54]

Patchset 6 (id:??) landed as
https://crrev.com/4a86c9a04f09c3a0cbf741feb535a2680c1faa79
Cr-Commit-Position: refs/heads/master@{#371792}