[New multicol] Eliminate the need for RenderMultiColumnBlock.

[New multicol] Eliminate the need for RenderMultiColumnBlock.

Having a distinct renderer for multicol is problematic, both
table cells and list items may become multicol containers. Also,
if we want to use the new multicol implementation to do
pagination, we need RenderView to be able to do multicol as well.

Moved most of the logic over to RenderMultiColumnFlowThread, but some
hooks are needed in RenderBlockFlow as well.

This is based on work by hyatt@apple.com in WebKit. Original patches:
https://bugs.webkit.org/show_bug.cgi?id=127365
https://bugs.webkit.org/show_bug.cgi?id=127565

Added Layout tests for multicol list-item and table-cell.

Also added tests for turning a regular block into multicol, and back into a
regular block (passed before, passes now).

BUG=none

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

[eseidel, 2014-02-25 22:11:31]

OK.

[eseidel, 2014-02-25 22:16:05]

lgtm

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

https://codereview.chromium.org/179993006/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBlockFlow.cpp:181: if (RenderMultiColumnFlowThread*
flowThread = multiColumnFlowThread()) {
nit: I probably would have written this as an early-out instead of a long if
block.

https://codereview.chromium.org/179993006/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBlockFlow.cpp:195: relayoutChildren = true;
This is just return true.

https://codereview.chromium.org/179993006/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBlockFlow.cpp:207: } else if (hasColumns()) {
So this will go away soon, right?

https://codereview.chromium.org/179993006/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBlockFlow.cpp:2783: rareData.m_multiColumnFlowThread
= flowThread;
Do we need to ASSERT that it doesnt' arleady have one?  I guess
multiColumnFlowThread() above should have already ensured that. :)

[mstensho (USE GERRIT), 2014-02-25 23:17:52]

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

https://codereview.chromium.org/179993006/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBlockFlow.cpp:181: if (RenderMultiColumnFlowThread*
flowThread = multiColumnFlowThread()) {
On 2014/02/25 22:16:05, eseidel wrote:
> nit: I probably would have written this as an early-out instead of a long if
> block.

Done.

https://codereview.chromium.org/179993006/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBlockFlow.cpp:195: relayoutChildren = true;
On 2014/02/25 22:16:05, eseidel wrote:
> This is just return true.

Done.

https://codereview.chromium.org/179993006/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBlockFlow.cpp:207: } else if (hasColumns()) {
On 2014/02/25 22:16:05, eseidel wrote:
> So this will go away soon, right?

Yes. :)

https://codereview.chromium.org/179993006/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBlockFlow.cpp:2783: rareData.m_multiColumnFlowThread
= flowThread;
On 2014/02/25 22:16:05, eseidel wrote:
> Do we need to ASSERT that it doesnt' arleady have one?  I guess
> multiColumnFlowThread() above should have already ensured that. :)

Done.

You're right about multiColumnFlowThread(), but an assertion is even more
obvious.

[mstensho (USE GERRIT), 2014-02-25 23:18:01]

The CQ bit was checked by mstensho@opera.com

