Bug 165042

Summary: [css-grid] Convert grid representation into a class
Product: WebKit Reporter: Sergio Villar Senin <svillar>
Component: New BugsAssignee: Sergio Villar Senin <svillar>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, darin, esprehn+autocc, glenn, jfernandez, joepeck, kondapallykalyan, mcatanzaro, rego, svillar
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 165007    
Attachments:
Description Flags
Patch
none
Patch rego: review+

Sergio Villar Senin
Reported 2016-11-23 01:51:42 PST
[css-grid] Convert grid representation into a class
Attachments
Patch (10.82 KB, patch)
2016-11-23 01:57 PST, Sergio Villar Senin 		no flags
DetailsFormatted DiffDiff
Patch (10.87 KB, patch)
2016-11-23 09:04 PST, Sergio Villar Senin 		rego: review+
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Sergio Villar Senin
Comment 1 2016-11-23 01:57:42 PST
Created attachment 295362 [details] Patch
Javier Fernandez
Comment 2 2016-11-23 04:53:03 PST
Comment on attachment 295362 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=295362&action=review I like the change, I think it's a great code refactoring that improves code readability. > Source/WebCore/rendering/RenderGrid.cpp:54 > + for (size_t row = oldRowSize; row < numRows(); ++row) Isn' t numRows() the same value than maximumRowSize ? > Source/WebCore/rendering/RenderGrid.cpp:55 > + m_grid[row].grow(numColumns()); Perhaps we should cache numColums() value. > Source/WebCore/rendering/RenderGrid.cpp:58 > + if (maximumColumnSize > numColumns()) { If we have numColumns() cached, we can use it here as well. > Source/WebCore/rendering/RenderGrid.cpp:59 > + for (size_t row = 0; row < numRows(); ++row) Ditto. > Source/WebCore/rendering/RenderGrid.cpp:1687 > + m_grid.insert(*child, GridArea(area.rows, area.columns)); Why not using static initializer ? m_grid.insert(*child, { area.rows, area.columns }); > Source/WebCore/rendering/RenderGrid.cpp:2726 > + return m_grid.numRows() ? m_grid.numColumns() : GridPositionsResolver::explicitGridColumnCount(style(), m_autoRepeatColumns); I wonder whether it'd be better/clearer to define a more explicit method to figure out the grid size (eg, isEmtty(), has{Rows, Columns} )
Sergio Villar Senin
Comment 3 2016-11-23 08:41:19 PST
Comment on attachment 295362 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=295362&action=review >> Source/WebCore/rendering/RenderGrid.cpp:54 >> + for (size_t row = oldRowSize; row < numRows(); ++row) > > Isn' t numRows() the same value than maximumRowSize ? Rigth. But that was already there, I just moved the code around >> Source/WebCore/rendering/RenderGrid.cpp:1687 >> + m_grid.insert(*child, GridArea(area.rows, area.columns)); > > Why not using static initializer ? > > m_grid.insert(*child, { area.rows, area.columns }); ACK >> Source/WebCore/rendering/RenderGrid.cpp:2726 >> + return m_grid.numRows() ? m_grid.numColumns() : GridPositionsResolver::explicitGridColumnCount(style(), m_autoRepeatColumns); > > I wonder whether it'd be better/clearer to define a more explicit method to figure out the grid size (eg, isEmtty(), has{Rows, Columns} ) It's used just here, I don't think it's really needed.
Sergio Villar Senin
Comment 4 2016-11-23 09:04:14 PST
Created attachment 295369 [details] Patch
Manuel Rego Casasnovas
Comment 5 2016-11-24 07:01:00 PST
Comment on attachment 295369 [details] Patch r=me
Sergio Villar Senin
Comment 6 2016-11-24 07:08:32 PST
Committed r208973: <http://trac.webkit.org/changeset/208973>
Note You need to log in before you can comment on or make changes to this bug.