[New Multicolumn] Add RenderMultiColumnSpannerSet.

[New Multicolumn] Add RenderMultiColumnSpannerSet.

This is in preparation for column-span:all support.

This CL puts basic set renderer insertion needed by column-span:all into
place. Full support for set management (cope with dynamic changes after
inital layout, etc.) and actual support for layout will be introduced in
follow-up CLs. Likewise for layout tests.

Each column-span:all renderer needs a corresponding
RenderMultiColumnSpannerSet. This means that if there's column content
preceding and following the spanner, we need a RenderMultiColumnSet both
before and after it (while, without spanners, there'd never be any need
for more than one column set).

Some extra attention is required when inserting flow thread
descendants now, because we need to figure out if the renderer
inserted should trigger creation of a new column set or a spanner set.

Wrote some unit tests. Layout tests not possible at this time, since this
CL has no (intentional) web-facing changes.

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

[mstensho (USE GERRIT), 2014-10-29 15:57:22]

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

[mstensho (USE GERRIT), 2014-10-29 15:57:23]

Even smaller CL. Julien: can you take a look, please?

[Julien - ping for review, 2014-11-05 16:52:39]

lgtm.

The testing for containingColumnSpannerSet seems very limited and would
definitely benefit from some extra work. The tests tend to do a bit too much (as
in testing most of the world) but I don't have a good answer to that (lack of
infrastructure and good unit tests writing skills). The 2nd point shouldn't hold
you from moving forward though.

https://codereview.chromium.org/688703003/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderMultiColumnFlowThread.cpp (right):

