[CSS Grid Layout] Introduce an explicit type for resolved grid positions

[CSS Grid Layout] Introduce an explicit type for resolved grid positions

We were using simple size_t integers to represent resolved grid
positions in our internal data structures. This change allows us to
clarify the code, avoid potential off by one mistakes, and move the
resolving code to a central place.

Based in a patch by Xan Lopez <xlopez@igalia.com>;
https://codereview.chromium.org/113943003/.

BUG=258258
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=171156

[Julien - ping for review, 2014-03-06 22:38:02]

Sorry for the delay (my review queue is winning these days).

What's your longer term plan? Do you intend to move the resolution code to an
external class and just return GridResolvedPosition from there?

https://codereview.chromium.org/166623002/diff/1/Source/core/rendering/style/...
File Source/core/rendering/style/GridResolvedPosition.h (right):

https://codereview.chromium.org/166623002/diff/1/Source/core/rendering/style/...
Source/core/rendering/style/GridResolvedPosition.h:38: }
IMHO we should just not have this constructor and always be explicit.

https://codereview.chromium.org/166623002/diff/1/Source/core/rendering/style/...
Source/core/rendering/style/GridResolvedPosition.h:55: size_t integerPosition()
const
Do we need the explicit "integer" in the getter/setter? I would expect a
resolved position to be an index so it can't be anything but an integer.

https://codereview.chromium.org/166623002/diff/1/Source/core/rendering/style/...
Source/core/rendering/style/GridResolvedPosition.h:64: 
The current API makes the code very verbose as we have to explicitly doing int
conversions.

It would be nice to implement the following:
- some comparison operators on GridResolvedPosition (<, <=, >, ...) between
GridResolvedPosition (and maybe also int?)
- void setPosition(const GridResolvedPosition&)
- operator int() (<- I am not really sure about this one as it would allow to
easily mix int / GridResolvedPosition and probably contained in the first
suggestion)

[Manuel Rego, 2014-03-07 14:32:14]

On 2014/03/06 22:38:02, Julien Chaffraix - PST wrote:
> Sorry for the delay (my review queue is winning these days).

No problem :-)

> What's your longer term plan? Do you intend to move the resolution code to an
> external class and just return GridResolvedPosition from there?

Yeah, Xan told me that you have already talked about it.
Basically from RenderGrid it will be always used GridResolvedPosition. And the
resolution code will be moved out of RenderGrid.

I've just uploaded a new patch applying the suggested changes.

>
https://codereview.chromium.org/166623002/diff/1/Source/core/rendering/style/...
> File Source/core/rendering/style/GridResolvedPosition.h (right):
> 
>
https://codereview.chromium.org/166623002/diff/1/Source/core/rendering/style/...
> Source/core/rendering/style/GridResolvedPosition.h:38: }
> IMHO we should just not have this constructor and always be explicit.

Sure.

>
https://codereview.chromium.org/166623002/diff/1/Source/core/rendering/style/...
> Source/core/rendering/style/GridResolvedPosition.h:55: size_t
integerPosition()
> const
> Do we need the explicit "integer" in the getter/setter? I would expect a
> resolved position to be an index so it can't be anything but an integer.

Yes, I agree. I've changed it removing "integer".

>
https://codereview.chromium.org/166623002/diff/1/Source/core/rendering/style/...
> Source/core/rendering/style/GridResolvedPosition.h:64: 
> The current API makes the code very verbose as we have to explicitly doing int
> conversions.

This is an intermediate result, as the final code won't need these methods.
 
> It would be nice to implement the following:
> - some comparison operators on GridResolvedPosition (<, <=, >, ...) between
> GridResolvedPosition (and maybe also int?)

Yeah, I've done this in the next patch.

> - void setPosition(const GridResolvedPosition&)

In my second patch, instead of setPostion() I'm using operator =.

> - operator int() (<- I am not really sure about this one as it would allow to
> easily mix int / GridResolvedPosition and probably contained in the first
> suggestion)

I also have this in the next patch, we need it for the following examples:
GridResolvedPosition a, b;
m_grid[a][b]

Or we would need to do:
m_grid[a.position()][b.position()]

Anyway if you think it's better I upload the whole changes in only 1 patch
I can do it too. Not a big deal.

[Julien - ping for review, 2014-03-10 18:29:04]

