Use LayoutObject to locate the flow thread.

Use LayoutObject to locate the flow thread.

No need for (poorly) duplicated efforts in DeprecatedPaintLayer.

Cleaned up the code a little while I was here. One of the transform checks was
getting in the way, so I removed them both. They did nothing useful anyway.
The flow thread (i.e. pagination layer) itself can never be transformed.

Also removed the fast-path completely, since most layers don't use it.

When all this was gone, it became obvious that we should just fold
updatePagination() into updatePaginationRecursive().

What really went wrong here was that we failed to re-establish the layers when
evacuating a flow thread (which happens when a multicol container ceases to be
one), so we just kept what we had, and the flow thread got deleted without any
of the decendant layers noticing (so that we were pointing to dead enclosing
pagination layers). There's code in LayoutBoxModelObject::moveChildTo() that
normally handles such things (notifications that involve re-establishing the
layers), but only for LayoutInline and LayoutBlock. In this case we were
dealing with a LayoutSVGRoot, which is LayoutReplaced. There could be other
problems with SVG with layers inside because of this, but in this bug, the
correct fix is to just disallow pagination-awareness inside SVG, which
LayoutObject::locateFlowThreadContainingBlock() already does for us, if we but
just bother to invoke it.

BUG=507992
R=chrishtr@chromium.org

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

[chrishtr, 2015-07-14 18:47:57]

I can't access the attached bug?

[chrishtr, 2015-07-14 18:54:01]

https://codereview.chromium.org/1235133004/diff/1/Source/core/paint/Deprecate...
File Source/core/paint/DeprecatedPaintLayer.cpp (right):

