Attachment 139571 Details for Bug 84920 – Proposed refactoring 2: taking comments into account.

[patch] Proposed refactoring 2: taking comments into account.

bug-84920-20120430190108.patch (text/plain), 12.63 KB, created by Julien Chaffraix on 2012-04-30 19:01:09 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Julien Chaffraix

Created: 2012-04-30 19:01:09 PDT

Size: 12.63 KB

patch

obsolete

>Subversion Revision: 115695
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 9082ce27b882b20ef3a185eb0614831037c8c3cf..ded270e0ee03678bdfe7a99f32b66350ba268473 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,62 @@
>+2012-04-30  Julien Chaffraix  <jchaffraix@webkit.org>
>+
>+        Add ASSERTs to avoid querying dirtied z-index or normal flow lists on RenderLayer
>+        https://bugs.webkit.org/show_bug.cgi?id=84920
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Covered by existing tests in Debug (at least several time!).
>+
>+        This change adds some ASSERTs on RenderLayer that prevent any use of its lists if they
>+        are dirtied.
>+
>+        We didn't change how non-stacking context behaves (clearing the z-index lists when dirtying)
>+        as it is more secure than lazily clearing them when updateZOrderLists is called.
>+
>+        * rendering/RenderLayer.cpp:
>+        (WebCore::RenderLayer::dirtyZOrderLists):
>+        Added a comment as to why we can't ASSERT that we are in a stacking context here.
>+
>+        (WebCore::RenderLayer::rebuildZOrderLists):
>+        Added an ASSERT that we only rebuild z-index lists for dirtied stacking context.
>+
>+        (WebCore::RenderLayer::updateLayerListsIfNeeded):
>+        Updated to ensure that the reflection layer has its layers updated too. This was triggering
>+        the new ASSERTs on fast/runins/run-in-layer-not-removed-crash.html.
>+
>+        (WebCore::RenderLayer::updateCompositingAndLayerListsIfNeeded):
>+        Updated to use the new isDirtyStackingContext function.
>+
>+        * rendering/RenderLayer.h:
>+        (WebCore::RenderLayer::isDirtyStackingContext):
>+        New helper function. Also made updateLayerListsIfNeeded() the only way
>+        to update layer. That should prevent any misuse.
>+
>+        (WebCore::RenderLayer::posZOrderList):
>+        (WebCore::RenderLayer::negZOrderList):
>+        (WebCore::RenderLayer::normalFlowList):
>+        ASSERT that we don't query any of the previous lists if they are dirty.
>+
>+        (WebCore::RenderLayer::updateZOrderLists):
>+        Updated to clear the dirty flag for dirty non-stacking context as we properly
>+        clear them when dirtyZOrderLists() is called.
>+
>+        * rendering/RenderLayerCompositor.cpp:
>+        (WebCore::RenderLayerCompositor::addToOverlapMapRecursive):
>+        (WebCore::RenderLayerCompositor::computeCompositingRequirements):
>+        (WebCore::RenderLayerCompositor::rebuildCompositingLayerTree):
>+        Removed the explicit ASSERTs.
>+
>+        (WebCore::RenderLayerCompositor::updateLayerTreeGeometry):
>+        (WebCore::RenderLayerCompositor::canBeComposited):
>+        Disabled compositing on RenderLayer in flow thread. Because flow thread's
>+        RenderLayer are not collected as part of RenderLayer's lists and could be composited,
>+        this was causing the new ASSERTs to trigger (e.g. on fast/regions/webkit-flow-renderer-layer.html).
>+
>+        * rendering/RenderTreeAsText.cpp:
>+        (WebCore::writeLayers):
>+        Updated to use updateLayerListsIfNeeded().
>+
> 2012-04-30  Oliver Hunt  <oliver@apple.com>
> 
>         Harden arithmetic in ImageBufferDataCG
>diff --git a/Source/WebCore/rendering/RenderLayer.cpp b/Source/WebCore/rendering/RenderLayer.cpp
>index cbf5241699569905887e356d0f9425170c9d7098..f42147321bf4cda35fe471a97ac5661db1af8dad 100644
>--- a/Source/WebCore/rendering/RenderLayer.cpp
>+++ b/Source/WebCore/rendering/RenderLayer.cpp
>@@ -4456,6 +4456,8 @@ static inline bool compareZIndex(RenderLayer* first, RenderLayer* second)
> void RenderLayer::dirtyZOrderLists()
> {
>     ASSERT(m_layerListMutationAllowed);
>+    // We cannot assume that we are called on a stacking context as it
>+    // is called when we just got demoted from being a stacking context.
> 
>     if (m_posZOrderList)
>         m_posZOrderList->clear();
>@@ -4493,6 +4495,7 @@ void RenderLayer::dirtyNormalFlowList()
> void RenderLayer::rebuildZOrderLists()
> {
>     ASSERT(m_layerListMutationAllowed);
>+    ASSERT(isDirtyStackingContext());
> 
> #if USE(ACCELERATED_COMPOSITING)
>     bool includeHiddenLayers = compositor()->inCompositingMode();
>@@ -4565,13 +4568,18 @@ void RenderLayer::updateLayerListsIfNeeded()
> {
>     updateZOrderLists();
>     updateNormalFlowList();
>+
>+    if (RenderLayer* reflectionLayer = this->reflectionLayer()) {
>+        reflectionLayer->updateZOrderLists();
>+        reflectionLayer->updateNormalFlowList();
>+    }
> }
> 
> void RenderLayer::updateCompositingAndLayerListsIfNeeded()
> {
> #if USE(ACCELERATED_COMPOSITING)
>     if (compositor()->inCompositingMode()) {
>-        if ((isStackingContext() && m_zOrderListsDirty) || m_normalFlowListDirty)
>+        if (isDirtyStackingContext() || m_normalFlowListDirty)
>             compositor()->updateCompositingLayers(CompositingUpdateOnHitTest, this);
>         return;
>     }
>diff --git a/Source/WebCore/rendering/RenderLayer.h b/Source/WebCore/rendering/RenderLayer.h
>index 1c29b080d8ebe092a6dd3b9fda8edf6b6c6ccbb2..19b5fdb12b222fda557d5ffdd8275390f03cb42c 100644
>--- a/Source/WebCore/rendering/RenderLayer.h
>+++ b/Source/WebCore/rendering/RenderLayer.h
>@@ -385,13 +385,15 @@ public:
> 
>     void dirtyZOrderLists();
>     void dirtyStackingContextZOrderLists();
>-    void updateZOrderLists();
>-    Vector<RenderLayer*>* posZOrderList() const { return m_posZOrderList; }
>-    Vector<RenderLayer*>* negZOrderList() const { return m_negZOrderList; }
>+
>+    Vector<RenderLayer*>* posZOrderList() const { ASSERT(!m_zOrderListsDirty); return m_posZOrderList; }
>+    Vector<RenderLayer*>* negZOrderList() const { ASSERT(!m_zOrderListsDirty); return m_negZOrderList; }
> 
>     void dirtyNormalFlowList();
>-    void updateNormalFlowList();
>-    Vector<RenderLayer*>* normalFlowList() const { return m_normalFlowList; }
>+    Vector<RenderLayer*>* normalFlowList() const { ASSERT(!m_normalFlowListDirty); return m_normalFlowList; }
>+
>+    // Update our normal and z-index lists.
>+    void updateLayerListsIfNeeded();
> 
>     // FIXME: We should ASSERT(!m_visibleContentStatusDirty) here, but see https://bugs.webkit.org/show_bug.cgi?id=71044
>     // ditto for hasVisibleDescendant(), see https://bugs.webkit.org/show_bug.cgi?id=71277
>@@ -598,8 +600,13 @@ public:
> #endif
> 
> private:
>+    void updateZOrderLists();
>     void rebuildZOrderLists();
> 
>+    void updateNormalFlowList();
>+
>+    bool isDirtyStackingContext() const { return m_zOrderListsDirty && isStackingContext(); }
>+
>     void computeRepaintRects(LayoutPoint* offsetFromRoot = 0);
>     void clearRepaintRects();
> 
>@@ -630,7 +637,6 @@ private:
> 
>     void collectLayers(bool includeHiddenLayers, Vector<RenderLayer*>*&, Vector<RenderLayer*>*&);
> 
>-    void updateLayerListsIfNeeded();
>     void updateCompositingAndLayerListsIfNeeded();
> 
>     void paintLayer(RenderLayer* rootLayer, GraphicsContext*, const LayoutRect& paintDirtyRect,
>@@ -917,8 +923,18 @@ private:
> 
> inline void RenderLayer::updateZOrderLists()
> {
>-    if (!m_zOrderListsDirty || !isStackingContext())
>+    if (!m_zOrderListsDirty)
>         return;
>+
>+    if (!isStackingContext()) {
>+        // Dirtied non-stacking contexts should have had their list
>+        // cleared when getting dirtied.
>+        ASSERT(!m_negZOrderList || m_negZOrderList->isEmpty());
>+        ASSERT(!m_posZOrderList || m_posZOrderList->isEmpty());
>+        m_zOrderListsDirty = false;
>+        return;
>+    }
>+
>     rebuildZOrderLists();
> }
> 
>diff --git a/Source/WebCore/rendering/RenderLayerCompositor.cpp b/Source/WebCore/rendering/RenderLayerCompositor.cpp
>index 02f5169060f9a6eab95a4597a2e5cc4ef15f2772..7e33584217b9f4913382ac1b499fbb8d41ad66a1 100644
>--- a/Source/WebCore/rendering/RenderLayerCompositor.cpp
>+++ b/Source/WebCore/rendering/RenderLayerCompositor.cpp
>@@ -657,7 +657,6 @@ void RenderLayerCompositor::addToOverlapMapRecursive(OverlapMap& overlapMap, Ren
>         }
>     }
> 
>-    ASSERT(!layer->m_normalFlowListDirty);
>     if (Vector<RenderLayer*>* normalFlowList = layer->normalFlowList()) {
>         size_t listSize = normalFlowList->size();
>         for (size_t i = 0; i < listSize; ++i) {
>@@ -689,9 +688,8 @@ void RenderLayerCompositor::addToOverlapMapRecursive(OverlapMap& overlapMap, Ren
> void RenderLayerCompositor::computeCompositingRequirements(RenderLayer* layer, OverlapMap* overlapMap, struct CompositingState& compositingState, bool& layersChanged)
> {
>     layer->updateLayerPosition();
>-    layer->updateZOrderLists();
>-    layer->updateNormalFlowList();
>-    
>+    layer->updateLayerListsIfNeeded();
>+
>     // Clear the flag
>     layer->setHasCompositingDescendant(false);
>     
>@@ -743,7 +741,6 @@ void RenderLayerCompositor::computeCompositingRequirements(RenderLayer* layer, O
> #endif
> 
>     if (layer->isStackingContext()) {
>-        ASSERT(!layer->m_zOrderListsDirty);
>         if (Vector<RenderLayer*>* negZOrderList = layer->negZOrderList()) {
>             size_t listSize = negZOrderList->size();
>             for (size_t i = 0; i < listSize; ++i) {
>@@ -764,7 +761,6 @@ void RenderLayerCompositor::computeCompositingRequirements(RenderLayer* layer, O
>         }
>     }
>     
>-    ASSERT(!layer->m_normalFlowListDirty);
>     if (Vector<RenderLayer*>* normalFlowList = layer->normalFlowList()) {
>         size_t listSize = normalFlowList->size();
>         for (size_t i = 0; i < listSize; ++i) {
>@@ -811,8 +807,10 @@ void RenderLayerCompositor::computeCompositingRequirements(RenderLayer* layer, O
>     }
> 
>     ASSERT(willBeComposited == needsToBeComposited(layer));
>-    if (layer->reflectionLayer())
>+    if (layer->reflectionLayer()) {
>+        // FIXME: Shouldn't we call computeCompositingRequirements to handle a reflection overlapping with another renderer?
>         layer->reflectionLayer()->setMustOverlapCompositedLayers(willBeComposited);
>+    }
> 
>     // Subsequent layers in the parent stacking context also need to composite.
>     if (childState.m_subtreeIsCompositing)
>@@ -934,8 +932,6 @@ void RenderLayerCompositor::rebuildCompositingLayerTree(RenderLayer* layer, Vect
> #endif
> 
>     if (layer->isStackingContext()) {
>-        ASSERT(!layer->m_zOrderListsDirty);
>-
>         if (Vector<RenderLayer*>* negZOrderList = layer->negZOrderList()) {
>             size_t listSize = negZOrderList->size();
>             for (size_t i = 0; i < listSize; ++i) {
>@@ -949,7 +945,6 @@ void RenderLayerCompositor::rebuildCompositingLayerTree(RenderLayer* layer, Vect
>             childList.append(layerBacking->foregroundLayer());
>     }
> 
>-    ASSERT(!layer->m_normalFlowListDirty);
>     if (Vector<RenderLayer*>* normalFlowList = layer->normalFlowList()) {
>         size_t listSize = normalFlowList->size();
>         for (size_t i = 0; i < listSize; ++i) {
>@@ -1118,8 +1113,6 @@ void RenderLayerCompositor::updateLayerTreeGeometry(RenderLayer* layer, int dept
> #endif
> 
>     if (layer->isStackingContext()) {
>-        ASSERT(!layer->m_zOrderListsDirty);
>-
>         if (Vector<RenderLayer*>* negZOrderList = layer->negZOrderList()) {
>             size_t listSize = negZOrderList->size();
>             for (size_t i = 0; i < listSize; ++i)
>@@ -1127,7 +1120,6 @@ void RenderLayerCompositor::updateLayerTreeGeometry(RenderLayer* layer, int dept
>         }
>     }
> 
>-    ASSERT(!layer->m_normalFlowListDirty);
>     if (Vector<RenderLayer*>* normalFlowList = layer->normalFlowList()) {
>         size_t listSize = normalFlowList->size();
>         for (size_t i = 0; i < listSize; ++i)
>@@ -1411,7 +1403,9 @@ bool RenderLayerCompositor::requiresCompositingLayer(const RenderLayer* layer) c
> 
> bool RenderLayerCompositor::canBeComposited(const RenderLayer* layer) const
> {
>-    return m_hasAcceleratedCompositing && layer->isSelfPaintingLayer();
>+    // FIXME: We disable AC for elements in RenderFlowThread as it doesn't work properly.
>+    // See http://webkit.org/b/84900 to re-enable it.
>+    return m_hasAcceleratedCompositing && layer->isSelfPaintingLayer() && !layer->renderer()->inRenderFlowThread();
> }
> 
> bool RenderLayerCompositor::requiresOwnBackingStore(const RenderLayer* layer, const RenderLayer* compositingAncestorLayer) const
>diff --git a/Source/WebCore/rendering/RenderTreeAsText.cpp b/Source/WebCore/rendering/RenderTreeAsText.cpp
>index 55d68ed5b390a4dd9d02ade33a6b718ae6888091..320d4e706a3d6961c777a8ca7da34a57a0e844c5 100644
>--- a/Source/WebCore/rendering/RenderTreeAsText.cpp
>+++ b/Source/WebCore/rendering/RenderTreeAsText.cpp
>@@ -708,8 +708,7 @@ static void writeLayers(TextStream& ts, const RenderLayer* rootLayer, RenderLaye
>     l->calculateRects(rootLayer, 0, paintDirtyRect, layerBounds, damageRect, clipRectToApply, outlineRect, true);
> 
>     // Ensure our lists are up-to-date.
>-    l->updateZOrderLists();
>-    l->updateNormalFlowList();
>+    l->updateLayerListsIfNeeded();
> 
>     bool shouldPaint = (behavior & RenderAsTextShowAllLayers) ? true : l->intersectsDamageRect(layerBounds, damageRect.rect(), rootLayer);
>     Vector<RenderLayer*>* negList = l->negZOrderList();

Flags:

simon.fraser: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 84920: 138920 | 139571