Bug 149876 - [css-grid] Include freeSpace in GridSizingData struct
Summary: [css-grid] Include freeSpace in GridSizingData struct
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified All
: P2 Normal
Assignee: Sergio Villar Senin
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-10-07 08:31 PDT by Sergio Villar Senin
Modified: 2015-10-09 05:57 PDT (History)
11 users (show)

See Also:


Attachments
Patch (12.44 KB, patch)
2015-10-07 08:46 PDT, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch (12.49 KB, patch)
2015-10-08 05:20 PDT, 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-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.
Comment 1 Sergio Villar Senin 2015-10-07 08:46:06 PDT
Created attachment 262610 [details]
Patch
Comment 2 Darin Adler 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.
Comment 3 Sergio Villar Senin 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.
Comment 4 Sergio Villar Senin 2015-10-08 05:20:45 PDT
Created attachment 262688 [details]
Patch
Comment 5 Sergio Villar Senin 2015-10-09 05:57:21 PDT
Committed r190784: <http://trac.webkit.org/changeset/190784>