Bug 104700 – [CSS Grid Layout] Implement grid items sizing for fixed minmax grid tracks

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Julien Chaffraix]

After bug 103799, we successfully parse minmax functions but don't properly size the grid items as we don't implement the specification's layout algorithm.

As a first step to support content based minmax, this bug will implement the correct sizing for fixed minmax e.g. minmax(40px, 40%) or minmax(40vh, 10em)

[Julien Chaffraix]

Created attachment 178869 [details]
Proposed implementation: Feedback seeked on whether testing logical width and logical height independently is sufficient.

[Build Bot]

Comment on attachment 178869 [details]
Proposed implementation: Feedback seeked on whether testing logical width and logical height independently is sufficient.

Attachment 178869 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15278498

[Tony Chang]

Comment on attachment 178869 [details]
Proposed implementation: Feedback seeked on whether testing logical width and logical height independently is sufficient.

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

Mac failure is real:

/Volumes/Data/EWS/WebKit/Source/WebCore/rendering/RenderGrid.cpp:192:63: error: unused parameter 'direction' [-Werror,-Wunused-parameter]
void RenderGrid::distributeSpaceToTracks(TrackSizingDirection direction, Vector<GridTrack>& tracks, LayoutUnit availableLogicalSpace)

> Source/WebCore/ChangeLog:9
> +        to resolve any sizing function that doesn't size based on the content (mincontent, maxcontent).

Nit: min-content, max-content.

> Source/WebCore/rendering/RenderGrid.cpp:125
> +        const Length& minTrackLength = trackStyles[i].minTrackBreadth();
> +        const Length& maxTrackLength = trackStyles[i].maxTrackBreadth();

I think most of the time we don't bother with using a const reference since Length is pretty small (actually, looks like it's trying to be small, but failing-- I'll fix that in a separate patch).

> Source/WebCore/rendering/RenderGrid.cpp:155
> +        const Length& minTrackBreadth = trackStyles[i].minTrackBreadth();
> +        const Length& maxTrackBreadth = trackStyles[i].maxTrackBreadth();

Nit: Maybe drop the const &.

> Source/WebCore/rendering/RenderGrid.cpp:160
> +        // Ignore the max track breadth if it is smaller than the min track breadth.

Remove this comment. It describe's the code and doesn't answer 'why'.

> Source/WebCore/rendering/RenderGrid.cpp:162
> +        if (track.m_maxBreadth < track.m_usedBreadth)
> +            track.m_maxBreadth = track.m_usedBreadth;

Nit: This could be track.m_maxBreadth = std::max(track.m_maxBreadth, track.m_usedBreadth).

> Source/WebCore/rendering/RenderGrid.cpp:179
> +    // FIXME: We stil need to support calc() here (bug 103761).

Nit: stil -> still

> Source/WebCore/rendering/RenderGrid.cpp:187
> +bool RenderGrid::sortByGridTrackGrowthPotential(GridTrack* track1, GridTrack* track2)

Does this function need to be part of RenderGrid?  I would declare it static to the cpp file.

> Source/WebCore/rendering/RenderGrid.cpp:194
> +    const unsigned tracksSize = tracks.size();

size_t and I would drop the const.  Most other uses of const are for global values.

> Source/WebCore/rendering/RenderGrid.cpp:196
> +    for (unsigned i = 0; i < tracksSize; ++i)

size_t

