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

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

The CSS Box Alignment spec states that we should ignore the collapsed
tracks when computing the Distribution Alignment space to assign to the
different grid tracks.

BUG=707633
Review-Url: https://codereview.chromium.org/2825133003
Cr-Commit-Position: refs/heads/master@{#468598}
Committed: https://chromium.googlesource.com/chromium/src/+/032e6e69429f3d726bcf71d342752a7f04824533

[jfernandez, 2017-04-18 23:04:57]

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

[jfernandez, 2017-04-18 23:04:58]

The CL doesn't provide new tests because it will be covered by new external Web
Platform Tests, which are pending to be merged in the following PR:

https://github.com/w3c/web-platform-tests/pull/5600

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

LGTM, but let's wait for the tests before we land this.

I've also reviewed the tests on WPT repo. Thanks for doing it!

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

https://codereview.chromium.org/2825133003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1381: }
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;

https://codereview.chromium.org/2825133003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1412: size_t
remaining_empty_tracks = number_of_collapsed_tracks;
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.

[Manuel Rego, 2017-04-19 10:37:16]

A nit about the description, I'm not sure if "Distribution Alignment gaps"
is the right concept. Note than for stretch the collapsed tracks are also
ignored.
Could you please reword that sentence?

[jfernandez, 2017-04-19 11:05:36]

Description was changed from

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

The CSS Box Alignment spec states that we should ignore the collapsed
tracks when computing the Distribution Alignment gaps to assign to the
different grid tracks.

BUG=707633
==========

to

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

The CSS Box Alignment spec states that we should ignore the collapsed
tracks when computing the Distribution Alignment space to assign to the
different grid tracks.

BUG=707633
==========

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

Thanks for the review.

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

https://codereview.chromium.org/2825133003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1381: }
On 2017/04/19 10:33:51, Manuel Rego wrote:
> 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;

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.

https://codereview.chromium.org/2825133003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1412: size_t
remaining_empty_tracks = number_of_collapsed_tracks;
On 2017/04/19 10:33:51, Manuel Rego wrote:
> 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.

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.

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

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

https://codereview.chromium.org/2825133003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1381: }
On 2017/04/19 11:05:51, jfernandez wrote:
> On 2017/04/19 10:33:51, Manuel Rego wrote:
> > 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;
> 
> 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.

Yeah, I didn't realize. Good point.

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

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

https://codereview.chromium.org/2825133003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1369: auto& grid =
track_sizing_algorithm_.GetGrid();
You don't need to do this. You already have grid_ attribute in LayoutGrid, no
need to get the reference from the algorithm.

https://codereview.chromium.org/2825133003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1381: }
On 2017/04/19 11:05:51, jfernandez wrote:
> On 2017/04/19 10:33:51, Manuel Rego wrote:
> > 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;
> 
> 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).

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

https://codereview.chromium.org/2825133003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1401:
!grid.IsEmptyAutoRepeatTrack(direction, i + 1))
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]

Thanks for the review. I'm still waiting for the WPT, so there is still time for
additional feedback.

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

https://codereview.chromium.org/2825133003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1381: }
On 2017/04/20 08:49:36, svillar wrote:
> On 2017/04/19 11:05:51, jfernandez wrote:
> > On 2017/04/19 10:33:51, Manuel Rego wrote:
> > > 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;
> > 
> > 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).
> 
> Adding one arithmetic operation per layout is not a problem at all. I also
> prefer rego's version as it's more compact.

Acknowledged.

https://codereview.chromium.org/2825133003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1401:
!grid.IsEmptyAutoRepeatTrack(direction, i + 1))
On 2017/04/20 08:49:37, svillar wrote:
> 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.

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]

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

https://codereview.chromium.org/2825133003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1401:
!grid.IsEmptyAutoRepeatTrack(direction, i + 1))
On 2017/04/20 17:29:47, jfernandez wrote:
> On 2017/04/20 08:49:37, svillar wrote:
> > 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.
> 
> 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 ? 

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.

[Manuel Rego, 2017-04-24 07:34:48]

BTW, the tests have been already imported:
https://chromium.googlesource.com/chromium/src/+/fa550e2af24a576a19bb3f3c7498...

[jfernandez, 2017-04-24 20:13:18]

PTAL

[svillar, 2017-04-25 08:03:29]

I haven't reviewed the tests but we clearly need more to check the issue I
mentioned.

https://codereview.chromium.org/2825133003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/2825133003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1408: auto& grid =
track_sizing_algorithm_.GetGrid();
Same comment, I know this was already here, but you don't need to ask the
algorithm for the grid. This method is only called from LayoutGridItems which is
called during layout so you can directly use the attribute grid_ here.

https://codereview.chromium.org/2825133003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1454: gap_accumulator -
last_empty_track * offset.distribution_offset;
I think this is wrong. Instead of the last empty track you need a counter of the
empty tracks. For example a grid with 10 tracks and track 9 is empty, when
computing the track 10 you'd be substracting 9 * offset instead just 1 offset.

[jfernandez, 2017-04-25 10:55:08]

Please @sergio take another look. I changed the imported tests so that they
detect the issue you have identified.

