[New Multicolumn] Add support for column-span:all

[New Multicolumn] Add support for column-span:all

A column spanner needs to break out from regular column layout, so
that it's sized by the multicol container, rather by the used column
width (the width of the flow thread). A spanner will terminate any
regular column content preceding it, establish its own
RenderMultiColumnSpannerSet to follow the RenderMultiColumnSet
established by the preceding column content. Regular column content
following the spanner will be represented by another
RenderMultiColumnSet following the spanner set.

Since the renderer with column-span:all is still part of the flow
thread, some hooks are needed in block layout code: we need some
special code for sizing and also for block direction margin
handling. Spanner margins may not collapse with anything inside
columns. Margin properties are copied to the spanner set, so that
block direction margins may be handled during column set layout,
and be ignored during flow thread layout.

Some extra attention is required when inserting and removing flow
thread descendants now, because we need to figure out if the renderer
inserted should trigger creation of a new multi column set or a
spanner set. If a spanner is inserted in the middle of a multi column
set, we need to detect this, split the set and put the spanner set
inbetween.

BUG=347325

[mstensho (USE GERRIT), 2014-09-19 14:49:37]

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

[Julien - ping for review, 2014-09-30 00:39:32]

I haven't had time to dive deeply into the patch as it's really too big for me
to review :-(

If you want more timely reviews, I would advise splitting this patch into
smaller chunks.

https://codereview.chromium.org/584033002/diff/1/Source/core/rendering/Layout...
File Source/core/rendering/LayoutState.cpp (right):

https://codereview.chromium.org/584033002/diff/1/Source/core/rendering/Layout...
Source/core/rendering/LayoutState.cpp:94: // multicol container inside something
paginated).
This seems like a big limitation of this patch. I assume this matches the
current code but I would like that explicitly mentioned.

https://codereview.chromium.org/584033002/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderBlock.cpp (left):

https://codereview.chromium.org/584033002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBlock.cpp:1077: child->removeFromRenderFlowThread();
Are we sure we don't insert children into the flow-thread that needs to be
cleaned up now?

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

https://codereview.chromium.org/584033002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBlockFlow.cpp:557: if (childIsColumnSpanner) {
Nit: This looks like it should be a RAII so that we enter / leave are always
paired.

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

https://codereview.chromium.org/584033002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBox.cpp:1497: cb =
flowThreadContainingBlock()->containingBlock();
It seems like we should be updating containgBlock() / container() for column
span. One issue to handle (probably in a follow-up patch) is containing block
changes (where our layout marks don't follow the tree anymore, see the
positioned element logic).

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

https://codereview.chromium.org/584033002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderMultiColumnFlowThread.cpp:362:
spannerSet->setChildNeedsLayout();
2 mistakes here:
- by default setChildNeedsLayout marks the containing block chain, which is
something you don't want! It is a programming mistake to call it this way during
layout and was the source of a lot of bugs in the past.
- setChildNeedsLayout has a SubtreeLayoutScope parameter that you should pass.
It is used for the purpose of avoiding the previous issues. See how other
renderers are dealing with that. It also would give you some extra guarantees
(like that it's a descendant that you are checking above :)).

I think this warrants a FIXME as we probably could avoid this heavy handed
marking in some cases.

https://codereview.chromium.org/584033002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderMultiColumnFlowThread.cpp:412: for (; descendant;
descendant = descendant->nextInPreOrder(subtreeRoot)) {
Modifying the passed-in parameter is a bad smell, especially since you save it 2
lines above.

[mstensho (USE GERRIT), 2014-09-30 20:20:28]

Yeah, sorry for the rather big patch. If you have suggestions on how to split it
into sensible pieces, I'd be more than happy to do so. But, to me, this patch
does only one thing: introduce support for column-span:all, and I cannot see any
useful intermediary states between not supporting it and supporting it. I think
reviewers need the big picture to make sense of it anyway.

https://codereview.chromium.org/584033002/diff/1/Source/core/rendering/Layout...
File Source/core/rendering/LayoutState.cpp (right):

https://codereview.chromium.org/584033002/diff/1/Source/core/rendering/Layout...
Source/core/rendering/LayoutState.cpp:94: // multicol container inside something
paginated).
On 2014/09/30 00:39:32, Julien Chaffraix - PST wrote:
> This seems like a big limitation of this patch. I assume this matches the
> current code but I would like that explicitly mentioned.

Done. More comment.

This patch doesn't introduce this limitation, though. It just relies on it. :)

https://codereview.chromium.org/584033002/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderBlock.cpp (left):

https://codereview.chromium.org/584033002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBlock.cpp:1077: child->removeFromRenderFlowThread();
On 2014/09/30 00:39:32, Julien Chaffraix - PST wrote:
> Are we sure we don't insert children into the flow-thread that needs to be
> cleaned up now?

This is an anonymous block, and just became childless. Anonymous blocks don't do
anything special inside a flow thread. The real reason why I had to remove this,
though, is that it calls removeFromRenderFlowThread() while being "half-dead" -
or at least half-detached. This confused flowThreadDescendantWillBeRemoved(),
since that method attempts to walk the subtree.

removeFromRenderFlowThread() is still called from willBeRemovedFromTree(), and
that should be sufficient. It's a pity that removeLeftoverAnonymousBlock() needs
to tear down its child single-handedly, but I'm sure there's a reason for that.
Might as well be related to crbug.com/417556 for all I know. :)

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

https://codereview.chromium.org/584033002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBox.cpp:1497: cb =
flowThreadContainingBlock()->containingBlock();
On 2014/09/30 00:39:32, Julien Chaffraix - PST wrote:
> It seems like we should be updating containgBlock() / container() for column
> span. One issue to handle (probably in a follow-up patch) is containing block
> changes (where our layout marks don't follow the tree anymore, see the
> positioned element logic).

Yeah, I've been thinking about this, and it's certainly worth considering. But
yeah, not as part of this CL.

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

https://codereview.chromium.org/584033002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderMultiColumnFlowThread.cpp:362:
spannerSet->setChildNeedsLayout();
On 2014/09/30 00:39:32, Julien Chaffraix - PST wrote:
> 2 mistakes here:
> - by default setChildNeedsLayout marks the containing block chain, which is
> something you don't want! It is a programming mistake to call it this way
during
> layout and was the source of a lot of bugs in the past.
> - setChildNeedsLayout has a SubtreeLayoutScope parameter that you should pass.
> It is used for the purpose of avoiding the previous issues. See how other
> renderers are dealing with that. It also would give you some extra guarantees
> (like that it's a descendant that you are checking above :)).
> 
> I think this warrants a FIXME as we probably could avoid this heavy handed
> marking in some cases.

Done.

Uploaded this change separately, as it modified code that I'd already modified
because of another code review issue that you had raised.

https://codereview.chromium.org/584033002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderMultiColumnFlowThread.cpp:412: for (; descendant;
descendant = descendant->nextInPreOrder(subtreeRoot)) {
On 2014/09/30 00:39:32, Julien Chaffraix - PST wrote:
> Modifying the passed-in parameter is a bad smell, especially since you save it
2
> lines above.

Done.

[mstensho (USE GERRIT), 2014-10-29 16:14:32]

Closing this. Breaking into smaller parts, starting with
https://codereview.chromium.org/688703003/