[css-grid] Isolate internal grid size from the actual grid size

[css-grid] Isolate internal grid size from the actual grid size

LayoutGrid has an internal representation of a grid used to place grid
items, compute grid positions, run the track sizing algorithm etc. That data
structure normally has exactly the same size as the actual grid specified
using the grid-template-xxx properties (or any other shorthand). But in some
cases, like for example when the grid is empty, the internal data structure
does not really match the actual grid. In the particular case of empty grids
no memory allocations are done to create a grid representation as it is
not needed.

This is the first required step of the process of isolating the data used by
the grid track sizing algorithm from the actual internal state of the
LayoutGrid object.

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

[svillar, 2016-11-21 10:42:31]

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

[eae, 2016-11-21 16:57:12]

OK, LGTM

[Manuel Rego, 2016-11-22 09:57:24]

LGTM, just wondering why we don't replace all the
calls to gridColumn|RowCount() by numTracks().

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

https://codereview.chromium.org/2521553002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:433: direction == ForRows ?
gridRowCount() : gridColumnCount(),
Why you don't use numTracks() here?

https://codereview.chromium.org/2521553002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:2119: crossDirection ==
ForColumns ? gridColumnCount() : gridRowCount();
Ditto.

https://codereview.chromium.org/2521553002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:2204: : gridRowCount();
Ditto.

https://codereview.chromium.org/2521553002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:2253: : gridRowCount();
Ditto.

[Manuel Rego, 2016-11-22 09:58:27]

Typo on the description "it as it is".

[svillar, 2016-11-22 10:05:47]

Description was changed from

==========
[css-grid] Isolate internal grid size from the actual grid size

LayoutGrid has an internal representation of a grid used to place grid
items, compute grid positions, run the track sizing algorithm etc. That data
structure normally has exactly the same size as the actual grid specified
using the grid-template-xxx properties (or any other shorthand). But in some
cases, like for example when the grid is empty, the internal data structure
does not really match the actual grid. In the particular case of empty grids
no memory allocations are done to create a grid representation it as it is
not needed.

This is the first required step of the process of isolating the data used by
the grid track sizing algorithm from the actual internal state of the
LayoutGrid object.

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

to

==========
[css-grid] Isolate internal grid size from the actual grid size

LayoutGrid has an internal representation of a grid used to place grid
items, compute grid positions, run the track sizing algorithm etc. That data
structure normally has exactly the same size as the actual grid specified
using the grid-template-xxx properties (or any other shorthand). But in some
cases, like for example when the grid is empty, the internal data structure
does not really match the actual grid. In the particular case of empty grids
no memory allocations are done to create a grid representation as it is
not needed.

This is the first required step of the process of isolating the data used by
the grid track sizing algorithm from the actual internal state of the
LayoutGrid object.

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

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

On 2016/11/22 09:57:24, Manuel Rego wrote:
> LGTM, just wondering why we don't replace all the
> calls to gridColumn|RowCount() by numTracks().

If I change all the calls then then there is no change :) apart from the name.
The point of this change is precisely to separate the calls that need the size
of the internal representation from the calls that need the actual size of the
grid.

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

The CQ bit was checked by svillar@igalia.com

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

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

[Manuel Rego, 2016-11-22 10:15:40]

On 2016/11/22 10:10:57, svillar wrote:
> On 2016/11/22 09:57:24, Manuel Rego wrote:
> > LGTM, just wondering why we don't replace all the
> > calls to gridColumn|RowCount() by numTracks().
> 
> If I change all the calls then then there is no change :) apart from the name.
> The point of this change is precisely to separate the calls that need the size
> of the internal representation from the calls that need the actual size of the
> grid.

Then it would be nice to add a TODO or comment to avoid confusions between
numTracks() and gridColumn|RowCount().

[jfernandez, 2016-11-22 10:44:50]

On 2016/11/22 10:15:40, Manuel Rego wrote:
> On 2016/11/22 10:10:57, svillar wrote:
> > On 2016/11/22 09:57:24, Manuel Rego wrote:
> > > LGTM, just wondering why we don't replace all the
> > > calls to gridColumn|RowCount() by numTracks().
> > 
> > If I change all the calls then then there is no change :) apart from the
name.
> > The point of this change is precisely to separate the calls that need the
size
> > of the internal representation from the calls that need the actual size of
the
> > grid.
> 
> Then it would be nice to add a TODO or comment to avoid confusions between
> numTracks() and gridColumn|RowCount().

