Issue 2080643002: [css-grid] Implement repeat(auto-fit)

Issue 2080643002: [css-grid] Implement repeat(auto-fit) (Closed)

Created:
4 years, 6 months ago by svillar

Modified:
4 years, 5 months ago

Reviewers:
cbiesinger, jfernandez, eae, rune, Manuel Rego, Timothy Loh

CC:
chromium-reviews, jfernandez, szager+layoutwatch_chromium.org, blink-reviews-style_chromium.org, zoltan1, svillar, blink-reviews-css, pdr+renderingwatchlist_chromium.org, leviw+renderwatch, dglazkov+blink, apavlov+blink_chromium.org, jchaffraix+rendering, darktears, blink-reviews, eae+blinkwatch, blink-reviews-layout_chromium.org, rwlbuis

Base URL:
https://chromium.googlesource.com/chromium/src.git@master

Target Ref:
refs/pending/heads/master

Project:
chromium

Visibility:
Public.

More Reviews

Description

[css-grid] Implement repeat(auto-fit) The auto-fit keyword works exactly as the already implemented auto-fill except that all empty tracks collapse (became 0px). Absolutely positioned items do not participate on the layout of the grid so they are not considered (a grid with only absolutely positioned items is considered an empty grid). Whenever a track collapses the gutters on either side do also collapse. When a collapsed track’s gutters collapse, they coincide exactly. If one side of a collapsed track does not have a gutter then collapsing its gutters results in no gutter on either “side” of the collapsed track. Uncommented the auto-fit cases from Mozilla tests. They have to be adapted as the reftest machinery requires all the content to be rendered in the original 800x600 viewport. BUG=589460 Committed: https://crrev.com/80ac577b86f1dc0b64339f6f6f566d163ee9215c Cr-Commit-Position: refs/heads/master@{#405849}

Patch Set 1 #

Patch Set 2 : Build (debug) fix #

Total comments: 28

Patch Set 3 : Review fixes #

Patch Set 4 : Collapse empty tracks (do not drop them) #

Total comments: 47

Patch Set 5 : Patch for landing #

Patch Set 6 : New tests. Improved comments #

Patch Set 7 : Rebased patch for landing #

Patch Set 8 : Patch for landing v2 #

Patch Set 9 : Patch for landing v3 #

