Generalize visible descendant dirty bits to prepare for more properties.

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 732546f8d4907f780ca9d272f37c741b3f9649e8..cdf8f373db5a20caa5d53a6043bff2d57e6620f5 100644
--- a/third_party/WebKit/Source/core/paint/PaintLayer.cpp
+++ b/third_party/WebKit/Source/core/paint/PaintLayer.cpp
@@ -133,7 +133,7 @@ PaintLayer::PaintLayer(LayoutBoxModelObject* layoutObject)
       m_isRootLayer(layoutObject->isLayoutView()),
       m_isVisibleContentDirty(true),
       m_hasVisibleContent(false),
-      m_isVisibleDescendantDirty(false),
+      m_needsDescendantDependentFlagsUpdate(false),
       m_hasVisibleDescendant(false),
 #if DCHECK_IS_ON()
       m_needsPositionUpdate(true),
@@ -311,11 +311,6 @@ void PaintLayer::updateLayerPositionRecursive() {
       scrollAnimator->updateAfterLayout();
   }
 
-  // FIXME: We should be able to remove this call because we don't care about
-  // any descendant-dependent flags, but code somewhere else is reading these
-  // flags and depending on us to update them.
-  updateDescendantDependentFlags();

[chrishtr, 2016/12/01 16:13:37]

This is not needed because it will happen later during the compositing update
step.
I also verified that there are no callers outside of compositing or later in the
lifecycle that need any of these flags. See the attached bug, which links to 
a doc that investigates all call sites.

-
   for (PaintLayer* child = firstChild(); child; child = child->nextSibling())
     child->updateLayerPositionRecursive();
 }
@@ -641,27 +636,18 @@ void PaintLayer::mapRectToPaintInvalidationBacking(
 
 void PaintLayer::dirtyVisibleContentStatus() {
   m_isVisibleContentDirty = true;
-  if (parent())
-    parent()->dirtyAncestorChainVisibleDescendantStatus();
+  markAncestorChainForDescendantDependentFlagsUpdate();

[Xianzhu, 2016/12/01 17:59:32]

This will also set m_needsDescendantDependentFlagsUpdate for |this|. Is this
expected?

[chrishtr, 2016/12/01 18:05:22]

It's intended yes. It's because visible contents updated for |this| in
updateDescendantDependentFlags.

   // Non-self-painting layers paint into their ancestor layer, and count as part
   // of the "visible contents" of the parent, so we need to dirty it.
   if (!isSelfPaintingLayer())
     parent()->dirtyVisibleContentStatus();
 }
 
-void PaintLayer::potentiallyDirtyVisibleContentStatus(EVisibility visibility) {
-  if (m_isVisibleContentDirty)
-    return;
-  if (hasVisibleContent() == (visibility == EVisibility::Visible))

[chrishtr, 2016/12/01 16:13:37]

This is cleanup for what looks like unnecessary complexity. abarth added this
code in this patch: https://codereview.chromium.org/453113002
which was trying to avoid reading dirty state, by adding a
potentiallyDirtyVisibleContentStatus method which bailed if the value was dirty.
The code in LayoutObject that is the call site has since been simplified to
avoid conditionally dirtying visible content state based on current visible
state, so we can now just remove this code and unconditionally dirty the
state in LayoutObject.

-    return;
-  dirtyVisibleContentStatus();
-}
-
-void PaintLayer::dirtyAncestorChainVisibleDescendantStatus() {
+void PaintLayer::markAncestorChainForDescendantDependentFlagsUpdate() {
   for (PaintLayer* layer = this; layer; layer = layer->parent()) {
-    if (layer->m_isVisibleDescendantDirty)
+    if (layer->m_needsDescendantDependentFlagsUpdate)
       break;
-    layer->m_isVisibleDescendantDirty = true;
+    layer->m_needsDescendantDependentFlagsUpdate = true;
   }
 }
 
@@ -694,7 +680,7 @@ void PaintLayer::updateScrollingStateAfterCompositingChange() {
 }
 
 void PaintLayer::updateDescendantDependentFlags() {
-  if (m_isVisibleDescendantDirty) {
+  if (m_needsDescendantDependentFlagsUpdate) {
     m_hasVisibleDescendant = false;
 
     for (PaintLayer* child = firstChild(); child;
@@ -705,7 +691,7 @@ void PaintLayer::updateDescendantDependentFlags() {
         m_hasVisibleDescendant = true;
     }
 
-    m_isVisibleDescendantDirty = false;
+    m_needsDescendantDependentFlagsUpdate = false;
   }
 
   if (m_isVisibleContentDirty) {
@@ -1277,12 +1263,10 @@ void PaintLayer::addChild(PaintLayer* child, PaintLayer* beforeChild) {
   if (!child->isSelfPaintingLayer())
     dirtyVisibleContentStatus();
 
-  dirtyAncestorChainVisibleDescendantStatus();
+  markAncestorChainForDescendantDependentFlagsUpdate();
   dirtyAncestorChainHasSelfPaintingLayerDescendantStatus();
 
   child->setNeedsRepaint();
-
-  child->updateDescendantDependentFlags();

[chrishtr, 2016/12/01 16:13:37]

Same here - not necessary.

 }
 
 PaintLayer* PaintLayer::removeChild(PaintLayer* oldChild) {
@@ -1321,10 +1305,8 @@ PaintLayer* PaintLayer::removeChild(PaintLayer* oldChild) {
 
   dirtyAncestorChainHasSelfPaintingLayerDescendantStatus();
 
-  oldChild->updateDescendantDependentFlags();
-
   if (oldChild->m_hasVisibleContent || oldChild->m_hasVisibleDescendant)
-    dirtyAncestorChainVisibleDescendantStatus();
+    markAncestorChainForDescendantDependentFlagsUpdate();
 
   if (oldChild->enclosingPaginationLayer())
     oldChild->clearPaginationRecursive();
@@ -3006,8 +2988,6 @@ void PaintLayer::styleDidChange(StyleDifference diff,
   // to recompute the bit once scrollbars have been updated.
   updateSelfPaintingLayer();
 
-  updateDescendantDependentFlags();

[chrishtr, 2016/12/01 16:13:37]

Same here - not necessary.

-
   updateTransform(oldStyle, layoutObject()->styleRef());
   updateFilters(oldStyle, layoutObject()->styleRef());
   updateClipPath(oldStyle, layoutObject()->styleRef());