Bug 140883

Summary: [CSS Grid Layout] Tracks growing beyond limits when they should not
Product: WebKit Reporter: Sergio Villar Senin <svillar>
Component: CSSAssignee: Sergio Villar Senin <svillar>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, darin, esprehn+autocc, glenn, hyatt, jfernandez, kling, koivisto, kondapallykalyan, rego, rniwa, svillar
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 140763    
Bug Blocks: 60731, 141422    
Attachments:
Description Flags
Patch darin: review+

Sergio Villar Senin
Reported 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.
Attachments
Patch (20.38 KB, patch)
2015-02-16 04:28 PST, Sergio Villar Senin 		darin: review+
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Sergio Villar Senin
Comment 1 2015-02-16 04:28:42 PST
Created attachment 246644 [details] Patch
Sergio Villar Senin
Comment 2 2015-02-24 08:03:56 PST
Ping reviewers.
Darin Adler
Comment 3 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?
Sergio Villar Senin
Comment 4 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.
Sergio Villar Senin
Comment 5 2015-02-25 02:54:21 PST
Committed r180623: <http://trac.webkit.org/changeset/180623>
Note You need to log in before you can comment on or make changes to this bug.