[New Multicolumn] Let a spanner's containing block be the multicol container.

[New Multicolumn] Let a spanner's containing block be the multicol container.

This helps avoid some cheating and tightrope walking that we would
have to perform when laying out spanners while canceling out all ill
effects that the pagination code could cause. A spanner is no longer
part of the flow thread (although it is a descendant), much like
absolutely positioned descendants whose containing block is outside
the multicol container. It's no longer laid out during flow thread
layout.

RenderMultiColumnSpannerSet has never really been a "set", and now it
doesn't even inherit from RenderMultiColumnSet, so a rename seemed
prudent. It's now called RenderMultiColumnSpannerPlaceholder. The
placeholder will be in charge of laying out the spanner and
positioning it correctly. It will also be in charge of invalidating,
painting and hit-testing the spanner.

Instead of a hashmap that maps column-span:all renderers to their
corresponding placeholder, store a pointer directly in the rare-data
of the RenderBox that is column-span:all. A spanner isn't part of the
flow thread anymore, so locating the spanner's flow thread is a
meaningless or hacky task, so storing this map in the flow thread
would just be messy.

This change contains the bare minimum to avoid assertions during
layout and paint invalidation. There's no actual support for laying
out (or painting, etc.) spanners properly yet.

BUG=347325
R=jchaffraix@chromium.org

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

[mstensho (USE GERRIT), 2014-12-10 13:49:56]

This is a prelude for column-span:all layout support. Next episode (actual
layout support) here: https://codereview.chromium.org/792803002/

Together, these two CLs replace my old attempt:
https://codereview.chromium.org/712553003/

[mstensho (USE GERRIT), 2014-12-10 13:54:22]

On 2014/12/10 13:49:56, mstensho wrote:
> This is a prelude for column-span:all layout support. Next episode (actual
> layout support) here: https://codereview.chromium.org/792803002/
> 
> Together, these two CLs replace my old attempt:
> https://codereview.chromium.org/712553003/

Oh, and if you have a better name than RenderMultiColumnSpannerPlaceholder,
shout! :)

[Julien - ping for review, 2014-12-11 19:09:38]

overall lgtm, I would like to double-check the updated version but this is
addressing all the concerns that I had so far with the architecture.

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

https://codereview.chromium.org/789433006/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBlockFlow.cpp:1065: }
We have some code to avoid laying out some children that are "special". It's
currently used for renderers that needs some special treatment (e.g. <legend>).
Maybe we could somehow extend the layoutSpecialExcludedChild() (methinks we
could have several of these now).

Also mildly orthogonal is that it is starting to be *really* hard to know what
should and shouldn't be laid out during "normal" layout (not positioned, not
special, not column-spanner, ...) so we will want to add some child iterator.

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

https://codereview.chromium.org/789433006/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBlockFlow.h:237: void
updateBlockChildDirtyBitsBeforeLayout(bool relayoutChildren, RenderBox&);
final? Or override?

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