https://codereview.chromium.org/1235133004/diff/1/Source/core/paint/Deprecate...
Source/core/paint/DeprecatedPaintLayer.cpp:482: if
(!m_stackingNode->isTreatedAsStackingContextForPainting() &&
!layoutObject()->isColumnSpanAll()) {
Is this fast path needed for correctness?

Also, you mention the flow thread cannot be transformed. Is there an explicit
layout test that it cannot?

[mstensho (USE GERRIT), 2015-07-14 19:13:39]

You have access to the bug now. Sorry that I forgot it in the first place.

https://codereview.chromium.org/1235133004/diff/1/Source/core/paint/Deprecate...
File Source/core/paint/DeprecatedPaintLayer.cpp (right):

https://codereview.chromium.org/1235133004/diff/1/Source/core/paint/Deprecate...
Source/core/paint/DeprecatedPaintLayer.cpp:482: if
(!m_stackingNode->isTreatedAsStackingContextForPainting() &&
!layoutObject()->isColumnSpanAll()) {
On 2015/07/14 18:54:01, chrishtr wrote:
> Is this fast path needed for correctness?

No, I don't think so. I really hope not. :) I realize now that we actually hit
the fast path very rarely, isTreatedAsStackingContextForPainting() meant
something other than what I thought (it includes anything non-statically
positioned). Maybe just kill it, then?
 
> Also, you mention the flow thread cannot be transformed. Is there an explicit
> layout test that it cannot?

A flow thread is an anonymous layout object, and there's no way CSS can specify
a transform on it.

[chrishtr, 2015-07-14 19:46:44]

https://codereview.chromium.org/1235133004/diff/1/Source/core/paint/Deprecate...
File Source/core/paint/DeprecatedPaintLayer.cpp (right):

https://codereview.chromium.org/1235133004/diff/1/Source/core/paint/Deprecate...
Source/core/paint/DeprecatedPaintLayer.cpp:482: if
(!m_stackingNode->isTreatedAsStackingContextForPainting() &&
!layoutObject()->isColumnSpanAll()) {
On 2015/07/14 at 19:13:39, mstensho wrote:
> On 2015/07/14 18:54:01, chrishtr wrote:
> > Is this fast path needed for correctness?
> 
> No, I don't think so. I really hope not. :) I realize now that we actually hit
the fast path very rarely, isTreatedAsStackingContextForPainting() meant
something other than what I thought (it includes anything non-statically
positioned). Maybe just kill it, then?

Let's kill it.
>  
> > Also, you mention the flow thread cannot be transformed. Is there an
explicit
> > layout test that it cannot?
> 
> A flow thread is an anonymous layout object, and there's no way CSS can
specify a transform on it.

Ah right.

[mstensho (USE GERRIT), 2015-07-14 19:52:34]

https://codereview.chromium.org/1235133004/diff/1/Source/core/paint/Deprecate...
File Source/core/paint/DeprecatedPaintLayer.cpp (right):

https://codereview.chromium.org/1235133004/diff/1/Source/core/paint/Deprecate...
Source/core/paint/DeprecatedPaintLayer.cpp:482: if
(!m_stackingNode->isTreatedAsStackingContextForPainting() &&
!layoutObject()->isColumnSpanAll()) {
On 2015/07/14 19:46:44, chrishtr wrote:
> On 2015/07/14 at 19:13:39, mstensho wrote:
> > On 2015/07/14 18:54:01, chrishtr wrote:
> > > Is this fast path needed for correctness?
> > 
> > No, I don't think so. I really hope not. :) I realize now that we actually
hit
> the fast path very rarely, isTreatedAsStackingContextForPainting() meant
> something other than what I thought (it includes anything non-statically
> positioned). Maybe just kill it, then?
> 
> Let's kill it.
> >  
> > > Also, you mention the flow thread cannot be transformed. Is there an
> explicit
> > > layout test that it cannot?
> > 
> > A flow thread is an anonymous layout object, and there's no way CSS can
> specify a transform on it.
> 
> Ah right.

Done.

[chrishtr, 2015-07-14 19:57:43]

https://codereview.chromium.org/1235133004/diff/20001/Source/core/paint/Depre...
File Source/core/paint/DeprecatedPaintLayer.cpp (right):

https://codereview.chromium.org/1235133004/diff/20001/Source/core/paint/Depre...
Source/core/paint/DeprecatedPaintLayer.cpp:481: m_enclosingPaginationLayer =
containingFlowThread->layer();
Should there be code that nulls out m_enclosingPaginationLayer when pagination
goes away?

[mstensho (USE GERRIT), 2015-07-14 20:05:32]

https://codereview.chromium.org/1235133004/diff/20001/Source/core/paint/Depre...
File Source/core/paint/DeprecatedPaintLayer.cpp (right):

https://codereview.chromium.org/1235133004/diff/20001/Source/core/paint/Depre...
Source/core/paint/DeprecatedPaintLayer.cpp:481: m_enclosingPaginationLayer =
containingFlowThread->layer();
On 2015/07/14 19:57:43, chrishtr wrote:
> Should there be code that nulls out m_enclosingPaginationLayer when pagination
> goes away?

That shouldn't be necessary, since updatePaginationRecursive() (its only caller)
does it.

[chrishtr, 2015-07-14 20:11:08]

https://codereview.chromium.org/1235133004/diff/20001/Source/core/paint/Depre...
File Source/core/paint/DeprecatedPaintLayer.cpp (right):

https://codereview.chromium.org/1235133004/diff/20001/Source/core/paint/Depre...
Source/core/paint/DeprecatedPaintLayer.cpp:458: updatePagination();
I think you can blow away DeprecatedPaintLayer::updatePagination and just put
some code like this here:

m_enclosingPaginationLayer = layoutObject()->flowThreadContainingBlock()

(The code at lines 466-477 look redundant with
LayoutObject::flowThreadContainingBlock, so those can go.)

[mstensho (USE GERRIT), 2015-07-14 20:19:00]

On 2015/07/14 20:11:08, chrishtr wrote:
>
https://codereview.chromium.org/1235133004/diff/20001/Source/core/paint/Depre...
> File Source/core/paint/DeprecatedPaintLayer.cpp (right):
> 
>
https://codereview.chromium.org/1235133004/diff/20001/Source/core/paint/Depre...
> Source/core/paint/DeprecatedPaintLayer.cpp:458: updatePagination();
> I think you can blow away DeprecatedPaintLayer::updatePagination and just put
> some code like this here:
> 
> m_enclosingPaginationLayer = layoutObject()->flowThreadContainingBlock()
> 
> (The code at lines 466-477 look redundant with
> LayoutObject::flowThreadContainingBlock, so those can go.)

Great idea! Done.

[chrishtr, 2015-07-14 20:21:20]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2015-07-14 20:21:20]

lgtm

[commit-bot: I haz the power, 2015-07-14 20:21:33]

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

[commit-bot: I haz the power, 2015-07-14 22:18:37]

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