Self-collapsing block gets layout set during margin collapsing

Self-collapsing block gets layout set during margin collapsing

After the child's layout in layoutBlockChild() we do margin collapsing and float
clearing. If either of those change the child's logical top we may mark the child
for another layout and lay it out again, so we can't ASSERT that the child doesn't
need layout between those two things happening.

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

[rhogan, 2015-07-09 17:53:14]

robhogan@gmail.com changed reviewers:
+ mstensho@opera.com

[mstensho (USE GERRIT), 2015-07-09 19:21:46]

mstensho@opera.com changed reviewers:
+ leviw@chromium.org

[mstensho (USE GERRIT), 2015-07-09 19:21:47]

Just a couple of notes for now. Not a complete review. I'm adding leviw to the
list of reviewers, in the hope that he knows way more about this than I do. I'll
get back to a more proper review later, unless leviw beats me to it.

https://codereview.chromium.org/1222693005/diff/1/LayoutTests/fast/block/self...
File
LayoutTests/fast/block/self-collapsing-block-gets-layout-during-margin-collapsing.html
(right):

https://codereview.chromium.org/1222693005/diff/1/LayoutTests/fast/block/self...
LayoutTests/fast/block/self-collapsing-block-gets-layout-during-margin-collapsing.html:3:
* {
There seriously has to exist a possible test case that doesn't use universal
selectors. This test looks like it's written by a fuzzer. Could you try to make
it look a bit more hand-made and to-the-point in general? :)

https://codereview.chromium.org/1222693005/diff/1/Source/core/layout/LayoutBl...
File Source/core/layout/LayoutBlockFlow.h (right):

https://codereview.chromium.org/1222693005/diff/1/Source/core/layout/LayoutBl...
Source/core/layout/LayoutBlockFlow.h:469: LayoutUnit collapseMargins(LayoutBox&
child, MarginInfo&, bool childIsSelfCollapsing, bool childDiscardMarginBefore,
bool childDiscardMarginAfter);
Three bools is kind of insane.

[leviw_travelin_and_unemployed, 2015-07-09 20:33:54]

*sigh*

Our margin collapse code is teh suck. We 'predict' (re-use the old margin
values) our logical top, but if we get a new value and we have floats, we have
to force a subsequent layout with the correct top value. Basically, we do an
extra layout on first-layout every time there is margin collapse, or if someone
messes with margin values.

This is a reasonable patch, though I'll echo the sentiment about having a
less-gross test case, which should be possible.

[rhogan, 2015-07-13 18:34:43]

On 2015/07/09 at 20:33:54, leviw wrote:
> *sigh*
> 
> Our margin collapse code is teh suck. We 'predict' (re-use the old margin
values) our logical top, but if we get a new value and we have floats, we have
to force a subsequent layout with the correct top value. Basically, we do an
extra layout on first-layout every time there is margin collapse, or if someone
messes with margin values.
> 
> This is a reasonable patch, though I'll echo the sentiment about having a
less-gross test case, which should be possible.

Updated patch..

[leviw_travelin_and_unemployed, 2015-07-13 18:56:22]

The CQ bit was checked by leviw@chromium.org

[leviw_travelin_and_unemployed, 2015-07-13 18:56:24]

lgtm

[commit-bot: I haz the power, 2015-07-13 18:56:40]

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

[commit-bot: I haz the power, 2015-07-13 20:30:12]

Committed patchset #4 (id:60001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=198807