Bug 140883 – [CSS Grid Layout] Tracks growing beyond limits when they should not

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

[Sergio Villar Senin]

The problem is that we're using a vector of indexes pointing to the vector with the list of affected tracks. Those indexes become easily incorrect because the first thing we do in distributeSpaceToTracks() is to sort the vector with the affected tracks by growth potential.

[Sergio Villar Senin]

Created attachment 246644 [details]
Patch

[Sergio Villar Senin]

Ping reviewers.

[Darin Adler]

Comment on attachment 246644 [details]
Patch

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

> Source/WebCore/rendering/RenderGrid.cpp:82
> +    LayoutUnit m_plannedSize { 0 };

Public data members are an anti-pattern we should strive to avoid. Is there some way to do this without having it be public? Even an accessor function that returns a reference is considered slightly better.

> Source/WebCore/rendering/RenderGrid.cpp:685
> +        Vector<GridTrack*>* tracksToGrowBeyondGrowthLimits = sizingData.growBeyondGrowthLimitsTracks.isEmpty() ? &sizingData.filteredTracks : &sizingData.growBeyondGrowthLimitsTracks;

Seems we should use auto* or auto here instead of Vector<GridTrack*>*.

> Source/WebCore/rendering/RenderGrid.h:58
> +    typedef void (GridTrack::* AccumulatorGrowFunction)(LayoutUnit);

I don’t see anything in this patch that requires that this type be public. What did I miss?

[Sergio Villar Senin]

Comment on attachment 246644 [details]
Patch

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

>> Source/WebCore/rendering/RenderGrid.cpp:82
>> +    LayoutUnit m_plannedSize { 0 };
> 
> Public data members are an anti-pattern we should strive to avoid. Is there some way to do this without having it be public? Even an accessor function that returns a reference is considered slightly better.

I know but the code with getters/setters was full of boilerplate. I thought about returning a reference but didn't consider it was specially better.

>> Source/WebCore/rendering/RenderGrid.cpp:685
>> +        Vector<GridTrack*>* tracksToGrowBeyondGrowthLimits = sizingData.growBeyondGrowthLimitsTracks.isEmpty() ? &sizingData.filteredTracks : &sizingData.growBeyondGrowthLimitsTracks;
> 
> Seems we should use auto* or auto here instead of Vector<GridTrack*>*.

Yep.

>> Source/WebCore/rendering/RenderGrid.h:58
>> +    typedef void (GridTrack::* AccumulatorGrowFunction)(LayoutUnit);
> 
> I don’t see anything in this patch that requires that this type be public. What did I miss?

You're right. Somehow I slipped it in.

[Sergio Villar Senin]

Committed r180623: <http://trac.webkit.org/changeset/180623>