Created attachment 295362 [details] Patch

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} )

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.

Created attachment 295369 [details] Patch

Comment on attachment 295369 [details] Patch r=me

Committed r208973: <http://trac.webkit.org/changeset/208973>