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

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

 /*
  * Copyright (C) 2006, 2007, 2008, 2009, 2010, 2011, 2012 Apple Inc. All rights reserved.
  *
  * Portions are Copyright (C) 1998 Netscape Communications Corporation.
  *
  * Other contributors:
  *   Robert O'Callahan <roc+@cs.cmu.edu>
  *   David Baron <dbaron@fas.harvard.edu>
  *   Christian Biesinger <cbiesinger@web.de>
  *   Randall Jesup <rjesup@wgate.com>
@@ skipping 1180 matching lines @@
     if (child->stackingNode()->isStacked() || child->firstChild()) {
         // Dirty the z-order list in which we are contained. The ancestorStackingContextNode() can be null in the
         // case where we're building up generated content layers. This is ok, since the lists will start
         // off dirty in that case anyway.
         child->stackingNode()->dirtyStackingContextZOrderLists();
     }
 
     // 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();
 
     child->setNeedsRepaint();
 
     child->updateDescendantDependentFlags();
 }
 
 PaintLayer* PaintLayer::removeChild(PaintLayer* oldChild)
@@ skipping 29 matching lines @@
     dirtyAncestorChainHasSelfPaintingLayerDescendantStatus();
 
     oldChild->updateDescendantDependentFlags();
 
     if (oldChild->m_hasVisibleContent || oldChild->m_hasVisibleDescendant)
         dirtyAncestorChainVisibleDescendantStatus();
 
     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;
 }
 
 void PaintLayer::removeOnlyThisLayer()
 {
     if (!m_parent)
         return;
 
@@ skipping 1166 matching lines @@
 }
 
 void PaintLayer::updateSelfPaintingLayer()
 {
     bool isSelfPaintingLayer = shouldBeSelfPaintingLayer();
     if (this->isSelfPaintingLayer() == isSelfPaintingLayer)
         return;
 
     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()
 {
     PaintLayer* layer = this;
     while (layer && !layer->isSelfPaintingLayer())
         layer = layer->parent();
     return layer;
 }
 
@@ skipping 430 matching lines @@
 
 void showLayerTree(const blink::LayoutObject* layoutObject)
 {
     if (!layoutObject) {
         fprintf(stderr, "Cannot showLayerTree. Root is (nil)\n");
         return;
     }
     showLayerTree(layoutObject->enclosingLayer());
 }
 #endif