display:none iframes cause repeated compositing flushing
authorsimon.fraser@apple.com <simon.fraser@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 4 May 2015 23:31:10 +0000 (23:31 +0000)
committersimon.fraser@apple.com <simon.fraser@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 4 May 2015 23:31:10 +0000 (23:31 +0000)
https://bugs.webkit.org/show_bug.cgi?id=144529

Reviewed by Darin Adler.

Source/WebCore:

FrameView::updateLayoutAndStyleIfNeededRecursive() only forces layout on rendered
frames, by virtue of using its Widget children which are FrameViews.

However, FrameView::flushCompositingStateIncludingSubframes() iterated over
all frames, and return false if any subframe needed layout. Thus, if it saw
non-rendered frames (which are never laid out), it would return false,
which causes the CFRunLoopObserver that drives flushing to run again.

Fix by having FrameView::flushCompositingStateIncludingSubframes() only check
rendered frames, using FrameTree::traverseNextRendered() (which needs to be public).

Also change FrameView::needsStyleRecalcOrLayout() and FrameView::updateLayoutAndStyleIfNeededRecursive()
to fetch the list of FrameViews using FrameTree's nextRenderedSibling(), rather than using
the Widget tree, since we'd like to eventually remove Widgets, and using the Frame
tree matches flushCompositingStateIncludingSubframes() and other code.

Test: compositing/iframes/display-none-subframe.html

* page/FrameTree.h:
* page/FrameView.cpp:
(WebCore::FrameView::flushCompositingStateIncludingSubframes):
(WebCore::FrameView::needsStyleRecalcOrLayout):
(WebCore::FrameView::renderedChildFrameViews): Helper that returns a vector
of Ref<FrameView>s for rendered frames only.
(WebCore::FrameView::updateLayoutAndStyleIfNeededRecursive):
* page/FrameView.h:

LayoutTests:

Test with a display:none iframe that triggers a single compositing flush,
then counts how many occur in 10ms.

* compositing/iframes/display-none-subframe-expected.txt: Added.
* compositing/iframes/display-none-subframe.html: Added.

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

LayoutTests/ChangeLog
LayoutTests/compositing/iframes/display-none-subframe-expected.txt [new file with mode: 0644]
LayoutTests/compositing/iframes/display-none-subframe.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/page/FrameTree.h
Source/WebCore/page/FrameView.cpp
Source/WebCore/page/FrameView.h

index 81bc2e9..6d5b05c 100644 (file)
@@ -1,5 +1,18 @@
 2015-05-04  Simon Fraser  <simon.fraser@apple.com>
 
+        display:none iframes cause repeated compositing flushing
+        https://bugs.webkit.org/show_bug.cgi?id=144529
+
+        Reviewed by Darin Adler.
+        
+        Test with a display:none iframe that triggers a single compositing flush,
+        then counts how many occur in 10ms.
+
+        * compositing/iframes/display-none-subframe-expected.txt: Added.
+        * compositing/iframes/display-none-subframe.html: Added.
+
+2015-05-04  Simon Fraser  <simon.fraser@apple.com>
+
         Fix updating of tiled backing opaquenss when the page background color changes
         https://bugs.webkit.org/show_bug.cgi?id=144600
         rdar://problem/20723035