Created: 4 years, 5 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+1167 lines, -1690 lines)			Patch
A +	third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-auto-fit-columns.html	View	1 2 3 4 5	8 chunks	+12 lines, -12 lines	0 comments	Download
A +	third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-auto-fit-rows.html	View	1 2 3 4 5	8 chunks	+12 lines, -12 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-element-auto-repeat-get-set.html	View	1 2 3	1 chunk	+5 lines, -3 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-element-auto-repeat-get-set-expected.txt	View	1 2 3	1 chunk	+14 lines, -6 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-only-abspos-item-computed-style-crash.html	View	1 2 3	3 chunks	+16 lines, -0 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-only-abspos-item-computed-style-crash-expected.txt	View	1 2 3	1 chunk	+8 lines, -0 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-positioned-items-padding.html	View	1 2 3 4	3 chunks	+43 lines, -0 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-template-columns-rows-computed-style-gaps-content-alignment.html	View	1 2 3 4	3 chunks	+43 lines, -1 line	0 comments	Download
M	third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-template-columns-rows-computed-style-gaps-content-alignment-expected.txt	View	1 2 3	1 chunk	+112 lines, -0 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/fast/css-grid-layout/mozilla/grid-repeat-auto-fill-fit-001.html	View		2 chunks	+52 lines, -53 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/fast/css-grid-layout/mozilla/grid-repeat-auto-fill-fit-001-expected.html	View		2 chunks	+57 lines, -54 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/fast/css-grid-layout/mozilla/grid-repeat-auto-fill-fit-002.html	View		1 chunk	+51 lines, -52 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/fast/css-grid-layout/mozilla/grid-repeat-auto-fill-fit-002-expected.html	View		1 chunk	+51 lines, -51 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/fast/css-grid-layout/mozilla/grid-repeat-auto-fill-fit-003.html	View		2 chunks	+56 lines, -52 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/fast/css-grid-layout/mozilla/grid-repeat-auto-fill-fit-003-expected.html	View		2 chunks	+57 lines, -51 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/fast/css-grid-layout/mozilla/grid-repeat-auto-fill-fit-004.html	View		2 chunks	+56 lines, -52 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/fast/css-grid-layout/mozilla/grid-repeat-auto-fill-fit-004-expected.html	View		2 chunks	+57 lines, -51 lines	0 comments	Download
D	third_party/WebKit/LayoutTests/fast/css-grid-layout/mozilla/grid-repeat-auto-fill-fit-005.html	View		1 chunk	+0 lines, -378 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/fast/css-grid-layout/mozilla/grid-repeat-auto-fill-fit-005-expected.html	View	1 2	1 chunk	+0 lines, -384 lines	0 comments	Download
A +	third_party/WebKit/LayoutTests/fast/css-grid-layout/mozilla/grid-repeat-auto-fill-fit-005-part-1.html	View		4 chunks	+4 lines, -147 lines	0 comments	Download
A +	third_party/WebKit/LayoutTests/fast/css-grid-layout/mozilla/grid-repeat-auto-fill-fit-005-part-1-expected.html	View	1 2	3 chunks	+4 lines, -144 lines	0 comments	Download
A +	third_party/WebKit/LayoutTests/fast/css-grid-layout/mozilla/grid-repeat-auto-fill-fit-005-part-2.html	View		4 chunks	+6 lines, -148 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/fast/css-grid-layout/mozilla/grid-repeat-auto-fill-fit-005-part-2-expected.html	View	1 2 3	1 chunk	+244 lines, -0 lines	0 comments	Download
M	third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp	View	1 2 3 4 5 6	1 chunk	+8 lines, -14 lines	0 comments	Download
M	third_party/WebKit/Source/core/layout/LayoutGrid.h	View	1 2 3 4 5 6 7 8	5 chunks	+14 lines, -7 lines	0 comments	Download
M	third_party/WebKit/Source/core/layout/LayoutGrid.cpp	View	1 2 3 4 5 6 7 8	19 chunks	+185 lines, -18 lines	0 comments	Download

Messages

Total messages: 39 (15 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

jfernandez

lgtm https://codereview.chromium.org/2080643002/diff/20001/third_party/WebKit/Source/core/layout/LayoutGrid.cpp File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/2080643002/diff/20001/third_party/WebKit/Source/core/layout/LayoutGrid.cpp#newcode1329 third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1329: if (!emptyTracksCounter) Isn't this name a bit confusing ...

4 years, 6 months ago (2016-06-21 07:40:27 UTC) #4

Manuel Rego

Nice patch but I've some questions as it's quite complex. BTW, why you didn't add ...

4 years, 6 months ago (2016-06-21 21:57:38 UTC) #6

svillar

I still need an OWNER for the style/ changes, @rune @timloh ?

4 years, 6 months ago (2016-06-23 07:37:51 UTC) #9

svillar

Thanks for both reviews. https://codereview.chromium.org/2080643002/diff/20001/third_party/WebKit/LayoutTests/fast/css-grid-layout/mozilla/grid-repeat-auto-fill-fit-005-expected.html File third_party/WebKit/LayoutTests/fast/css-grid-layout/mozilla/grid-repeat-auto-fill-fit-005-expected.html (right): https://codereview.chromium.org/2080643002/diff/20001/third_party/WebKit/LayoutTests/fast/css-grid-layout/mozilla/grid-repeat-auto-fill-fit-005-expected.html#newcode1 third_party/WebKit/LayoutTests/fast/css-grid-layout/mozilla/grid-repeat-auto-fill-fit-005-expected.html:1: <!DOCTYPE HTML> On 2016/06/21 21:57:37, ...

4 years, 6 months ago (2016-06-23 08:09:43 UTC) #10

Thanks for both reviews.

https://codereview.chromium.org/2080643002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/css-grid-layout/mozilla/grid-repeat-auto-fill-fit-005-expected.html
(right):

https://codereview.chromium.org/2080643002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css-grid-layout/mozilla/grid-repeat-auto-fill-fit-005-expected.html:1:
<!DOCTYPE HTML>
On 2016/06/21 21:57:37, Manuel Rego wrote:
> Shouldn't you remove this file too?

Yes, somehow I forgot

https://codereview.chromium.org/2080643002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp
(right):

https://codereview.chromium.org/2080643002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp:781: if
(!m_hasRepeatAutoFit || m_repetitions || i < m_insertionPoint) {
On 2016/06/21 21:57:37, Manuel Rego wrote:
> Do we really need to check if m_hasRepeatAutoFit?
> I think the only important thing to know here is if m_repetitions
> is 0 or not, maybe I'm missing something.

Yes we do.

If m_repetitions is 0 then there are 2 possibilities:
1) there is no auto repeat with auto-fit
2) there is an auto-fit which dropped all the tracks.

