[css-grid] Convert grid representation into a class

[Sergio Villar Senin]

[css-grid] Convert grid representation into a class

[Sergio Villar Senin]

Created attachment 295362 [details]
Patch

[Javier Fernandez]

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 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]

Created attachment 295369 [details]
Patch

[Manuel Rego Casasnovas]

Comment on attachment 295369 [details]
Patch

r=me

[Sergio Villar Senin]

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