On 2014/03/07 14:32:14, Manuel Rego wrote:
> On 2014/03/06 22:38:02, Julien Chaffraix - PST wrote:
> > Sorry for the delay (my review queue is winning these days).
> 
> No problem :-)
> 
> > What's your longer term plan? Do you intend to move the resolution code to
an
> > external class and just return GridResolvedPosition from there?
> 
> Yeah, Xan told me that you have already talked about it.
> Basically from RenderGrid it will be always used GridResolvedPosition. And the
> resolution code will be moved out of RenderGrid.
> 
> I've just uploaded a new patch applying the suggested changes.
> 
> >
>
https://codereview.chromium.org/166623002/diff/1/Source/core/rendering/style/...
> > File Source/core/rendering/style/GridResolvedPosition.h (right):
> > 
> >
>
https://codereview.chromium.org/166623002/diff/1/Source/core/rendering/style/...
> > Source/core/rendering/style/GridResolvedPosition.h:38: }
> > IMHO we should just not have this constructor and always be explicit.
> 
> Sure.
> 
> >
>
https://codereview.chromium.org/166623002/diff/1/Source/core/rendering/style/...
> > Source/core/rendering/style/GridResolvedPosition.h:55: size_t
> integerPosition()
> > const
> > Do we need the explicit "integer" in the getter/setter? I would expect a
> > resolved position to be an index so it can't be anything but an integer.
> 
> Yes, I agree. I've changed it removing "integer".
> 
> >
>
https://codereview.chromium.org/166623002/diff/1/Source/core/rendering/style/...
> > Source/core/rendering/style/GridResolvedPosition.h:64: 
> > The current API makes the code very verbose as we have to explicitly doing
int
> > conversions.
> 
> This is an intermediate result, as the final code won't need these methods.
>  
> > It would be nice to implement the following:
> > - some comparison operators on GridResolvedPosition (<, <=, >, ...) between
> > GridResolvedPosition (and maybe also int?)
> 
> Yeah, I've done this in the next patch.
> 
> > - void setPosition(const GridResolvedPosition&)
> 
> In my second patch, instead of setPostion() I'm using operator =.
> 
> > - operator int() (<- I am not really sure about this one as it would allow
to
> > easily mix int / GridResolvedPosition and probably contained in the first
> > suggestion)
> 
> I also have this in the next patch, we need it for the following examples:
> GridResolvedPosition a, b;
> m_grid[a][b]
> 
> Or we would need to do:
> m_grid[a.position()][b.position()]
> 
> Anyway if you think it's better I upload the whole changes in only 1 patch
> I can do it too. Not a big deal.

As it seems that your follow-up patch solves most of my review issues, I think
it's worth squashing them together. Thanks for trying to keep your changes
small, always appreciated.

[Manuel Rego, 2014-03-11 17:00:26]

On 2014/03/10 18:29:04, Julien Chaffraix - PST wrote:
> As it seems that your follow-up patch solves most of my review issues, I think
> it's worth squashing them together. Thanks for trying to keep your changes
> small, always appreciated.

Uploaded new patch. I'm using some comparison operators with size_t and also
defining the operator size_t(). We could avoid it and add a getter.

Take into account that the code would need to be updated in some places
in order to use the getter. For example:
    m_grid[a][b]
vs:
    m_grid[a.position()][b.position()]

[Julien - ping for review, 2014-03-21 23:49:27]

The gist of the patch is here. Some comments but it looks pretty neat now.

https://codereview.chromium.org/166623002/diff/80001/Source/core/css/parser/B...
File Source/core/css/parser/BisonCSSParser-in.cpp (right):

https://codereview.chromium.org/166623002/diff/80001/Source/core/css/parser/B...
Source/core/css/parser/BisonCSSParser-in.cpp:83: #include
"core/rendering/style/GridResolvedPosition.h"
Is this needed when no code uses it?

https://codereview.chromium.org/166623002/diff/80001/Source/core/rendering/Re...
File Source/core/rendering/RenderGrid.cpp (right):

https://codereview.chromium.org/166623002/diff/80001/Source/core/rendering/Re...
Source/core/rendering/RenderGrid.cpp:794: maximumColumnIndex =
std::max<size_t>(maximumColumnIndex,
columnPositions->resolvedFinalPosition.next());
Do we need GridResolvedPosition::next() or would the cast to size_t allow us to
do columnPositions->resolvedFinalPosition + 1?

(I am fine with next(), it's just that it feels weird in this case, especially
since you added the cast to size_t)

https://codereview.chromium.org/166623002/diff/80001/Source/core/rendering/Re...
Source/core/rendering/RenderGrid.cpp:954: for (GridResolvedPosition
trackPosition = span.resolvedInitialPosition; trackPosition <=
span.resolvedFinalPosition; ++trackPosition)
FWIW this seems to be repeated a lot, maybe it would be worth adding
GridResolvedPositionIterator at some point (not as part of this patch though).

