Align RenderLayer's descendant dependent flags semantics
authorjchaffraix@webkit.org <jchaffraix@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 19 Jun 2012 23:39:16 +0000 (23:39 +0000)
committerjchaffraix@webkit.org <jchaffraix@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 19 Jun 2012 23:39:16 +0000 (23:39 +0000)
https://bugs.webkit.org/show_bug.cgi?id=89241

Reviewed by Simon Fraser.

Refactoring only, covered by existing tests.

This change aligns the naming and implementation of the different descendant
dependent flags. While aligning, I found some bugs (inefficiencies) with how
the visible descendant flags was computed.

* rendering/RenderLayer.cpp:
(WebCore::RenderLayer::setHasVisibleContent):
Changed this method to not take a boolean as every callers was passing 'true'.
Reworked the logic under this assumption.

(WebCore::RenderLayer::dirtyVisibleContentStatus):
Updated after the following renaming.

(WebCore::RenderLayer::dirtyAncestorChainVisibleDescendantStatus):
Renamed this function from dirtyVisibleDescendantStatus to match the hasSelfPaintingLayer
naming and implementation.

(WebCore::RenderLayer::setAncestorChainHasVisibleDescendant):
Added this function to factor the visible descendant setting out of defunct childVisibilityChanged.
Also improved the efficiency of the function by clearing the dirty flag as it goes up (an unnoticed bug).

(WebCore::RenderLayer::addChild):
(WebCore::RenderLayer::removeChild):
Updated to use the new functions lieu of childVisibilityChanged.
* rendering/RenderLayer.h:
(RenderLayer::childVisibilityChanged):
Removed this function as it wasn't adding much and it's a lot more clear to call
dirtyAncestorChainVisibleDescendantStatus / setAncestorChainHasVisibleDescendant.

* rendering/RenderObject.cpp:
(WebCore::RenderObject::styleWillChange):
* rendering/RenderObjectChildList.cpp:
(WebCore::RenderObjectChildList::appendChildNode):
(WebCore::RenderObjectChildList::insertChildNode):
Updated those callers after removing the boolean parameter from setHasVisibleContent.

git-svn-id: http://svn.webkit.org/repository/webkit/trunk@120770 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog
Source/WebCore/rendering/RenderLayer.cpp
Source/WebCore/rendering/RenderLayer.h
Source/WebCore/rendering/RenderObject.cpp
Source/WebCore/rendering/RenderObjectChildList.cpp

index b125082a99e7c31f7f98e776361fdb1599c8698f..df1e419eaf68799e0a6bdf02c023ce633ed35311 100644 (file)
@@ -1,3 +1,47 @@
+2012-06-19  Julien Chaffraix  <jchaffraix@webkit.org>
+
+        Align RenderLayer's descendant dependent flags semantics
+        https://bugs.webkit.org/show_bug.cgi?id=89241
+
+        Reviewed by Simon Fraser.
+
+        Refactoring only, covered by existing tests.
+
+        This change aligns the naming and implementation of the different descendant
+        dependent flags. While aligning, I found some bugs (inefficiencies) with how
+        the visible descendant flags was computed.
+
+        * rendering/RenderLayer.cpp:
+        (WebCore::RenderLayer::setHasVisibleContent):
+        Changed this method to not take a boolean as every callers was passing 'true'.
+        Reworked the logic under this assumption.
+
+        (WebCore::RenderLayer::dirtyVisibleContentStatus):
+        Updated after the following renaming.
+
+        (WebCore::RenderLayer::dirtyAncestorChainVisibleDescendantStatus):
+        Renamed this function from dirtyVisibleDescendantStatus to match the hasSelfPaintingLayer
+        naming and implementation.
+
+        (WebCore::RenderLayer::setAncestorChainHasVisibleDescendant):
+        Added this function to factor the visible descendant setting out of defunct childVisibilityChanged.
+        Also improved the efficiency of the function by clearing the dirty flag as it goes up (an unnoticed bug).
+
+        (WebCore::RenderLayer::addChild):
+        (WebCore::RenderLayer::removeChild):
+        Updated to use the new functions lieu of childVisibilityChanged.
+        * rendering/RenderLayer.h:
+        (RenderLayer::childVisibilityChanged):
+        Removed this function as it wasn't adding much and it's a lot more clear to call
+        dirtyAncestorChainVisibleDescendantStatus / setAncestorChainHasVisibleDescendant.
+
+        * rendering/RenderObject.cpp:
+        (WebCore::RenderObject::styleWillChange):
+        * rendering/RenderObjectChildList.cpp:
+        (WebCore::RenderObjectChildList::appendChildNode):
+        (WebCore::RenderObjectChildList::insertChildNode):
+        Updated those callers after removing the boolean parameter from setHasVisibleContent.
+
 2012-06-19  Andrey Adaikin  <aandrey@chromium.org>
 
         Web Inspector: Extract InjectedScriptBase class from the InjectedScript