You need a way to differentiate between these two cases.

https://codereview.chromium.org/2080643002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp:787: //
function. We should also take the removed tracks into account for the following
lines.
On 2016/06/21 21:57:37, Manuel Rego wrote:
> I guess we could add an assert here to check that m_repetitions is 0.

Do we? There is an if (m_repetitions) with an early return just above so I guess
is pretty obvious from the code.

https://codereview.chromium.org/2080643002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp:792:
appendLines(lineNamesValue, i + 1, NamedLines);
On 2016/06/21 21:57:37, Manuel Rego wrote:
> I don't understand this else part.
> 
> I guess that here i > m_insertionPoint
> (otherwise it should have gone through the previous ifs).
> I don't get why we want the "i + 1" line.

So we're in the case of having dropped all the repetitions. For something like
this

[a] repeat(auto-fit, 10px) [b] 20px [c]

the way we store line names is more or less a hash map like this:
(0, a)
(1, b)
(2, c)

if we drop all the auto repetitions we should end up with something like this

[a b] 20px [c]

as you can see when we ask for lines at index 1 (remember that they start on 0)
we should not return  'b' (line 1) but 'c' (line 1+1). 

They key is that when we store the line names we assume that there is always 1
repetition for autorepeat (which is true for auto-fill but might be false for
auto-fit), that's why we need to readjust the code here.

https://codereview.chromium.org/2080643002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp:839: bool
trackListIsEmpty = trackSizes.isEmpty() && (autoRepeatTrackSizes.isEmpty() ||
!repetitions);
On 2016/06/21 21:57:37, Manuel Rego wrote:
> I guess checking !repetitions is enough:
>    bool trackListIsEmpty = trackSizes.isEmpty() && !repetitions;
> 
> If I'm not wrong if autoRepeatTrackSizes.isEmpty()
> autoRepeatCountForDirection() will return 0.

Yes we can do that in this case.

https://codereview.chromium.org/2080643002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/2080643002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1311: {
On 2016/06/21 21:57:38, Manuel Rego wrote:
> I think we need a comment explaining what this method is returning
> as I think it's pretty complex.
> 
> E.g. repeat(auto-fit, 100px) with an available space of 300px
> without items. This method will return: [1, 2, 3]
> 
> However, if there's 1 item in the 2nd column: [1, 1, 2]

Acknowledged.

https://codereview.chromium.org/2080643002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1321: size_t
emptyTracksCount = 0;
On 2016/06/21 21:57:37, Manuel Rego wrote:
> emptyTracksCount and emptyTracksCounter in the very same function
> seems quite confusing.

I agree, I even had build issues because I was replacing them with the other
one.

