Update PaintLayer::needsPaintPhaseXXX flags when add/remove layer on style change

third_party/WebKit/Source/core/paint/PaintLayer.cpp

Index: third_party/WebKit/Source/core/paint/PaintLayer.cpp
diff --git a/third_party/WebKit/Source/core/paint/PaintLayer.cpp b/third_party/WebKit/Source/core/paint/PaintLayer.cpp
index 5eed81877f073cfb7a536b2120991c542dc3b312..f9d52d6067206cbbf207c6b3a00e0875fc7538c9 100644
--- a/third_party/WebKit/Source/core/paint/PaintLayer.cpp
+++ b/third_party/WebKit/Source/core/paint/PaintLayer.cpp
@@ -1198,6 +1198,8 @@ void PaintLayer::addChild(PaintLayer* child, PaintLayer* beforeChild)
     // Non-self-painting children paint into this layer, so the visible contents status of this layer is affected.
     if (!child->isSelfPaintingLayer())
         dirtyVisibleContentStatus();
+    else if (PaintLayer* enclosingSelfPaintingLayer = this->enclosingSelfPaintingLayer())
+        child->mergeNeedsPaintPhaseFlagsFrom(*enclosingSelfPaintingLayer);
 
     dirtyAncestorChainVisibleDescendantStatus();
     dirtyAncestorChainHasSelfPaintingLayerDescendantStatus();
@@ -1247,6 +1249,11 @@ PaintLayer* PaintLayer::removeChild(PaintLayer* oldChild)
     if (oldChild->enclosingPaginationLayer())
         oldChild->clearPaginationRecursive();
 
+    if (oldChild->isSelfPaintingLayer()) {
+        if (PaintLayer* enclosingSelfPaintingLayer = this->enclosingSelfPaintingLayer())

[pdr., 2016/04/08 04:19:00]

enclosingSelfPaintingLayer is O(n) so it looks like this code will introduce an
O(n^2) when tearing down the paint layer tree (or the layout object tree through
LayoutObject::willBeRemovedFromTree). I don't have a good intuition about the
performance of these cases... do you think this is okay in terms of performance?

[Xianzhu, 2016/04/08 18:08:48]

Because except for non-composited non-stacked overflow clipping object and
non-composited non-stacked LayoutParts, all layers are self-painting, so
normally we should find a self-painting layer fast, seldom hit the worst case
O(n).

+            enclosingSelfPaintingLayer->mergeNeedsPaintPhaseFlagsFrom(*oldChild);

[pdr., 2016/04/08 04:19:00]

I'm new to this code... does "removeChild" always remove the child layer and
merge the child's contents into the parent layer? (If so, your codechange here
makes sense.)

[Xianzhu, 2016/04/08 18:08:48]

Good catch. We should avoid the operation when this method is called from
LayoutObject::willBeRemovedFromTree(). Moved the logic into
PaintLayer::removeOnlyThisLayer() which is to handle the case that an object
changes style and remove the existing layer.

[pdr., 2016/04/08 19:46:07]

Because PaintLayer::{insert,remove}OnlyThisLayer() now differs from addChild by
needing to update these flags, what do you think about renaming these functions
to something like PaintLayer::{insert,remove}OnlyThisLayerAfterStyleChange()?
Alternatively, we could just add a comment.

[Xianzhu, 2016/04/08 21:30:55]

Done renaming.

+    }
+
     setNeedsRepaint();
 
     return oldChild;
@@ -2433,8 +2440,18 @@ void PaintLayer::updateSelfPaintingLayer()
 
     m_isSelfPaintingLayer = isSelfPaintingLayer;
 
-    if (parent())
-        parent()->dirtyAncestorChainHasSelfPaintingLayerDescendantStatus();
+    if (PaintLayer* parent = this->parent()) {
+        parent->dirtyAncestorChainHasSelfPaintingLayerDescendantStatus();
+
+        if (PaintLayer* enclosingSelfPaintingLayer = parent->enclosingSelfPaintingLayer()) {

[pdr., 2016/04/08 04:19:00]

Are these codepaths tested? I removed these lines and no tests failed.

[Xianzhu, 2016/04/08 18:08:48]

Added these code because I was afraid there were cases self-painting-layer
change not to cause layout and paint invalidation, but failed to find a test
case. Rethought about this, and it seems that all self-painting-layer status
change will cause at least paint invalidation. For example, on
compositing-status change of a scrolling layer, we'll invalidate all objects on
the layer.

Remove these code.

+            if (isSelfPaintingLayer) {
+                mergeNeedsPaintPhaseFlagsFrom(*enclosingSelfPaintingLayer);
+            } else {
+                enclosingSelfPaintingLayer->mergeNeedsPaintPhaseFlagsFrom(*this);
+                clearNeedsPaintPhaseFlags();
+            }
+        }
+    }

[pdr., 2016/04/08 04:19:00]

Is the call to clearNeedsPaintPhaseFlags needed or is it okay to leave stale
bits on a non-self-painting-layer?

If clearNeedsPaintPhaseFlags is needed, do we need to also call
clearNeedsPaintPhaseFlags in the cases where there isn't a layer parent, and in
cases where there isn't an enclosing self painting layer?

[Xianzhu, 2016/04/08 18:08:48]

Good questions. I think this affects how accurately we keep the bits thus
performance but not a big deal as we already don't clear the bits on
self-painting layers once set. The stale bits on a non-self-painting-layer may
affect the result of line 2448 when the layer becomes self-painting again. Would
like to remove clearNeedsPaintPhaseFlags().

 }
 
 PaintLayer* PaintLayer::enclosingSelfPaintingLayer()