Changeset 116195 in webkit

Timestamp:

May 4, 2012 5:38:03 PM (12 years ago)

Author:

shawnsingh@chromium.org

Message:

[chromium] Changes to layer tree structure need to be tracked properly
​https://bugs.webkit.org/show_bug.cgi?id=85421

Reviewed by Adrienne Walker.

Unit test added: TreeSynchronizerTest.syncSimpleTreeAndTrackStackingOrderChange

Source/WebCore:

Earlier, we were relying on WebCore behavior that always called
setNeedsDisplay whenever the layer tree structure changed.
However, in general it is more correct to consider layer tree
changes even when things don't need repainting; for example Aura
code is encountring this bug now. This patch corrects the
compositor so that layer tree structural changes are considered
property changes, without requiring that layers needed to be
repainted.

• platform/graphics/chromium/LayerChromium.cpp:

(WebCore::LayerChromium::LayerChromium):
(WebCore::LayerChromium::insertChild):
(WebCore::LayerChromium::pushPropertiesTo):

• platform/graphics/chromium/LayerChromium.h:

(LayerChromium):

• platform/graphics/chromium/cc/CCLayerImpl.cpp:

(WebCore::CCLayerImpl::setStackingOrderChanged):
(WebCore):

• platform/graphics/chromium/cc/CCLayerImpl.h:

(CCLayerImpl):

Source/WebKit/chromium:

• tests/TreeSynchronizerTest.cpp:

(WebKitTests):
(WebKitTests::TEST):

Location:

trunk/Source

Files:

• WebCore/ChangeLog (modified) (1 diff)
• WebCore/platform/graphics/chromium/LayerChromium.cpp (modified) (4 diffs)
• WebCore/platform/graphics/chromium/LayerChromium.h (modified) (1 diff)
• WebCore/platform/graphics/chromium/cc/CCLayerImpl.cpp (modified) (1 diff)
• WebCore/platform/graphics/chromium/cc/CCLayerImpl.h (modified) (1 diff)
• WebKit/chromium/ChangeLog (modified) (1 diff)
• WebKit/chromium/tests/TreeSynchronizerTest.cpp (modified) (1 diff)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2012-05-04  Shawn Singh  <shawnsingh@chromium.org>
+
+        [chromium] Changes to layer tree structure need to be tracked properly
+        https://bugs.webkit.org/show_bug.cgi?id=85421
+
+        Reviewed by Adrienne Walker.
+
+        Unit test added: TreeSynchronizerTest.syncSimpleTreeAndTrackStackingOrderChange
+
+        Earlier, we were relying on WebCore behavior that always called
+        setNeedsDisplay whenever the layer tree structure changed.
+        However, in general it is more correct to consider layer tree
+        changes even when things don't need repainting; for example Aura
+        code is encountring this bug now. This patch corrects the
+        compositor so that layer tree structural changes are considered
+        property changes, without requiring that layers needed to be
+        repainted.
+
+        * platform/graphics/chromium/LayerChromium.cpp:
+        (WebCore::LayerChromium::LayerChromium):
+        (WebCore::LayerChromium::insertChild):
+        (WebCore::LayerChromium::pushPropertiesTo):
+        * platform/graphics/chromium/LayerChromium.h:
+        (LayerChromium):
+        * platform/graphics/chromium/cc/CCLayerImpl.cpp:
+        (WebCore::CCLayerImpl::setStackingOrderChanged):
+        (WebCore):
+        * platform/graphics/chromium/cc/CCLayerImpl.h:
+        (CCLayerImpl):
+
 2012-05-04  Jeffrey Pfau  <jpfau@apple.com>
 

trunk/Source/WebCore/platform/graphics/chromium/LayerChromium.cpp

@@ -58 +58 @@
 LayerChromium::LayerChromium()
     : m_needsDisplay(false)
+    , m_stackingOrderChanged(false)
     , m_layerId(s_nextLayerId++)
     , m_parent(0)
@@ -154 +155 @@
     child->removeFromParent();
     child->setParent(this);
+    child->m_stackingOrderChanged = true;
     m_children.insert(index, child);
     setNeedsCommit();
@@ -515 +517 @@
     layer->setSentScrollDelta(IntSize());
 
