[css-grid] Refactor GridSpan to avoid pointers

[css-grid] Refactor GridSpan to avoid pointers

Add new boolean to know if a GridSpan is definite or indefinite.
That way we don't need to use pointers and we can always have two
GridSpans in GridCoordinate, if the position is "auto" the GridSpan will
be marked as indefinite. This will allow in a follow-up patch to avoid
repeated calls to methods that resolve positions.

Most operations in GridSpan are restricted to definite GridSpans (access
to positions, iterator, etc.). For indefinite GridSpans we only need to
know that they're indefinite we shouldn't use the rest of the data.

No new tests, no change of behavior.

BUG=444011
Committed: https://crrev.com/1ac0ca9794efed848b7546eeb54fdf7d7850c680
Cr-Commit-Position: refs/heads/master@{#361119}

[Manuel Rego, 2015-11-19 21:43:11]

rego@igalia.com changed reviewers:
+ cbiesinger@chromium.org, esprehn@chromium.org, svillar@igalia.com,
timloh@chromium.org

[Manuel Rego, 2015-11-19 21:43:12]

New refactoring patch following the plan described at
http://code.google.com/p/chromium/issues/detail?id=444011#c7

[cbiesinger, 2015-11-19 22:22:08]

lgtm with nits below

https://codereview.chromium.org/1459373002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/style/GridCoordinate.h (right):

https://codereview.chromium.org/1459373002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/style/GridCoordinate.h:119: ASSERT(isDefinite());
Seems slightly weird that you can't compare a definite span against an
indefinite one (returning false), why?

https://codereview.chromium.org/1459373002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/style/GridCoordinate.h:164: ,
m_resolvedFinalPosition(std::min(resolvedFinalPosition.toInt(), kGridMaxTracks))
Why didn't you also initialize m_definite in the constructor list here?

[Manuel Rego, 2015-11-20 08:53:45]

Thanks for the review, now we need a OWNER for the other folders.

https://codereview.chromium.org/1459373002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/style/GridCoordinate.h (right):

https://codereview.chromium.org/1459373002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/style/GridCoordinate.h:119: ASSERT(isDefinite());
On 2015/11/19 22:22:08, cbiesinger wrote:
> Seems slightly weird that you can't compare a definite span against an
> indefinite one (returning false), why?

We don't need it right now, but I agree that it might be nice to allow it
(it might be needed in the future).
So I've modified the method accordingly.

https://codereview.chromium.org/1459373002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/style/GridCoordinate.h:164: ,
m_resolvedFinalPosition(std::min(resolvedFinalPosition.toInt(), kGridMaxTracks))
On 2015/11/19 22:22:08, cbiesinger wrote:
> Why didn't you also initialize m_definite in the constructor list here?

Ups, I forgot to do it there. Fixed now. :-)

[rune, 2015-11-23 10:30:34]

rune@opera.com changed reviewers:
+ rune@opera.com

[rune, 2015-11-23 10:30:35]

lgtm with nits.

https://codereview.chromium.org/1459373002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/style/GridCoordinate.h (right):

https://codereview.chromium.org/1459373002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/GridCoordinate.h:61: span.m_definite =
false;
I would make "definite" a constructor parameter instead.

We'd also normally make this an enum instead of bool since GridSpan(0, 1,
Indefinite) would be more readable than GridSpan(0, 1, false).

[Manuel Rego, 2015-11-23 15:08:09]

The CQ bit was checked by rego@igalia.com

[Manuel Rego, 2015-11-23 15:08:09]

The patchset sent to the CQ was uploaded after l-g-t-m from
cbiesinger@chromium.org, rune@opera.com
Link to the patchset: https://codereview.chromium.org/1459373002/#ps40001
(title: "Patch for landing")

[commit-bot: I haz the power, 2015-11-23 15:08:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1459373002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1459373002/40001

[Manuel Rego, 2015-11-23 15:27:14]

Thanks for the review!

https://codereview.chromium.org/1459373002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/style/GridCoordinate.h (right):

https://codereview.chromium.org/1459373002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/GridCoordinate.h:61: span.m_definite =
false;
On 2015/11/23 10:30:35, rune wrote:
> I would make "definite" a constructor parameter instead.
> 
> We'd also normally make this an enum instead of bool since GridSpan(0, 1,
> Indefinite) would be more readable than GridSpan(0, 1, false).

Done.

[commit-bot: I haz the power, 2015-11-23 16:25:42]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-11-23 16:26:47]

Patchset 3 (id:??) landed as
https://crrev.com/1ac0ca9794efed848b7546eeb54fdf7d7850c680
Cr-Commit-Position: refs/heads/master@{#361119}