Bug 159771 - [css-grid] Implement repeat(auto-fit)
Summary: [css-grid] Implement repeat(auto-fit)
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sergio Villar Senin
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-07-14 09:18 PDT by Sergio Villar Senin
Modified: 2016-07-26 01:59 PDT (History)
7 users (show)

See Also:


Attachments
Patch (206.52 KB, patch)
2016-07-14 09:42 PDT, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch (208.52 KB, patch)
2016-07-15 01:28 PDT, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch (208.56 KB, patch)
2016-07-19 06:42 PDT, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch (209.01 KB, patch)
2016-07-19 07:31 PDT, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch for landing (210.53 KB, patch)
2016-07-25 01:22 PDT, Sergio Villar Senin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sergio Villar Senin 2016-07-14 09:18:50 PDT
[css-grid] Implement repeat(auto-fit)
Comment 1 Sergio Villar Senin 2016-07-14 09:42:02 PDT
Created attachment 283652 [details]
Patch
Comment 2 Javier Fernandez 2016-07-14 16:11:18 PDT
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.
Comment 3 Sergio Villar Senin 2016-07-15 01:28:11 PDT
Created attachment 283749 [details]
Patch
Comment 4 Sergio Villar Senin 2016-07-19 06:42:26 PDT
Created attachment 284000 [details]
Patch
Comment 5 Sergio Villar Senin 2016-07-19 07:31:30 PDT
Created attachment 284004 [details]
Patch
Comment 6 Javier Fernandez 2016-07-21 05:12:39 PDT
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 ?
Comment 7 Sergio Villar Senin 2016-07-22 06:35:14 PDT
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.
Comment 8 Darin Adler 2016-07-22 22:45:00 PDT
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".
Comment 9 Sergio Villar Senin 2016-07-25 01:22:55 PDT
Created attachment 284469 [details]
Patch for landing
Comment 10 WebKit Commit Bot 2016-07-25 01:58:11 PDT
Comment on attachment 284469 [details]
Patch for landing

Clearing flags on attachment: 284469

Committed r203680: <http://trac.webkit.org/changeset/203680>
Comment 11 WebKit Commit Bot 2016-07-25 01:58:15 PDT
All reviewed patches have been landed.  Closing bug.
Comment 12 Darin Adler 2016-07-25 09:49:18 PDT
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.
Comment 13 Sergio Villar Senin 2016-07-26 01:59:40 PDT
(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