https://codereview.chromium.org/2080643002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1329: if
(!emptyTracksCounter)
On 2016/06/21 07:40:26, jfernandez wrote:
> Isn't this name a bit confusing ? I understood the first time I read it that
you
> were entering in this branch if tracksCounter was NOT empty. 

Well it's a pointer that's the standard way to know that it's nullptr

> What about explicitly comparing with nullptr ? 

Against coding style

https://codereview.chromium.org/2080643002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1329: if
(!emptyTracksCounter)
On 2016/06/21 21:57:37, Manuel Rego wrote:
> On 2016/06/21 07:40:26, jfernandez wrote:
> > Isn't this name a bit confusing ? I understood the first time I read it that
> you
> > were entering in this branch if tracksCounter was NOT empty. 
> 
> Maybe we can rename emptyTracksCounter to droppedTracksCounter
> to avoid confusion.
> Also m_emptyColumnsCounter to m_droppedColumnsCounter, etc.

OK

https://codereview.chromium.org/2080643002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1399: /* auto-fit */
On 2016/06/21 21:57:38, Manuel Rego wrote:
> Nit: I would move this to a separated method,
> it doesn't use any of the local variables defined above.

I guess it's a matter of taste. I don't usually like to create more methods if
they are going to be called just once (unless they clearly simplify the code).
In this case I think having this code here helps understanding how the
auto-repeat flow computation goes.

https://codereview.chromium.org/2080643002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/style/GridArea.h (right):

https://codereview.chromium.org/2080643002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/GridArea.h:150: void
adjustForDroppedTracks(size_t numDroppedTracks);
On 2016/06/21 21:57:38, Manuel Rego wrote:
> Why the implementation is not here like for the rest of the class?
> 
> Nit: I'd rename the argument to droppedTracksCount or simply droppedTracks.

Because it's kind of a non-written rule of the project, if the implementation is
not a one liner, it's better to have the implementation inlined. This way the
API of the class is much cleaner.

https://codereview.chromium.org/2080643002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/GridArea.h:186:
DCHECK_GE(static_cast<size_t>(m_startLine), numDroppedTracks);
On 2016/06/21 21:57:38, Manuel Rego wrote:
> You can use startLine() to avoid the manual cast here.

awesome

https://codereview.chromium.org/2080643002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/GridArea.h:187:
DCHECK_GE(static_cast<size_t>(m_endLine), numDroppedTracks);
On 2016/06/21 21:57:38, Manuel Rego wrote:
> m_endLine is always greater than m_startLine,
> so this doesn't seem needed.
> 
> Anyway we can keep it, but probably using endLine() too.

Acknowledged.

svillar

https://codereview.chromium.org/2080643002/diff/20001/third_party/WebKit/Source/core/layout/LayoutGrid.cpp File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/2080643002/diff/20001/third_party/WebKit/Source/core/layout/LayoutGrid.cpp#newcode1329 third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1329: if (!emptyTracksCounter) On 2016/06/21 21:57:37, Manuel Rego wrote: > ...

4 years, 6 months ago (2016-06-23 08:19:14 UTC) #11

Manuel Rego

Ok, thanks for the info. Non-owner LGTM. You missed my previous question about tests: > ...

4 years, 5 months ago (2016-06-27 05:59:37 UTC) #12

svillar

New version of the patch that matches the latest spec. Instead of dropping tracks we ...

4 years, 5 months ago (2016-07-07 12:06:07 UTC) #14

Manuel Rego

Wow, this is getting really complex. I've several inline comments but the patch looks great! ...

4 years, 5 months ago (2016-07-08 11:12:35 UTC) #15

svillar

Thanks for the detailed review! https://codereview.chromium.org/2080643002/diff/60001/third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-template-columns-rows-computed-style-gaps-content-alignment.html File third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-template-columns-rows-computed-style-gaps-content-alignment.html (right): https://codereview.chromium.org/2080643002/diff/60001/third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-template-columns-rows-computed-style-gaps-content-alignment.html#newcode23 third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-template-columns-rows-computed-style-gaps-content-alignment.html:23: .item { background: gray} ...