https://codereview.chromium.org/166623002/diff/80001/Source/core/rendering/Re...
Source/core/rendering/RenderGrid.cpp:1127: static inline bool
isInSameRowBeforeDirtyArea(const GridCoordinate& coordinate,
GridResolvedPosition row, const GridSpan& dirtiedColumns)
const GridResolvedPosition& to avoid the copy.

https://codereview.chromium.org/166623002/diff/80001/Source/core/rendering/Re...
Source/core/rendering/RenderGrid.cpp:1132: static inline bool
isInSameRowAfterDirtyArea(const GridCoordinate& coordinate, GridResolvedPosition
row, const GridSpan& dirtiedColumns)
Same comment.

https://codereview.chromium.org/166623002/diff/80001/Source/core/rendering/st...
File Source/core/rendering/style/GridResolvedPosition.cpp (right):

https://codereview.chromium.org/166623002/diff/80001/Source/core/rendering/st...
Source/core/rendering/style/GridResolvedPosition.cpp:19: PassOwnPtr<GridSpan>
GridResolvedPosition::resolveGridPositionsFromStyle(const RenderStyle& style,
const RenderBox* gridItem, GridTrackSizingDirection direction)
Nit: s/style/gridContainerStyle/

Also shouldn't |gridItem| be a reference?

https://codereview.chromium.org/166623002/diff/80001/Source/core/rendering/st...
Source/core/rendering/style/GridResolvedPosition.cpp:178: }
It really seems like a lot of this code should be moved to a Factory or a
dedicated resolver. I think it's fine as a first step but @esprehn would feel
stronger about that.

https://codereview.chromium.org/166623002/diff/80001/Source/core/rendering/st...
File Source/core/rendering/style/GridResolvedPosition.h (right):

https://codereview.chromium.org/166623002/diff/80001/Source/core/rendering/st...
Source/core/rendering/style/GridResolvedPosition.h:27: 
Maybe worth adding a class comment that this is an index into the grid array.
It's also a glorified size_t just for the purpose of knowing what we manipulate
in the grid code.

https://codereview.chromium.org/166623002/diff/80001/Source/core/rendering/st...
Source/core/rendering/style/GridResolvedPosition.h:58: size_t integerPosition;
No need to define this variable here. Better just below when you set it.

[Manuel Rego, 2014-03-24 23:28:26]

Thanks for the review, I've just uploaded a new version of the patch.

On 2014/03/21 23:49:27, Julien Chaffraix - PST wrote:
>
https://codereview.chromium.org/166623002/diff/80001/Source/core/css/parser/B...
> Source/core/css/parser/BisonCSSParser-in.cpp:83: #include
> "core/rendering/style/GridResolvedPosition.h"
> Is this needed when no code uses it?

No it was there due to a previous state now the code is in CSSPropertyParser. 

>
https://codereview.chromium.org/166623002/diff/80001/Source/core/rendering/Re...
> Source/core/rendering/RenderGrid.cpp:794: maximumColumnIndex =
> std::max<size_t>(maximumColumnIndex,
> columnPositions->resolvedFinalPosition.next());
> Do we need GridResolvedPosition::next() or would the cast to size_t allow us
to
> do columnPositions->resolvedFinalPosition + 1?

Yes, next() is not needed anymore. At the end I added the cast to size_t
for clarity using m_grid so next() can be removed now.

>
https://codereview.chromium.org/166623002/diff/80001/Source/core/rendering/Re...
> Source/core/rendering/RenderGrid.cpp:954: for (GridResolvedPosition
> trackPosition = span.resolvedInitialPosition; trackPosition <=
> span.resolvedFinalPosition; ++trackPosition)
> FWIW this seems to be repeated a lot, maybe it would be worth adding
> GridResolvedPositionIterator at some point (not as part of this patch though).

Ok, I can take a look in a later patch.

>
https://codereview.chromium.org/166623002/diff/80001/Source/core/rendering/Re...
> Source/core/rendering/RenderGrid.cpp:1127: static inline bool
> isInSameRowBeforeDirtyArea(const GridCoordinate& coordinate,
> GridResolvedPosition row, const GridSpan& dirtiedColumns)
> const GridResolvedPosition& to avoid the copy.

Fixed.

