Bug 159771 – [css-grid] Implement repeat(auto-fit)

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sergio Villar Senin]

[css-grid] Implement repeat(auto-fit)

[Sergio Villar Senin]

Created attachment 283652 [details]
Patch

[Javier Fernandez]

Comment on attachment 283652 [details]
Patch

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

> Source/WebCore/ChangeLog:14
> +        trackâs gutters collapse, they coincide exactly. If one side of a collapsed track does not

typo: trackâs

> Source/WebCore/ChangeLog:36
> +        (WebCore::RenderGrid::guttersSize): Computes the gap between a startLine and a endLine. Thi

typo: "Thi"  "a endline"

> Source/WebCore/rendering/RenderGrid.cpp:465
> +    if (isRowAxis ? !m_autoRepeatEmptyColumns : !m_autoRepeatEmptyRows)

I think this check is a bit confusing. Even that is only used here, we could define a new bool variable like "autoRepeatEmptyTracks" which would use isRowAxis to determine its value. 
Another alternative would be to define a more complex, but clearer, like (isRowAxis && m_autoRepeatEmptyColumns) || (!isRowAxis && m_autoRepeatEmptyRows).

> Source/WebCore/rendering/RenderGrid.cpp:487
> +            ASSERT(nonEmptyTracksBeforeStartLine >= 1);

Wouldn't be better "> 0" ?

> Source/WebCore/rendering/RenderGrid.cpp:810
> +    bool hasEmptyTracksForDirection = direction == ForColumns ? !!m_autoRepeatEmptyColumns : !!m_autoRepeatEmptyRows;

We are doing this check in several places, so perhaps an utility function would be useful.

> Source/WebCore/rendering/RenderGrid.cpp:812
> +        return GridTrackSize(Length(Fixed), Length(Fixed));

This should just be: 
   return {Length(Fixed), Length(Fixed)}

> Source/WebCore/rendering/RenderGrid.cpp:1644
> +    bool hasCollapsedTracks = isRowAxis ? !!m_autoRepeatEmptyColumns : !!m_autoRepeatEmptyRows;

We could use here the above mentioned utility function.

> Source/WebCore/rendering/RenderGrid.cpp:1935
> +        bool hasEmptyTracksForDirection = isRowAxis ? !!m_autoRepeatEmptyColumns : !!m_autoRepeatEmptyRows;

Ditto.

[Sergio Villar Senin]

Created attachment 283749 [details]
Patch

[Sergio Villar Senin]

Created attachment 284000 [details]
Patch

[Sergio Villar Senin]

Created attachment 284004 [details]
Patch

[Javier Fernandez]

Comment on attachment 284004 [details]
Patch

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

> Source/WebCore/rendering/RenderGrid.cpp:559
> +            ASSERT(nonEmptyTracksBeforeStartLine);

Wouldn't be better to be more explicit in this assertion and check for nonEmptyTracksBeforeStartLine > 0 ?