4 years, 5 months ago (2016-07-11 12:46:19 UTC) #16

Thanks for the detailed review!

https://codereview.chromium.org/2080643002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-template-columns-rows-computed-style-gaps-content-alignment.html
(right):

https://codereview.chromium.org/2080643002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-template-columns-rows-computed-style-gaps-content-alignment.html:23:
.item { background: gray}
On 2016/07/08 11:12:34, Manuel Rego wrote:
> We only have 2 items as maximum, so I guess this is not applied to any of them
> as below we define the background for the first and last items.

Acknowledged.

https://codereview.chromium.org/2080643002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-template-columns-rows-computed-style-gaps-content-alignment.html:76:
function testGapsAlignmentWithAutoFit(id, expectedColumns, expectedRows) {
On 2016/07/08 11:12:34, Manuel Rego wrote:
> Nice method, why not use it for the previous tests too?

They were already there but I can migrate them if you like.

https://codereview.chromium.org/2080643002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/2080643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:503: bool
LayoutGrid::isEmptyAutoRepeatTrack(GridTrackSizingDirection direction, size_t
lineNumber) const
On 2016/07/08 11:12:34, Manuel Rego wrote:
> We usually use just "line" to refer to the other line arguments
> that store line indexes (starting on 0).
> Otherwise I'd think this method expects a line number (from the spec),
> the ones starting on 1.

Acknowledged.

https://codereview.chromium.org/2080643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:513: LayoutUnit
LayoutGrid::guttersSize(GridTrackSizingDirection direction, size_t startLine,
size_t span) const
On 2016/07/08 11:12:35, Manuel Rego wrote:
> In most of the cases startLine is simply 0.
> I'd move it to the last parameter and make it optional.

That's correct, but I did it like this for API coherence. I'm calling
guttersSize with a starline and then a span. Having first the span and then the
start line looks weird to me from an API POV.

It's like if you define a function like this void f(end, start).

https://codereview.chromium.org/2080643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:530: LayoutUnit
gutterAccumulator;
On 2016/07/08 11:12:34, Manuel Rego wrote:
> Nit: You call this gapAccumulator in a different method.

Acknowledged.

https://codereview.chromium.org/2080643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:539: // a non collapsed
track. Should we find a non collapsed track we need to add that gap.
On 2016/07/08 11:12:34, Manuel Rego wrote:
> Nit: s/Should/If/ (it's clearer to me).

Acknowledged.

https://codereview.chromium.org/2080643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:550: // collapsed track.
Should we find a non collapsed track we need to add that gap.
On 2016/07/08 11:12:35, Manuel Rego wrote:
> Ditto.

Acknowledged.

https://codereview.chromium.org/2080643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:864: // Collapse empty auto
repeat tracks if auto-fit
On 2016/07/08 11:12:34, Manuel Rego wrote:
> Nit: Missing dot at the end.

Acknowledged.

https://codereview.chromium.org/2080643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:866: || (direction ==
ForRows && m_autoRepeatEmptyRows &&
m_autoRepeatEmptyRows->contains(translatedIndex)))
On 2016/07/08 11:12:34, Manuel Rego wrote:
> Cannot we use isEmptyAutoRepeatTrack() here?

