Summary: | [CSS Grid Layout] Tracks growing beyond limits when they should not | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Sergio Villar Senin <svillar> | ||||
Component: | CSS | Assignee: | 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
Sergio Villar Senin
2015-01-26 03:25:16 PST
Created attachment 246644 [details]
Patch
Ping reviewers. 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 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. Committed r180623: <http://trac.webkit.org/changeset/180623> |