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

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

Column spanners are implemented as siblings of RenderMultiColumnSet
objects (i.e. the regions for the column rows). This means that they
are pulled out from the flow thread tree where they would otherwise
live.

A placeholder is put in the flow thread tree where the spanner's
renderer would otherwise live. This is needed in order make sure that
we interrupt line layout before after the spanner. We also need this
to be able to switch from one multicol set to the next.

Some extra logic is required when dynamically inserting and removing
flow thread descendants now, because we need to figure out if the
renderer added should trigger creation of new multi column sets. 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 in the middle.

Wrote a bunch of tests. A few of the tests were copied from existing
(old-impl) tests and put in a separate directory. That directory can
be wiped when we turn on the new multicol implementation by default.

BUG=347325

[mstensho (USE GERRIT), 2014-05-27 16:19:20]

Depends on https://codereview.chromium.org/295373006/ - not functionality-wise,
but they modify some code in the same areas.

[Julien - ping for review, 2014-06-18 00:51:59]

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

https://codereview.chromium.org/296413007/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBlockFlow.cpp:652:
flowThread->flowThreadDescendantBoxLaidOut(child,
offsetFromLogicalTopOfFirstPage() + newHeight);
I don't like this pattern because it makes little sense to notify your container
that you were laid out as it is the one calling layout on you.

[rune, 2014-06-19 14:24:33]

Review in progress. Posting what I have so far.

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

https://codereview.chromium.org/296413007/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderMultiColumnFlowThread.cpp:303: return
firstMultiColumnSet();
A single set is still the case for RenderPagedFlowThread, right?

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

https://codereview.chromium.org/296413007/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderMultiColumnFlowThread.cpp:323: static bool
isValidColumnSpanner(RenderMultiColumnFlowThread* flowThread, RenderObject*
descendant)
Why not make this a member method of the RenderMultiColumnFlowThread class
instead (possibly rename since _is_ would not be a property of "this")?

