[New Multicolumn] Intrinsic/preferred logical width calculation for spanners.

[New Multicolumn] Intrinsic/preferred logical width calculation for spanners.

Need to skip multicol adjustment of preferred widths when it comes to spanners.
Their preferred widths should not be multiplied by the number of columns.
Conversely, when encountering column-span:all renderers somewhere inside flow
threads, for preferred width calculation, skip them, since they're not column
content.

When a multicol container asks a child spanner placeholder about its preferred
logical width, relay to the actual column-span:all renderer.

BUG=347325
R=jchaffraix@chromium.org

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

[Julien - ping for review, 2015-01-26 14:50:13]

https://codereview.chromium.org/879483002/diff/1/LayoutTests/fast/multicol/dy...
File
LayoutTests/fast/multicol/dynamic/insert-spanner-into-stf-constrained-width.html
(right):

https://codereview.chromium.org/879483002/diff/1/LayoutTests/fast/multicol/dy...
LayoutTests/fast/multicol/dynamic/insert-spanner-into-stf-constrained-width.html:13:
<div id="stf" style="font:20px Ahem; float:left; overflow:hidden; color:blue;
background:blue;" data-expected-width="100" data-expected-height="100">
Do we need float + width: 0 to re-implement width: -webkit-min-content?

https://codereview.chromium.org/879483002/diff/1/LayoutTests/fast/multicol/dy...
File
LayoutTests/fast/multicol/dynamic/insert-spanner-into-stf-unconstrained-width.html
(right):

https://codereview.chromium.org/879483002/diff/1/LayoutTests/fast/multicol/dy...
LayoutTests/fast/multicol/dynamic/insert-spanner-into-stf-unconstrained-width.html:13:
<div id="stf" style="font:20px Ahem; float:left; overflow:hidden; color:blue;
background:blue;" data-expected-width="220" data-expected-height="220">
Same question here with width: -webkit-max-content?

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

