Disallow relayout roots inside multicol.

Disallow relayout roots inside multicol.

It's just too complicated to figure out if it's safe to let a descendant of a
multicol container be a relayout root, so disallow it.

BUG=515260
R=jchaffraix@chromium.org,leviw@chromium.org

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

[Julien - ping for review, 2015-08-03 16:47:59]

lgtm

https://codereview.chromium.org/1271513004/diff/1/LayoutTests/fast/multicol/s...
File LayoutTests/fast/multicol/span/add-spanner-inside-overflow-hidden.html
(right):

https://codereview.chromium.org/1271513004/diff/1/LayoutTests/fast/multicol/s...
LayoutTests/fast/multicol/span/add-spanner-inside-overflow-hidden.html:7: <div
style="-webkit-columns:2; width:20em; height:10em; line-height:2em;
overflow:hidden;">
Do we really need both this overflow: hidden and the one just below? Same
question with width?

[mstensho (USE GERRIT), 2015-08-04 12:06:07]

https://codereview.chromium.org/1271513004/diff/1/LayoutTests/fast/multicol/s...
File LayoutTests/fast/multicol/span/add-spanner-inside-overflow-hidden.html
(right):

https://codereview.chromium.org/1271513004/diff/1/LayoutTests/fast/multicol/s...
LayoutTests/fast/multicol/span/add-spanner-inside-overflow-hidden.html:7: <div
style="-webkit-columns:2; width:20em; height:10em; line-height:2em;
overflow:hidden;">
On 2015/08/03 16:47:59, Julien Chaffraix - PST wrote:
> Do we really need both this overflow: hidden and the one just below? Same
> question with width?

When inserting a spanner, we insert a spanner placeholder as a child of the
multicol container. This will mark the multicol container for layout. If we had
no relayout root here, we'd end up marking the ancestry chain all the way up to
LayoutView, and when the LayoutView is marked for layout, we remove any subtree
roots that were previously marked for layout
(clearLayoutSubtreeRootsAndMarkContainingBlocks()), and prevent subtree roots
from being added, until we have laid out.

But sure there are ways to trigger this without two overflow:hiddens. Added a
couple of more tests. One even failed in assertLaidOut()! I put everything in
fast/multicol/dynamic/ instead of in fast/multicol/span/ . This is not so much
about spanners; it's just that spanners are useful here to cause general misery
in the multicol code (I wasn't able to think of any way to cause trouble without
using spanners, but it might have been possible).

[mstensho (USE GERRIT), 2015-08-05 07:23:28]

The CQ bit was checked by mstensho@opera.com

[mstensho (USE GERRIT), 2015-08-05 07:23:28]

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/1271513004/#ps20001
(title: "Code review - more tests - put everything in fast/multicol/dynamic/")

[commit-bot: I haz the power, 2015-08-05 07:23:32]

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

[commit-bot: I haz the power, 2015-08-05 07:26:41]

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

[Julien - ping for review, 2015-08-05 16:34:26]

Forgot to hit reply (sorry!), the proposed changes were great.

https://codereview.chromium.org/1271513004/diff/1/LayoutTests/fast/multicol/s...
File LayoutTests/fast/multicol/span/add-spanner-inside-overflow-hidden.html
(right):

https://codereview.chromium.org/1271513004/diff/1/LayoutTests/fast/multicol/s...
LayoutTests/fast/multicol/span/add-spanner-inside-overflow-hidden.html:7: <div
style="-webkit-columns:2; width:20em; height:10em; line-height:2em;
overflow:hidden;">
On 2015/08/04 at 12:06:07, mstensho wrote:
> On 2015/08/03 16:47:59, Julien Chaffraix - PST wrote:
> > Do we really need both this overflow: hidden and the one just below? Same
> > question with width?
> 
> When inserting a spanner, we insert a spanner placeholder as a child of the
multicol container. This will mark the multicol container for layout. If we had
no relayout root here, we'd end up marking the ancestry chain all the way up to
LayoutView, and when the LayoutView is marked for layout, we remove any subtree
roots that were previously marked for layout
(clearLayoutSubtreeRootsAndMarkContainingBlocks()), and prevent subtree roots
from being added, until we have laid out.
> 
> But sure there are ways to trigger this without two overflow:hiddens. Added a
couple of more tests. One even failed in assertLaidOut()! I put everything in
fast/multicol/dynamic/ instead of in fast/multicol/span/ . This is not so much
about spanners; it's just that spanners are useful here to cause general misery
in the multicol code (I wasn't able to think of any way to cause trouble without
using spanners, but it might have been possible).

That's reasonable, I was just trying to make sure we had reduced the test cases
and got carried away (forgot we needed at least some relayout root with width /
height).