[css-grid] Too many gaps with trailing collapsing tracks

[css-grid] Too many gaps with trailing collapsing tracks

The total size of gaps were incorrectly computed whenever there were
trailing collapsed tracks (with collapsed gaps). The problem was that we
were trying to optimize too much the amount of hash table queries required
to know the gaps between two lines. We were considering that a gap always
exist between 2 consecutive tracks if the first one is not empty. That's
generally true (for both NOTEMPTY - NOTEMPTY and NOTEMPTY - EMPTY+
- NOTEMPTY sequences) but not for all the cases (NOTEMPTY - EMPTY+).

BUG=645746
Committed: https://crrev.com/011d6fa1bdaa3f942a389ae3b55d20f681c63bbf
Cr-Commit-Position: refs/heads/master@{#418209}

[svillar, 2016-09-12 16:40:10]

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

[eae, 2016-09-12 19:24:32]

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

https://codereview.chromium.org/2336663002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:570: gapAccumulator -= gap;
Do we want to add a DCHECK here to ensure that it never goes below zero or is
that OK?

[Manuel Rego, 2016-09-12 19:50:17]

lgtm

Nice catch.

https://codereview.chromium.org/2336663002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-auto-fit-columns.html
(right):

https://codereview.chromium.org/2336663002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/css-grid-layout/grid-auto-fit-columns.html:157:
<div class="item" style="grid-column-start: 1" data-offset-x="0"
data-offset-y="0" data-expected-width="200" data-expected-height="25"></div>
Super nit, but you could simply use "grid-column" instead of
"grid-column-start" (as we usually do in most of the tests).

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

https://codereview.chromium.org/2336663002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:570: gapAccumulator -= gap;
On 2016/09/12 19:24:32, eae wrote:
> Do we want to add a DCHECK here to ensure that it never goes below zero or is
> that OK?

Right now "gapAccumulator > 0" implies that "gapAccumulator - gap"
won't be below zero. But I agree we could add the DCHECK to be safe
in future modifications of the code.

[svillar, 2016-09-13 07:36:28]

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

https://codereview.chromium.org/2336663002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutGrid.cpp:570: gapAccumulator -= gap;
On 2016/09/12 19:24:32, eae wrote:
> Do we want to add a DCHECK here to ensure that it never goes below zero or is
> that OK?

So gapAccumulator is only increased in "gap" chunks so checking gapAccumulator
should be enough.

[svillar, 2016-09-13 08:15:31]

The CQ bit was checked by svillar@igalia.com

[svillar, 2016-09-13 08:15:32]

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

[commit-bot: I haz the power, 2016-09-13 08:15:46]

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-09-13 09:53:40]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-09-13 09:55:46]

Description was changed from

==========
[css-grid] Too many gaps with trailing collapsing tracks

The total size of gaps were incorrectly computed whenever there were
trailing collapsed tracks (with collapsed gaps). The problem was that we
were trying to optimize too much the amount of hash table queries required
to know the gaps between two lines. We were considering that a gap always
exist between 2 consecutive tracks if the first one is not empty. That's
generally true (for both NOTEMPTY - NOTEMPTY and NOTEMPTY - EMPTY+
- NOTEMPTY sequences) but not for all the cases (NOTEMPTY - EMPTY+).

BUG=645746
==========

to

==========
[css-grid] Too many gaps with trailing collapsing tracks

The total size of gaps were incorrectly computed whenever there were
trailing collapsed tracks (with collapsed gaps). The problem was that we
were trying to optimize too much the amount of hash table queries required
to know the gaps between two lines. We were considering that a gap always
exist between 2 consecutive tracks if the first one is not empty. That's
generally true (for both NOTEMPTY - NOTEMPTY and NOTEMPTY - EMPTY+
- NOTEMPTY sequences) but not for all the cases (NOTEMPTY - EMPTY+).

BUG=645746

Committed: https://crrev.com/011d6fa1bdaa3f942a389ae3b55d20f681c63bbf
Cr-Commit-Position: refs/heads/master@{#418209}
==========

[commit-bot: I haz the power, 2016-09-13 09:55:47]

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