https://codereview.chromium.org/879483002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBlock.cpp:3091: if (child->isRenderFlowThread()) {
This looks nasty. Why don't we patch the flow thread's preferred logical widths
computation to do the following? (RenderBlockLineLayout has some similar code so
it's likely broken after this change)

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

https://codereview.chromium.org/879483002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderMultiColumnSpannerPlaceholder.cpp:64: }
I am not sure I understand why we need to do this. The flow thread is a direct
child of the multi-column block so we already get this information.

[mstensho (USE GERRIT), 2015-01-26 16:45:16]

https://codereview.chromium.org/879483002/diff/1/LayoutTests/fast/multicol/dy...
File
LayoutTests/fast/multicol/dynamic/insert-spanner-into-stf-constrained-width.html
(right):

https://codereview.chromium.org/879483002/diff/1/LayoutTests/fast/multicol/dy...
LayoutTests/fast/multicol/dynamic/insert-spanner-into-stf-constrained-width.html:13:
<div id="stf" style="font:20px Ahem; float:left; overflow:hidden; color:blue;
background:blue;" data-expected-width="100" data-expected-height="100">
On 2015/01/26 14:50:12, Julien Chaffraix - CET wrote:
> Do we need float + width: 0 to re-implement width: -webkit-min-content?

Done.

https://codereview.chromium.org/879483002/diff/1/LayoutTests/fast/multicol/dy...
File
LayoutTests/fast/multicol/dynamic/insert-spanner-into-stf-unconstrained-width.html
(right):

https://codereview.chromium.org/879483002/diff/1/LayoutTests/fast/multicol/dy...
LayoutTests/fast/multicol/dynamic/insert-spanner-into-stf-unconstrained-width.html:13:
<div id="stf" style="font:20px Ahem; float:left; overflow:hidden; color:blue;
background:blue;" data-expected-width="220" data-expected-height="220">
On 2015/01/26 14:50:12, Julien Chaffraix - CET wrote:
> Same question here with width: -webkit-max-content?

Done.

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

https://codereview.chromium.org/879483002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBlock.cpp:3091: if (child->isRenderFlowThread()) {
On 2015/01/26 14:50:13, Julien Chaffraix - CET wrote:
> This looks nasty. Why don't we patch the flow thread's preferred logical
widths
> computation to do the following?

I did this because I felt that providing the preferred widths in "multicol
container dimensions" would be bad, since everything in the flow thread is
supposed to have the same size as one column. It's inconsistent if
logicalWidth() is for one column, while minPreferredLogicalWidth() is for the
entire multicol container.

> (RenderBlockLineLayout has some similar code so
> it's likely broken after this change)

A RenderFlowThread is never inline (aka. a multicol container always has block
children), so that's not an issue.

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

https://codereview.chromium.org/879483002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderMultiColumnSpannerPlaceholder.cpp:64: }
On 2015/01/26 14:50:13, Julien Chaffraix - CET wrote:
> I am not sure I understand why we need to do this. The flow thread is a direct
> child of the multi-column block so we already get this information.

That's "renderer*IN*FlowThread", i.e. the column-span:all thingy. I've been
wondering about the greatness of this name myself...

[Julien - ping for review, 2015-01-27 09:31:37]

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

https://codereview.chromium.org/879483002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderMultiColumnSpannerPlaceholder.cpp:64: }
On 2015/01/26 16:45:15, mstensho wrote:
> On 2015/01/26 14:50:13, Julien Chaffraix - CET wrote:
> > I am not sure I understand why we need to do this. The flow thread is a
direct
> > child of the multi-column block so we already get this information.
> 
> That's "renderer*IN*FlowThread", i.e. the column-span:all thingy. I've been
> wondering about the greatness of this name myself...

I can't think of a better name actually but it is confusing indeed.

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

https://codereview.chromium.org/879483002/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderBlock.cpp:2959: // The new ("region based") multicol
implementation will do this adjustment on the flow thread,
Let's remove the use of 'new' as we will want to switch soonish.

[mstensho (USE GERRIT), 2015-01-27 10:20:23]

I stole code from RenderBlock::adjustIntrinsicLogicalWidthsForColumns() and put
it in RenderMultiColumnFlowThread::computePreferredLogicalWidths() (I could have
just modified adjustIntrinsicLogicalWidthsForColumns() instead to make it work
in both implementations, but it would require some ifs and pointers). The
preferred min/max widths stored in and returned from RenderMultiColumnFlowThread
are now for the multicol container, not for columns.

Works just as well as my previous proposal, or actually, slightly better, but
insignificantly so: RenderRegion::computeIntrinsicLogicalWidths() is implemented
by asking the flow thread. It would previously return the preferred column
widths (incorrect), but would now return the preferred multicol content width
(correct). Doesn't really matter in practice, though. And I think this method
should just go away.

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

https://codereview.chromium.org/879483002/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderBlock.cpp:2959: // The new ("region based") multicol
implementation will do this adjustment on the flow thread,
On 2015/01/27 09:31:37, Julien Chaffraix - CET wrote:
> Let's remove the use of 'new' as we will want to switch soonish.

Done.

[Julien - ping for review, 2015-01-27 11:02:44]

lgtm

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

https://codereview.chromium.org/879483002/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderMultiColumnFlowThread.cpp:590: RenderBlockFlow*
multicolContainer = multiColumnBlockFlow();
const RenderBlockFlow* (you're not supposed to modify it)

https://codereview.chromium.org/879483002/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderMultiColumnFlowThread.cpp:605: // resolved
column-count really should be 1.
In the end, is that really a FIXME? (seems like the rhetoric is that it's bad
but it's fine)

[mstensho (USE GERRIT), 2015-01-27 11:54:42]

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

https://codereview.chromium.org/879483002/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderMultiColumnFlowThread.cpp:590: RenderBlockFlow*
multicolContainer = multiColumnBlockFlow();
On 2015/01/27 11:02:43, Julien Chaffraix - CET wrote:
> const RenderBlockFlow* (you're not supposed to modify it)

Done.

https://codereview.chromium.org/879483002/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderMultiColumnFlowThread.cpp:605: // resolved
column-count really should be 1.
On 2015/01/27 11:02:43, Julien Chaffraix - CET wrote:
> In the end, is that really a FIXME? (seems like the rhetoric is that it's bad
> but it's fine)

Done.

[mstensho (USE GERRIT), 2015-01-27 11:55:09]

The CQ bit was checked by mstensho@opera.com

[commit-bot: I haz the power, 2015-01-27 11:56:15]

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

[commit-bot: I haz the power, 2015-01-27 13:27:54]

Committed patchset #5 (id:80001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=189030