[CSS Grid Layout] Clear stretched height when conditions don't allow it.

[CSS Grid Layout] Clear stretched height when conditions don't allow it.

Box Alignment spec states that stretch is only possible when height is
'auto' and no 'auto' margins are used.

It might be the case that style changes so that stretching is not allowed,
hence we need to detect it and clear the override height the stretching
algorithm previously set. The new layout triggered by the style change
will then set grid item's height according to the new style rules.

BUG=490043
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=196543

[jfernandez, 2015-05-20 14:33:32]

jfernandez@igalia.com changed reviewers:
+ dsinclair@chromium.org, jchaffraix@chromium.org, leviw@chromium.org

[Julien - ping for review, 2015-06-03 21:33:13]

lgtm

https://codereview.chromium.org/1145183003/diff/1/LayoutTests/fast/css-grid-l...
File
LayoutTests/fast/css-grid-layout/grid-items-should-not-be-stretched-when-height-or-margin-change.html
(right):

https://codereview.chromium.org/1145183003/diff/1/LayoutTests/fast/css-grid-l...
LayoutTests/fast/css-grid-layout/grid-items-should-not-be-stretched-when-height-or-margin-change.html:13:
<p>The grids bellow had initially 'stretched' items, but we have changed
'height' and 'margin' to values which don't allow stretch. This test verifies
that the layout algorithm properly detects such changes and clear the override
height accordingly.</p>
s/bellow/below/.

https://codereview.chromium.org/1145183003/diff/1/Source/core/layout/LayoutGr...
File Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/1145183003/diff/1/Source/core/layout/LayoutGr...
Source/core/layout/LayoutGrid.cpp:1573: if
(child.hasOverrideLogicalContentHeight())
I would just call clearOverrideLogicalContentHeight() directly. This avoids a
branch (and back in the days when this involved a hash lookup also a lookup).

https://codereview.chromium.org/1145183003/diff/1/Source/core/layout/LayoutGr...
Source/core/layout/LayoutGrid.cpp:1574:
child.clearOverrideLogicalContentHeight();
Thinking out loud here, we should probably think about some design pattern to
avoid that. I have been beaten by similar bugs and I am sure there are more
lurking.

[jfernandez, 2015-06-05 00:05:30]

Applied suggested changed and trying CQ for landing.

https://codereview.chromium.org/1145183003/diff/1/LayoutTests/fast/css-grid-l...
File
LayoutTests/fast/css-grid-layout/grid-items-should-not-be-stretched-when-height-or-margin-change.html
(right):

https://codereview.chromium.org/1145183003/diff/1/LayoutTests/fast/css-grid-l...
LayoutTests/fast/css-grid-layout/grid-items-should-not-be-stretched-when-height-or-margin-change.html:13:
<p>The grids bellow had initially 'stretched' items, but we have changed
'height' and 'margin' to values which don't allow stretch. This test verifies
that the layout algorithm properly detects such changes and clear the override
height accordingly.</p>
On 2015/06/03 21:33:12, Julien Chaffraix - PST wrote:
> s/bellow/below/.

Done.

https://codereview.chromium.org/1145183003/diff/1/Source/core/layout/LayoutGr...
File Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/1145183003/diff/1/Source/core/layout/LayoutGr...
Source/core/layout/LayoutGrid.cpp:1573: if
(child.hasOverrideLogicalContentHeight())
On 2015/06/03 21:33:13, Julien Chaffraix - PST wrote:
> I would just call clearOverrideLogicalContentHeight() directly. This avoids a
> branch (and back in the days when this involved a hash lookup also a lookup).

Done.

https://codereview.chromium.org/1145183003/diff/1/Source/core/layout/LayoutGr...
Source/core/layout/LayoutGrid.cpp:1574:
child.clearOverrideLogicalContentHeight();
On 2015/06/03 21:33:13, Julien Chaffraix - PST wrote:
> Thinking out loud here, we should probably think about some design pattern to
> avoid that. I have been beaten by similar bugs and I am sure there are more
> lurking.

Yes, I've got the same feeling. I'll add t my TODO list and try to find a more
conscious approach to handle overrideLogicalContentHeight logic.

[jfernandez, 2015-06-05 00:05:43]

The CQ bit was checked by jfernandez@igalia.com

[jfernandez, 2015-06-05 00:05:44]

The patchset sent to the CQ was uploaded after l-g-t-m from
jchaffraix@chromium.org
Link to the patchset: https://codereview.chromium.org/1145183003/#ps20001
(title: "Applied suggested changes.")

[commit-bot: I haz the power, 2015-06-05 00:06:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1145183003/20001

[commit-bot: I haz the power, 2015-06-05 01:34:17]

Committed patchset #2 (id:20001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=196543