- Revision
- 183777
- Author
- [email protected]
- Date
- 2015-05-04 16:31:10 -0700 (Mon, 04 May 2015)
Log 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.
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (183776 => 183777)
--- trunk/LayoutTests/ChangeLog 2015-05-04 23:10:34 UTC (rev 183776)
+++ trunk/LayoutTests/ChangeLog 2015-05-04 23:31:10 UTC (rev 183777)
@@ -1,5 +1,18 @@
2015-05-04 Simon Fraser <[email protected]>
+ 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 <[email protected]>
+
Fix updating of tiled backing opaquenss when the page background color changes
https://bugs.webkit.org/show_bug.cgi?id=144600
rdar://problem/20723035
Added: trunk/LayoutTests/compositing/iframes/display-none-subframe-expected.txt (0 => 183777)
--- trunk/LayoutTests/compositing/iframes/display-none-subframe-expected.txt (rev 0)
+++ trunk/LayoutTests/compositing/iframes/display-none-subframe-expected.txt 2015-05-04 23:31:10 UTC (rev 183777)
@@ -0,0 +1,3 @@
+PASS: saw 1 or 2 layer flushes
+
+
Added: trunk/LayoutTests/compositing/iframes/display-none-subframe.html (0 => 183777)
--- trunk/LayoutTests/compositing/iframes/display-none-subframe.html (rev 0)
+++ trunk/LayoutTests/compositing/iframes/display-none-subframe.html 2015-05-04 23:31:10 UTC (rev 183777)
@@ -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=""
+</body>
+</html>
Modified: trunk/Source/WebCore/ChangeLog (183776 => 183777)
--- trunk/Source/WebCore/ChangeLog 2015-05-04 23:10:34 UTC (rev 183776)
+++ trunk/Source/WebCore/ChangeLog 2015-05-04 23:31:10 UTC (rev 183777)
@@ -1,3 +1,37 @@
+2015-05-04 Simon Fraser <[email protected]>
+
+ 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 <[email protected]>
Unreviewed. Fix build with SECURITY_ASSERTIONS enabled.
Modified: trunk/Source/WebCore/page/FrameTree.h (183776 => 183777)
--- trunk/Source/WebCore/page/FrameTree.h 2015-05-04 23:10:34 UTC (rev 183776)
+++ trunk/Source/WebCore/page/FrameTree.h 2015-05-04 23:31:10 UTC (rev 183777)
@@ -55,6 +55,9 @@
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 @@
Frame* scopedChild(const AtomicString& name, TreeScope*) const;
unsigned scopedChildCount(TreeScope*) const;
- Frame* firstRenderedChild() const;
- Frame* nextRenderedSibling() const;
-
Frame& m_thisFrame;
Frame* m_parent;
Modified: trunk/Source/WebCore/page/FrameView.cpp (183776 => 183777)
--- trunk/Source/WebCore/page/FrameView.cpp 2015-05-04 23:10:34 UTC (rev 183776)
+++ trunk/Source/WebCore/page/FrameView.cpp 2015-05-04 23:31:10 UTC (rev 183777)
@@ -1066,8 +1066,10 @@
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 @@
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 @@
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,21 +4059,11 @@
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));
- }
+ // 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();
- for (unsigned i = 0; i < childViews.size(); ++i)
- childViews[i]->updateLayoutAndStyleIfNeededRecursive();
-
// A child frame may have dirtied us during its layout.
frame().document()->updateStyleIfNeeded();
if (needsLayout())
Modified: trunk/Source/WebCore/page/FrameView.h (183776 => 183777)
--- trunk/Source/WebCore/page/FrameView.h 2015-05-04 23:10:34 UTC (rev 183776)
+++ trunk/Source/WebCore/page/FrameView.h 2015-05-04 23:31:10 UTC (rev 183777)
@@ -517,6 +517,9 @@
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