Totally, but I still need the null check here (won't add it to the function as
it's always called when there are auto repetitions but in this case).

https://codereview.chromium.org/2080643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1263:
DCHECK(itemSpan.isTranslatedDefinite());
On 2016/07/08 11:12:35, Manuel Rego wrote:
> I don't believe we need this here.
> 
> We've an ASSERT at the end of LayoutGrid::placeItemsOnGrid()
> to verify that all the cached spans are "TranslatedDefinite".
> Also GridSpan::startLine() method has already the same ASSERT inside.

Acknowledged.

https://codereview.chromium.org/2080643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1453:
std::unique_ptr<TrackIndexSet> emptyTrackIndexes;
On 2016/07/08 11:12:34, Manuel Rego wrote:
> I guess you can initialize it here already so you can avoid the if the for
> below.

What if there is no empty track? I'd need to create and destroy the object.

https://codereview.chromium.org/2080643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1464: for (size_t
trackIndex = firstAutoRepeatTrack; trackIndex < lastAutoRepeatTrack;
++trackIndex) {
On 2016/07/08 11:12:35, Manuel Rego wrote:
> The for is the same, maybe you can move the outer if inside it.

If I do that I'd evaluate the if() condition for every single iteration.
Although it's more code it's much more efficient. Also you cannot create a
GridIterator for empty grids.

https://codereview.chromium.org/2080643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1732: Vector<LayoutUnit>
LayoutGrid::trackSizesForComputedStyle(GridTrackSizingDirection direction) const
On 2016/07/08 11:12:35, Manuel Rego wrote:
> We've been computing almost the same information in
> LayoutGrid::populateGridPositionsForDirection().
> 
> It'd be great to be able to reuse it. Otherwise we might introduce bugs later
> when we modify something in LayoutGrid::populateGridPositionsForDirection()
> and forgot about changing this method.

In one method we're computing the line positions while in the other we're
computing the track sizes. I agree that they're related but they are not
definitely the same.

If we want to reuse the grid line positions then we have to store some extra
information, like the size of the gaps for each line.

Note that having a method like this already improves the current situation,
because the track sizes were computed outside the grid code by the
CSSValueMapping object which was already manipulating the offset and gaps
between tracks, something that should be kept inside the grid.

https://codereview.chromium.org/2080643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1736: size_t numPositions =
positions.size();
On 2016/07/08 11:12:34, Manuel Rego wrote:
> Maybe we can rename numPositions to numTracks:
>   numTracks = positions.size() - 1;
> 
> Then we can use it later.

Hmm not sure what we would win with that, we still need numPositions - 2 in a
couple of places. Also the loop is iterating positions so it'd be a bit weird to
have a (i < numTracks) as the exit condition (we know about the close
relationship between tracks and positions but perhaps is not that evident for an
outsider).

https://codereview.chromium.org/2080643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1765: }
On 2016/07/08 11:12:35, Manuel Rego wrote:
> Could we early return if (remainingEmptyTracks == 0) or not?

Totally the opposite :). If none of the remaining tracks are empty this means
that you have to remove the gaps for all of them (note that the first loop of
this method does not remove gaps if we have empty auto repeat tracks so we have
to remove them here if needed).

