LayoutObject::paintingLayer() to correctly handle column-span:all

LayoutObject::paintingLayer() to correctly handle column-span:all

When finding the layer painting an object, we should traverse up the tree
through spanner placeholder when meeting a column-span:all object.

BUG=609720
TEST=fast/multicol/span/remove-span-all-under-layer.html

Committed: https://crrev.com/8895ae9799f7de65a747c97b6f2442d83717463d
Cr-Commit-Position: refs/heads/master@{#392409}

[mstensho (USE GERRIT), 2016-05-06 19:59:37]

mstensho@opera.com changed reviewers:
+ mstensho@opera.com

[mstensho (USE GERRIT), 2016-05-06 19:59:38]

Spanners... the Knights of Blink Chess.

lgtm with the suggested test reliability improvement.

https://codereview.chromium.org/1955073002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/multicol/span/remove-span-all-under-layer.html
(right):

https://codereview.chromium.org/1955073002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/multicol/span/remove-span-all-under-layer.html:2:
<div style="-webkit-columns:2;">
Optional nit: no need to -webkit- prefix this or column-span anymore.

https://codereview.chromium.org/1955073002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/multicol/span/remove-span-all-under-layer.html:10:
onload = function() {
Probably safer to put "document.body.offsetTop;" in front of the display style
change, to be sure that we have laid out at that point. There should be no need
to wrap it inside an onload handler, though.

[Xianzhu, 2016-05-06 20:59:37]

Description was changed from

==========
Correct enclosingLayer() for column-span:all

The enclosingLayer() column-span:all should be the enclosingLayer()
of its place holder, instead of the enclosingLayer() in its normal
layout tree.

BUG=609720
TEST=fast/multicol/span/remove-span-all-under-layer.html
==========

to

==========
Correct enclosingLayer() for column-span:all

The enclosingLayer() column-span:all should be the enclosingLayer()
of its place holder, instead of the enclosingLayer() in its normal
layout tree hierarchy.

BUG=609720
TEST=fast/multicol/span/remove-span-all-under-layer.html
==========

[Xianzhu, 2016-05-06 21:00:03]

Description was changed from

==========
Correct enclosingLayer() for column-span:all

The enclosingLayer() column-span:all should be the enclosingLayer()
of its place holder, instead of the enclosingLayer() in its normal
layout tree hierarchy.

BUG=609720
TEST=fast/multicol/span/remove-span-all-under-layer.html
==========

to

==========
Correct enclosingLayer() for column-span:all

The enclosingLayer() column-span:all should be the enclosingLayer()
of its place holder, instead of the enclosingLayer() in layout tree
hierarchy.

BUG=609720
TEST=fast/multicol/span/remove-span-all-under-layer.html
==========

[Xianzhu, 2016-05-07 21:02:25]

Changing enclosingLayer() breaks existing code manipulating PaintLayer tree
structure, e.g. when LayoutObject::willBeRemovedFromTree() calling
LayoutObject::removeLayers().

The new patch set adds LayoutObject::paintingLayer() to get the layer that will
paint the object. Ptal.

[Xianzhu, 2016-05-07 21:04:49]

Description was changed from

==========
Correct enclosingLayer() for column-span:all

The enclosingLayer() column-span:all should be the enclosingLayer()
of its place holder, instead of the enclosingLayer() in layout tree
hierarchy.

BUG=609720
TEST=fast/multicol/span/remove-span-all-under-layer.html
==========

to

==========
LayoutObject::paintingLayer() to correctly handle column-span:all

When finding the layer painting an object, we should traverse up the tree
through spanner placeholder when meeting a column-span:all object. 

BUG=609720
TEST=fast/multicol/span/remove-span-all-under-layer.html
==========

[mstensho (USE GERRIT), 2016-05-09 08:20:42]

lgtm, with suggestions.

https://codereview.chromium.org/1955073002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/1955073002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:629: for (const
LayoutObject* current = this; current; current = current->isColumnSpanAll() ?
current->spannerPlaceholder() : current->parent()) {
To make it look slightly less special, we could just do
current->containingBlock() or (container() if you prefer that) if it's a
spanner, since the spanner placeholder never establishes a layer. That's what
the assertEnclosingSelfPaintingLayerHasSetNeedsRepaint() function used to do,
FWIW.

https://codereview.chromium.org/1955073002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:1276:
assertPaintingLayerHasSetNeedsRepaint();
Might consider just doing ASSERT(!paintingLayer() ||
paintingLayer()->needsRepaint()) instead when you want to check for this, to
eliminate the need for assertPaintingLayerHasSetNeedsRepaint() and the #if
ENABLE(ASSERT) stuff, but I'm fine either way.

[Xianzhu, 2016-05-09 17:14:20]

https://codereview.chromium.org/1955073002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/1955073002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:629: for (const
LayoutObject* current = this; current; current = current->isColumnSpanAll() ?
current->spannerPlaceholder() : current->parent()) {
On 2016/05/09 08:20:42, mstensho wrote:
> To make it look slightly less special, we could just do
> current->containingBlock() or (container() if you prefer that) if it's a
> spanner, since the spanner placeholder never establishes a layer. That's what
> the assertEnclosingSelfPaintingLayerHasSetNeedsRepaint() function used to do,
> FWIW.

Done.

https://codereview.chromium.org/1955073002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:1276:
assertPaintingLayerHasSetNeedsRepaint();
On 2016/05/09 08:20:42, mstensho wrote:
> Might consider just doing ASSERT(!paintingLayer() ||
> paintingLayer()->needsRepaint()) instead when you want to check for this, to
> eliminate the need for assertPaintingLayerHasSetNeedsRepaint() and the #if
> ENABLE(ASSERT) stuff, but I'm fine either way.

Acknowledged. Done.

[Xianzhu, 2016-05-09 17:14:28]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2016-05-09 17:14:29]

The patchset sent to the CQ was uploaded after l-g-t-m from mstensho@opera.com
Link to the patchset: https://codereview.chromium.org/1955073002/#ps40001
(title: " ")

[commit-bot: I haz the power, 2016-05-09 17:14:52]

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

[commit-bot: I haz the power, 2016-05-09 20:15:23]

Description was changed from

==========
LayoutObject::paintingLayer() to correctly handle column-span:all

When finding the layer painting an object, we should traverse up the tree
through spanner placeholder when meeting a column-span:all object. 

BUG=609720
TEST=fast/multicol/span/remove-span-all-under-layer.html
==========

to

==========
LayoutObject::paintingLayer() to correctly handle column-span:all

When finding the layer painting an object, we should traverse up the tree
through spanner placeholder when meeting a column-span:all object. 

BUG=609720
TEST=fast/multicol/span/remove-span-all-under-layer.html
==========

[commit-bot: I haz the power, 2016-05-09 20:15:23]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-05-09 20:16:13]

Description was changed from

==========
LayoutObject::paintingLayer() to correctly handle column-span:all

When finding the layer painting an object, we should traverse up the tree
through spanner placeholder when meeting a column-span:all object. 

BUG=609720
TEST=fast/multicol/span/remove-span-all-under-layer.html
==========

to

==========
LayoutObject::paintingLayer() to correctly handle column-span:all

When finding the layer painting an object, we should traverse up the tree
through spanner placeholder when meeting a column-span:all object.

BUG=609720
TEST=fast/multicol/span/remove-span-all-under-layer.html

Committed: https://crrev.com/8895ae9799f7de65a747c97b6f2442d83717463d
Cr-Commit-Position: refs/heads/master@{#392409}
==========

[commit-bot: I haz the power, 2016-05-09 20:16:14]

Patchset 3 (id:??) landed as
https://crrev.com/8895ae9799f7de65a747c97b6f2442d83717463d
Cr-Commit-Position: refs/heads/master@{#392409}