diff --git a/LayoutTests/compositing/iframes/display-none-subframe-expected.txt b/LayoutTests/compositing/iframes/display-none-subframe-expected.txt
new file mode 100644 (file)
index 0000000..2336069
--- /dev/null
@@ -0,0 +1,3 @@
+PASS: saw 1 or 2 layer flushes
+
+
diff --git a/LayoutTests/compositing/iframes/display-none-subframe.html b/LayoutTests/compositing/iframes/display-none-subframe.html
new file mode 100644 (file)
index 0000000..4c94aa6
--- /dev/null
@@ -0,0 +1,65 @@
+<!DOCTYPE html>
+
+<html>
+<head>
+    <style>
+        #box {
+            width: 100px;
+            height: 100px;
+            margin: 10px;
+            background-color: blue;
+        }
+        
+        .composited {
+            -webkit-transform: translateZ(0);
+        }
+        
+        iframe {
+            display: none;
+        }
+    </style>
+    <script>
+        if (window.testRunner) {
+            testRunner.dumpAsText();
+            testRunner.waitUntilDone();
+        }
+
+        function doTest()
+        {
+            document.body.offsetWidth;
+            
+            if (window.internals)
+                internals.startTrackingLayerFlushes();
+
+            document.getElementById('box').classList.add('composited');
+
+            if (window.internals)
+                internals.updateLayoutIgnorePendingStylesheetsAndRunPostLayoutTasks();
+
+            window.setTimeout(function() {
+                var flushCount = 0;
+                if (window.internals)
+                    flushCount = internals.layerFlushCount();
+                
+                var result;
+                if (flushCount == 1 || flushCount == 2)
+                    result = 'PASS: saw 1 or 2 layer flushes';
+                else
+                    result = 'FAIL: saw ' + flushCount + ' layer flushes, expected 1 or possibly 2.';
+                    
+                document.getElementById('result').textContent = result;
+
+                if (window.testRunner)
+                    testRunner.notifyDone();
+            }, 10);
+        }
+        
+        window.addEventListener('load', doTest, false);
+    </script>
+</head>
+<body>
+    <p id="result"></p>
+    <div id="box"></div>
+    <iframe src="resources/subframe.html"></iframe>
+</body>
+</html>
index a65c26b..d51ab5c 100644 (file)
@@ -1,3 +1,37 @@
+2015-05-04  Simon Fraser  <simon.fraser@apple.com>
+
+        display:none iframes cause repeated compositing flushing
+        https://bugs.webkit.org/show_bug.cgi?id=144529
+
+        Reviewed by Darin Adler.
+        
+        FrameView::updateLayoutAndStyleIfNeededRecursive() only forces layout on rendered
+        frames, by virtue of using its Widget children which are FrameViews.
+        
+        However, FrameView::flushCompositingStateIncludingSubframes() iterated over
+        all frames, and return false if any subframe needed layout. Thus, if it saw
+        non-rendered frames (which are never laid out), it would return false,
+        which causes the CFRunLoopObserver that drives flushing to run again.
+        
+        Fix by having FrameView::flushCompositingStateIncludingSubframes() only check
+        rendered frames, using FrameTree::traverseNextRendered() (which needs to be public).
+        
+        Also change FrameView::needsStyleRecalcOrLayout() and FrameView::updateLayoutAndStyleIfNeededRecursive()
+        to fetch the list of FrameViews using FrameTree's nextRenderedSibling(), rather than using
+        the Widget tree, since we'd like to eventually remove Widgets, and using the Frame
+        tree matches flushCompositingStateIncludingSubframes() and other code.
+
+        Test: compositing/iframes/display-none-subframe.html
+
+        * page/FrameTree.h:
+        * page/FrameView.cpp:
+        (WebCore::FrameView::flushCompositingStateIncludingSubframes):
+        (WebCore::FrameView::needsStyleRecalcOrLayout):
+        (WebCore::FrameView::renderedChildFrameViews): Helper that returns a vector
+        of Ref<FrameView>s for rendered frames only.
+        (WebCore::FrameView::updateLayoutAndStyleIfNeededRecursive):
+        * page/FrameView.h:
+
 2015-05-04  Chris Dumez  <cdumez@apple.com>
 
         Unreviewed. Fix build with SECURITY_ASSERTIONS enabled.
