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

third_party/WebKit/Source/core/layout/LayoutGrid.cpp

 /*
  * Copyright (C) 2011 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2. Redistributions in binary form must reproduce the above copyright
  *    notice, this list of conditions and the following disclaimer in the
@@ skipping 1348 matching lines @@
   // Since we add alignment offsets and track gutters, grid lines are not always
   // adjacent. Hence we will have to assume from now on that we just store
   // positions of the initial grid lines of each track, except the last one,
   // which is the only one considered as a final grid line of a track.
 
   // The grid container's frame elements (border, padding and <content-position>
   // offset) are sensible to the inline-axis flow direction. However, column
   // 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();
 
     // Adjust collapsed gaps. Collapsed tracks cause the surrounding gutters to
     // collapse (they coincide exactly) except on the edges of the grid where
     // 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)) {
           --remaining_empty_tracks;
         } else {
           // Add gap between consecutive non empty tracks. Add it also just once
           // for an arbitrary number of empty tracks between two non empty ones.
           bool all_remaining_tracks_are_empty =
               remaining_empty_tracks == (last_line - i);
           if (!all_remaining_tracks_are_empty ||
@@ skipping 942 matching lines @@
   if (direction == kForRows)
     return grid.NumTracks(kForRows);
 
   return grid.NumTracks(kForRows)
              ? grid.NumTracks(kForColumns)
              : GridPositionsResolver::ExplicitGridColumnCount(
                    StyleRef(), grid.AutoRepeatTracks(kForColumns));
 }
 
 }  // namespace blink