Bug 140883 - [CSS Grid Layout] Tracks growing beyond limits when they should not
Summary: [CSS Grid Layout] Tracks growing beyond limits when they should not
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sergio Villar Senin
URL:
Keywords:
Depends on: 140763
Blocks: 60731 141422
  Show dependency treegraph
 
Reported: 2015-01-26 03:25 PST by Sergio Villar Senin
Modified: 2015-02-25 02:54 PST (History)
12 users (show)

See Also:


Attachments
Patch (20.38 KB, patch)
2015-02-16 04:28 PST, Sergio Villar Senin
darin: review+
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 2015-01-26 03:25:16 PST
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.
Comment 1 Sergio Villar Senin 2015-02-16 04:28:42 PST
Created attachment 246644 [details]
Patch
Comment 2 Sergio Villar Senin 2015-02-24 08:03:56 PST
Ping reviewers.
Comment 3 Darin Adler 2015-02-24 09:13:55 PST
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?
Comment 4 Sergio Villar Senin 2015-02-25 02:11:00 PST
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.
Comment 5 Sergio Villar Senin 2015-02-25 02:54:21 PST
Committed r180623: <http://trac.webkit.org/changeset/180623>