+    layer->setStackingOrderChanged(m_stackingOrderChanged);
+
     if (maskLayer())
         maskLayer()->pushPropertiesTo(layer->maskLayer());
@@ -523 +527 @@
 
     // Reset any state that should be cleared for the next update.
+    m_stackingOrderChanged = false;
     m_updateRect = FloatRect();
 }

trunk/Source/WebCore/platform/graphics/chromium/LayerChromium.h

@@ -265 +265 @@
     bool m_needsDisplay;
 
+    // Tracks whether this layer may have changed stacking order with its siblings.
+    bool m_stackingOrderChanged;
+
     // The update rect is the region of the compositor resource that was actually updated by the compositor.
     // For layers that may do updating outside the compositor's control (i.e. plugin layers), this information

trunk/Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.cpp

@@ -266 +266 @@
 }
 
+void CCLayerImpl::setStackingOrderChanged(bool stackingOrderChanged)
+{
+    // We don't need to store this flag; we only need to track that the change occurred.
+    if (stackingOrderChanged)
+        noteLayerPropertyChangedForSubtree();
+}
+
 void CCLayerImpl::noteLayerPropertyChangedForSubtree()
 {

trunk/Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.h

@@ -230 +230 @@
     String layerTreeAsText() const;
 
+    void setStackingOrderChanged(bool);
+
     bool layerPropertyChanged() const { return m_layerPropertyChanged; }
     void resetAllChangeTrackingForSubtree();

trunk/Source/WebKit/chromium/ChangeLog

@@ +1 @@
+2012-05-04  Shawn Singh  <shawnsingh@chromium.org>
+
+        [chromium] Changes to layer tree structure need to be tracked properly
+        https://bugs.webkit.org/show_bug.cgi?id=85421
+
+        Reviewed by Adrienne Walker.
+
+        Unit test added: TreeSynchronizerTest.syncSimpleTreeAndTrackStackingOrderChange
+
+        * tests/TreeSynchronizerTest.cpp:
+        (WebKitTests):
+        (WebKitTests::TEST):
+
 2012-05-04  Tony Chang  <tony@chromium.org>
 

trunk/Source/WebKit/chromium/tests/TreeSynchronizerTest.cpp

@@ -198 +198 @@
 }
 
+// Constructs a very simple tree and checks that a stacking-order change is tracked properly.
+TEST(TreeSynchronizerTest, syncSimpleTreeAndTrackStackingOrderChange)
+{
+    DebugScopedSetImplThread impl;
+    Vector<int> ccLayerDestructionList;
+
+    // Set up the tree and sync once. child2 needs to be synced here, too, even though we
+    // remove it to set up the intended scenario.
+    RefPtr<LayerChromium> layerTreeRoot = MockLayerChromium::create(&ccLayerDestructionList);
+    RefPtr<LayerChromium> child2 = MockLayerChromium::create(&ccLayerDestructionList);
+    layerTreeRoot->addChild(MockLayerChromium::create(&ccLayerDestructionList));
+    layerTreeRoot->addChild(child2);
+    OwnPtr<CCLayerImpl> ccLayerTreeRoot = TreeSynchronizer::synchronizeTrees(layerTreeRoot.get(), nullptr);
+    expectTreesAreIdentical(layerTreeRoot.get(), ccLayerTreeRoot.get());
+    ccLayerTreeRoot->resetAllChangeTrackingForSubtree();
+
+    // re-insert the layer and sync again.
+    child2->removeFromParent();
+    layerTreeRoot->addChild(child2);
+    ccLayerTreeRoot = TreeSynchronizer::synchronizeTrees(layerTreeRoot.get(), ccLayerTreeRoot.release());
+    expectTreesAreIdentical(layerTreeRoot.get(), ccLayerTreeRoot.get());
+
+    // Check that the impl thread properly tracked the change.
+    EXPECT_FALSE(ccLayerTreeRoot->layerPropertyChanged());
+    EXPECT_FALSE(ccLayerTreeRoot->children()[0]->layerPropertyChanged());
+    EXPECT_TRUE(ccLayerTreeRoot->children()[1]->layerPropertyChanged());
+}
+
 TEST(TreeSynchronizerTest, syncSimpleTreeAndProperties)
 {