[css-grid] Ignore collapsed tracks on content-distribution alignment

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 957ecad4da47dc281d667cbd7d469519da0556cd..98adfd19fd3af04df9ba4a853f839ba2d617a393 100644
--- a/third_party/WebKit/Source/core/layout/LayoutGrid.cpp
+++ b/third_party/WebKit/Source/core/layout/LayoutGrid.cpp
@@ -1366,32 +1366,41 @@ void LayoutGrid::PopulateGridPositionsForDirection(
   // lines positions are 'direction' unaware. This simplification allows us to
   // use the same indexes to identify the columns independently on the
   // inline-axis direction.
+  auto& grid = track_sizing_algorithm_.GetGrid();

[svillar, 2017/04/20 08:49:37]

You don't need to do this. You already have grid_ attribute in LayoutGrid, no
need to get the reference from the algorithm.

   bool is_row_axis = direction == kForColumns;
   auto& tracks = track_sizing_algorithm_.Tracks(direction);
   size_t number_of_tracks = tracks.size();
   size_t number_of_lines = number_of_tracks + 1;
   size_t last_line = number_of_lines - 1;
+  size_t number_of_non_empty_tracks = number_of_tracks;
+  size_t number_of_collapsed_tracks = 0;
+  bool has_collapsed_tracks = grid.HasAutoRepeatEmptyTracks(direction);
+  if (has_collapsed_tracks) {
+    number_of_collapsed_tracks = grid.AutoRepeatEmptyTracks(direction)->size();
+    number_of_non_empty_tracks -= number_of_collapsed_tracks;
+  }

[Manuel Rego, 2017/04/19 10:33:51]

Nit: I think you can change this by something like:

  bool has_collapsed_tracks = grid.HasAutoRepeatEmptyTracks(direction);
  size_t number_of_collapsed_tracks = has_collapsed_tracks ?
grid.AutoRepeatEmptyTracks(direction)->size()) ? 0;
  size_t number_of_non_empty_tracks = number_of_tracks -
number_of_collapsed_tracks;

[jfernandez, 2017/04/19 11:05:51]

yes, I thought about that, but with this new code we are adding
an additional arithmetic operation in every case, even when 
number_of_collapesed_tracks is 0 (the most common case).

We could add a new conditional operation here, but then it'd be
equivalent to the previous code, but slightly worse IMHO.

[Manuel Rego, 2017/04/19 11:46:08]

Yeah, I didn't realize. Good point.

[svillar, 2017/04/20 08:49:36]

Adding one arithmetic operation per layout is not a problem at all. I also
prefer rego's version as it's more compact.

[jfernandez, 2017/04/20 17:29:47]

Acknowledged.

   ContentAlignmentData offset = ComputeContentPositionAndDistributionOffset(
       direction, track_sizing_algorithm_.FreeSpace(direction).value(),
-      number_of_tracks);
+      number_of_non_empty_tracks);
   auto& positions = is_row_axis ? column_positions_ : row_positions_;
   positions.Resize(number_of_lines);
   auto border_and_padding =
       is_row_axis ? BorderAndPaddingLogicalLeft() : BorderAndPaddingBefore();
   positions[0] = border_and_padding + offset.position_offset;
-  const Grid& grid = track_sizing_algorithm_.GetGrid();
   if (number_of_lines > 1) {
     // If we have collapsed tracks we just ignore gaps here and add them later
     // as we might not compute the gap between two consecutive tracks without
     // examining the surrounding ones.
-    bool has_collapsed_tracks = grid.HasAutoRepeatEmptyTracks(direction);
     LayoutUnit gap = !has_collapsed_tracks
                          ? GridGapForDirection(direction, kTrackSizing)
                          : LayoutUnit();
     size_t next_to_last_line = number_of_lines - 2;
-    for (size_t i = 0; i < next_to_last_line; ++i)
-      positions[i + 1] = positions[i] + offset.distribution_offset +
-                         tracks[i].BaseSize() + gap;
+    for (size_t i = 0; i < next_to_last_line; ++i) {
+      positions[i + 1] = positions[i] + tracks[i].BaseSize() + gap;
+      if (!has_collapsed_tracks ||
+          !grid.IsEmptyAutoRepeatTrack(direction, i + 1))

[svillar, 2017/04/20 08:49:37]

In contrast this might be potentially problematic as we'd be adding one hash
search per track. The good thing is that it's easy to read.

[jfernandez, 2017/04/20 17:29:47]

Only in the case that there are collapsed tracks. I'm not sure we can avoid
checking whether the track was collapsed or not. I think we can't rely on the
BaseSize being 0 because if that was the actual value set, we should not ignore
it, right ? 

[svillar, 2017/04/21 07:37:51]

No because as you said 0px is a valid value for rows, also an empty
auto/min-content/max-content track would be 0px too.

I was thinking about adding the offset unconditionally at first. The we would
start iterating the list of empty tracks and then selectively remove the gap
from all the tracks in between. Not sure if that would be terribly complicated,
at least we could give it a try. If it eventually becomes too complicated we can
fallback to this original implementation.

+        positions[i + 1] += offset.distribution_offset;
+    }
     positions[last_line] =
         positions[next_to_last_line] + tracks[next_to_last_line].BaseSize();
 
@@ -1400,8 +1409,7 @@ void LayoutGrid::PopulateGridPositionsForDirection(
     // they become 0.
     if (has_collapsed_tracks) {
       gap = GridGapForDirection(direction, kTrackSizing);
-      size_t remaining_empty_tracks =
-          grid.AutoRepeatEmptyTracks(direction)->size();
+      size_t remaining_empty_tracks = number_of_collapsed_tracks;

[Manuel Rego, 2017/04/19 10:33:51]

We're mixing "collapsed" and "empty" terms...
Not sure what's the best idea:
1) Use empty.
2) Use empty for now, and rename everything in a follow-up patch.
3) Use collapsed in new stuff (like you're doing now).
4) Use collapsed in new stuff, and rename the old empty in a follow-up patch.

I think I prefer 2) or 4) but maybe I'm missing something else.

[jfernandez, 2017/04/19 11:05:51]

Im using the old notation, hence if we want to rename these
concepts, better doing it in a follow-up patch and in the whole
class.

       LayoutUnit gap_accumulator;
       for (size_t i = 1; i < last_line; ++i) {
         if (grid.IsEmptyAutoRepeatTrack(direction, i - 1)) {