> Source/WebCore/rendering/RenderGrid.cpp:201
> +    for (unsigned i = 0; i < tracksSize; ++i) {

size_t

[Tony Chang]

(In reply to comment #3)
> actually, looks like it's trying to be small, but failing-- I'll fix that in a separate patch.

Oh, I'm wrong, it's already bit packed well.

[Julien Chaffraix]

Comment on attachment 178869 [details]
Proposed implementation: Feedback seeked on whether testing logical width and logical height independently is sufficient.

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

>> Source/WebCore/rendering/RenderGrid.cpp:125
>> +        const Length& maxTrackLength = trackStyles[i].maxTrackBreadth();
> 
> I think most of the time we don't bother with using a const reference since Length is pretty small (actually, looks like it's trying to be small, but failing-- I'll fix that in a separate patch).

True, but it's always better to avoid a copy if we can avoid it, which we can in this case.

>> Source/WebCore/rendering/RenderGrid.cpp:162
>> +            track.m_maxBreadth = track.m_usedBreadth;
> 
> Nit: This could be track.m_maxBreadth = std::max(track.m_maxBreadth, track.m_usedBreadth).

It can!

>> Source/WebCore/rendering/RenderGrid.cpp:187
>> +bool RenderGrid::sortByGridTrackGrowthPotential(GridTrack* track1, GridTrack* track2)
> 
> Does this function need to be part of RenderGrid?  I would declare it static to the cpp file.

Unfortunately it has to be because we are accessing GridTrack which is declared as a private class inside RenderGrid. We could make GridTrack publicly declared which would remove this need though. We could also pull it out of RenderGrid to avoid having to rewrite that to RenderGrid::GridTrack. Doing those 2 would probably be better though but I don't feel strongly either way.

>> Source/WebCore/rendering/RenderGrid.cpp:201
>> +    for (unsigned i = 0; i < tracksSize; ++i) {
> 
> size_t

There is an issue with using size_t here: LayoutUnit doesn't handle size_t in cross-platform manner. See the FIXME at the beginning of LayoutUnit and https://bugs.webkit.org/show_bug.cgi?id=83848#c19 for more details. It's possible to fix that by introducing the operator/() for all primitive types. Maybe it's time to nix this FIXME.

[Tony Chang]

(In reply to comment #5)
> (From update of attachment 178869 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=178869&action=review
> 
> >> Source/WebCore/rendering/RenderGrid.cpp:187
> >> +bool RenderGrid::sortByGridTrackGrowthPotential(GridTrack* track1, GridTrack* track2)
> > 
> > Does this function need to be part of RenderGrid?  I would declare it static to the cpp file.
> 
> Unfortunately it has to be because we are accessing GridTrack which is declared as a private class inside RenderGrid. We could make GridTrack publicly declared which would remove this need though. We could also pull it out of RenderGrid to avoid having to rewrite that to RenderGrid::GridTrack. Doing those 2 would probably be better though but I don't feel strongly either way.

I think that would be a good refactor.

[Julien Chaffraix]

Created attachment 179182 [details]
Updated change: made GridTrack public in WebCore namespace and switched the code to use size_t.

[Julien Chaffraix]

CC'ing Darin & Emil as they may have some comment on the LayoutUnit change.

[Build Bot]

Comment on attachment 179182 [details]
Updated change: made GridTrack public in WebCore namespace and switched the code to use size_t.

Attachment 179182 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/15313088

[Peter Beverloo (cr-android ews)]

Comment on attachment 179182 [details]
Updated change: made GridTrack public in WebCore namespace and switched the code to use size_t.

Attachment 179182 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/15316107

[Tony Chang]

FWIW, I would do the LayoutUnit change in a separate bug first.

[Emil A Eklund]

Comment on attachment 179182 [details]
Updated change: made GridTrack public in WebCore namespace and switched the code to use size_t.

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

> Source/WebCore/platform/LayoutUnit.h:73
>      LayoutUnit(int value) { setValue(value); }

Please remove the FIXME comment on line 67-70 as you are in effect adding support for size_t. Yay!

> Source/WebCore/platform/LayoutUnit.h:88
> +        m_value = clampTo<unsigned long long>(value * kFixedPointDenominator, static_cast<unsigned long long>(INT_MIN), static_cast<unsigned long long>(INT_MAX));

Assigning the return value from clampTo<unsigned long long> to m_value  do an implicit cast from an unsigned long long to a signed int. This will likely trigger a compiler warning if SATURATED_LAYOUT_ARITHMETIC is enabled. You'll might want to add a static_cast or do clampTo<int>.

> Source/WebCore/platform/LayoutUnit.h:305
> +    static bool isInBounds(unsigned long value)

Instead of adding two new isInBounds methods how about using the unsigned version directly and do a cast the REPORT_OVERFLOW call in the subpixel disabled constructors? I.e. REPORT_OVERFLOW(isInBounds(static_cast<unsigned>(value)));

[Julien Chaffraix]

Tony & Emil, thanks for the comment, it sounds like I should just fix bug 83848 for good instead of cutting corners by implementing half of it. Marking this bug as blocked on bug 83848.

[Julien Chaffraix]

Created attachment 181574 [details]
Updated patch, now that the size_t operations are supported.

[Tony Chang]

Comment on attachment 181574 [details]
Updated patch, now that the size_t operations are supported.

Thanks!

[Early Warning System Bot]

Comment on attachment 181574 [details]
Updated patch, now that the size_t operations are supported.

Attachment 181574 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15743768

[Early Warning System Bot]

Comment on attachment 181574 [details]
Updated patch, now that the size_t operations are supported.

Attachment 181574 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15731906

[Build Bot]

Comment on attachment 181574 [details]
Updated patch, now that the size_t operations are supported.

Attachment 181574 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/15739902

[Build Bot]

Comment on attachment 181574 [details]
Updated patch, now that the size_t operations are supported.

Attachment 181574 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15754044

[Julien Chaffraix]

Comment on attachment 181574 [details]
Updated patch, now that the size_t operations are supported.

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

> Source/WebCore/rendering/RenderGrid.cpp:153
> +    LayoutUnit availableLogicalSpace = (direction == ForColumns) ? availableLogicalWidth() : availableLogicalHeight();

The EWS is choking on this line. I forgot to update it to availableLogicalHeight(IncludeMarginBorderPadding).

[Julien Chaffraix]

Created attachment 181581 [details]
Updated patch for the EWS.

[WebKit Review Bot]

Comment on attachment 181581 [details]
Updated patch for the EWS.

Clearing flags on attachment: 181581

Committed r139025: <http://trac.webkit.org/changeset/139025>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Dimitri Glazkov (Google)]

The newly added test seems to be timing out:

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=fast%2Fcss-grid-layout%2Fpercent-resolution-grid-item.html%2Cfast%2Fcss-grid-layout%2Fbreadth-size-resolution-grid.html%2Cfast%2Fcss-grid-layout%2Fplace-cell-by-index.html%2Cfast%2Fcss-grid-layout%2Fpercent-padding-margin-resolution-grid-item.html%2Cfast%2Fcss-grid-layout%2Fgrid-columns-rows-get-set-multiple.html%2Cfast%2Fcss-grid-layout%2Fcalc-resolution-grid-item.html

[Dimitri Glazkov (Google)]

(In reply to comment #24)
> The newly added test seems to be timing out:

Oh, it _made_ some tests start time out. Bad stuff?

[Julien Chaffraix]

(In reply to comment #25)
> (In reply to comment #24)
> > The newly added test seems to be timing out:
> 
> Oh, it _made_ some tests start time out. Bad stuff?

The timeout only impacts Win7 Dbg(1), all the other platforms are just fine, including Win7 Dbg(2). The fact that another bot with a similar configuration is fine with the change makes me think the bot is acting up and the change is fine.

[Dimitri Glazkov (Google)]

(In reply to comment #26)
> (In reply to comment #25)
> > (In reply to comment #24)
> > > The newly added test seems to be timing out:
> > 
> > Oh, it _made_ some tests start time out. Bad stuff?
> 
> The timeout only impacts Win7 Dbg(1), all the other platforms are just fine, including Win7 Dbg(2). The fact that another bot with a similar configuration is fine with the change makes me think the bot is acting up and the change is fine.

Win7 Dbg(1) and (2) run two different sets of tests.

[Julien Chaffraix]

The tests that forced this bug re-opened are passing AFAICT. Closing back.