index ec3632cb233631590c059a29097194f38a0491a1..94a837800fa03d20e32ea79c445be2232c71ce06 100644 (file)
@@ -632,53 +632,56 @@ void RenderLayer::updatePagination()
     }
 }
 
-void RenderLayer::setHasVisibleContent(bool b)
+void RenderLayer::setHasVisibleContent()
 { 
-    if (m_hasVisibleContent == b && !m_visibleContentStatusDirty)
+    if (m_hasVisibleContent && !m_visibleContentStatusDirty) {
+        ASSERT(!parent() || parent()->hasVisibleDescendant());
         return;
+    }
+
     m_visibleContentStatusDirty = false; 
-    m_hasVisibleContent = b;
-    if (m_hasVisibleContent) {
-        computeRepaintRects();
-        if (!isNormalFlowOnly()) {
-            for (RenderLayer* sc = stackingContext(); sc; sc = sc->stackingContext()) {
-                sc->dirtyZOrderLists();
-                if (sc->hasVisibleContent())
-                    break;
-            }
+    m_hasVisibleContent = true;
+    computeRepaintRects();
+    if (!isNormalFlowOnly()) {
+        // We don't collect invisible layers in z-order lists if we are not in compositing mode.
+        // As we became visible, we need to dirty our stacking contexts ancestors to be properly
+        // collected. FIXME: When compositing, we could skip this dirtying phase.
+        for (RenderLayer* sc = stackingContext(); sc; sc = sc->stackingContext()) {
+            sc->dirtyZOrderLists();
+            if (sc->hasVisibleContent())
+                break;
         }
     }
+
     if (parent())
-        parent()->childVisibilityChanged(m_hasVisibleContent);
+        parent()->setAncestorChainHasVisibleDescendant();
 }
 
 void RenderLayer::dirtyVisibleContentStatus() 
 { 
     m_visibleContentStatusDirty = true; 
     if (parent())
-        parent()->dirtyVisibleDescendantStatus();
+        parent()->dirtyAncestorChainVisibleDescendantStatus();
 }
 
-void RenderLayer::childVisibilityChanged(bool newVisibility) 
-{ 
-    if (m_hasVisibleDescendant == newVisibility || m_visibleDescendantStatusDirty)
-        return;
-    if (newVisibility) {
-        RenderLayer* l = this;
-        while (l && !l->m_visibleDescendantStatusDirty && !l->m_hasVisibleDescendant) {
-            l->m_hasVisibleDescendant = true;
-            l = l->parent();
-        }
-    } else 
-        dirtyVisibleDescendantStatus();
+void RenderLayer::dirtyAncestorChainVisibleDescendantStatus()
+{
+    for (RenderLayer* layer = this; layer; layer = layer->parent()) {
+        if (layer->m_visibleDescendantStatusDirty)
+            break;
+
+        layer->m_visibleDescendantStatusDirty = true;
+    }
 }
 
-void RenderLayer::dirtyVisibleDescendantStatus()
+void RenderLayer::setAncestorChainHasVisibleDescendant()
 {
-    RenderLayer* l = this;
-    while (l && !l->m_visibleDescendantStatusDirty) {
-        l->m_visibleDescendantStatusDirty = true;
-        l = l->parent();
+    for (RenderLayer* layer = this; layer; layer = layer->parent()) {
+        if (!layer->m_visibleDescendantStatusDirty && layer->hasVisibleDescendant())
+            break;
+
+        layer->m_hasVisibleDescendant = true;
+        layer->m_visibleDescendantStatusDirty = false;
     }
 }
 
@@ -1277,7 +1280,7 @@ void RenderLayer::addChild(RenderLayer* child, RenderLayer* beforeChild)
 
     child->updateDescendantDependentFlags();
     if (child->m_hasVisibleContent || child->m_hasVisibleDescendant)
-        childVisibilityChanged(true);
+        setAncestorChainHasVisibleDescendant();
 
     if (child->isSelfPaintingLayer() || child->hasSelfPaintingLayerDescendant())
         setAncestorChainHasSelfPaintingLayerDescendant();
@@ -1320,7 +1323,7 @@ RenderLayer* RenderLayer::removeChild(RenderLayer* oldChild)
     
     oldChild->updateDescendantDependentFlags();
     if (oldChild->m_hasVisibleContent || oldChild->m_hasVisibleDescendant)
-        childVisibilityChanged(false);
+        dirtyAncestorChainVisibleDescendantStatus();
 
     if (oldChild->isSelfPaintingLayer() || oldChild->hasSelfPaintingLayerDescendant())
         dirtyAncestorChainHasSelfPaintingLayerDescendantStatus();
index 5a5e2446d45129895dd08fd34021442d48d3dfa9..28d7e5627c77cf083d4a2c65e80da12b917de7e0 100644 (file)
@@ -441,7 +441,8 @@ public:
     // ditto for hasVisibleDescendant(), see https://bugs.webkit.org/show_bug.cgi?id=71277
     bool hasVisibleContent() const { return m_hasVisibleContent; }
     bool hasVisibleDescendant() const { return m_hasVisibleDescendant; }
-    void setHasVisibleContent(bool);
+
+    void setHasVisibleContent();
     void dirtyVisibleContentStatus();
 
     // FIXME: We should ASSERT(!m_hasSelfPaintingLayerDescendantDirty); here but we hit the same bugs as visible content above.
@@ -782,8 +783,8 @@ private:
     
     void updateScrollableAreaSet(bool hasOverflow);
 
-    void childVisibilityChanged(bool newVisibility);
-    void dirtyVisibleDescendantStatus();
+    void dirtyAncestorChainVisibleDescendantStatus();
+    void setAncestorChainHasVisibleDescendant();
 
     void updateDescendantDependentFlags();
 
index 8037760b1fd77d94b74e8f48f8d51a2a1468fa3d..be90dd6ba4d1ab2e7de9617254954bd926986262 100755 (executable)
@@ -1789,7 +1789,7 @@ void RenderObject::styleWillChange(StyleDifference diff, const RenderStyle* newS
             if (m_style->visibility() != newStyle->visibility()) {
                 if (RenderLayer* l = enclosingLayer()) {
                     if (newStyle->visibility() == VISIBLE)
-                        l->setHasVisibleContent(true);
+                        l->setHasVisibleContent();
                     else if (l->hasVisibleContent() && (this == l->renderer() || l->renderer()->style()->visibility() != VISIBLE)) {
                         l->dirtyVisibleContentStatus();
                         if (diff > StyleDifferenceRepaintLayer)
index f0cf6aff8e75c59f6ffc511a55f056db65b4939c..f545297d99d8da27cf74b323312d65efc8e2f92b 100644 (file)
@@ -194,7 +194,7 @@ void RenderObjectChildList::appendChildNode(RenderObject* owner, RenderObject* n
             if (!layer)
                 layer = owner->enclosingLayer();
             if (layer)
-                layer->setHasVisibleContent(true);
+                layer->setHasVisibleContent();
         }
 
         if (newChild->isListItem())
@@ -261,7 +261,7 @@ void RenderObjectChildList::insertChildNode(RenderObject* owner, RenderObject* c
             if (!layer)
                 layer = owner->enclosingLayer();
             if (layer)
-                layer->setHasVisibleContent(true);
+                layer->setHasVisibleContent();
         }
 
         if (child->isListItem())