>
https://codereview.chromium.org/166623002/diff/80001/Source/core/rendering/Re...
> Source/core/rendering/RenderGrid.cpp:1132: static inline bool
> isInSameRowAfterDirtyArea(const GridCoordinate& coordinate,
GridResolvedPosition
> row, const GridSpan& dirtiedColumns)
> Same comment.

Fixed too.

>
https://codereview.chromium.org/166623002/diff/80001/Source/core/rendering/st...
> Source/core/rendering/style/GridResolvedPosition.cpp:19: PassOwnPtr<GridSpan>
> GridResolvedPosition::resolveGridPositionsFromStyle(const RenderStyle& style,
> const RenderBox* gridItem, GridTrackSizingDirection direction)
> Nit: s/style/gridContainerStyle/

Nice suggestion. I've changed this in the other methods too, as it was repeated.

> Also shouldn't |gridItem| be a reference?

Fixed too in resolveGridPositionsFromAutoPlacementPosition().

>
https://codereview.chromium.org/166623002/diff/80001/Source/core/rendering/st...
> Source/core/rendering/style/GridResolvedPosition.cpp:178: }
> It really seems like a lot of this code should be moved to a Factory or a
> dedicated resolver. I think it's fine as a first step but @esprehn would feel
> stronger about that.

I think we can do it later if needed.

>
https://codereview.chromium.org/166623002/diff/80001/Source/core/rendering/st...
> Source/core/rendering/style/GridResolvedPosition.h:27: 
> Maybe worth adding a class comment that this is an index into the grid array.
> It's also a glorified size_t just for the purpose of knowing what we
manipulate
> in the grid code.

Added.

>
https://codereview.chromium.org/166623002/diff/80001/Source/core/rendering/st...
> Source/core/rendering/style/GridResolvedPosition.h:58: size_t integerPosition;
> No need to define this variable here. Better just below when you set it.

Yes. Fixed.

[Julien - ping for review, 2014-04-03 22:26:18]

lgtm.

Elliott should really be the one to give the final review as it's what he
offered but you now have the option of moving forward.

https://codereview.chromium.org/166623002/diff/100001/Source/core/rendering/s...
File Source/core/rendering/style/GridResolvedPosition.cpp (right):

https://codereview.chromium.org/166623002/diff/100001/Source/core/rendering/s...
Source/core/rendering/style/GridResolvedPosition.cpp:8: #include
"core/rendering/RenderGrid.h"
This doesn't seem like the right #include to me as we shouldn't rely on
RenderGrid in this file (the whole point is giving some encapsulation).

https://codereview.chromium.org/166623002/diff/100001/Source/core/rendering/s...
File Source/core/rendering/style/GridResolvedPosition.h (right):

https://codereview.chromium.org/166623002/diff/100001/Source/core/rendering/s...
Source/core/rendering/style/GridResolvedPosition.h:76: }
Shouldn't the default copy assignment do what we want here?

[Manuel Rego, 2014-04-04 21:44:54]

On 2014/04/03 22:26:18, Julien Chaffraix - PST wrote:
> lgtm.

Uploaded new version with the suggested changes in the last review.

> Elliott should really be the one to give the final review as it's what he
> offered but you now have the option of moving forward.

So, @esprehn would you mind to take a look? Thanks.

[esprehn, 2014-04-08 22:14:58]

This seems okay, I don't remember all the details of the original patch though.

lgtm

https://codereview.chromium.org/166623002/diff/120001/Source/core/css/CSSGrid...
File Source/core/css/CSSGridTemplateAreasValue.cpp (right):

https://codereview.chromium.org/166623002/diff/120001/Source/core/css/CSSGrid...
Source/core/css/CSSGridTemplateAreasValue.cpp:55: if (row >=
coordinate.rows.resolvedInitialPosition && row <=
coordinate.rows.resolvedFinalPosition)
This compares the new type to a size_t?

https://codereview.chromium.org/166623002/diff/120001/Source/core/rendering/R...
File Source/core/rendering/RenderGrid.cpp (right):

https://codereview.chromium.org/166623002/diff/120001/Source/core/rendering/R...
Source/core/rendering/RenderGrid.cpp:765: OwnPtr<GridSpan> rowPositions =
GridResolvedPosition::resolveGridPositionsFromStyle(*style(), *child, ForRows);
It's kind of weird that the method that creates GridSpan objects is on
GridResolvedPosition.

https://codereview.chromium.org/166623002/diff/120001/Source/core/rendering/s...
File Source/core/rendering/style/GridResolvedPosition.h (right):