[commit-bot: I haz the power, 2014-02-25 23:18:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mstensho@opera.com/179993006/20001

[commit-bot: I haz the power, 2014-02-25 23:29:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mstensho@opera.com/179993006/20001

[eseidel, 2014-02-25 23:37:08]

lgtm

https://codereview.chromium.org/179993006/diff/20001/LayoutTests/fast/multico...
File LayoutTests/fast/multicol/newmulticol/regular-block-becomes-multicol.html
(right):

https://codereview.chromium.org/179993006/diff/20001/LayoutTests/fast/multico...
LayoutTests/fast/multicol/newmulticol/regular-block-becomes-multicol.html:11:
elm.style.WebkitColumns = 4;
Why do we set WebKitColumsn here?

https://codereview.chromium.org/179993006/diff/20001/LayoutTests/fast/multico...
File LayoutTests/fast/multicol/newmulticol/table-cell.html (right):

https://codereview.chromium.org/179993006/diff/20001/LayoutTests/fast/multico...
LayoutTests/fast/multicol/newmulticol/table-cell.html:14: <div
style="display:table-cell; -webkit-columns:3; columns:3; -webkit-column-gap:0;
column-gap:0; orphans:1; widows:1;">
Why set -webkit-columsn here?

[mstensho (USE GERRIT), 2014-02-25 23:52:47]

https://codereview.chromium.org/179993006/diff/20001/LayoutTests/fast/multico...
File LayoutTests/fast/multicol/newmulticol/regular-block-becomes-multicol.html
(right):

https://codereview.chromium.org/179993006/diff/20001/LayoutTests/fast/multico...
LayoutTests/fast/multicol/newmulticol/regular-block-becomes-multicol.html:11:
elm.style.WebkitColumns = 4;
On 2014/02/25 23:37:09, eseidel wrote:
> Why do we set WebKitColumsn here?

That's still the name of the property in Blink. It's still not unprefixed.
I plan to fix that once the old multicol code is gone.

https://codereview.chromium.org/179993006/diff/20001/LayoutTests/fast/multico...
File LayoutTests/fast/multicol/newmulticol/table-cell.html (right):

https://codereview.chromium.org/179993006/diff/20001/LayoutTests/fast/multico...
LayoutTests/fast/multicol/newmulticol/table-cell.html:14: <div
style="display:table-cell; -webkit-columns:3; columns:3; -webkit-column-gap:0;
column-gap:0; orphans:1; widows:1;">
On 2014/02/25 23:37:09, eseidel wrote:
> Why set -webkit-columsn here?

That's still the name of the property in Blink. It's still not unprefixed.
I plan to fix that once the old multicol code is gone.

[ojan, 2014-02-26 02:56:38]

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

https://codereview.chromium.org/179993006/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderBlockFlow.cpp:179: RenderObject*
RenderBlockFlow::layoutSpecialExcludedChild(bool relayoutChildren,
SubtreeLayoutScope& layoutScope)
Makes me a little sad to add more overrides of this function. I've been hoping
we could get rid of this function. Although, I have no concrete plans on how. I
just think it's messy and confusing.

In either case, I don't actually think it's important enough to block this
patch, especially since I don't have an alternate suggestion. Just voicing my
sadness. :)

[Paweł Hajdan Jr., 2014-02-26 04:39:47]

The CQ bit was unchecked by phajdan.jr@chromium.org

[Paweł Hajdan Jr., 2014-02-26 05:29:38]

The CQ bit was checked by phajdan.jr@chromium.org

[commit-bot: I haz the power, 2014-02-26 05:29:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mstensho@opera.com/179993006/20001

[mstensho (USE GERRIT), 2014-02-26 08:33:00]

On 2014/02/26 02:56:38, ojan wrote:
>
https://codereview.chromium.org/179993006/diff/20001/Source/core/rendering/Re...
> File Source/core/rendering/RenderBlockFlow.cpp (right):
> 
>
https://codereview.chromium.org/179993006/diff/20001/Source/core/rendering/Re...
> Source/core/rendering/RenderBlockFlow.cpp:179: RenderObject*
> RenderBlockFlow::layoutSpecialExcludedChild(bool relayoutChildren,
> SubtreeLayoutScope& layoutScope)
> Makes me a little sad to add more overrides of this function. I've been hoping
> we could get rid of this function. Although, I have no concrete plans on how.
I
> just think it's messy and confusing.
> 
> In either case, I don't actually think it's important enough to block this
> patch, especially since I don't have an alternate suggestion. Just voicing my
> sadness. :)

I hear you. Not to happy about it myself, although this patch doesn't really
introduce it; it just moves it from RenderMultiColumnBlock. I hope to get rid of
it in the future. I plan to submit a patch where the new multicol code no longer
mutates the render object tree during layout, and when that one has landed (will
be part of the column span patch, I think), it should at least be possible to
avoid this "hack". But we still need some way of treating the
RenderMultiColumnFlowThread child specially. It has to be laid out, but it
shouldn't take up any space in its container.

[commit-bot: I haz the power, 2014-02-26 10:47:17]

Change committed as 167891