Perhaps we should try with a more descriptive name.

[commit-bot: I haz the power, 2016-11-22 11:45:38]

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

[commit-bot: I haz the power, 2016-11-22 11:45:39]

Failed to apply patch for third_party/WebKit/Source/core/layout/LayoutGrid.cpp:
While running git apply --index -p1;
  error: patch failed: third_party/WebKit/Source/core/layout/LayoutGrid.cpp:513
  error: third_party/WebKit/Source/core/layout/LayoutGrid.cpp: patch does not
apply

Patch:       third_party/WebKit/Source/core/layout/LayoutGrid.cpp
Index: third_party/WebKit/Source/core/layout/LayoutGrid.cpp
diff --git a/third_party/WebKit/Source/core/layout/LayoutGrid.cpp
b/third_party/WebKit/Source/core/layout/LayoutGrid.cpp
index
cb52b7acaa913ca94f88f90c69cc728421147d4e..9df052fa4d7ca981d5638f6e6a117d17759a9a0f
100644
--- a/third_party/WebKit/Source/core/layout/LayoutGrid.cpp
+++ b/third_party/WebKit/Source/core/layout/LayoutGrid.cpp
@@ -400,15 +400,7 @@ bool LayoutGrid::namedGridLinesDefinitionDidChange(
 
 size_t LayoutGrid::gridColumnCount() const {
   DCHECK(!m_gridIsDirty);
-  // Due to limitations in our internal representation, we cannot know the
-  // number of columns from m_grid *if* there is no row (because m_grid would
be
-  // empty). That's why in that case we need to get it from the style. Note
that
-  // we know for sure that there are't any implicit tracks, because not having
-  // rows implies that there are no "normal" children (out-of-flow children are
-  // not stored in m_grid).
-  return m_grid.size() ? m_grid[0].size()
-                       : GridPositionsResolver::explicitGridColumnCount(
-                             styleRef(), m_autoRepeatColumns);
+  return m_grid.size() ? m_grid[0].size() : 0;
 }
 
 size_t LayoutGrid::gridRowCount() const {
@@ -513,7 +505,7 @@ void LayoutGrid::layoutBlock(bool relayoutChildren) {
     updateAutoRepeatTracksAndSetDirtyIfNeeded(TrackSizing);
     placeItemsOnGrid();
 
-    GridSizingData sizingData(gridColumnCount(), gridRowCount());
+    GridSizingData sizingData(numTracks(ForColumns), numTracks(ForRows));
 
     // 1- First, the track sizing algorithm is used to resolve the sizes of the
     // grid columns.
@@ -691,7 +683,7 @@ void LayoutGrid::computeIntrinsicLogicalWidths(
       IntrinsicSizeComputation);
   const_cast<LayoutGrid*>(this)->placeItemsOnGrid();
 
-  GridSizingData sizingData(gridColumnCount(), gridRowCount());
+  GridSizingData sizingData(numTracks(ForColumns), numTracks(ForRows));
   computeTrackSizesForIndefiniteSize(ForColumns, sizingData, minLogicalWidth,
                                      maxLogicalWidth);
 
@@ -2028,10 +2020,14 @@ void LayoutGrid::placeItemsOnGrid() {
     insertItemIntoGrid(*child, area);
   }
 
-  DCHECK_GE(gridRowCount(), GridPositionsResolver::explicitGridRowCount(
-                                *style(), m_autoRepeatRows));
-  DCHECK_GE(gridColumnCount(), GridPositionsResolver::explicitGridColumnCount(
-                                   *style(), m_autoRepeatColumns));
+#if ENABLE(ASSERT)
+  if (!m_gridItemArea.isEmpty()) {
+    DCHECK_GE(gridRowCount(), GridPositionsResolver::explicitGridRowCount(
+                                  *style(), m_autoRepeatRows));
+    DCHECK_GE(gridColumnCount(),
GridPositionsResolver::explicitGridColumnCount(
+                                     *style(), m_autoRepeatColumns));
+  }
+#endif
 
   placeSpecifiedMajorAxisItemsOnGrid(specifiedMajorAxisAutoGridItems);
   placeAutoMajorAxisItemsOnGrid(autoMajorAxisAutoGridItems);
@@ -2545,7 +2541,7 @@ void LayoutGrid::offsetAndBreadthForPositionedChild(
                                             : child.style()->gridRowStart();
   GridPosition endPosition = isForColumns ? child.style()->gridColumnEnd()
                                           : child.style()->gridRowEnd();
-  int lastLine = isForColumns ? gridColumnCount() : gridRowCount();
+  int lastLine = numTracks(direction);
 
   bool startIsAuto =
       startPosition.isAuto() ||
@@ -3501,4 +3497,19 @@ bool LayoutGrid::cachedHasDefiniteLogicalHeight() const {
   return m_hasDefiniteLogicalHeight.value();
 }
 
+size_t LayoutGrid::numTracks(GridTrackSizingDirection direction) const {
+  // Due to limitations in our internal representation, we cannot know the
+  // number of columns from m_grid *if* there is no row (because m_grid would
be
+  // empty). That's why in that case we need to get it from the style. Note
that
+  // we know for sure that there are't any implicit tracks, because not having
+  // rows implies that there are no "normal" children (out-of-flow children are
+  // not stored in m_grid).
+  if (direction == ForRows)
+    return m_grid.size();
+
+  return m_grid.size() ? m_grid[0].size()
+                       : GridPositionsResolver::explicitGridColumnCount(
+                             styleRef(), m_autoRepeatColumns);
+}
+
 }  // namespace blink

[svillar, 2016-11-22 14:06:49]

The CQ bit was checked by svillar@igalia.com

[svillar, 2016-11-22 14:06:49]

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/2521553002/#ps20001
(title: "Patch for landing. Rebased")

[commit-bot: I haz the power, 2016-11-22 14:07:10]

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 15:47:09]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1479823609075800,
"parent_rev": "6715340e977dc57e3c9e9690acaf50f315d6f5d6", "commit_rev":
"1e2131271f15775a83d2a3db897a323277d74742"}

[commit-bot: I haz the power, 2016-11-22 15:47:31]

Description was changed from

==========
[css-grid] Isolate internal grid size from the actual grid size

LayoutGrid has an internal representation of a grid used to place grid
items, compute grid positions, run the track sizing algorithm etc. That data
structure normally has exactly the same size as the actual grid specified
using the grid-template-xxx properties (or any other shorthand). But in some
cases, like for example when the grid is empty, the internal data structure
does not really match the actual grid. In the particular case of empty grids
no memory allocations are done to create a grid representation as it is
not needed.

This is the first required step of the process of isolating the data used by
the grid track sizing algorithm from the actual internal state of the
LayoutGrid object.

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

to

==========
[css-grid] Isolate internal grid size from the actual grid size

LayoutGrid has an internal representation of a grid used to place grid
items, compute grid positions, run the track sizing algorithm etc. That data
structure normally has exactly the same size as the actual grid specified
using the grid-template-xxx properties (or any other shorthand). But in some
cases, like for example when the grid is empty, the internal data structure
does not really match the actual grid. In the particular case of empty grids
no memory allocations are done to create a grid representation as it is
not needed.

This is the first required step of the process of isolating the data used by
the grid track sizing algorithm from the actual internal state of the
LayoutGrid object.

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

[commit-bot: I haz the power, 2016-11-22 15:47:33]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-11-22 15:49:44]