https://codereview.chromium.org/2080643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1960: if (endLine &&
(endLine < lastLine)) {
On 2016/07/08 11:12:35, Manuel Rego wrote:
> Why you removed "> 0" for endLine?
> endLine is an int, probably better to keep the "endLine > 0" here.

Oh my mistake, I did it due to the coding style

https://codereview.chromium.org/2080643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1961: end -=
guttersSize(direction, endLine - 1, 2);
On 2016/07/08 11:12:34, Manuel Rego wrote:
> Cannot we use gridGapForDirection(direction) like you do below?
> 
> It seems weird to me that this code is different to the if a few lines below.

No we cannot. gridGapForDirection just returns the gap from the style. Here we
need the actual gap between 2 tracks (it might have collapsed so it could be
0px)

It looks like I forgot to change the one bellow (and also that we need a test
for that case).

https://codereview.chromium.org/2080643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:2073: bool
hasEmptyLinesForDirection = isRowAxis ? !!m_autoRepeatEmptyColumns :
!!m_autoRepeatEmptyRows;
On 2016/07/08 11:12:35, Manuel Rego wrote:
> s/hasEmptyLinesForDirection/hasEmptyTracksForDirection/
> 
> I believe "track" is more accurate than "line" here.

Oh yeah, empty lines makes no sense indeed.

svillar

Addressed review comments. Fixed a bug with gaps and positioned items (with new tests). The ...

4 years, 5 months ago (2016-07-13 12:08:13 UTC) #18

Manuel Rego

The last version looks good to me too. Some minor comments inline. https://codereview.chromium.org/2080643002/diff/60001/third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-template-columns-rows-computed-style-gaps-content-alignment.html File third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-template-columns-rows-computed-style-gaps-content-alignment.html ...

4 years, 5 months ago (2016-07-14 09:10:51 UTC) #20

The last version looks good to me too.
Some minor comments inline.

https://codereview.chromium.org/2080643002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-template-columns-rows-computed-style-gaps-content-alignment.html
(right):

https://codereview.chromium.org/2080643002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-template-columns-rows-computed-style-gaps-content-alignment.html:76:
function testGapsAlignmentWithAutoFit(id, expectedColumns, expectedRows) {
On 2016/07/11 12:46:18, svillar wrote:
> On 2016/07/08 11:12:34, Manuel Rego wrote:
> > Nice method, why not use it for the previous tests too?
> 
> They were already there but I can migrate them if you like.

Yeah I know but it'd be nice to migrate the old ones.

https://codereview.chromium.org/2080643002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/2080643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:539: // a non collapsed
track. Should we find a non collapsed track we need to add that gap.
On 2016/07/11 12:46:18, svillar wrote:
> On 2016/07/08 11:12:34, Manuel Rego wrote:
> > Nit: s/Should/If/ (it's clearer to me).
> 
> Acknowledged.

Ping ;)

https://codereview.chromium.org/2080643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1453:
std::unique_ptr<TrackIndexSet> emptyTrackIndexes;
On 2016/07/11 12:46:19, svillar wrote:
> On 2016/07/08 11:12:34, Manuel Rego wrote:
> > I guess you can initialize it here already so you can avoid the if the for
> > below.
> 
> What if there is no empty track? I'd need to create and destroy the object.

True, I didn't realize before.

https://codereview.chromium.org/2080643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1732: Vector<LayoutUnit>
LayoutGrid::trackSizesForComputedStyle(GridTrackSizingDirection direction) const
On 2016/07/11 12:46:19, svillar wrote:
> On 2016/07/08 11:12:35, Manuel Rego wrote:
> > We've been computing almost the same information in
> > LayoutGrid::populateGridPositionsForDirection().
> > 
> > It'd be great to be able to reuse it. Otherwise we might introduce bugs
later
> > when we modify something in LayoutGrid::populateGridPositionsForDirection()
> > and forgot about changing this method.
> 
> In one method we're computing the line positions while in the other we're
> computing the track sizes. I agree that they're related but they are not
> definitely the same.
> 
> If we want to reuse the grid line positions then we have to store some extra
> information, like the size of the gaps for each line.
> 
> Note that having a method like this already improves the current situation,
> because the track sizes were computed outside the grid code by the
> CSSValueMapping object which was already manipulating the offset and gaps
> between tracks, something that should be kept inside the grid.

Yeah I agree. My concern was that there're some duplicated complex logic,
that might be hard to keep updated in both places at the same time.

One idea could be to generate this Vector on populateGridPositionsForDirection()
and just return it here. But I also don't like it a lot. :-/

https://codereview.chromium.org/2080643002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1959: // overlapping the
grid'd edge).
On 2016/07/08 11:12:35, Manuel Rego wrote:
> Apart from the typo "grid'd" I don't get the part in parenthesis.
> Could you clarify it?

Ping. I don't understand the part between parenthesis
"(note that a collapsed track at the end of the grid
 has its end line overlapping the grid'd edge)."

Imagine a grid with:
* width: 500px;
* grid-template-columns: repeat(auto-fit, [a] 200px [b]);

If the 2nd track collapse we'll have:
[a] 200px [a b] 0px [b]

The grid's edge is 500px not related at all with the collapsed track.