https://codereview.chromium.org/789433006/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBox.cpp:1671: void
RenderBox::setSpannerPlaceholder(RenderMultiColumnSpannerPlaceholder*
placeholder)
We could maybe split setSpannerPlaceHolder (with ASSERT(placeholder) vs
clearSpannerPlaceholder.

This function is not too complicated but it does have 2 clear separated callers.

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

https://codereview.chromium.org/789433006/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBox.h:406: virtual
RenderMultiColumnSpannerPlaceholder* spannerPlaceholder() const override final {
return m_rareData ? m_rareData->m_spannerPlaceholder : 0; }
We just put final per
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/sjotlpNyF-8

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

https://codereview.chromium.org/789433006/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderMultiColumnFlowThread.cpp:170: if
(columnBox->isRenderMultiColumnSet()) {
Should we early continue by flipping the condition? (I prefer avoiding too much
indentation but could live with the current structure).

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

https://codereview.chromium.org/789433006/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderMultiColumnFlowThread.h:119: return lastSiblingBox
!= this ? lastSiblingBox : 0;
This comparison warrants some explanations as it doesn't match
firstMultiColumnBox.

Also why don't we try the previous child box if that doesn't work?

https://codereview.chromium.org/789433006/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderMultiColumnFlowThread.h:122: static RenderBox*
previousSiblingMultiColumnBoxOf(RenderBox* renderer)
Those are plain weird being static instead of on RenderBox (maybe with an ASSERT
that we are in a flow thread).

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

https://codereview.chromium.org/789433006/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderMultiColumnSpannerPlaceholder.cpp:67:
m_rendererInFlowThread->invalidateTreeIfNeeded(newPaintInvalidationState);
A normal implementation would call into the base class to generate the
invalidations, clear the flags and make sure the children generate the
invalidations.

We reuse a RenderBox but it seems like this object could be anything and not
just a RenderObject, is it possible this kind of weird functions points at this
flaw in the design?

[mstensho (USE GERRIT), 2014-12-11 21:01:14]

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

https://codereview.chromium.org/789433006/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBlockFlow.cpp:1065: }
On 2014/12/11 19:09:37, Julien Chaffraix - PST wrote:
> We have some code to avoid laying out some children that are "special". It's
> currently used for renderers that needs some special treatment (e.g.
<legend>).
> Maybe we could somehow extend the layoutSpecialExcludedChild() (methinks we
> could have several of these now).

Then we would need to have layoutSpecialExcludedChild() return a list of
renderers, since there may be more than one spanner (even along with e.g. a
legend, if this is a fieldset). BTW: I'm planning to add some code for the
placeholders here, as you may have noticed already. So it won't be just a
"continue" eventually.

> Also mildly orthogonal is that it is starting to be *really* hard to know what
> should and shouldn't be laid out during "normal" layout (not positioned, not
> special, not column-spanner, ...) so we will want to add some child iterator.

Yeah, in this sense the spanners are neither more nor less special than floats
and absposses. So I think they should be treated in similar ways in the code.
Which makes me think that for now, the spanner skipping belongs right here.

I think it's rather clean to have everything here, rather than wasting time on
building a list of excluded renderers, but in any case, I don't think changing
this should be part of this CL.

Or did you want me to change it right away?

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

https://codereview.chromium.org/789433006/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBox.cpp:1671: void
RenderBox::setSpannerPlaceholder(RenderMultiColumnSpannerPlaceholder*
placeholder)
On 2014/12/11 19:09:37, Julien Chaffraix - PST wrote:
> We could maybe split setSpannerPlaceHolder (with ASSERT(placeholder) vs
> clearSpannerPlaceholder.
> 
> This function is not too complicated but it does have 2 clear separated
callers.

Done.

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

https://codereview.chromium.org/789433006/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBox.h:406: virtual
RenderMultiColumnSpannerPlaceholder* spannerPlaceholder() const override final {
return m_rareData ? m_rareData->m_spannerPlaceholder : 0; }
On 2014/12/11 19:09:37, Julien Chaffraix - PST wrote:
> We just put final per
> https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/sjotlpNyF-8

Done.

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

https://codereview.chromium.org/789433006/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderMultiColumnFlowThread.cpp:170: if
(columnBox->isRenderMultiColumnSet()) {
On 2014/12/11 19:09:37, Julien Chaffraix - PST wrote:
> Should we early continue by flipping the condition? (I prefer avoiding too
much
> indentation but could live with the current structure).

Done.

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

https://codereview.chromium.org/789433006/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderMultiColumnFlowThread.h:119: return lastSiblingBox
!= this ? lastSiblingBox : 0;
On 2014/12/11 19:09:37, Julien Chaffraix - PST wrote:
> This comparison warrants some explanations as it doesn't match
> firstMultiColumnBox.

Done.

> Also why don't we try the previous child box if that doesn't work?

The flow thread needs to be the first child. If the last child is the flow
thread, it means that we have no column boxes (spanner placeholders or column
sets).

https://codereview.chromium.org/789433006/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderMultiColumnFlowThread.h:122: static RenderBox*
previousSiblingMultiColumnBoxOf(RenderBox* renderer)
On 2014/12/11 19:09:38, Julien Chaffraix - PST wrote:
> Those are plain weird being static instead of on RenderBox (maybe with an
ASSERT
> that we are in a flow thread).

Indeed. I was afraid to bloat RenderBox too much, but if you like it, fine. :)

Done.

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

https://codereview.chromium.org/789433006/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderMultiColumnSpannerPlaceholder.cpp:67:
m_rendererInFlowThread->invalidateTreeIfNeeded(newPaintInvalidationState);
On 2014/12/11 19:09:38, Julien Chaffraix - PST wrote:
> A normal implementation would call into the base class to generate the
> invalidations, clear the flags and make sure the children generate the
> invalidations.

There'll be no children, but yes, I really think we should let the base class do
whatever it wants to do.

Done.

> We reuse a RenderBox but it seems like this object could be anything and not
> just a RenderObject, is it possible this kind of weird functions points at
this
> flaw in the design?

What flaw? Mine? :) Sorry, I don't quite understand.

[Julien - ping for review, 2014-12-11 23:45:44]

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

https://codereview.chromium.org/789433006/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBlockFlow.cpp:1065: }
On 2014/12/11 21:01:14, mstensho wrote:
> On 2014/12/11 19:09:37, Julien Chaffraix - PST wrote:
> > We have some code to avoid laying out some children that are "special". It's
> > currently used for renderers that needs some special treatment (e.g.
> <legend>).
> > Maybe we could somehow extend the layoutSpecialExcludedChild() (methinks we
> > could have several of these now).
> 
> Then we would need to have layoutSpecialExcludedChild() return a list of
> renderers, since there may be more than one spanner (even along with e.g. a
> legend, if this is a fieldset). BTW: I'm planning to add some code for the
> placeholders here, as you may have noticed already. So it won't be just a
> "continue" eventually.
> 
> > Also mildly orthogonal is that it is starting to be *really* hard to know
what
> > should and shouldn't be laid out during "normal" layout (not positioned, not
> > special, not column-spanner, ...) so we will want to add some child
iterator.
> 
> Yeah, in this sense the spanners are neither more nor less special than floats
> and absposses. So I think they should be treated in similar ways in the code.
> Which makes me think that for now, the spanner skipping belongs right here.
> 
> I think it's rather clean to have everything here, rather than wasting time on
> building a list of excluded renderers, but in any case, I don't think changing
> this should be part of this CL.
> 
> Or did you want me to change it right away?