index c036783..490cc50 100644 (file)
@@ -55,6 +55,9 @@ namespace WebCore {
         Frame* firstChild() const { return m_firstChild.get(); }
         Frame* lastChild() const { return m_lastChild; }
 
+        Frame* firstRenderedChild() const;
+        Frame* nextRenderedSibling() const;
+
         WEBCORE_EXPORT bool isDescendantOf(const Frame* ancestor) const;
         
         WEBCORE_EXPORT Frame* traverseNext(const Frame* stayWithin = nullptr) const;
@@ -90,9 +93,6 @@ namespace WebCore {
         Frame* scopedChild(const AtomicString& name, TreeScope*) const;
         unsigned scopedChildCount(TreeScope*) const;
 
-        Frame* firstRenderedChild() const;
-        Frame* nextRenderedSibling() const;
-
         Frame& m_thisFrame;
 
         Frame* m_parent;
index 62b976a..0da0757 100644 (file)
@@ -1066,8 +1066,10 @@ bool FrameView::isEnclosedInCompositingLayer() const
 bool FrameView::flushCompositingStateIncludingSubframes()
 {
     bool allFramesFlushed = flushCompositingStateForThisFrame(&frame());
-    
-    for (Frame* child = frame().tree().firstChild(); child; child = child->tree().traverseNext(m_frame.ptr())) {
+
+    for (Frame* child = frame().tree().firstRenderedChild(); child; child = child->tree().traverseNextRendered(m_frame.ptr())) {
+        if (!child->view())
+            continue;
         bool flushed = child->view()->flushCompositingStateForThisFrame(&frame());
         allFramesFlushed &= flushed;
     }
@@ -2593,16 +2595,8 @@ bool FrameView::needsStyleRecalcOrLayout(bool includeSubframes) const
     if (!includeSubframes)
         return false;
 
-    // Find child frames via the Widget tree, as updateLayoutAndStyleIfNeededRecursive() does.
-    Vector<Ref<FrameView>, 16> childViews;
-    childViews.reserveInitialCapacity(children().size());
-    for (auto& widget : children()) {
-        if (is<FrameView>(*widget))
-            childViews.uncheckedAppend(downcast<FrameView>(*widget));
-    }
-
-    for (unsigned i = 0; i < childViews.size(); ++i) {
-        if (childViews[i]->needsStyleRecalcOrLayout())
+    for (auto& frameView : renderedChildFrameViews()) {
+        if (frameView->needsStyleRecalcOrLayout())
             return true;
     }
 
@@ -4036,6 +4030,17 @@ void FrameView::paintOverhangAreas(GraphicsContext* context, const IntRect& hori
     ScrollView::paintOverhangAreas(context, horizontalOverhangArea, verticalOverhangArea, dirtyRect);
 }
 
+FrameView::FrameViewList FrameView::renderedChildFrameViews() const
+{
+    FrameViewList childViews;
+    for (Frame* frame = m_frame->tree().firstRenderedChild(); frame; frame = frame->tree().nextRenderedSibling()) {
+        if (frame->view())
+            childViews.append(*frame->view());
+    }
+    
+    return childViews;
+}
+
 void FrameView::updateLayoutAndStyleIfNeededRecursive()
 {
     // We have to crawl our entire tree looking for any FrameViews that need
@@ -4054,20 +4059,10 @@ void FrameView::updateLayoutAndStyleIfNeededRecursive()
     if (needsLayout())
         layout();
 
-    // Grab a copy of the children() set, as it may be mutated by the following updateLayoutAndStyleIfNeededRecursive
-    // calls, as they can potentially re-enter a layout of the parent frame view, which may add/remove scrollbars
-    // and thus mutates the children() set.
-    // We use the Widget children because walking the Frame tree would include display:none frames.
-    // FIXME: use FrameTree::traverseNextRendered().
-    Vector<Ref<FrameView>, 16> childViews;
-    childViews.reserveInitialCapacity(children().size());
-    for (auto& widget : children()) {
-        if (is<FrameView>(*widget))
-            childViews.uncheckedAppend(downcast<FrameView>(*widget));
-    }
-
-    for (unsigned i = 0; i < childViews.size(); ++i)
-        childViews[i]->updateLayoutAndStyleIfNeededRecursive();
+    // Grab a copy of the child views, as the list may be mutated by the following updateLayoutAndStyleIfNeededRecursive
+    // calls, as they can potentially re-enter a layout of the parent frame view.
+    for (auto& frameView : renderedChildFrameViews())
+        frameView->updateLayoutAndStyleIfNeededRecursive();
 
     // A child frame may have dirtied us during its layout.
     frame().document()->updateStyleIfNeeded();
index 8160c86..9df4b42 100644 (file)
@@ -517,6 +517,9 @@ public:
 
     const HashSet<Widget*>& widgetsInRenderTree() const { return m_widgetsInRenderTree; }
 
+    typedef Vector<Ref<FrameView>, 16> FrameViewList;
+    FrameViewList renderedChildFrameViews() const;
+
     void addTrackedRepaintRect(const FloatRect&);
 
     // exposedRect represents WebKit's understanding of what part