[New Multicolumn] Handle overflow inside spanners correctly.

[New Multicolumn] Handle overflow inside spanners correctly.

BUG=347325
R=jchaffraix@chromium.org

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

[Julien - ping for review, 2015-01-20 14:07:39]

lgtm

https://codereview.chromium.org/803213004/diff/1/LayoutTests/fast/multicol/sp...
File LayoutTests/fast/multicol/span/overflow-on-viewport.html (right):

https://codereview.chromium.org/803213004/diff/1/LayoutTests/fast/multicol/sp...
LayoutTests/fast/multicol/span/overflow-on-viewport.html:7: <p
style="position:fixed;">You should see the word 'PASS' below.</p>
Again I would like to see a *description* of what you're testing in these tests.
That makes it easier to triage a potential regression as well as help people
reading the test to understand the intent.

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

https://codereview.chromium.org/803213004/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderMultiColumnSpannerPlaceholder.cpp:59: // container
and beyond.
Nit: Let's balance the comment better over the 2 lines.

https://codereview.chromium.org/803213004/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderMultiColumnSpannerPlaceholder.cpp:62:
addLayoutOverflow(m_rendererInFlowThread->layoutOverflowRect());
I would recommend moving this code to an override of computeOverflow() and call
it here.

[mstensho (USE GERRIT), 2015-01-20 16:31:57]

https://codereview.chromium.org/803213004/diff/1/LayoutTests/fast/multicol/sp...
File LayoutTests/fast/multicol/span/overflow-on-viewport.html (right):

https://codereview.chromium.org/803213004/diff/1/LayoutTests/fast/multicol/sp...
LayoutTests/fast/multicol/span/overflow-on-viewport.html:7: <p
style="position:fixed;">You should see the word 'PASS' below.</p>
On 2015/01/20 14:07:38, Julien Chaffraix - CET wrote:
> Again I would like to see a *description* of what you're testing in these
tests.
> That makes it easier to triage a potential regression as well as help people
> reading the test to understand the intent.

Done.

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

https://codereview.chromium.org/803213004/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderMultiColumnSpannerPlaceholder.cpp:59: // container
and beyond.
On 2015/01/20 14:07:38, Julien Chaffraix - CET wrote:
> Nit: Let's balance the comment better over the 2 lines.

This was what I got from emacs. :-P

Done.

https://codereview.chromium.org/803213004/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderMultiColumnSpannerPlaceholder.cpp:62:
addLayoutOverflow(m_rendererInFlowThread->layoutOverflowRect());
On 2015/01/20 14:07:38, Julien Chaffraix - CET wrote:
> I would recommend moving this code to an override of computeOverflow() and
call
> it here.

Contrary to what I actually think I told you yesterday, that the spanner
placeholder is a RenderBlockFlow, it's *not*. Actually it's just a RenderBox
(because it doesn't need to be more than that). So I can't do that, since
computeOverflow() needs RenderBlock or better.

So I can't do this.

[mstensho (USE GERRIT), 2015-01-20 16:32:29]

The CQ bit was checked by mstensho@opera.com

[commit-bot: I haz the power, 2015-01-20 16:33:22]

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

[commit-bot: I haz the power, 2015-01-20 17:43:20]

Committed patchset #2 (id:20001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=188681