No, we shouldn't do it as part of this change. The pressure is just building to
think about a cleaner design around
renderers-in-the-tree-but-specially-laid-out.

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

https://codereview.chromium.org/789433006/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderMultiColumnSpannerPlaceholder.cpp:67:
m_rendererInFlowThread->invalidateTreeIfNeeded(newPaintInvalidationState);
On 2014/12/11 21:01:14, mstensho wrote:
> On 2014/12/11 19:09:38, Julien Chaffraix - PST wrote:
> > A normal implementation would call into the base class to generate the
> > invalidations, clear the flags and make sure the children generate the
> > invalidations.
> 
> There'll be no children, but yes, I really think we should let the base class
do
> whatever it wants to do.
> 
> Done.
> 
> > We reuse a RenderBox but it seems like this object could be anything and not
> > just a RenderObject, is it possible this kind of weird functions points at
> this
> > flaw in the design?
> 
> What flaw? Mine? :) Sorry, I don't quite understand.

The fact that we need such fake overridden function may point to a fault in the
design. That's what I tried to say but re-reading the comment, I am less
convinced (and my usual argument for that is RenderScrollbar, which is a
RenderBox, but I was convinced it's not crazy).

[mstensho (USE GERRIT), 2014-12-12 06:58:26]

The CQ bit was checked by mstensho@opera.com

[commit-bot: I haz the power, 2014-12-12 06:59:04]

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

[mstensho (USE GERRIT), 2014-12-12 07:27:46]

The CQ bit was unchecked by mstensho@opera.com

[mstensho (USE GERRIT), 2014-12-12 07:31:09]

The CQ bit was checked by mstensho@opera.com

[commit-bot: I haz the power, 2014-12-12 07:31:20]

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

[commit-bot: I haz the power, 2014-12-12 09:00:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-12-12 09:00:44]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/3...)

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

The CQ bit was checked by mstensho@opera.com

[commit-bot: I haz the power, 2014-12-12 09:03:13]

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

[commit-bot: I haz the power, 2014-12-12 12:20:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-12-12 12:20:59]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/41207)

[mstensho (USE GERRIT), 2014-12-12 13:37:58]

The CQ bit was checked by mstensho@opera.com

[commit-bot: I haz the power, 2014-12-12 13:38:45]

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

[commit-bot: I haz the power, 2014-12-12 15:50:52]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=187033