https://codereview.chromium.org/688703003/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderMultiColumnFlowThread.cpp:240: // We cannot handle
immediate column set siblings (and there's no need for it, either). There
Nit: Let's move 'There' to the next line so that we don't split the sentence.

https://codereview.chromium.org/688703003/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderMultiColumnFlowThread.cpp:261: // becoming a
spanner.
I would be good to quote from the specification as this just re-iterates what
the code is doing.

https://codereview.chromium.org/688703003/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderMultiColumnFlowThread.cpp:266: // Needs to be in a
block container, and not e.g. a table.
It should be: block-flow container

https://codereview.chromium.org/688703003/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderMultiColumnFlowThread.cpp:271: for (RenderBox*
ancestor = toRenderBox(descendant)->parentBox(); ancestor; ancestor =
ancestor->parentBox()) {
I don't think you should not use parentBox as it doesn't follow the containing
block chain (which is really what you want as your talking about with 'inside').

for (RenderBlock* ancestor = descendant->containingBlock(); ancestor; ancestor =
ancestor->containingBlock())

https://codereview.chromium.org/688703003/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderMultiColumnFlowThread.cpp:272: if
(ancestor->isRenderFlowThread()) {
Shouldn't we just check flowThreadState()? If that's an option, I would rather
check it once outside the loop than repeatedly.

https://codereview.chromium.org/688703003/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderMultiColumnFlowThread.cpp:273: // Don't allow any
intervening non-multicol fragmentation contexts. The spec doesn't say
We don't allow non-multicol fragmentation contexts so this comment seems off.

https://codereview.chromium.org/688703003/diff/40001/Source/core/rendering/Re...
File Source/core/rendering/RenderMultiColumnFlowThread.h (right):

https://codereview.chromium.org/688703003/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderMultiColumnFlowThread.h:147: virtual bool
isDescendantValidColumnSpanner(RenderObject* descendant) const;
If we follow the correct English order, it should be
descendantIsValidColumnSpanner.

https://codereview.chromium.org/688703003/diff/40001/Source/core/rendering/Re...
File Source/core/rendering/RenderMultiColumnFlowThreadTest.cpp (right):

https://codereview.chromium.org/688703003/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderMultiColumnFlowThreadTest.cpp:27: String
columnSetSignature(const char* multicolId);
The signature based check saves quite some code but there is a pattern in your
test cases: whenever you have a spanner (not other renderers btw), you want to
check the containing column set.

https://codereview.chromium.org/688703003/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderMultiColumnFlowThreadTest.cpp:70:
TEST_F(MultiColumnRenderingTest, BasicSanityTest)
There is quite some overlap with the other tests, like OneBlock (as the column
set has a single child which is a block renderer).

When reading the name, I thought this was a sanity check where we would check
some very simple characteristics of the tree that is required for other tests to
pass.

I think this test is valuable but we should rename it to e.g.
OneBlockWithInDepthTreeStructureCheck

https://codereview.chromium.org/688703003/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderMultiColumnFlowThreadTest.cpp:126:
TEST_F(MultiColumnRenderingTest, ContentAndSpanner)
Maybe better name ContentThenSpanner or SpannerFollowingContent (it's backwards
so really not great)?

[mstensho (USE GERRIT), 2014-11-05 21:43:06]

https://codereview.chromium.org/688703003/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderMultiColumnFlowThread.cpp (right):

https://codereview.chromium.org/688703003/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderMultiColumnFlowThread.cpp:240: // We cannot handle
immediate column set siblings (and there's no need for it, either). There
On 2014/11/05 16:52:38, Julien Chaffraix - PST wrote:
> Nit: Let's move 'There' to the next line so that we don't split the sentence.

Done.

https://codereview.chromium.org/688703003/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderMultiColumnFlowThread.cpp:261: // becoming a
spanner.
On 2014/11/05 16:52:38, Julien Chaffraix - PST wrote:
> I would be good to quote from the specification as this just re-iterates what
> the code is doing.

Done.

https://codereview.chromium.org/688703003/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderMultiColumnFlowThread.cpp:266: // Needs to be in a
block container, and not e.g. a table.
On 2014/11/05 16:52:38, Julien Chaffraix - PST wrote:
> It should be: block-flow container

Done.

The spec calls those things "block containers", though.

https://codereview.chromium.org/688703003/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderMultiColumnFlowThread.cpp:271: for (RenderBox*
ancestor = toRenderBox(descendant)->parentBox(); ancestor; ancestor =
ancestor->parentBox()) {
On 2014/11/05 16:52:38, Julien Chaffraix - PST wrote:
> I don't think you should not use parentBox as it doesn't follow the containing
> block chain (which is really what you want as your talking about with
'inside').
> 
> for (RenderBlock* ancestor = descendant->containingBlock(); ancestor; ancestor
=
> ancestor->containingBlock())

The spec doesn't have much to offer regarding this, but yes, it seems reasonable
to prevent spanners inside an abspos whose containing block is an ancestor of
the flow thread. But then we also need to make sure that containingBlock()
doesn't jump to an ancestor of the flow thread.

Done.

https://codereview.chromium.org/688703003/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderMultiColumnFlowThread.cpp:272: if
(ancestor->isRenderFlowThread()) {
On 2014/11/05 16:52:38, Julien Chaffraix - PST wrote:
> Shouldn't we just check flowThreadState()? If that's an option, I would rather
> check it once outside the loop than repeatedly.

flowThreadState() just tells you if you're a flow thread or inside of one, so
that won't do.

https://codereview.chromium.org/688703003/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderMultiColumnFlowThread.cpp:273: // Don't allow any
intervening non-multicol fragmentation contexts. The spec doesn't say
On 2014/11/05 16:52:38, Julien Chaffraix - PST wrote:
> We don't allow non-multicol fragmentation contexts so this comment seems off.

There's paged containers (overflow:-webkit-paged-[xy]), which don't count as
multicol in this case. If you stumble upon one of those on your way, just refuse
to turn the renderer in question into a spanner.

https://codereview.chromium.org/688703003/diff/40001/Source/core/rendering/Re...
File Source/core/rendering/RenderMultiColumnFlowThread.h (right):

https://codereview.chromium.org/688703003/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderMultiColumnFlowThread.h:147: virtual bool
isDescendantValidColumnSpanner(RenderObject* descendant) const;
On 2014/11/05 16:52:38, Julien Chaffraix - PST wrote:
> If we follow the correct English order, it should be
> descendantIsValidColumnSpanner.

Done.

https://codereview.chromium.org/688703003/diff/40001/Source/core/rendering/Re...
File Source/core/rendering/RenderMultiColumnFlowThreadTest.cpp (right):

https://codereview.chromium.org/688703003/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderMultiColumnFlowThreadTest.cpp:27: String
columnSetSignature(const char* multicolId);
On 2014/11/05 16:52:39, Julien Chaffraix - PST wrote:
> The signature based check saves quite some code but there is a pattern in your
> test cases: whenever you have a spanner (not other renderers btw), you want to
> check the containing column set.

For some of the tests that we currently have, columnSetSignature() is kind of
redundant, but I don't plan to call containingColumnSpannerSet() and stuff in
all future tests.

(But good point, anyway, I should call containingColumnSpannerSet() for some
other renderers as well, and see that I get nullptr).

https://codereview.chromium.org/688703003/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderMultiColumnFlowThreadTest.cpp:70:
TEST_F(MultiColumnRenderingTest, BasicSanityTest)
On 2014/11/05 16:52:39, Julien Chaffraix - PST wrote:
> There is quite some overlap with the other tests, like OneBlock (as the column
> set has a single child which is a block renderer).
> 
> When reading the name, I thought this was a sanity check where we would check
> some very simple characteristics of the tree that is required for other tests
to
> pass.
> 
> I think this test is valuable but we should rename it to e.g.
> OneBlockWithInDepthTreeStructureCheck

Done.

https://codereview.chromium.org/688703003/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderMultiColumnFlowThreadTest.cpp:126:
TEST_F(MultiColumnRenderingTest, ContentAndSpanner)
On 2014/11/05 16:52:39, Julien Chaffraix - PST wrote:
> Maybe better name ContentThenSpanner or SpannerFollowingContent (it's
backwards
> so really not great)?

Done.

[mstensho (USE GERRIT), 2014-11-05 21:43:16]

The CQ bit was checked by mstensho@opera.com

[commit-bot: I haz the power, 2014-11-05 21:43:48]

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

[mstensho (USE GERRIT), 2014-11-05 23:07:37]

The CQ bit was unchecked by mstensho@opera.com

[mstensho (USE GERRIT), 2014-11-05 23:16:22]

https://codereview.chromium.org/688703003/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderMultiColumnFlowThread.cpp (right):

https://codereview.chromium.org/688703003/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderMultiColumnFlowThread.cpp:273: // Don't allow any
intervening non-multicol fragmentation contexts. The spec doesn't say
On 2014/11/05 21:43:06, mstensho wrote:
> On 2014/11/05 16:52:38, Julien Chaffraix - PST wrote:
> > We don't allow non-multicol fragmentation contexts so this comment seems
off.
> 
> There's paged containers (overflow:-webkit-paged-[xy]), which don't count as
> multicol in this case. If you stumble upon one of those on your way, just
refuse
> to turn the renderer in question into a spanner.

Nonono! Wrong! Well, yes, there may be paged containers, but spanner candidates
will always be considered against their nearest containing flow thread, and
PagedRenderFlowThread has an override for this method, to prevent spanners.

I'll remove the comment, assert that ancestor == this and return true.

[mstensho (USE GERRIT), 2014-11-05 23:17:29]

The CQ bit was checked by mstensho@opera.com

[commit-bot: I haz the power, 2014-11-05 23:18:27]

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

[commit-bot: I haz the power, 2014-11-06 00:26:56]

Committed patchset #5 (id:80001) as 184887