Bug 149876

Summary: [css-grid] Include freeSpace in GridSizingData struct
Product: WebKit Reporter: Sergio Villar Senin <svillar>
Component: Layout and RenderingAssignee: Sergio Villar Senin <svillar>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, darin, dino, esprehn+autocc, glenn, jfernandez, kling, koivisto, kondapallykalyan, rego, svillar
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: All   
Attachments:
Description Flags
Patch
none
Patch darin: review+

Sergio Villar Senin
Reported 2015-10-07 08:31:25 PDT
We're passing freeSpaceForRows and freeSpaceForColumns to a bunch of functions along with the GridSizingData struct when we should really include them both in the struct as we're talking about temporary data that is only used for the layout process.
Attachments
Patch (12.44 KB, patch)
2015-10-07 08:46 PDT, Sergio Villar Senin
no flags
Patch (12.49 KB, patch)
2015-10-08 05:20 PDT, Sergio Villar Senin
darin: review+
Sergio Villar Senin
Comment 1 2015-10-07 08:46:06 PDT
Darin Adler
Comment 2 2015-10-07 09:11:18 PDT
Comment on attachment 262610 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=262610&action=review Need a new patch; looks like this one doesn’t build. > Source/WebCore/rendering/RenderGrid.cpp:214 > + GridSizingData(unsigned gridColumnCount, unsigned gridRowCount, const LayoutUnit& freeSpaceForColumns, const LayoutUnit& freeSpaceForRows) LayoutUnit is small enough that we probably want to just pass LayoutUnit, not const LayoutUnit&. > Source/WebCore/rendering/RenderGrid.cpp:358 > + const_cast<RenderGrid*>(this)->computeUsedBreadthOfGridTracks(ForColumns, sizingData); The const_cast in this function is strange. Doesn’t seem like the right pattern for functions with side effects. Not new to this patch, though.
Sergio Villar Senin
Comment 3 2015-10-08 05:17:15 PDT
(In reply to comment #2) > Comment on attachment 262610 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=262610&action=review > > Need a new patch; looks like this one doesn’t build. Beh, I somehow committed a patch without one of the replacements.
Sergio Villar Senin
Comment 4 2015-10-08 05:20:45 PDT
Sergio Villar Senin
Comment 5 2015-10-09 05:57:21 PDT
Note You need to log in before you can comment on or make changes to this bug.