Changeset 183777 in webkit

Timestamp:

May 4, 2015 4:31:10 PM (9 years ago)

Author:

Simon Fraser

Message:

display:none iframes cause repeated compositing flushing
​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.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/compositing/iframes/display-none-subframe-expected.txt (added)
• LayoutTests/compositing/iframes/display-none-subframe.html (added)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/page/FrameTree.h (modified) (2 diffs)
• Source/WebCore/page/FrameView.cpp (modified) (4 diffs)
• Source/WebCore/page/FrameView.h (modified) (1 diff)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/WebCore/page/FrameTree.h

@@ -56 +56 @@
         Frame* lastChild() const { return m_lastChild; }
 
+        Frame* firstRenderedChild() const;
+        Frame* nextRenderedSibling() const;
+
         WEBCORE_EXPORT bool isDescendantOf(const Frame* ancestor) const;
         
@@ -91 +94 @@
         unsigned scopedChildCount(TreeScope*) const;
 
-        Frame* firstRenderedChild() const;
-        Frame* nextRenderedSibling() const;
-
         Frame& m_thisFrame;
 

trunk/Source/WebCore/page/FrameView.cpp

@@ -1067 +1067 @@
 {
     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;
@@ -2594 +2596 @@
         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;
     }
@@ -4037 +4031 @@
 }
 
+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()
 {
@@ -4055 +4060 @@
         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.

trunk/Source/WebCore/page/FrameView.h

@@ -518 +518 @@
     const HashSet<Widget*>& widgetsInRenderTree() const { return m_widgetsInRenderTree; }
 
+    typedef Vector<Ref<FrameView>, 16> FrameViewList;
+    FrameViewList renderedChildFrameViews() const;
+
     void addTrackedRepaintRect(const FloatRect&);