[New Multicolumn] Support dynamic insertion and removal of multicol content.

[New Multicolumn] Support dynamic insertion and removal of multicol content.

Special attention is required when a dynamic change requires a column spanner
placeholder to be inserted. When a spanner is inserted, we need to create a
placeholder. If the spanner was inserted in the middle of regular column
content, it means that there's a column set there that we need to split, so
that we get one column row before the spanner and one column row after. We may
also have to create an additional column set when inserting column set, e.g.
when it's inserted between adjacent spanners.

Removal of spanners was already being handled properly (in terms of merging any
surrounding column sets). But removing superfluous column sets when column
content is removed wasn't taken care of until this CL.

Also had to fix how spanner placeholders are torn down, as we cannot notify the
flow thread both in RenderBox and RenderObject, when a spanner is removed. If
we first did it in RenderBox, the spanner placeholder would be destroyed at
that time. This would mean that when we finally got to
RenderObject::removeFromRenderFlowThreadRecursive(), the ex-spanner would
notify the thread again for the same renderer (which is bad in any regard), and
this time the flow thread would think that we're removing actual column content
(since there's no longer a placeholder associated with it). The unit test
MultiColumnTreeModifyingTest.InsertSpannerAndRemove would assert without this
change, as we wouldn't be able to find a column set to remove (since there is
none in that case, since the multicol container only contains a sole spanner).

Added a bunch of layout and unit tests. The unit tests need to modify the
render tree on their own, so we need to tell the document that this needs to be
allowed.

BUG=347325
R=jchaffraix@chromium.org

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

[Julien - ping for review, 2015-01-23 09:13:56]

lgtm

https://codereview.chromium.org/869583002/diff/1/LayoutTests/fast/multicol/dy...
File
LayoutTests/fast/multicol/dynamic/insert-block-before-spanner-before-content.html
(right):

https://codereview.chromium.org/869583002/diff/1/LayoutTests/fast/multicol/dy...
LayoutTests/fast/multicol/dynamic/insert-block-before-spanner-before-content.html:13:
<p>There should be two lines below with the word "PASS". Letter spacing is
expected to vary.</p>
3 lines below

https://codereview.chromium.org/869583002/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderMultiColumnFlowThread.cpp (right):

https://codereview.chromium.org/869583002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderMultiColumnFlowThread.cpp:462: RenderObject*
previousRenderer = renderer->previousInPreOrder(this);
You're using |this| as the |root| instead of |descendant| (in the for-loop
above), is that expected?

https://codereview.chromium.org/869583002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderMultiColumnFlowThread.cpp:555: }
Shouldn't you ASSERT that you either have no children or are surrounded by
spanner holder?

https://codereview.chromium.org/869583002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderMultiColumnFlowThread.cpp:573: }
Adding a comment to explain the whole strategy would be useful. You implicitly
assume a certain structure for the tree that would be nice to be made explicit.

[mstensho (USE GERRIT), 2015-01-23 12:15:50]

https://codereview.chromium.org/869583002/diff/1/LayoutTests/fast/multicol/dy...
File
LayoutTests/fast/multicol/dynamic/insert-block-before-spanner-before-content.html
(right):

https://codereview.chromium.org/869583002/diff/1/LayoutTests/fast/multicol/dy...
LayoutTests/fast/multicol/dynamic/insert-block-before-spanner-before-content.html:13:
<p>There should be two lines below with the word "PASS". Letter spacing is
expected to vary.</p>
On 2015/01/23 09:13:56, Julien Chaffraix - CET wrote:
> 3 lines below

Done.

https://codereview.chromium.org/869583002/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderMultiColumnFlowThread.cpp (right):

https://codereview.chromium.org/869583002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderMultiColumnFlowThread.cpp:462: RenderObject*
previousRenderer = renderer->previousInPreOrder(this);
On 2015/01/23 09:13:56, Julien Chaffraix - CET wrote:
> You're using |this| as the |root| instead of |descendant| (in the for-loop
> above), is that expected?

Yes, |this| is the flow thread, and that's all we really want to stay within.

<div style="-webkit-columns:3;">
    <h1>a</h1>
    <div id="spanner" style="display:none; -webkit-column-span:all;">b</div>
    <h2>c</h2>
</div>

Now imagine that you later set display:block on "spanner". That would become
both |renderer| and |descendant| in this code. Staying within it for
previousInPreOrder() not what we want; we want to get the H1 renderer, not 0.

If we got 0 here in this case, that would have us enter the first if-block, and
insert the spanner placeholder before the first (and only) column set, resulting
in this layout:

b
a          c

While we should get:

a
b
c

... since we actually end up in the last else-block, where we split the column
set around the spanner placeholder.

Expanded the above comment a little bit, to make it clearer that the flow thread
is the thing to stay within.

https://codereview.chromium.org/869583002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderMultiColumnFlowThread.cpp:555: }
On 2015/01/23 09:13:56, Julien Chaffraix - CET wrote:
> Shouldn't you ASSERT that you either have no children or are surrounded by
> spanner holder?

Discussed offline that what we already have is enough.

https://codereview.chromium.org/869583002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderMultiColumnFlowThread.cpp:573: }
On 2015/01/23 09:13:56, Julien Chaffraix - CET wrote:
> Adding a comment to explain the whole strategy would be useful. You implicitly
> assume a certain structure for the tree that would be nice to be made
explicit.

Done.

Added some header file documentation, and referred to it at the top of this
method and the preceding one.

[mstensho (USE GERRIT), 2015-01-23 12:16:04]

The CQ bit was checked by mstensho@opera.com

[commit-bot: I haz the power, 2015-01-23 12:16:42]

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

[commit-bot: I haz the power, 2015-01-23 13:32:34]

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