[css-grid] Move more LayoutGrid attributes to Grid class

[css-grid] Move more LayoutGrid attributes to Grid class

Moved the remaining attributes of LayoutGrid to Grid class. In particular
m_autoRepeat{Columns|Rows}, m_autoRepeatEmpty{Columns|Rows} and
m_orderIterator. All of them are now private attributes of the Grid class so
clients should use the getters/setters provided by this CL.

This change allows to definitely remove the grid{Column|Row}Count() methods
from LayoutGrid. These two became Grid::numTracks() which returns the number
of tracks in the data structure used to represent the grid (currently
a matrix). Contrary to that, LayoutGrid::numTracks() returns the actual size
of the grid.

BUG=627812
Committed: https://crrev.com/bf7e60b4260824122d9522220919766cd58458c7
Cr-Commit-Position: refs/heads/master@{#434510}

[svillar, 2016-11-24 14:42:50]

svillar@igalia.com changed reviewers:
+ cbiesinger@chromium.org, eae@chromium.org, jfernandez@igalia.com,
rego@igalia.com

[jfernandez, 2016-11-24 17:28:14]

Very nice refactoring and my comments are just suggestions for improvements, not
a blocker at all. hence, LGTM from my side.

https://codereview.chromium.org/2525363002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/2525363002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:963: const GridSpan span =
m_grid.gridItemSpan(*gridItem, direction);
Why not using const GridSpan& span like you do some lines below.

https://codereview.chromium.org/2525363002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:2048: size_t
autoRepeatColumns;
I guess it's not that important, but we could save some assignment and a few
lines if we define the autoRepeatColumns with a ternary instruction. Perhaps the
line is too long and end up not very clear, so I trust your taste on following
or not this suggestion.

https://codereview.chromium.org/2525363002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:3568: return
m_grid.numTracks(ForRows)
It's not the first time I see this pattern I I have to say I don't get used to
it. It's unnatural to read that we return the number of columns if numRows is
not 0. I know this is because our internal structure requires to have rows in
order to store columns, but I thought the goal we pursued with this patch was to
decouple our logic from the grid internal structure. 

Can't we have some function that to retrieve the columns, which can check
internally if it's possible to compute them, returning 0 or a default value
otherwise ? 

Or perhaps less sophisticated, just a function that determines whether we can
compute columns or not.

[svillar, 2016-11-25 12:31:42]

Thanks for the review!!!

https://codereview.chromium.org/2525363002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/2525363002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:963: const GridSpan span =
m_grid.gridItemSpan(*gridItem, direction);
On 2016/11/24 17:28:14, jfernandez wrote:
> Why not using const GridSpan& span like you do some lines below. 

I'm not changing that, it was in the original code. But I can take the chance to
change it if you want.

https://codereview.chromium.org/2525363002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:2048: size_t
autoRepeatColumns;
On 2016/11/24 17:28:14, jfernandez wrote:
> I guess it's not that important, but we could save some assignment and a few
> lines if we define the autoRepeatColumns with a ternary instruction. Perhaps
the
> line is too long and end up not very clear, so I trust your taste on following
> or not this suggestion.

Yeah I like the ternary too. The thing is that with the new formatting it
becomes really messy sometimes.

In any case there is a hidden reason why I would not pay much attention to these
lines, and it's that they're going to go away soon ;)

https://codereview.chromium.org/2525363002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:3568: return
m_grid.numTracks(ForRows)
On 2016/11/24 17:28:14, jfernandez wrote:
> It's not the first time I see this pattern I I have to say I don't get used to
> it. It's unnatural to read that we return the number of columns if numRows is
> not 0. I know this is because our internal structure requires to have rows in
> order to store columns, but I thought the goal we pursued with this patch was
to
> decouple our logic from the grid internal structure. 

Yeah you're right. This method is basically the only one which still "knows"
that we're using a matrix to store the grid data. We should definitely fix this,
but I'd propose to do it in a followup patch.
 
> Can't we have some function that to retrieve the columns, which can check
> internally if it's possible to compute them, returning 0 or a default value
> otherwise ? 

Yeah, but that would force us to precompute the default value always even if not
needed. Perhaps not terribly expensive anyway.

> Or perhaps less sophisticated, just a function that determines whether we can
> compute columns or not. 

Something like that will still mean that we know about the internal
representation.

[svillar, 2016-11-25 14:17:22]

The CQ bit was checked by svillar@igalia.com

[svillar, 2016-11-25 14:17:23]

The patchset sent to the CQ was uploaded after l-g-t-m from
jfernandez@igalia.com
Link to the patchset: https://codereview.chromium.org/2525363002/#ps20001
(title: "Patch for landing")

[commit-bot: I haz the power, 2016-11-25 14:17:35]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-25 15:41:34]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1480083442828200,
"parent_rev": "13519c92477e9e194ccf4b8952f71bbfa4e06d1e", "commit_rev":
"521a6026895909072c09e3d7fa21ddeaf7d9c30c"}

[commit-bot: I haz the power, 2016-11-25 15:42:01]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-11-25 15:44:06]

Description was changed from

==========
[css-grid] Move more LayoutGrid attributes to Grid class

Moved the remaining attributes of LayoutGrid to Grid class. In particular
m_autoRepeat{Columns|Rows}, m_autoRepeatEmpty{Columns|Rows} and
m_orderIterator. All of them are now private attributes of the Grid class so
clients should use the getters/setters provided by this CL.

This change allows to definitely remove the grid{Column|Row}Count() methods
from LayoutGrid. These two became Grid::numTracks() which returns the number
of tracks in the data structure used to represent the grid (currently
a matrix). Contrary to that, LayoutGrid::numTracks() returns the actual size
of the grid.

BUG=627812
==========

to

==========
[css-grid] Move more LayoutGrid attributes to Grid class

Moved the remaining attributes of LayoutGrid to Grid class. In particular
m_autoRepeat{Columns|Rows}, m_autoRepeatEmpty{Columns|Rows} and
m_orderIterator. All of them are now private attributes of the Grid class so
clients should use the getters/setters provided by this CL.

This change allows to definitely remove the grid{Column|Row}Count() methods
from LayoutGrid. These two became Grid::numTracks() which returns the number
of tracks in the data structure used to represent the grid (currently
a matrix). Contrary to that, LayoutGrid::numTracks() returns the actual size
of the grid.

BUG=627812

Committed: https://crrev.com/bf7e60b4260824122d9522220919766cd58458c7
Cr-Commit-Position: refs/heads/master@{#434510}
==========

[commit-bot: I haz the power, 2016-11-25 15:44:07]

Patchset 2 (id:??) landed as
https://crrev.com/bf7e60b4260824122d9522220919766cd58458c7
Cr-Commit-Position: refs/heads/master@{#434510}