Bug 141422 – [CSS Grid Layout] Update track sizes after distributing extra space

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Sergio Villar Senin]

Both old and new tracks sizing algorithms instruct us to update the sizes of the content sized tracks only after distributing the extra space for all the items in the same span group. So far we have been doing it inside distributeSpaceToTracks(), i.e., once for every single item. That is wrong because it makes the algorithm order dependent.

The problem is that our code is like this:

resolveContentBasedTrackSizingFunctions()
    for each item resolveContentBasedTrackSizingFunctionsForItems() (x4)
        distributeSpaceToTracks()
            updateTrackSizes();

We must move the update out of the distributeSpaceToTracks(). We also need to call resolveContentBasedTrackSizingFunctionsForItems() with all the items with a particular span instead of one by one.

[Sergio Villar Senin]

Created attachment 247328 [details]
Patch

[Sergio Villar Senin]

Ping

[Sergio Villar Senin]

Knock, knock reviewers :)

[Sergio Villar Senin]

Any chance to get this reviewed? It's more than 1 month old

[Antti Koivisto]

Comment on attachment 247328 [details]
Patch

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

Ok but...

> Source/WebCore/rendering/RenderGrid.cpp:619
> +struct GridItemsSpanGroupRange {
> +    Vector<GridItemWithSpan>::iterator rangeStart;
> +    Vector<GridItemWithSpan>::iterator rangeEnd;
> +};

Maybe you could use WTF::IteratorRange?

> Source/WebCore/rendering/RenderGrid.cpp:694
> +            GridTrack& track = (direction == ForColumns) ? sizingData.columnTracks[trackPosition.toInt()] : sizingData.rowTracks[trackPosition.toInt()];

Indexing to array with unchecked.

> Source/WebCore/rendering/RenderGrid.cpp:696
> +            spanningTracksSize += (track.*correctedTrackSize)();
> +            if (!(trackSize.*filterFunction)())

Raw function pointers.

> Source/WebCore/rendering/RenderGrid.cpp:699
> +            sizingData.filteredTracks.append(&track);

Raw pointer.

All this stuff looks scary and potentially insecure. I don't doubt that the logic is sound but is there any way to make this code look less like a disaster waiting to happen?

[Antti Koivisto]

Comment on attachment 247328 [details]
Patch

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

> Source/WebCore/rendering/RenderGrid.cpp:645
> +        resolveContentBasedTrackSizingFunctionsForItems(direction, sizingData, spanGroupRange, &GridTrackSize::hasMinOrMaxContentMinTrackBreadth, &RenderGrid::minContentForChild, &GridTrack::baseSize, &GridTrack::baseSize, &GridTrack::setBaseSize, &GridTrackSize::hasMinContentMinTrackBreadthAndMinOrMaxContentMaxTrackBreadth);

The argument list is rather incomprehensible. Maybe you should consider passing them as a struct.

[zalan]

Comment on attachment 247328 [details]
Patch

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

> Source/WebCore/rendering/RenderGrid.cpp:82
> +    const LayoutUnit& plannedSize() const { return m_plannedSize; }

Any particular reason to return a reference here?

> Source/WebCore/rendering/RenderGrid.cpp:84
> +    void setPlannedSize(const LayoutUnit& plannedSize)

const LayoutUnit& plannedSize -> LayoutUnit plannedSize

> Source/WebCore/rendering/RenderGrid.cpp:89
> +    LayoutUnit& tempSize() { return m_tempSize; }

Any particular reason to return a reference here?

[Sergio Villar Senin]

Comment on attachment 247328 [details]
Patch

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

>> Source/WebCore/rendering/RenderGrid.cpp:82
>> +    const LayoutUnit& plannedSize() const { return m_plannedSize; }
> 
> Any particular reason to return a reference here?

It was suggested by Darin in a previous patch to avoid giving public access to the attribute. But since we have setPlannedSize() now we can remove the reference.

>> Source/WebCore/rendering/RenderGrid.cpp:84
>> +    void setPlannedSize(const LayoutUnit& plannedSize)
> 
> const LayoutUnit& plannedSize -> LayoutUnit plannedSize

OK

>> Source/WebCore/rendering/RenderGrid.cpp:89
>> +    LayoutUnit& tempSize() { return m_tempSize; }
> 
> Any particular reason to return a reference here?

That's because that particular attribute is basically initialized and then incremented several times. So in order to avoid setTempSize(tempSize() + increment) I decided to return a reference and directly do tempSize() += increment

>> Source/WebCore/rendering/RenderGrid.cpp:619
>> +};
> 
> Maybe you could use WTF::IteratorRange?

Interesting, I'll try indeed.

>> Source/WebCore/rendering/RenderGrid.cpp:645
>> +        resolveContentBasedTrackSizingFunctionsForItems(direction, sizingData, spanGroupRange, &GridTrackSize::hasMinOrMaxContentMinTrackBreadth, &RenderGrid::minContentForChild, &GridTrack::baseSize, &GridTrack::baseSize, &GridTrack::setBaseSize, &GridTrackSize::hasMinContentMinTrackBreadthAndMinOrMaxContentMaxTrackBreadth);
> 
> The argument list is rather incomprehensible. Maybe you should consider passing them as a struct.

Agree, I'll do it in a follow-up patch.

>> Source/WebCore/rendering/RenderGrid.cpp:699
>> +            sizingData.filteredTracks.append(&track);
> 
> Raw pointer.
> 
> All this stuff looks scary and potentially insecure. I don't doubt that the logic is sound but is there any way to make this code look less like a disaster waiting to happen?

This particular change does not add any of the issues you are mentioning. So although I agree with your smart comments I think we should fix those in a different bug in order to isolate these changes in the algorithm.

[Antti Koivisto]

> This particular change does not add any of the issues you are mentioning. So
> although I agree with your smart comments I think we should fix those in a
> different bug in order to isolate these changes in the algorithm.

Right, that's why I r+'d this. It was more of a general comment.

[Sergio Villar Senin]

Committed r182472: <http://trac.webkit.org/changeset/182472>

[Michael Catanzaro]

(In reply to comment #10)
> Committed r182472: <http://trac.webkit.org/changeset/182472>

Some time travel in the changelog entry dates. I dunno if we normally fix that or not.

[Darin Adler]

We usually don’t bother, but any committer can fix it if they like. Not worth posting a patch just to do so.