Description was changed from

==========
[css-grid] Isolate internal grid size from the actual grid size

LayoutGrid has an internal representation of a grid used to place grid
items, compute grid positions, run the track sizing algorithm etc. That data
structure normally has exactly the same size as the actual grid specified
using the grid-template-xxx properties (or any other shorthand). But in some
cases, like for example when the grid is empty, the internal data structure
does not really match the actual grid. In the particular case of empty grids
no memory allocations are done to create a grid representation as it is
not needed.

This is the first required step of the process of isolating the data used by
the grid track sizing algorithm from the actual internal state of the
LayoutGrid object.

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

to

==========
[css-grid] Isolate internal grid size from the actual grid size

LayoutGrid has an internal representation of a grid used to place grid
items, compute grid positions, run the track sizing algorithm etc. That data
structure normally has exactly the same size as the actual grid specified
using the grid-template-xxx properties (or any other shorthand). But in some
cases, like for example when the grid is empty, the internal data structure
does not really match the actual grid. In the particular case of empty grids
no memory allocations are done to create a grid representation as it is
not needed.

This is the first required step of the process of isolating the data used by
the grid track sizing algorithm from the actual internal state of the
LayoutGrid object.

BUG=627812

Committed: https://crrev.com/70a22ecc29b1197a37f860b88fd0dd56b2e9cd2d
Cr-Commit-Position: refs/heads/master@{#433870}
==========

[commit-bot: I haz the power, 2016-11-22 15:49:45]

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