[css-grid] Convert GridRepresentation into an actual class

[css-grid] Convert GridRepresentation into an actual class

So far grids are represented as Vectors of Vectors. There are a couple of
issues associated to that decision. First or all, the source code in
LayoutGrid assumes the existence of that data structure, meaning that we
cannot eventually change it without changing a lot of code. Apart from the
coupling there is another issue, LayoutGrid is full of methods to access and
manipulate that data structure.

Instead, it'd be much better to have a Grid class encapsulating both the
data structures and the methods required to access/manipulate it. Note that
follow-up patches will move more data and procedures into this new class
from the LayoutGrid code.

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

[svillar, 2016-11-21 15:16:15]

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

[eae, 2016-11-21 16:20:27]

Nice cleanup! LGTM

[Manuel Rego, 2016-11-22 10:07:18]

Nice patch, LGTM!

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

https://codereview.chromium.org/2522503002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.h:348: typedef
Vector<Vector<GridCell>> GridAsVector;
Or GridAsMatrix?

https://codereview.chromium.org/2522503002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.h:365: void clear();
Nit: Why this is not defined here?

It seems strange a few methods are written only here
and this one is not (when it's just one line).

https://codereview.chromium.org/2522503002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.h:369: GridAsVector m_grid;
Nit: I think it's kind of confusing to have 2 "m_grid"
even if they're in separated classes.

[svillar, 2016-11-22 10:15:18]

Thanks for the reviews!

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

https://codereview.chromium.org/2522503002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.h:348: typedef
Vector<Vector<GridCell>> GridAsVector;
On 2016/11/22 10:07:17, Manuel Rego wrote:
> Or GridAsMatrix?

If you don't feel strongly about it I'd prefer to keep it as it is, mainly in
order to avoid a lot of refactorings just because this name.

https://codereview.chromium.org/2522503002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.h:365: void clear();
On 2016/11/22 10:07:17, Manuel Rego wrote:
> Nit: Why this is not defined here?
> 
> It seems strange a few methods are written only here
> and this one is not (when it's just one line).

I don't think it's strange. Don't know if this is still valid in Blink, but in
WebKit is a quite common idiom. The rationale is that oneliners keep the class
definition pretty clean. If you start to add implementations to the class
definition then everything becomes a mess, and the API of the class is not that
easy to identify.

https://codereview.chromium.org/2522503002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.h:369: GridAsVector m_grid;
On 2016/11/22 10:07:17, Manuel Rego wrote:
> Nit: I think it's kind of confusing to have 2 "m_grid"
> even if they're in separated classes.

I agree but it's a private method, so there should not be any issue associated
to that, i.e. clients of this class are not aware of the existence of this
attribute.

[svillar, 2016-11-22 10:19:10]

The CQ bit was checked by svillar@igalia.com

[commit-bot: I haz the power, 2016-11-22 10:19:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-22 10:19:25]

This CL has an open dependency (Issue 2521553002 Patch 1). Please resolve the
dependency and try again.
If you are sure that there is no real dependency, please use one of the options
listed in https://goo.gl/9Es4OR to land the CL.

[Manuel Rego, 2016-11-22 10:38:01]

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

https://codereview.chromium.org/2522503002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.h:348: typedef
Vector<Vector<GridCell>> GridAsVector;
On 2016/11/22 10:15:18, svillar wrote:
> On 2016/11/22 10:07:17, Manuel Rego wrote:
> > Or GridAsMatrix?
> 
> If you don't feel strongly about it I'd prefer to keep it as it is, mainly in
> order to avoid a lot of refactorings just because this name.

I'd prefer to change it if possible.

https://codereview.chromium.org/2522503002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.h:365: void clear();
On 2016/11/22 10:15:18, svillar wrote:
> On 2016/11/22 10:07:17, Manuel Rego wrote:
> > Nit: Why this is not defined here?
> > 
> > It seems strange a few methods are written only here
> > and this one is not (when it's just one line).
> 
> I don't think it's strange. Don't know if this is still valid in Blink, but in
> WebKit is a quite common idiom. The rationale is that oneliners keep the class
> definition pretty clean. If you start to add implementations to the class
> definition then everything becomes a mess, and the API of the class is not
that
> easy to identify.

It's strange to me compared to shrinkToFit() for example. :-)
But I don't care it was just a picky comment.

https://codereview.chromium.org/2522503002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.h:369: GridAsVector m_grid;
On 2016/11/22 10:15:18, svillar wrote:
> On 2016/11/22 10:07:17, Manuel Rego wrote:
> > Nit: I think it's kind of confusing to have 2 "m_grid"
> > even if they're in separated classes.
> 
> I agree but it's a private method, so there should not be any issue associated
> to that, i.e. clients of this class are not aware of the existence of this
> attribute.

I know but still, when you see the LayoutGrid code you'll see
2 m_grid in different places. If you're eventually moving this
to a different file, I'm fine with keeping it. Otherwise,
I'd rename it.

[svillar, 2016-11-22 16:50:25]

The CQ bit was checked by svillar@igalia.com

[svillar, 2016-11-22 16:50:25]

The patchset sent to the CQ was uploaded after l-g-t-m from rego@igalia.com,
eae@chromium.org
Link to the patchset: https://codereview.chromium.org/2522503002/#ps20001
(title: "Pach for landing. Rebased")

[commit-bot: I haz the power, 2016-11-22 16:50:41]

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-22 18:36:15]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1479833425619130,
"parent_rev": "92dab71e13cc124e30dcb460382dd70a64fcb81b", "commit_rev":
"d73c43910abe359ac1ee3162d82e80df4862c2dd"}

[commit-bot: I haz the power, 2016-11-22 18:36:41]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-11-22 18:41:17]

Description was changed from

==========
[css-grid] Convert GridRepresentation into an actual class

So far grids are represented as Vectors of Vectors. There are a couple of
issues associated to that decision. First or all, the source code in
LayoutGrid assumes the existence of that data structure, meaning that we
cannot eventually change it without changing a lot of code. Apart from the
coupling there is another issue, LayoutGrid is full of methods to access and
manipulate that data structure.

Instead, it'd be much better to have a Grid class encapsulating both the
data structures and the methods required to access/manipulate it. Note that
follow-up patches will move more data and procedures into this new class
from the LayoutGrid code.

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

to

==========
[css-grid] Convert GridRepresentation into an actual class

So far grids are represented as Vectors of Vectors. There are a couple of
issues associated to that decision. First or all, the source code in
LayoutGrid assumes the existence of that data structure, meaning that we
cannot eventually change it without changing a lot of code. Apart from the
coupling there is another issue, LayoutGrid is full of methods to access and
manipulate that data structure.

Instead, it'd be much better to have a Grid class encapsulating both the
data structures and the methods required to access/manipulate it. Note that
follow-up patches will move more data and procedures into this new class
from the LayoutGrid code.

BUG=627812

Committed: https://crrev.com/2c3a3d0ac5362c2d07040a821bb3a22f701d58b6
Cr-Commit-Position: refs/heads/master@{#433922}
==========

[commit-bot: I haz the power, 2016-11-22 18:41:18]

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