> Source/WebCore/rendering/RenderGrid.cpp:573
> +        for (auto it = ++currentEmptyTrack; it != endEmptyTrack; ++it) {

If we are not interested on the modifying currentEmptyTrack, shouldn't be simpler to do 
  auto it = currentEmptyTrack + 1;

> Source/WebCore/rendering/RenderGrid.cpp:574
> +            ASSERT(nonEmptyTracksAfterEndLine >= 1);

Isn't this assertion equivalent to nonEmptyTracksAfterEndLine > 0 ?

> Source/WebCore/rendering/RenderGrid.cpp:1503
> +        emptyTrackIndexes = std::make_unique<OrderedTrackIndexSet>();

Are we going to return nullptr beyond this point ? Otherwise, why we aren't initializing the emptyTrackIndexes pointer in the definition ?

> Source/WebCore/rendering/RenderGrid.cpp:1756
> +        return tracks;

Why not defining the "tracks" variable after the condition, so we would early return an empty vector if it's satisfied ?

> Source/WebCore/rendering/RenderGrid.cpp:1770
> +    gap = gridGapForDirection(direction);

Didn't we get the gap value already some lines above ?

[Sergio Villar Senin]

Comment on attachment 284004 [details]
Patch

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

Thanks for the review.

>> Source/WebCore/rendering/RenderGrid.cpp:559
>> +            ASSERT(nonEmptyTracksBeforeStartLine);
> 
> Wouldn't be better to be more explicit in this assertion and check for nonEmptyTracksBeforeStartLine > 0 ?

I can do it.

>> Source/WebCore/rendering/RenderGrid.cpp:573
>> +        for (auto it = ++currentEmptyTrack; it != endEmptyTrack; ++it) {
> 
> If we are not interested on the modifying currentEmptyTrack, shouldn't be simpler to do 
>   auto it = currentEmptyTrack + 1;

Not sure which one is simpler :). Anyway I can change it.

>> Source/WebCore/rendering/RenderGrid.cpp:574
>> +            ASSERT(nonEmptyTracksAfterEndLine >= 1);
> 
> Isn't this assertion equivalent to nonEmptyTracksAfterEndLine > 0 ?

As you wish

>> Source/WebCore/rendering/RenderGrid.cpp:1503
>> +        emptyTrackIndexes = std::make_unique<OrderedTrackIndexSet>();
> 
> Are we going to return nullptr beyond this point ? Otherwise, why we aren't initializing the emptyTrackIndexes pointer in the definition ?

Yes, check the else part, there might not be any empty line.

>> Source/WebCore/rendering/RenderGrid.cpp:1756
>> +        return tracks;
> 
> Why not defining the "tracks" variable after the condition, so we would early return an empty vector if it's satisfied ?

I do want to return an empty vector if the condition is satisfied. If not I want to fill that vector with the sizes of the tracks.

>> Source/WebCore/rendering/RenderGrid.cpp:1770
>> +    gap = gridGapForDirection(direction);
> 
> Didn't we get the gap value already some lines above ?

No. 

LayoutUnit gap = !hasCollapsedTracks ? gridGapForDirection(direction) : LayoutUnit();

This means that here the gap was 0 on purpose, because we want to add the gaps here.

[Darin Adler]

Comment on attachment 284004 [details]
Patch

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

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1140
> +        Vector<LayoutUnit> computedTrackSizes = grid.trackSizesForComputedStyle(direction);

I think auto would be better.

> Source/WebCore/rendering/RenderGrid.cpp:1564
> +    // Compute collapsable tracks for auto-fit.

The word is spelled "collapsible".

[Sergio Villar Senin]

Created attachment 284469 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 284469 [details]
Patch for landing

Clearing flags on attachment: 284469

Committed r203680: <http://trac.webkit.org/changeset/203680>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 284469 [details]
Patch for landing

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

This already landed, but I had some comments.

> Source/WebCore/rendering/RenderGrid.cpp:514
> +    return direction == ForColumns ? !!m_autoRepeatEmptyColumns : !!m_autoRepeatEmptyRows;

I think it would compile without those !! sequences. But also, maybe an inline helper that returns the rows or columns:

    return autoRepeatEmptyRowsOrColumns(direction);

I think this would read better in almost every place we get at these directly.

> Source/WebCore/rendering/RenderGrid.cpp:520
> +    return direction == ForColumns ? m_autoRepeatEmptyColumns->contains(line) : m_autoRepeatEmptyRows->contains(line);

Then it would be:

    return autoRepeatEmptyRowsOrColumns(direction)->contains(line);

> Source/WebCore/rendering/RenderGrid.h:220
> +    std::unique_ptr<OrderedTrackIndexSet> m_autoRepeatEmptyColumns { nullptr };
> +    std::unique_ptr<OrderedTrackIndexSet> m_autoRepeatEmptyRows { nullptr };

No need to explicitly initialize smart pointers with nullptr; they start out as null regardless.

[Sergio Villar Senin]

(In reply to comment #12)
> Comment on attachment 284469 [details]
> Patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=284469&action=review
> 
> This already landed, but I had some comments.
> 
> > Source/WebCore/rendering/RenderGrid.cpp:514
> > +    return direction == ForColumns ? !!m_autoRepeatEmptyColumns : !!m_autoRepeatEmptyRows;
> 
> I think it would compile without those !! sequences. But also, maybe an
> inline helper that returns the rows or columns:

At least gcc was not able to convert it to a boolean.

>     return autoRepeatEmptyRowsOrColumns(direction);
> 
> I think this would read better in almost every place we get at these
> directly.
> 
> > Source/WebCore/rendering/RenderGrid.cpp:520
> > +    return direction == ForColumns ? m_autoRepeatEmptyColumns->contains(line) : m_autoRepeatEmptyRows->contains(line);
> 
> Then it would be:
> 
>     return autoRepeatEmptyRowsOrColumns(direction)->contains(line);

OK, I'll write it down for a future cleanup