https://codereview.chromium.org/296413007/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderMultiColumnFlowThread.cpp:366: // where it wound
otherwise occur (if it weren't a spanner) to becoming a sibling of the
"where it _would_ otherwise ..."

https://codereview.chromium.org/296413007/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderMultiColumnFlowThread.cpp:394: // would have been.
This is needed for a two reasons: We need a way of separating inline
"needed for a two reasons"?

https://codereview.chromium.org/296413007/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderMultiColumnFlowThread.cpp:397: // end flowing one
column set and move to the next one.
"move _on_ to the next one" perhaps?

https://codereview.chromium.org/296413007/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderMultiColumnFlowThread.cpp:453: // in the tree.
"into the tree"

https://codereview.chromium.org/296413007/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderMultiColumnFlowThread.cpp:586: break;
If I understand this correctly, if we ask for a regionAtBlockOffset during
layout, and that offset is past the last set that has been flowed, return the
last flowed set.

Is that something that typically can happen? What does that mean? I guess that
depends on what that returned RenderRegion is used for during layout.

[rune, 2014-07-01 08:41:39]

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

https://codereview.chromium.org/296413007/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderMultiColumnSet.cpp:80:
RenderMultiColumnSpannerPlaceholder* placeholder =
multiColumnFlowThread()->findColumnSpannerPlaceholder(sibling);
Perhaps add an ASSERT(placeholder) as well?

https://codereview.chromium.org/296413007/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderMultiColumnSet.cpp:119: // This is SLOW! But luckily
very uncommon.
Perhaps note in which cases it happens. Is it when the column-span property
changes on a block inside a multicolset, or an element with a column-span
renderer changes display type?

https://codereview.chromium.org/296413007/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderMultiColumnSet.cpp:120: return
precedesRenderer(firstRenderer, renderer) && precedesRenderer(renderer,
lastRenderer);
Do you need to check both? From where you call renders() you go through the
multicolumn sets from first to last. Would it be enough to call
precedesRenderer(renderer, lastRenderer), or is it possible at this point we're
looking for a spanner inside a spanner?

If it's the case that we can be looking at a RenderObject inside a spanner here,
can't the loop in findSetRendering be terminated if renderer precedes
firstRenderer?

https://codereview.chromium.org/296413007/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderObject.h (right):

https://codereview.chromium.org/296413007/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderObject.h:494: return isAnonymous() &&
(style()->display() == BLOCK || style()->display() == BOX) &&
style()->styleType() == NOPSEUDO && isRenderBlock() && !isListMarker() &&
!isRenderFlowThread() && !isRenderMultiColumnSet()
I guess the added check here is because of
RenderMultiColumnSpannerPlaceholder::createAnonymous(). Then why isn't this
testing isRenderMultiColumnSpannerPlaceholder() ?

[rune, 2014-07-01 09:37:57]

https://codereview.chromium.org/296413007/diff/1/LayoutTests/fast/multicol/sp...
File LayoutTests/fast/multicol/span/adjacent-spanners.html (right):

https://codereview.chromium.org/296413007/diff/1/LayoutTests/fast/multicol/sp...
LayoutTests/fast/multicol/span/adjacent-spanners.html:4:
internals.settings.setRegionBasedColumnsEnabled(true);
All setRegionBasedColumnsEnabled in the span directory should be removed and run
in the virtual testsuite instead.

[rune, 2014-07-01 09:44:19]

I consider myself finished with this round of reviewing.

I don't have enough knowledge about the render tree to say if the single flow
thread with spanner placeholders is the right/best way (tm) to implement this.
If for instance @jchaffraix would take a second look, that would be great. 

Otherwise, it's now time to start to address issues and reply to my questions of
ignorance.

[mstensho (USE GERRIT), 2014-08-20 17:26:30]

On 2014/06/18 00:51:59, Julien Chaffraix - PST wrote:
>
https://codereview.chromium.org/296413007/diff/1/Source/core/rendering/Render...
> File Source/core/rendering/RenderBlockFlow.cpp (right):
> 
>
https://codereview.chromium.org/296413007/diff/1/Source/core/rendering/Render...
> Source/core/rendering/RenderBlockFlow.cpp:652:
> flowThread->flowThreadDescendantBoxLaidOut(child,
> offsetFromLogicalTopOfFirstPage() + newHeight);
> I don't like this pattern because it makes little sense to notify your
container
> that you were laid out as it is the one calling layout on you.

I'm not too proud of this solution myself, but every descendant of a flow thread
needs to notify the flow thread when they have been laid out. Note that the flow
thread isn't necessary the container of this renderer. All we can be sure of is
that it's an ancestor. This is a way for the flow thread to detect spanner
placeholders, so that we proceed to the next column set when we detect one.

[Julien - ping for review, 2014-08-20 23:26:10]

On 2014/08/20 at 17:26:30, mstensho wrote:
> On 2014/06/18 00:51:59, Julien Chaffraix - PST wrote:
> >
https://codereview.chromium.org/296413007/diff/1/Source/core/rendering/Render...
> > File Source/core/rendering/RenderBlockFlow.cpp (right):
> > 
> >
https://codereview.chromium.org/296413007/diff/1/Source/core/rendering/Render...
> > Source/core/rendering/RenderBlockFlow.cpp:652:
> > flowThread->flowThreadDescendantBoxLaidOut(child,
> > offsetFromLogicalTopOfFirstPage() + newHeight);
> > I don't like this pattern because it makes little sense to notify your
container
> > that you were laid out as it is the one calling layout on you.
> 
> I'm not too proud of this solution myself, but every descendant of a flow
thread needs to notify the flow thread when they have been laid out. Note that
the flow thread isn't necessary the container of this renderer. All we can be
sure of is that it's an ancestor. This is a way for the flow thread to detect
spanner placeholders, so that we proceed to the next column set when we detect
one.

Why do they need to notify the flow thread? My assumption is that after calling
layoutChildren(), the flow thread knows that all its children are laid out. If
this is a matter of keeping track of the relevant children, I would rather do a
tree walk (to be optimized by keeping track of them as that's hairy).

[mstensho (USE GERRIT), 2014-08-26 09:43:58]

I've now commented on and/or addressed all code review issues raised, including
the flowThreadDescendantBoxLaidOut() one.

Please take a look.

https://codereview.chromium.org/296413007/diff/1/LayoutTests/fast/multicol/sp...
File LayoutTests/fast/multicol/span/adjacent-spanners.html (right):

https://codereview.chromium.org/296413007/diff/1/LayoutTests/fast/multicol/sp...
LayoutTests/fast/multicol/span/adjacent-spanners.html:4:
internals.settings.setRegionBasedColumnsEnabled(true);
On 2014/07/01 09:37:57, rune wrote:
> All setRegionBasedColumnsEnabled in the span directory should be removed and
run
> in the virtual testsuite instead.

Done.

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

https://codereview.chromium.org/296413007/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBlockFlow.cpp:652:
flowThread->flowThreadDescendantBoxLaidOut(child,
offsetFromLogicalTopOfFirstPage() + newHeight);
On 2014/06/18 00:51:59, Julien Chaffraix - PST wrote:
> I don't like this pattern because it makes little sense to notify your
container
> that you were laid out as it is the one calling layout on you.

Got rid of this by setting 'always' on column-break-before, rather than on
column-break-after on the spanner placeholder. This way, things (positions and
sizes) are in a good shape right after layout, so we can tell the flow thread to
advance to the next column set as part of regular layout, rather than having a
hook in RenderBlockFlow.

I wasn't too proud of this hook in RenderBlockFlow myself, but last time I tried
to use column-break-before:always on the spanner (this was before the summer),
it didn't work. But someone must have fixed something during the summer. :)

https://codereview.chromium.org/296413007/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBlockFlow.cpp:652:
flowThread->flowThreadDescendantBoxLaidOut(child,
offsetFromLogicalTopOfFirstPage() + newHeight);
On 2014/06/18 00:51:59, Julien Chaffraix - PST wrote:
> I don't like this pattern because it makes little sense to notify your
container
> that you were laid out as it is the one calling layout on you.

Done.

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

https://codereview.chromium.org/296413007/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderMultiColumnFlowThread.cpp:303: return
firstMultiColumnSet();
On 2014/06/19 14:24:33, rune wrote:
> A single set is still the case for RenderPagedFlowThread, right?

Yes.

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

https://codereview.chromium.org/296413007/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderMultiColumnFlowThread.cpp:323: static bool
isValidColumnSpanner(RenderMultiColumnFlowThread* flowThread, RenderObject*
descendant)
On 2014/06/19 14:24:32, rune wrote:
> Why not make this a member method of the RenderMultiColumnFlowThread class
> instead (possibly rename since _is_ would not be a property of "this")?

Done.

https://codereview.chromium.org/296413007/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderMultiColumnFlowThread.cpp:366: // where it wound
otherwise occur (if it weren't a spanner) to becoming a sibling of the
On 2014/06/19 14:24:33, rune wrote:
> "where it _would_ otherwise ..."

Done.

https://codereview.chromium.org/296413007/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderMultiColumnFlowThread.cpp:394: // would have been.
This is needed for a two reasons: We need a way of separating inline
On 2014/06/19 14:24:32, rune wrote:
> "needed for a two reasons"?

Done.

https://codereview.chromium.org/296413007/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderMultiColumnFlowThread.cpp:397: // end flowing one
column set and move to the next one.
On 2014/06/19 14:24:33, rune wrote:
> "move _on_ to the next one" perhaps?

Done.

https://codereview.chromium.org/296413007/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderMultiColumnFlowThread.cpp:453: // in the tree.
On 2014/06/19 14:24:32, rune wrote:
> "into the tree"

Done.

https://codereview.chromium.org/296413007/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderMultiColumnFlowThread.cpp:586: break;
On 2014/06/19 14:24:33, rune wrote:
> If I understand this correctly, if we ask for a regionAtBlockOffset during
> layout, and that offset is past the last set that has been flowed, return the
> last flowed set.
> 
> Is that something that typically can happen? What does that mean? I guess that
> depends on what that returned RenderRegion is used for during layout.
> 

Done.

I'm not aware of any usecase for searching forwards past the last set worked on,
so I'm removing that code. Additionally, I'm documenting why we have to search
backwards sometimes.

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

https://codereview.chromium.org/296413007/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderMultiColumnSet.cpp:80:
RenderMultiColumnSpannerPlaceholder* placeholder =
multiColumnFlowThread()->findColumnSpannerPlaceholder(sibling);
On 2014/07/01 08:41:38, rune wrote:
> Perhaps add an ASSERT(placeholder) as well?

Is it really necessary? It's gonna crash if it's NULL anyway.

https://codereview.chromium.org/296413007/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderMultiColumnSet.cpp:119: // This is SLOW! But luckily
very uncommon.
On 2014/07/01 08:41:38, rune wrote:
> Perhaps note in which cases it happens. Is it when the column-span property
> changes on a block inside a multicolset, or an element with a column-span
> renderer changes display type?

Done.

https://codereview.chromium.org/296413007/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderMultiColumnSet.cpp:120: return
precedesRenderer(firstRenderer, renderer) && precedesRenderer(renderer,
lastRenderer);
On 2014/07/01 08:41:38, rune wrote:
> Do you need to check both? From where you call renders() you go through the
> multicolumn sets from first to last. Would it be enough to call
> precedesRenderer(renderer, lastRenderer), or is it possible at this point
we're
> looking for a spanner inside a spanner?
> 
> If it's the case that we can be looking at a RenderObject inside a spanner
here,
> can't the loop in findSetRendering be terminated if renderer precedes
> firstRenderer?

Done.

We don't look inside spanners here, so no need to optimize for it.

https://codereview.chromium.org/296413007/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderObject.h (right):

https://codereview.chromium.org/296413007/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderObject.h:494: return isAnonymous() &&
(style()->display() == BLOCK || style()->display() == BOX) &&
style()->styleType() == NOPSEUDO && isRenderBlock() && !isListMarker() &&
!isRenderFlowThread() && !isRenderMultiColumnSet()
On 2014/07/01 08:41:39, rune wrote:
> I guess the added check here is because of
> RenderMultiColumnSpannerPlaceholder::createAnonymous(). Then why isn't this
> testing isRenderMultiColumnSpannerPlaceholder() ?

It's here to make sure that a column set isn't treated as a "regular" anonymous
block (as defined in CSS 2.1). Regular anonymous blocks may be deleted, merged
and split when contents require it (take a look at what happens deep inside
RenderBlock's addChild() and removeChild()). We don't want any of that to happen
to column sets. A spanner placeholder isn't a RenderBlock, so it's not necessary
to test for it here.

[rune, 2014-08-27 07:00:01]

You should have addressed issues before rebasing[1]

Seems something fundemental changed during rebase and it's really hard to figure
out how the issues were addressed.

[1] http://www.chromium.org/developers/contributing-code#TOC-Some-review-tips.

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

https://codereview.chromium.org/296413007/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderMultiColumnSet.cpp:80:
RenderMultiColumnSpannerPlaceholder* placeholder =
multiColumnFlowThread()->findColumnSpannerPlaceholder(sibling);
On 2014/08/26 09:43:58, mstensho wrote:
> On 2014/07/01 08:41:38, rune wrote:
> > Perhaps add an ASSERT(placeholder) as well?
> 
> Is it really necessary? It's gonna crash if it's NULL anyway.

It adds the information that it's not missing a null-check, but it's not
important to me.

https://codereview.chromium.org/296413007/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderObject.h (right):

https://codereview.chromium.org/296413007/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderObject.h:494: return isAnonymous() &&
(style()->display() == BLOCK || style()->display() == BOX) &&
style()->styleType() == NOPSEUDO && isRenderBlock() && !isListMarker() &&
!isRenderFlowThread() && !isRenderMultiColumnSet()
On 2014/08/26 09:43:58, mstensho wrote:
> On 2014/07/01 08:41:39, rune wrote:
> > I guess the added check here is because of
> > RenderMultiColumnSpannerPlaceholder::createAnonymous(). Then why isn't this
> > testing isRenderMultiColumnSpannerPlaceholder() ?
> 
> It's here to make sure that a column set isn't treated as a "regular"
anonymous
> block (as defined in CSS 2.1). Regular anonymous blocks may be deleted, merged
> and split when contents require it (take a look at what happens deep inside
> RenderBlock's addChild() and removeChild()). We don't want any of that to
happen
> to column sets. A spanner placeholder isn't a RenderBlock, so it's not
necessary
> to test for it here.

Acknowledged.

[mstensho (USE GERRIT), 2014-08-27 07:14:01]

On 2014/08/27 07:00:01, rune wrote:
> You should have addressed issues before rebasing[1]
> 
> Seems something fundemental changed during rebase and it's really hard to
figure
> out how the issues were addressed.

Sorry about that. Unfortunately, I didn't see that as an option for me, since
the branch was from May and my repo was pretty much up-to-date. So I had to
rebase before I could even compile it.

I got bitten by some RenderFlowThread cleanup that has taken place during the
summer. RenderFlowThread::columnSetAtBlockOffset() (method formerly known as
regionAtBlockOffset()) was removed during the summer, because it was unused at
the time, but this patch is going to need that code, so I put it back into
RenderMultiColumnFlowThread::columnSetAtBlockOffset(). Apart from that, I don't
recall anything fundamental.

[rune, 2014-08-27 08:17:39]

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

https://codereview.chromium.org/296413007/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderMultiColumnFlowThread.cpp:303: return
firstMultiColumnSet();
On 2014/08/26 09:43:58, mstensho wrote:
> On 2014/06/19 14:24:33, rune wrote:
> > A single set is still the case for RenderPagedFlowThread, right?
> 
> Yes.

Acknowledged.

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

https://codereview.chromium.org/296413007/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderMultiColumnFlowThread.cpp:323: static bool
isValidColumnSpanner(RenderMultiColumnFlowThread* flowThread, RenderObject*
descendant)
On 2014/08/26 09:43:58, mstensho wrote:
> On 2014/06/19 14:24:32, rune wrote:
> > Why not make this a member method of the RenderMultiColumnFlowThread class
> > instead (possibly rename since _is_ would not be a property of "this")?
> 
> Done.

Acknowledged.

https://codereview.chromium.org/296413007/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderMultiColumnFlowThread.cpp:366: // where it wound
otherwise occur (if it weren't a spanner) to becoming a sibling of the
On 2014/08/26 09:43:58, mstensho wrote:
> On 2014/06/19 14:24:33, rune wrote:
> > "where it _would_ otherwise ..."
> 
> Done.

Acknowledged.

https://codereview.chromium.org/296413007/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderMultiColumnFlowThread.cpp:394: // would have been.
This is needed for a two reasons: We need a way of separating inline
On 2014/08/26 09:43:58, mstensho wrote:
> On 2014/06/19 14:24:32, rune wrote:
> > "needed for a two reasons"?
> 
> Done.

Acknowledged.

https://codereview.chromium.org/296413007/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderMultiColumnFlowThread.cpp:397: // end flowing one
column set and move to the next one.
On 2014/08/26 09:43:58, mstensho wrote:
> On 2014/06/19 14:24:33, rune wrote:
> > "move _on_ to the next one" perhaps?
> 
> Done.

Acknowledged.

https://codereview.chromium.org/296413007/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderMultiColumnFlowThread.cpp:453: // in the tree.
On 2014/08/26 09:43:58, mstensho wrote:
> On 2014/06/19 14:24:32, rune wrote:
> > "into the tree"
> 
> Done.

Acknowledged.

https://codereview.chromium.org/296413007/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderMultiColumnFlowThread.cpp:586: break;
On 2014/08/26 09:43:58, mstensho wrote:
> On 2014/06/19 14:24:33, rune wrote:
> > If I understand this correctly, if we ask for a regionAtBlockOffset during
> > layout, and that offset is past the last set that has been flowed, return
the
> > last flowed set.
> > 
> > Is that something that typically can happen? What does that mean? I guess
that
> > depends on what that returned RenderRegion is used for during layout.
> > 
> 
> Done.
> 
> I'm not aware of any usecase for searching forwards past the last set worked
on,
> so I'm removing that code. Additionally, I'm documenting why we have to search
> backwards sometimes.

Acknowledged.

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

https://codereview.chromium.org/296413007/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderMultiColumnSet.cpp:119: // This is SLOW! But luckily
very uncommon.
On 2014/08/26 09:43:58, mstensho wrote:
> On 2014/07/01 08:41:38, rune wrote:
> > Perhaps note in which cases it happens. Is it when the column-span property
> > changes on a block inside a multicolset, or an element with a column-span
> > renderer changes display type?
> 
> Done.

Acknowledged.

https://codereview.chromium.org/296413007/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderMultiColumnSet.cpp:120: return
precedesRenderer(firstRenderer, renderer) && precedesRenderer(renderer,
lastRenderer);
On 2014/08/26 09:43:58, mstensho wrote:
> On 2014/07/01 08:41:38, rune wrote:
> > Do you need to check both? From where you call renders() you go through the
> > multicolumn sets from first to last. Would it be enough to call
> > precedesRenderer(renderer, lastRenderer), or is it possible at this point
> we're
> > looking for a spanner inside a spanner?
> > 
> > If it's the case that we can be looking at a RenderObject inside a spanner
> here,
> > can't the loop in findSetRendering be terminated if renderer precedes
> > firstRenderer?
> 
> Done.
> 
> We don't look inside spanners here, so no need to optimize for it.

I see that this code has been transformed in an optimization patch set. It would
have been better to take that change in a separate CL with a proper description.

[mstensho (USE GERRIT), 2014-08-27 08:24:05]

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

https://codereview.chromium.org/296413007/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderMultiColumnSet.cpp:120: return
precedesRenderer(firstRenderer, renderer) && precedesRenderer(renderer,
lastRenderer);
On 2014/08/27 08:17:38, rune wrote:
> On 2014/08/26 09:43:58, mstensho wrote:
> > On 2014/07/01 08:41:38, rune wrote:
> > > Do you need to check both? From where you call renders() you go through
the
> > > multicolumn sets from first to last. Would it be enough to call
> > > precedesRenderer(renderer, lastRenderer), or is it possible at this point
> > we're
> > > looking for a spanner inside a spanner?
> > > 
> > > If it's the case that we can be looking at a RenderObject inside a spanner
> > here,
> > > can't the loop in findSetRendering be terminated if renderer precedes
> > > firstRenderer?
> > 
> > Done.
> > 
> > We don't look inside spanners here, so no need to optimize for it.
> 
> I see that this code has been transformed in an optimization patch set. It
would
> have been better to take that change in a separate CL with a proper
description.

This code is new in this CL and an issue was raised against it, so I fixed it. I
must have misunderstood what you meant now. Please clarify.

[rune, 2014-08-27 08:36:31]

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

https://codereview.chromium.org/296413007/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderMultiColumnSet.cpp:120: return
precedesRenderer(firstRenderer, renderer) && precedesRenderer(renderer,
lastRenderer);
On 2014/08/27 08:24:05, mstensho wrote:
> On 2014/08/27 08:17:38, rune wrote:
> > On 2014/08/26 09:43:58, mstensho wrote:
> > > On 2014/07/01 08:41:38, rune wrote:
> > > > Do you need to check both? From where you call renders() you go through
> the
> > > > multicolumn sets from first to last. Would it be enough to call
> > > > precedesRenderer(renderer, lastRenderer), or is it possible at this
point
> > > we're
> > > > looking for a spanner inside a spanner?
> > > > 
> > > > If it's the case that we can be looking at a RenderObject inside a
spanner
> > > here,
> > > > can't the loop in findSetRendering be terminated if renderer precedes
> > > > firstRenderer?
> > > 
> > > Done.
> > > 
> > > We don't look inside spanners here, so no need to optimize for it.
> > 
> > I see that this code has been transformed in an optimization patch set. It
> would
> > have been better to take that change in a separate CL with a proper
> description.
> 
> This code is new in this CL and an issue was raised against it, so I fixed it.
I
> must have misunderstood what you meant now. Please clarify.

It such a long time since I reviewed this as well. I thought you could just
remove the first precedesRenderer() call, but perhaps that's what you've done in
practice, I need to take another look.

[mstensho (USE GERRIT), 2014-08-27 08:47:02]

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

https://codereview.chromium.org/296413007/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderMultiColumnSet.cpp:120: return
precedesRenderer(firstRenderer, renderer) && precedesRenderer(renderer,
lastRenderer);
On 2014/08/27 08:36:31, rune wrote:
> On 2014/08/27 08:24:05, mstensho wrote:
> > On 2014/08/27 08:17:38, rune wrote:
> > > On 2014/08/26 09:43:58, mstensho wrote:
> > > > On 2014/07/01 08:41:38, rune wrote:
> > > > > Do you need to check both? From where you call renders() you go
through
> > the
> > > > > multicolumn sets from first to last. Would it be enough to call
> > > > > precedesRenderer(renderer, lastRenderer), or is it possible at this
> point
> > > > we're
> > > > > looking for a spanner inside a spanner?
> > > > > 
> > > > > If it's the case that we can be looking at a RenderObject inside a
> spanner
> > > > here,
> > > > > can't the loop in findSetRendering be terminated if renderer precedes
> > > > > firstRenderer?
> > > > 
> > > > Done.
> > > > 
> > > > We don't look inside spanners here, so no need to optimize for it.
> > > 
> > > I see that this code has been transformed in an optimization patch set. It
> > would
> > > have been better to take that change in a separate CL with a proper
> > description.
> > 
> > This code is new in this CL and an issue was raised against it, so I fixed
it.
> I
> > must have misunderstood what you meant now. Please clarify.
> 
> It such a long time since I reviewed this as well. I thought you could just
> remove the first precedesRenderer() call, but perhaps that's what you've done
in
> practice, I need to take another look.

That's what I did in practice. But I lumped it all together in the calller,
since, by removing the second call, the renders() method could no longer really
be used to figure out if a renderer belongs to a set. You'd have to make ugly
assumptions about the caller iterating through all sets in a specific order.

[rune, 2014-08-27 09:23:44]

https://codereview.chromium.org/296413007/diff/200001/Source/core/rendering/R...
File Source/core/rendering/RenderMultiColumnFlowThread.cpp (right):

https://codereview.chromium.org/296413007/diff/200001/Source/core/rendering/R...
Source/core/rendering/RenderMultiColumnFlowThread.cpp:143: stopRenderer = 0;
The code in lastRendererInFlowThread() used flowThread()->lastLeafChild() for
this, will 0 make it go further than necessary?

[mstensho (USE GERRIT), 2014-08-27 10:12:36]

https://codereview.chromium.org/296413007/diff/200001/Source/core/rendering/R...
File Source/core/rendering/RenderMultiColumnFlowThread.cpp (right):

https://codereview.chromium.org/296413007/diff/200001/Source/core/rendering/R...
Source/core/rendering/RenderMultiColumnFlowThread.cpp:143: stopRenderer = 0;
On 2014/08/27 09:23:44, rune wrote:
> The code in lastRendererInFlowThread() used flowThread()->lastLeafChild() for
> this, will 0 make it go further than necessary?

No, because the stayWithin parameter to nextInPreOrder() is the flow thread.

[rune, 2014-08-27 10:22:27]

lgtm

Wait for feedback from jchaffraix@ on his issue(s).

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

https://codereview.chromium.org/296413007/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderMultiColumnSet.cpp:120: return
precedesRenderer(firstRenderer, renderer) && precedesRenderer(renderer,
lastRenderer);
On 2014/08/27 08:47:02, mstensho wrote:
> On 2014/08/27 08:36:31, rune wrote:
> > On 2014/08/27 08:24:05, mstensho wrote:
> > > On 2014/08/27 08:17:38, rune wrote:
> > > > On 2014/08/26 09:43:58, mstensho wrote:
> > > > > On 2014/07/01 08:41:38, rune wrote:
> > > > > > Do you need to check both? From where you call renders() you go
> through
> > > the
> > > > > > multicolumn sets from first to last. Would it be enough to call
> > > > > > precedesRenderer(renderer, lastRenderer), or is it possible at this
> > point
> > > > > we're
> > > > > > looking for a spanner inside a spanner?
> > > > > > 
> > > > > > If it's the case that we can be looking at a RenderObject inside a
> > spanner
> > > > > here,
> > > > > > can't the loop in findSetRendering be terminated if renderer
precedes
> > > > > > firstRenderer?
> > > > > 
> > > > > Done.
> > > > > 
> > > > > We don't look inside spanners here, so no need to optimize for it.
> > > > 
> > > > I see that this code has been transformed in an optimization patch set.
It
> > > would
> > > > have been better to take that change in a separate CL with a proper
> > > description.
> > > 
> > > This code is new in this CL and an issue was raised against it, so I fixed
> it.
> > I
> > > must have misunderstood what you meant now. Please clarify.
> > 
> > It such a long time since I reviewed this as well. I thought you could just
> > remove the first precedesRenderer() call, but perhaps that's what you've
done
> in
> > practice, I need to take another look.
> 
> That's what I did in practice. But I lumped it all together in the calller,
> since, by removing the second call, the renders() method could no longer
really
> be used to figure out if a renderer belongs to a set. You'd have to make ugly
> assumptions about the caller iterating through all sets in a specific order.

Acknowledged.

https://codereview.chromium.org/296413007/diff/200001/Source/core/rendering/R...
File Source/core/rendering/RenderMultiColumnFlowThread.cpp (right):

https://codereview.chromium.org/296413007/diff/200001/Source/core/rendering/R...
Source/core/rendering/RenderMultiColumnFlowThread.cpp:143: stopRenderer = 0;
On 2014/08/27 10:12:36, mstensho wrote:
> On 2014/08/27 09:23:44, rune wrote:
> > The code in lastRendererInFlowThread() used flowThread()->lastLeafChild()
for
> > this, will 0 make it go further than necessary?
> 
> No, because the stayWithin parameter to nextInPreOrder() is the flow thread.

Acknowledged.

[Julien - ping for review, 2014-09-03 23:22:23]

Sorry for being late at the review party. I haven't had time to dig deeper into
the patch but intent to do so tomorrow.

https://codereview.chromium.org/296413007/diff/200001/Source/core/rendering/R...
File Source/core/rendering/RenderFlowThread.h (right):

https://codereview.chromium.org/296413007/diff/200001/Source/core/rendering/R...
Source/core/rendering/RenderFlowThread.h:70: // Some renderers (column spanners)
are moved out of the flow thread to live among column
I would remove |some renderers| and just put |column spanners| directly.

https://codereview.chromium.org/296413007/diff/200001/Source/core/rendering/R...
Source/core/rendering/RenderFlowThread.h:74: // Called when a descendant of the
flow thread has been inserted.
This comment restates exactly what the name of the function says :-(

https://codereview.chromium.org/296413007/diff/200001/Source/core/rendering/R...
Source/core/rendering/RenderFlowThread.h:76: // Called when a sibling or
descendant of the flow thread is about to be removed.
Same comment.

https://codereview.chromium.org/296413007/diff/200001/Source/core/rendering/R...
File Source/core/rendering/RenderMultiColumnFlowThread.cpp (right):

https://codereview.chromium.org/296413007/diff/200001/Source/core/rendering/R...
Source/core/rendering/RenderMultiColumnFlowThread.cpp:176: // Move spanners back
to their original DOM position in the tree, and destroy the placeholders.
This is a big concern that we don't respect the DOM ordering.

The few examples that we have of differences between the DOM and the render tree
have been a security nightmare:
- CSS tables with the anonymous wrappers: this is the reason you have to check
if |beforeChild| has the same parent you do in addChild. This is still the cause
of much pain.
- generated content used to be anonymous elements and was also a pain so we
converted it into PseudoElement.
- run-in

Ideally we would rather have something that keeps the DOM intact and uses an
external data structure to re-order the elements at layout / paint time. This is
akin to how 'order' is implemented in CSS grid or flexbox.

You wanted to reuse RenderBlockFlow::layoutBlock's logic (which is great), you
will want to look at the layoutSpecialExcludedChild logic, which should be able
to accommodate your needs.

https://codereview.chromium.org/296413007/diff/200001/Source/core/rendering/R...
Source/core/rendering/RenderMultiColumnFlowThread.cpp:289: if
(prev->isRenderMultiColumnSet()) {
Nit: We usually prefer early return / break.

https://codereview.chromium.org/296413007/diff/200001/Source/core/rendering/R...
Source/core/rendering/RenderMultiColumnFlowThread.cpp:296: if
(next->isRenderMultiColumnSet()) {
Same comment.

https://codereview.chromium.org/296413007/diff/200001/Source/core/rendering/R...
Source/core/rendering/RenderMultiColumnFlowThread.cpp:530: // content is added
again later.
FWIW table used to have some layout bugs from this problem as we left empty
anonymous table rows into the tree.

https://codereview.chromium.org/296413007/diff/200001/Source/core/rendering/R...
File Source/core/rendering/RenderMultiColumnFlowThread.h (right):

https://codereview.chromium.org/296413007/diff/200001/Source/core/rendering/R...
Source/core/rendering/RenderMultiColumnFlowThread.h:159: virtual void
flowThreadRelativeWillBeRemoved(RenderObject*) OVERRIDE;
Relative seems too ambiguous to me (there is a relationships between every node
in the tree as they share the same ancestor).

https://codereview.chromium.org/296413007/diff/200001/Source/core/rendering/R...
Source/core/rendering/RenderMultiColumnFlowThread.h:174: RenderMultiColumnSet*
m_lastSetWorkedOn;
Wouldn't this be better passed in as a parameter?

https://codereview.chromium.org/296413007/diff/200001/Source/core/rendering/R...
Source/core/rendering/RenderMultiColumnFlowThread.h:178: bool m_inLayout; // Set
while we're laying out the flow thread, during which colum set heights are
unknown.
Maybe it would be better to call this m_ColumnSetHeightIsUnknown? While
m_isInLayout is OK (notice the 'is'), it seems very generic IMO.

[mstensho (USE GERRIT), 2014-09-04 12:14:02]

https://codereview.chromium.org/296413007/diff/200001/Source/core/rendering/R...
File Source/core/rendering/RenderFlowThread.h (right):

https://codereview.chromium.org/296413007/diff/200001/Source/core/rendering/R...
Source/core/rendering/RenderFlowThread.h:70: // Some renderers (column spanners)
are moved out of the flow thread to live among column
On 2014/09/03 23:22:22, Julien Chaffraix - PST wrote:
> I would remove |some renderers| and just put |column spanners| directly.

Done.

https://codereview.chromium.org/296413007/diff/200001/Source/core/rendering/R...
Source/core/rendering/RenderFlowThread.h:74: // Called when a descendant of the
flow thread has been inserted.
On 2014/09/03 23:22:22, Julien Chaffraix - PST wrote:
> This comment restates exactly what the name of the function says :-(

Done.

https://codereview.chromium.org/296413007/diff/200001/Source/core/rendering/R...
Source/core/rendering/RenderFlowThread.h:76: // Called when a sibling or
descendant of the flow thread is about to be removed.
On 2014/09/03 23:22:22, Julien Chaffraix - PST wrote:
> Same comment.

Done.

https://codereview.chromium.org/296413007/diff/200001/Source/core/rendering/R...
File Source/core/rendering/RenderMultiColumnFlowThread.cpp (right):

https://codereview.chromium.org/296413007/diff/200001/Source/core/rendering/R...
Source/core/rendering/RenderMultiColumnFlowThread.cpp:176: // Move spanners back
to their original DOM position in the tree, and destroy the placeholders.
On 2014/09/03 23:22:23, Julien Chaffraix - PST wrote:
> This is a big concern that we don't respect the DOM ordering.

I understand, and I admit that this is easily the biggest inconvenience with the
implementation I chose. It was chosen because of its low amount of intrusiveness
in other parts of layout (admittedly, there IS some stuff in RenderObject and
RenderBlockFlow with this solution, but other solutions would cause more). Once
renderer reordering is done, everything is straight-forward. The spanner becomes
a child of the multicol container, so that it gets the right size without any
hacks or hooks in the common/central rendering classes, and is positioned nicely
between column sets, and everything kind of Just Works. But I don't know... Too
much dynamite just to make it Just Work?

Another solution I was experimenting with early on didn't require this renderer
reordering. Instead, a spanner was left where it was defined in the DOM, inside
the flow thread. There would instead be inserted a special "column set"/"region
thing" for it among the other regular column sets. The spanner was essentially
part of the flow thread, only that we had to do some tricks when it came to
sizing (you need to use the ancestor multicol container to calculate your
available width, instead of just examining your parent block, because that
parent block is inside a column and is therefore much narrower). I seem to
recall messing with RenderObject::containingBlock() (or the other one with a
similar name, or maybe both) to achieve this. All in all, it worked, but it
required a non-negligible amount of hooks here and there in the code. So I went
for the renderer reparenting solution instead. Which incidentally is the
solution that David Hyatt initially suggested too. :)

A third solution could be to do something similar to what the current/old
implementation does: Insert anonymous multicol containers before and after the
spanner, and turn the actual multicol container into a regular block. This is
intrusive in the RenderBlock code, and I suppose this is code we should rather
get rid of, rather than using it in the new multicol implementation. I'm talking
about lots of stuff in RenderBlock::addChild*() (and methods like splitBlocks(),
splitFlow(), makeChildrenAnonymousColumnBlocks(), etc.) that's only there
because of the current/old multicol implementation of spanners.

> The few examples that we have of differences between the DOM and the render
tree
> have been a security nightmare:
> - CSS tables with the anonymous wrappers: this is the reason you have to check
> if |beforeChild| has the same parent you do in addChild. This is still the
cause
> of much pain.
> - generated content used to be anonymous elements and was also a pain so we
> converted it into PseudoElement.
> - run-in
> 
> Ideally we would rather have something that keeps the DOM intact and uses an
> external data structure to re-order the elements at layout / paint time. This
is
> akin to how 'order' is implemented in CSS grid or flexbox.

The 'order' property, as far as I know, only reorders within the same container.
Pretty similar to the section reordering in RenderTable  (topSection(),
sectionBelow(). etc.), I guess? Multicol spanners are reparented.

> You wanted to reuse RenderBlockFlow::layoutBlock's logic (which is great), you
> will want to look at the layoutSpecialExcludedChild logic, which should be
able
> to accommodate your needs.

I'm not sure how layoutSpecialExcludedChild() would help. The spanner may not be
a direct child of the multicol container (it may be inside blocks, tables and
what not). Besides, there may be more than one of them, so what would
layoutSpecialExcludedChild() return? I have a feeling that I misunderstood you
here. :)

https://codereview.chromium.org/296413007/diff/200001/Source/core/rendering/R...
Source/core/rendering/RenderMultiColumnFlowThread.cpp:289: if
(prev->isRenderMultiColumnSet()) {
On 2014/09/03 23:22:23, Julien Chaffraix - PST wrote:
> Nit: We usually prefer early return / break.

Done.

https://codereview.chromium.org/296413007/diff/200001/Source/core/rendering/R...
Source/core/rendering/RenderMultiColumnFlowThread.cpp:296: if
(next->isRenderMultiColumnSet()) {
On 2014/09/03 23:22:23, Julien Chaffraix - PST wrote:
> Same comment.

Done.

https://codereview.chromium.org/296413007/diff/200001/Source/core/rendering/R...
Source/core/rendering/RenderMultiColumnFlowThread.cpp:530: // content is added
again later.
On 2014/09/03 23:22:23, Julien Chaffraix - PST wrote:
> FWIW table used to have some layout bugs from this problem as we left empty
> anonymous table rows into the tree.

Funny that you should mention it. This is still an issue for tables. But we can
talk about that in https://codereview.chromium.org/540453003/ (which I filed
yesterday). :)

For column set, the idea is that if they are empty, they have no effect on
layout. Pretty big words, I know, but I'd rather make sure of that, instead of
spending time on nuking them here.

https://codereview.chromium.org/296413007/diff/200001/Source/core/rendering/R...
File Source/core/rendering/RenderMultiColumnFlowThread.h (right):

https://codereview.chromium.org/296413007/diff/200001/Source/core/rendering/R...
Source/core/rendering/RenderMultiColumnFlowThread.h:159: virtual void
flowThreadRelativeWillBeRemoved(RenderObject*) OVERRIDE;
On 2014/09/03 23:22:23, Julien Chaffraix - PST wrote:
> Relative seems too ambiguous to me (there is a relationships between every
node
> in the tree as they share the same ancestor).

Done.

https://codereview.chromium.org/296413007/diff/200001/Source/core/rendering/R...
Source/core/rendering/RenderMultiColumnFlowThread.h:174: RenderMultiColumnSet*
m_lastSetWorkedOn;
On 2014/09/03 23:22:23, Julien Chaffraix - PST wrote:
> Wouldn't this be better passed in as a parameter?

It's used in RenderMultiColumnFlowThread::columnSetAtBlockOffset() which may be
called from pretty much anywhere, so RenderMultiColumnFlowThread seems like the
best place to store it.

https://codereview.chromium.org/296413007/diff/200001/Source/core/rendering/R...
Source/core/rendering/RenderMultiColumnFlowThread.h:178: bool m_inLayout; // Set
while we're laying out the flow thread, during which colum set heights are
unknown.
On 2014/09/03 23:22:23, Julien Chaffraix - PST wrote:
> Maybe it would be better to call this m_ColumnSetHeightIsUnknown? While
> m_isInLayout is OK (notice the 'is'), it seems very generic IMO.

Done.

[Julien - ping for review, 2014-09-04 17:47:25]

https://codereview.chromium.org/296413007/diff/200001/Source/core/rendering/R...
File Source/core/rendering/RenderMultiColumnFlowThread.cpp (right):

https://codereview.chromium.org/296413007/diff/200001/Source/core/rendering/R...
Source/core/rendering/RenderMultiColumnFlowThread.cpp:176: // Move spanners back
to their original DOM position in the tree, and destroy the placeholders.
On 2014/09/04 12:14:02, mstensho wrote:
> On 2014/09/03 23:22:23, Julien Chaffraix - PST wrote:
> > This is a big concern that we don't respect the DOM ordering.
> 
> I understand, and I admit that this is easily the biggest inconvenience with
the
> implementation I chose. It was chosen because of its low amount of
intrusiveness
> in other parts of layout (admittedly, there IS some stuff in RenderObject and
> RenderBlockFlow with this solution, but other solutions would cause more).
Once
> renderer reordering is done, everything is straight-forward. The spanner
becomes
> a child of the multicol container, so that it gets the right size without any
> hacks or hooks in the common/central rendering classes, and is positioned
nicely
> between column sets, and everything kind of Just Works. But I don't know...
Too
> much dynamite just to make it Just Work?

It makes me very nervous. The reason is that all cases where the render tree
doesn't match the DOM tree, we have had a security / maintenance nightmare.

If you look at RenderMultiColumnFlowThread::flowThreadDescendantInserted, the
logic makes a ton of assumption about the structure of the tree, some of which
will probably be wrong.

> Another solution I was experimenting with early on didn't require this
renderer
> reordering. Instead, a spanner was left where it was defined in the DOM,
inside
> the flow thread. There would instead be inserted a special "column
set"/"region
> thing" for it among the other regular column sets. The spanner was essentially
> part of the flow thread, only that we had to do some tricks when it came to
> sizing (you need to use the ancestor multicol container to calculate your
> available width, instead of just examining your parent block, because that
> parent block is inside a column and is therefore much narrower). I seem to
> recall messing with RenderObject::containingBlock() (or the other one with a
> similar name, or maybe both) to achieve this. All in all, it worked, but it
> required a non-negligible amount of hooks here and there in the code. So I
went
> for the renderer reparenting solution instead. Which incidentally is the
> solution that David Hyatt initially suggested too. :)

The question is how intrusive is intrusive. :)

Without some details, it's difficult to say if it's an acceptable level of
intrusiveness, especially since we complained about the region code being too
intrusive.

> A third solution could be to do something similar to what the current/old
> implementation does: Insert anonymous multicol containers before and after the
> spanner, and turn the actual multicol container into a regular block. This is
> intrusive in the RenderBlock code, and I suppose this is code we should rather
> get rid of, rather than using it in the new multicol implementation. I'm
talking
> about lots of stuff in RenderBlock::addChild*() (and methods like
splitBlocks(),
> splitFlow(), makeChildrenAnonymousColumnBlocks(), etc.) that's only there
> because of the current/old multicol implementation of spanners.

Yes, that was one of the reason we invoked to remove the region code so it's
probably a show-stopper.

> > The few examples that we have of differences between the DOM and the render
> tree
> > have been a security nightmare:
> > - CSS tables with the anonymous wrappers: this is the reason you have to
check
> > if |beforeChild| has the same parent you do in addChild. This is still the
> cause
> > of much pain.
> > - generated content used to be anonymous elements and was also a pain so we
> > converted it into PseudoElement.
> > - run-in
> > 
> > Ideally we would rather have something that keeps the DOM intact and uses an
> > external data structure to re-order the elements at layout / paint time.
This
> is
> > akin to how 'order' is implemented in CSS grid or flexbox.
> 
> The 'order' property, as far as I know, only reorders within the same
container.
> Pretty similar to the section reordering in RenderTable  (topSection(),
> sectionBelow(). etc.), I guess? Multicol spanners are reparented.
> 
> > You wanted to reuse RenderBlockFlow::layoutBlock's logic (which is great),
you
> > will want to look at the layoutSpecialExcludedChild logic, which should be
> able
> > to accommodate your needs.
> 
> I'm not sure how layoutSpecialExcludedChild() would help. The spanner may not
be
> a direct child of the multicol container (it may be inside blocks, tables and
> what not). Besides, there may be more than one of them, so what would
> layoutSpecialExcludedChild() return? I have a feeling that I misunderstood you
> here. :)

I was thinking about extending the logic to allow multiple excluded children. We
can redesign the API to make sense with that in mind.

[mstensho (USE GERRIT), 2014-09-10 21:40:54]

https://codereview.chromium.org/296413007/diff/200001/Source/core/rendering/R...
File Source/core/rendering/RenderMultiColumnFlowThread.cpp (right):

https://codereview.chromium.org/296413007/diff/200001/Source/core/rendering/R...
Source/core/rendering/RenderMultiColumnFlowThread.cpp:176: // Move spanners back
to their original DOM position in the tree, and destroy the placeholders.
On 2014/09/04 17:47:24, Julien Chaffraix - PST wrote:
> On 2014/09/04 12:14:02, mstensho wrote:
> > Another solution I was experimenting with early on didn't require this
> renderer
> > reordering. Instead, a spanner was left where it was defined in the DOM,
> inside
> > the flow thread. There would instead be inserted a special "column
> set"/"region
> > thing" for it among the other regular column sets. The spanner was
essentially
> > part of the flow thread, only that we had to do some tricks when it came to
> > sizing (you need to use the ancestor multicol container to calculate your
> > available width, instead of just examining your parent block, because that
> > parent block is inside a column and is therefore much narrower). I seem to
> > recall messing with RenderObject::containingBlock() (or the other one with a
> > similar name, or maybe both) to achieve this. All in all, it worked, but it
> > required a non-negligible amount of hooks here and there in the code. So I
> went
> > for the renderer reparenting solution instead. Which incidentally is the
> > solution that David Hyatt initially suggested too. :)
> 
> The question is how intrusive is intrusive. :)

The answer is here (well, maybe): :)
https://codereview.chromium.org/551263005/

Take a look at files like RenderBlockFlow.cpp and let me know what you think.
Note that this has to be cleaned up (and documented), but you should get the
general idea.

[mstensho (USE GERRIT), 2014-09-16 14:27:13]

On 2014/09/10 21:40:54, mstensho wrote:
>
https://codereview.chromium.org/296413007/diff/200001/Source/core/rendering/R...
> File Source/core/rendering/RenderMultiColumnFlowThread.cpp (right):
> 
>
https://codereview.chromium.org/296413007/diff/200001/Source/core/rendering/R...
> Source/core/rendering/RenderMultiColumnFlowThread.cpp:176: // Move spanners
back
> to their original DOM position in the tree, and destroy the placeholders.
> On 2014/09/04 17:47:24, Julien Chaffraix - PST wrote:
> > On 2014/09/04 12:14:02, mstensho wrote:
> > > Another solution I was experimenting with early on didn't require this
> > renderer
> > > reordering. Instead, a spanner was left where it was defined in the DOM,
> > inside
> > > the flow thread. There would instead be inserted a special "column
> > set"/"region
> > > thing" for it among the other regular column sets. The spanner was
> essentially
> > > part of the flow thread, only that we had to do some tricks when it came
to
> > > sizing (you need to use the ancestor multicol container to calculate your
> > > available width, instead of just examining your parent block, because that
> > > parent block is inside a column and is therefore much narrower). I seem to
> > > recall messing with RenderObject::containingBlock() (or the other one with
a
> > > similar name, or maybe both) to achieve this. All in all, it worked, but
it
> > > required a non-negligible amount of hooks here and there in the code. So I
> > went
> > > for the renderer reparenting solution instead. Which incidentally is the
> > > solution that David Hyatt initially suggested too. :)
> > 
> > The question is how intrusive is intrusive. :)
> 
> The answer is here (well, maybe): :)
> https://codereview.chromium.org/551263005/
> 
> Take a look at files like RenderBlockFlow.cpp and let me know what you think.
> Note that this has to be cleaned up (and documented), but you should get the
> general idea.

Julien, have you had any chance to look at this yet?

[mstensho (USE GERRIT), 2014-09-19 14:50:08]

Chatted with Julien on IRC a couple of days ago, and he liked the new approach.
We agreed to drop this CL and file a new one. Here it is:
https://codereview.chromium.org/584033002/