https://codereview.chromium.org/166623002/diff/120001/Source/core/rendering/s...
Source/core/rendering/style/GridResolvedPosition.h:97: operator size_t() const
Doesn't having this operator just defeat the bug fixing?

[Manuel Rego, 2014-04-08 22:42:32]

On 2014/04/08 22:14:58, esprehn wrote:
> This seems okay, I don't remember all the details of the original patch
though.

Thanks for the review.

>
https://codereview.chromium.org/166623002/diff/120001/Source/core/css/CSSGrid...
> Source/core/css/CSSGridTemplateAreasValue.cpp:55: if (row >=
> coordinate.rows.resolvedInitialPosition && row <=
> coordinate.rows.resolvedFinalPosition)
> This compares the new type to a size_t?

Yes, otherwise we would need to add a getter position() or integerPosition() in
GridResolvedPosition. The code would be more verbose but the separation between
resolved positions and size_t would be clearer, it was something we already
talked
about in the first comments.

>
https://codereview.chromium.org/166623002/diff/120001/Source/core/rendering/R...
> Source/core/rendering/RenderGrid.cpp:765: OwnPtr<GridSpan> rowPositions =
> GridResolvedPosition::resolveGridPositionsFromStyle(*style(), *child,
ForRows);
> It's kind of weird that the method that creates GridSpan objects is on
> GridResolvedPosition.

Right now I cannot think in a better place for these methods.

>
https://codereview.chromium.org/166623002/diff/120001/Source/core/rendering/s...
> Source/core/rendering/style/GridResolvedPosition.h:97: operator size_t() const
> Doesn't having this operator just defeat the bug fixing?

If we don't add it, the code to access the grid will be more verbose:
    GridResolvedPosition a, b;
    m_grid[a][b]
vs:
    m_grid[a.position()][b.position()]

Again, like in the comparator I can change it if you think is needed.

[svillar, 2014-04-09 08:30:45]

On 2014/04/08 22:42:32, Manuel Rego wrote:
> On 2014/04/08 22:14:58, esprehn wrote:

>
https://codereview.chromium.org/166623002/diff/120001/Source/core/rendering/s...
> > Source/core/rendering/style/GridResolvedPosition.h:97: operator size_t()
const
> > Doesn't having this operator just defeat the bug fixing?
> 
> If we don't add it, the code to access the grid will be more verbose:
>     GridResolvedPosition a, b;
>     m_grid[a][b]
> vs:
>     m_grid[a.position()][b.position()]
> 
> Again, like in the comparator I can change it if you think is needed.

I also think that having the operator adds a kind of magic layer in between that
could make debugging/understanding the code much more difficult. What about
using ".toInt()", it isn't that long and as you said would make it clearer the
distinction between resolved positions and integers

[Manuel Rego, 2014-04-09 16:32:15]

On 2014/04/09 08:30:45, svillar wrote:
> On 2014/04/08 22:42:32, Manuel Rego wrote:
> > On 2014/04/08 22:14:58, esprehn wrote:
>
https://codereview.chromium.org/166623002/diff/120001/Source/core/rendering/s...
> > > Source/core/rendering/style/GridResolvedPosition.h:97: operator size_t()
> const
> > > Doesn't having this operator just defeat the bug fixing?
> > 
> > If we don't add it, the code to access the grid will be more verbose:
> >     GridResolvedPosition a, b;
> >     m_grid[a][b]
> > vs:
> >     m_grid[a.position()][b.position()]
> > 
> > Again, like in the comparator I can change it if you think is needed.
> 
> I also think that having the operator adds a kind of magic layer in between
that
> could make debugging/understanding the code much more difficult. What about
> using ".toInt()", it isn't that long and as you said would make it clearer the
> distinction between resolved positions and integers

Fair enough, I've uploaded a new patch using ".toInt()".

Thank you all 3 for the reviews!

[Manuel Rego, 2014-04-09 16:32:20]

The CQ bit was checked by rego@igalia.com

[commit-bot: I haz the power, 2014-04-09 16:32:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rego@igalia.com/166623002/140001

[commit-bot: I haz the power, 2014-04-09 17:05:20]

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

[commit-bot: I haz the power, 2014-04-09 17:05:20]

Try jobs failed on following builders:
  tryserver.blink on mac_blink_compile_dbg

[Manuel Rego, 2014-04-09 17:23:01]

The CQ bit was checked by rego@igalia.com

[commit-bot: I haz the power, 2014-04-09 17:23:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rego@igalia.com/166623002/160001

[commit-bot: I haz the power, 2014-04-09 18:43:27]

Change committed as 171156