<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.1//EN"
"http://www.w3.org/TR/xhtml11/DTD/xhtml11.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head><meta http-equiv="content-type" content="text/html; charset=utf-8" />
<title>[183777] trunk</title>
</head>
<body>
<style type="text/css"><!--
#msg dl.meta { border: 1px #006 solid; background: #369; padding: 6px; color: #fff; }
#msg dl.meta dt { float: left; width: 6em; font-weight: bold; }
#msg dt:after { content:':';}
#msg dl, #msg dt, #msg ul, #msg li, #header, #footer, #logmsg { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; }
#msg dl a { font-weight: bold}
#msg dl a:link { color:#fc3; }
#msg dl a:active { color:#ff0; }
#msg dl a:visited { color:#cc6; }
h3 { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; font-weight: bold; }
#msg pre { overflow: auto; background: #ffc; border: 1px #fa0 solid; padding: 6px; }
#logmsg { background: #ffc; border: 1px #fa0 solid; padding: 1em 1em 0 1em; }
#logmsg p, #logmsg pre, #logmsg blockquote { margin: 0 0 1em 0; }
#logmsg p, #logmsg li, #logmsg dt, #logmsg dd { line-height: 14pt; }
#logmsg h1, #logmsg h2, #logmsg h3, #logmsg h4, #logmsg h5, #logmsg h6 { margin: .5em 0; }
#logmsg h1:first-child, #logmsg h2:first-child, #logmsg h3:first-child, #logmsg h4:first-child, #logmsg h5:first-child, #logmsg h6:first-child { margin-top: 0; }
#logmsg ul, #logmsg ol { padding: 0; list-style-position: inside; margin: 0 0 0 1em; }
#logmsg ul { text-indent: -1em; padding-left: 1em; }#logmsg ol { text-indent: -1.5em; padding-left: 1.5em; }
#logmsg > ul, #logmsg > ol { margin: 0 0 1em 0; }
#logmsg pre { background: #eee; padding: 1em; }
#logmsg blockquote { border: 1px solid #fa0; border-left-width: 10px; padding: 1em 1em 0 1em; background: white;}
#logmsg dl { margin: 0; }
#logmsg dt { font-weight: bold; }
#logmsg dd { margin: 0; padding: 0 0 0.5em 0; }
#logmsg dd:before { content:'\00bb';}
#logmsg table { border-spacing: 0px; border-collapse: collapse; border-top: 4px solid #fa0; border-bottom: 1px solid #fa0; background: #fff; }
#logmsg table th { text-align: left; font-weight: normal; padding: 0.2em 0.5em; border-top: 1px dotted #fa0; }
#logmsg table td { text-align: right; border-top: 1px dotted #fa0; padding: 0.2em 0.5em; }
#logmsg table thead th { text-align: center; border-bottom: 1px solid #fa0; }
#logmsg table th.Corner { text-align: left; }
#logmsg hr { border: none 0; border-top: 2px dashed #fa0; height: 1px; }
#header, #footer { color: #fff; background: #636; border: 1px #300 solid; padding: 6px; }
#patch { width: 100%; }
#patch h4 {font-family: verdana,arial,helvetica,sans-serif;font-size:10pt;padding:8px;background:#369;color:#fff;margin:0;}
#patch .propset h4, #patch .binary h4 {margin:0;}
#patch pre {padding:0;line-height:1.2em;margin:0;}
#patch .diff {width:100%;background:#eee;padding: 0 0 10px 0;overflow:auto;}
#patch .propset .diff, #patch .binary .diff {padding:10px 0;}
#patch span {display:block;padding:0 10px;}
#patch .modfile, #patch .addfile, #patch .delfile, #patch .propset, #patch .binary, #patch .copfile {border:1px solid #ccc;margin:10px 0;}
#patch ins {background:#dfd;text-decoration:none;display:block;padding:0 10px;}
#patch del {background:#fdd;text-decoration:none;display:block;padding:0 10px;}
#patch .lines, .info {color:#888;background:#fff;}
--></style>
<div id="msg">
<dl class="meta">
<dt>Revision</dt> <dd><a href="http://trac.webkit.org/projects/webkit/changeset/183777">183777</a></dd>
<dt>Author</dt> <dd>simon.fraser@apple.com</dd>
<dt>Date</dt> <dd>2015-05-04 16:31:10 -0700 (Mon, 04 May 2015)</dd>
</dl>
<h3>Log Message</h3>
<pre>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.</pre>
<h3>Modified Paths</h3>
<ul>
<li><a href="#trunkLayoutTestsChangeLog">trunk/LayoutTests/ChangeLog</a></li>
<li><a href="#trunkSourceWebCoreChangeLog">trunk/Source/WebCore/ChangeLog</a></li>
<li><a href="#trunkSourceWebCorepageFrameTreeh">trunk/Source/WebCore/page/FrameTree.h</a></li>
<li><a href="#trunkSourceWebCorepageFrameViewcpp">trunk/Source/WebCore/page/FrameView.cpp</a></li>
<li><a href="#trunkSourceWebCorepageFrameViewh">trunk/Source/WebCore/page/FrameView.h</a></li>
</ul>
<h3>Added Paths</h3>
<ul>
<li><a href="#trunkLayoutTestscompositingiframesdisplaynonesubframeexpectedtxt">trunk/LayoutTests/compositing/iframes/display-none-subframe-expected.txt</a></li>
<li><a href="#trunkLayoutTestscompositingiframesdisplaynonesubframehtml">trunk/LayoutTests/compositing/iframes/display-none-subframe.html</a></li>
</ul>
</div>
<div id="patch">
<h3>Diff</h3>
<a id="trunkLayoutTestsChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/LayoutTests/ChangeLog (183776 => 183777)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/LayoutTests/ChangeLog 2015-05-04 23:10:34 UTC (rev 183776)
+++ trunk/LayoutTests/ChangeLog 2015-05-04 23:31:10 UTC (rev 183777)
</span><span class="lines">@@ -1,5 +1,18 @@
</span><span class="cx"> 2015-05-04 Simon Fraser <simon.fraser@apple.com>
</span><span class="cx">
</span><ins>+ 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>
+
</ins><span class="cx"> Fix updating of tiled backing opaquenss when the page background color changes
</span><span class="cx"> https://bugs.webkit.org/show_bug.cgi?id=144600
</span><span class="cx"> rdar://problem/20723035
</span></span></pre></div>
<a id="trunkLayoutTestscompositingiframesdisplaynonesubframeexpectedtxt"></a>
<div class="addfile"><h4>Added: trunk/LayoutTests/compositing/iframes/display-none-subframe-expected.txt (0 => 183777)</h4>
<pre class="diff"><span>
<span class="info">--- 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)
</span><span class="lines">@@ -0,0 +1,3 @@
</span><ins>+PASS: saw 1 or 2 layer flushes
+
+
</ins></span></pre></div>
<a id="trunkLayoutTestscompositingiframesdisplaynonesubframehtml"></a>
<div class="addfile"><h4>Added: trunk/LayoutTests/compositing/iframes/display-none-subframe.html (0 => 183777)</h4>
<pre class="diff"><span>
<span class="info">--- 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)
</span><span class="lines">@@ -0,0 +1,65 @@
</span><ins>+<!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>
</ins></span></pre></div>
<a id="trunkSourceWebCoreChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebCore/ChangeLog (183776 => 183777)</h4>
<pre class="diff"><span>
<span class="info">--- 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)
</span><span class="lines">@@ -1,3 +1,37 @@
</span><ins>+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:
+
</ins><span class="cx"> 2015-05-04 Chris Dumez <cdumez@apple.com>
</span><span class="cx">
</span><span class="cx"> Unreviewed. Fix build with SECURITY_ASSERTIONS enabled.
</span></span></pre></div>
<a id="trunkSourceWebCorepageFrameTreeh"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebCore/page/FrameTree.h (183776 => 183777)</h4>
<pre class="diff"><span>
<span class="info">--- 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)
</span><span class="lines">@@ -55,6 +55,9 @@
</span><span class="cx"> Frame* firstChild() const { return m_firstChild.get(); }
</span><span class="cx"> Frame* lastChild() const { return m_lastChild; }
</span><span class="cx">
</span><ins>+ Frame* firstRenderedChild() const;
+ Frame* nextRenderedSibling() const;
+
</ins><span class="cx"> WEBCORE_EXPORT bool isDescendantOf(const Frame* ancestor) const;
</span><span class="cx">
</span><span class="cx"> WEBCORE_EXPORT Frame* traverseNext(const Frame* stayWithin = nullptr) const;
</span><span class="lines">@@ -90,9 +93,6 @@
</span><span class="cx"> Frame* scopedChild(const AtomicString& name, TreeScope*) const;
</span><span class="cx"> unsigned scopedChildCount(TreeScope*) const;
</span><span class="cx">
</span><del>- Frame* firstRenderedChild() const;
- Frame* nextRenderedSibling() const;
-
</del><span class="cx"> Frame& m_thisFrame;
</span><span class="cx">
</span><span class="cx"> Frame* m_parent;
</span></span></pre></div>
<a id="trunkSourceWebCorepageFrameViewcpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebCore/page/FrameView.cpp (183776 => 183777)</h4>
<pre class="diff"><span>
<span class="info">--- 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)
</span><span class="lines">@@ -1066,8 +1066,10 @@
</span><span class="cx"> bool FrameView::flushCompositingStateIncludingSubframes()
</span><span class="cx"> {
</span><span class="cx"> bool allFramesFlushed = flushCompositingStateForThisFrame(&frame());
</span><del>-
- for (Frame* child = frame().tree().firstChild(); child; child = child->tree().traverseNext(m_frame.ptr())) {
</del><ins>+
+ for (Frame* child = frame().tree().firstRenderedChild(); child; child = child->tree().traverseNextRendered(m_frame.ptr())) {
+ if (!child->view())
+ continue;
</ins><span class="cx"> bool flushed = child->view()->flushCompositingStateForThisFrame(&frame());
</span><span class="cx"> allFramesFlushed &= flushed;
</span><span class="cx"> }
</span><span class="lines">@@ -2593,16 +2595,8 @@
</span><span class="cx"> if (!includeSubframes)
</span><span class="cx"> return false;
</span><span class="cx">
</span><del>- // 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())
</del><ins>+ for (auto& frameView : renderedChildFrameViews()) {
+ if (frameView->needsStyleRecalcOrLayout())
</ins><span class="cx"> return true;
</span><span class="cx"> }
</span><span class="cx">
</span><span class="lines">@@ -4036,6 +4030,17 @@
</span><span class="cx"> ScrollView::paintOverhangAreas(context, horizontalOverhangArea, verticalOverhangArea, dirtyRect);
</span><span class="cx"> }
</span><span class="cx">
</span><ins>+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;
+}
+
</ins><span class="cx"> void FrameView::updateLayoutAndStyleIfNeededRecursive()
</span><span class="cx"> {
</span><span class="cx"> // We have to crawl our entire tree looking for any FrameViews that need
</span><span class="lines">@@ -4054,21 +4059,11 @@
</span><span class="cx"> if (needsLayout())
</span><span class="cx"> layout();
</span><span class="cx">
</span><del>- // 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));
- }
</del><ins>+ // 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();
</ins><span class="cx">
</span><del>- for (unsigned i = 0; i < childViews.size(); ++i)
- childViews[i]->updateLayoutAndStyleIfNeededRecursive();
-
</del><span class="cx"> // A child frame may have dirtied us during its layout.
</span><span class="cx"> frame().document()->updateStyleIfNeeded();
</span><span class="cx"> if (needsLayout())
</span></span></pre></div>
<a id="trunkSourceWebCorepageFrameViewh"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebCore/page/FrameView.h (183776 => 183777)</h4>
<pre class="diff"><span>
<span class="info">--- 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)
</span><span class="lines">@@ -517,6 +517,9 @@
</span><span class="cx">
</span><span class="cx"> const HashSet<Widget*>& widgetsInRenderTree() const { return m_widgetsInRenderTree; }
</span><span class="cx">
</span><ins>+ typedef Vector<Ref<FrameView>, 16> FrameViewList;
+ FrameViewList renderedChildFrameViews() const;
+
</ins><span class="cx"> void addTrackedRepaintRect(const FloatRect&);
</span><span class="cx">
</span><span class="cx"> // exposedRect represents WebKit's understanding of what part
</span></span></pre>
</div>
</div>
</body>
</html>