I assume that the WTP exporter will create the PR automatically so that the
tests are updated upstream.

[svillar, 2017-04-25 15:12:25]

lgtm with the suggested change

Still, the tests still look too simple for me. We are not heavily testing the
feature. I would like to see more items so we could test how content is
distributed when there are multiple collapsed tracks, multiple non collapsed
tracks and alternate collapsed and non collapsed tracks. From what I have seen
we only have 2 items which do not overlap neither in rows nor in columns.

https://codereview.chromium.org/2825133003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/2825133003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1437: size_t
last_empty_track = 0;
Sorry for nitpicking but this name is unaccurate now as it is not the last empty
track. Apart from that I don't think you need it because you already have
remaining_empty_tracks which is decreased at the same time last_empty_track is
increased.

If I am not wrong you can use bellow:
positions[i] +=
    gap_accumulator - (number_of_collapsed_tracks - remaining_empty_tracks) *
offset.distribution_offset;

[jfernandez, 2017-04-26 17:36:39]

On 2017/04/25 15:12:25, svillar wrote:
> lgtm with the suggested change
> 
> Still, the tests still look too simple for me. We are not heavily testing the
> feature. I would like to see more items so we could test how content is
> distributed when there are multiple collapsed tracks, multiple non collapsed
> tracks and alternate collapsed and non collapsed tracks. From what I have seen
> we only have 2 items which do not overlap neither in rows nor in columns.
> 

As a matter of fact, some of the new tests detected a bug in current
patch. I'm not considering that space-around assign half of the
computed offset to the first and last tracks, so we can't just subtract
the same value.

>
https://codereview.chromium.org/2825133003/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right):
> 
>
https://codereview.chromium.org/2825133003/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1437: size_t
> last_empty_track = 0;
> Sorry for nitpicking but this name is unaccurate now as it is not the last
empty
> track. Apart from that I don't think you need it because you already have
> remaining_empty_tracks which is decreased at the same time last_empty_track is
> increased.
> 
> If I am not wrong you can use bellow:
> positions[i] +=
>     gap_accumulator - (number_of_collapsed_tracks - remaining_empty_tracks) *
> offset.distribution_offset;

[jfernandez, 2017-04-28 07:28:21]

Please, take a another look.

[jfernandez, 2017-04-28 07:29:48]

Patchset #5 (id:80001) has been deleted

[svillar, 2017-04-28 15:27:21]

You don't need it, but again lgtm. Good job!

[jfernandez, 2017-04-28 15:38:20]

The CQ bit was checked by jfernandez@igalia.com

[jfernandez, 2017-04-28 15:38:21]

The patchset sent to the CQ was uploaded after l-g-t-m from rego@igalia.com
Link to the patchset: https://codereview.chromium.org/2825133003/#ps100001
(title: "Optimizations in the code and new test cases.")

[commit-bot: I haz the power, 2017-04-28 15:38:41]

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, 2017-04-28 15:44:00]

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

[commit-bot: I haz the power, 2017-04-28 15:44:01]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[jfernandez, 2017-05-02 07:41:57]

The CQ bit was checked by jfernandez@igalia.com

[jfernandez, 2017-05-02 07:41:58]

The patchset sent to the CQ was uploaded after l-g-t-m from svillar@igalia.com,
rego@igalia.com
Link to the patchset: https://codereview.chromium.org/2825133003/#ps120001
(title: "Patch rebased.")

[commit-bot: I haz the power, 2017-05-02 07:42: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, 2017-05-02 09:27:08]

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

[commit-bot: I haz the power, 2017-05-02 09:27:10]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jfernandez, 2017-05-02 09:56:40]

The CQ bit was checked by jfernandez@igalia.com

[jfernandez, 2017-05-02 09:56:40]

The patchset sent to the CQ was uploaded after l-g-t-m from svillar@igalia.com,
rego@igalia.com
Link to the patchset: https://codereview.chromium.org/2825133003/#ps140001
(title: "Fixed error in the test expectations")

[commit-bot: I haz the power, 2017-05-02 09:56:51]

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, 2017-05-02 11:31:17]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1493719000008420,
"parent_rev": "e88bef52599e0baf99c16a0ce24cd7f85b11bccc", "commit_rev":
"032e6e69429f3d726bcf71d342752a7f04824533"}

[commit-bot: I haz the power, 2017-05-02 11:31:32]

Description was changed from

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

The CSS Box Alignment spec states that we should ignore the collapsed
tracks when computing the Distribution Alignment space to assign to the
different grid tracks.

BUG=707633
==========

to

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

The CSS Box Alignment spec states that we should ignore the collapsed
tracks when computing the Distribution Alignment space to assign to the
different grid tracks.

BUG=707633

Review-Url: https://codereview.chromium.org/2825133003
Cr-Commit-Position: refs/heads/master@{#468598}
Committed:
https://chromium.googlesource.com/chromium/src/+/032e6e69429f3d726bcf71d34275...
==========

[commit-bot: I haz the power, 2017-05-02 11:31:33]

Committed patchset #7 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/032e6e69429f3d726bcf71d34275...