Changeset 221596 in webkit

Timestamp:

Sep 4, 2017 4:50:04 PM (7 years ago)

Author:

Darin Adler

Message:

Follow up FrameView::updateLayoutAndStyleIfNeededRecursive changes with related improvements
​https://bugs.webkit.org/show_bug.cgi?id=176277

Reviewed by Antti Koivisto.

• page/FrameView.cpp:

(WebCore::FrameView::needsStyleRecalcOrLayout): Deleted. This function was only used
by an assertion inside updateLayoutAndStyleIfNeededRecursive, and thus there is no reason
for it to be in the header file, or for it to be a public member function.
(WebCore::appendRenderedChildren): Deleted. This function was only used inside
updateLayoutAndStyleIfNeededRecursive, and it is now packaged in an even better way
for efficient use inside that function.
(WebCore::FrameView::renderedChildFrameViews): Deleted. This function was only used
inside needsStyleRecalcOrLayout, and it's now packaged in a better way inside that function.
(WebCore::FrameView::updateLayoutAndStyleIfNeededRecursive): Created a new lambda called
nextRendereredDescendant that packages up the process of repeatedly iterating this view
and all of its descendants in an easy-to-use way. Replaces both of the functions above.
Rewrote to use it; it made the logic clear enough that it was good to get rid of the
updateOneFrame lambda, too. Added two separate functions, one that checks for needed
style recalculation and a separate one that checked for needed layout. Using those,
replaced the old single assertion with two separate assertions.

• page/FrameView.h: Removed needsStyleRecalcOrLayout, renderedChildFrameViews, and

FrameViewList.

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• page/FrameView.cpp (modified) (3 diffs)
• page/FrameView.h (modified) (2 diffs)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2017-09-03  Darin Adler  <darin@apple.com>
+
+        Follow up FrameView::updateLayoutAndStyleIfNeededRecursive changes with related improvements
+        https://bugs.webkit.org/show_bug.cgi?id=176277
+
+        Reviewed by Antti Koivisto.
+
+        * page/FrameView.cpp:
+        (WebCore::FrameView::needsStyleRecalcOrLayout): Deleted. This function was only used
+        by an assertion inside updateLayoutAndStyleIfNeededRecursive, and thus there is no reason
+        for it to be in the header file, or for it to be a public member function.
+        (WebCore::appendRenderedChildren): Deleted. This function was only used inside
+        updateLayoutAndStyleIfNeededRecursive, and it is now packaged in an even better way
+        for efficient use inside that function.
+        (WebCore::FrameView::renderedChildFrameViews): Deleted. This function was only used
+        inside needsStyleRecalcOrLayout, and it's now packaged in a better way inside that function.
+        (WebCore::FrameView::updateLayoutAndStyleIfNeededRecursive): Created a new lambda called
+        nextRendereredDescendant that packages up the process of repeatedly iterating this view
+        and all of its descendants in an easy-to-use way. Replaces both of the functions above.
+        Rewrote to use it; it made the logic clear enough that it was good to get rid of the
+        updateOneFrame lambda, too. Added two separate functions, one that checks for needed
+        style recalculation and a separate one that checked for needed layout. Using those,
+        replaced the old single assertion with two separate assertions.
+
+        * page/FrameView.h: Removed needsStyleRecalcOrLayout, renderedChildFrameViews, and
+        FrameViewList.
+
 2017-09-04  Wenson Hsieh  <wenson_hsieh@apple.com>
 

trunk/Source/WebCore/page/FrameView.cpp

@@ -3152 +3152 @@
 }
 
-bool FrameView::needsStyleRecalcOrLayout(bool includeSubframes) const
-{
-    if (frame().document() && frame().document()->childNeedsStyleRecalc())
-        return true;
-    
-    if (needsLayout())
-        return true;
-
-    if (!includeSubframes)
-        return false;
-
-    for (auto& frameView : renderedChildFrameViews()) {
-        if (frameView->needsStyleRecalcOrLayout())
-            return true;
-    }
-
-    return false;
-}
-
 bool FrameView::needsLayout() const
 {
@@ -4576 +4557 @@
 }
 
-static void appendRenderedChildren(FrameView& view, Deque<Ref<FrameView>, 16>& deque)
-{
-    for (Frame* frame = view.frame().tree().firstRenderedChild(); frame; frame = frame->tree().nextRenderedSibling()) {
-        if (frame->view())
-            deque.append(*frame->view());
-    }
-}
-
-// FIXME: Change the one remaining caller of this to use appendRenderedChildren above instead,
-// and then remove FrameViewList and renderedChildFrameViews.
-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()
 {
-    // Style updating, render tree, creation, and layout needs to be done multiple times
+    // Style updating, render tree creation, and layout needs to be done multiple times
     // for more than one reason. But one reason is that when an <object> element determines
     // what it needs to load a subframe, a second pass is needed. That requires update
@@ -4612 +4572 @@
     AnimationUpdateBlock animationUpdateBlock(&frame().animation());
 
-    auto updateOnce = [this] {
-        auto updateOneFrame = [] (FrameView& view) {
-            bool didWork = view.frame().document()->updateStyleIfNeeded();
-            if (view.needsLayout()) {
-                view.layout();
+    using DescendantsDeque = Deque<Ref<FrameView>, 16>;
+    auto nextRenderedDescendant = [this] (DescendantsDeque& descendantsDeque) -> RefPtr<FrameView> {
+        if (descendantsDeque.isEmpty())
+            descendantsDeque.append(*this);
+        else {
+            // Append renderered children after processing the parent, in case the processing
+            // affects the set of rendered children.
+            auto previousView = descendantsDeque.takeFirst();
+            for (auto* frame = previousView->frame().tree().firstRenderedChild(); frame; frame = frame->tree().nextRenderedSibling()) {
+                if (auto* view = frame->view())
+                    descendantsDeque.append(*view);
+            }
+            if (descendantsDeque.isEmpty())
+                return nullptr;
+        }
+        return descendantsDeque.first().ptr();
+    };
+
+    for (unsigned i = 0; i < maxUpdatePasses; ++i) {
+        bool didWork = false;
+        DescendantsDeque deque;
+        while (auto view = nextRenderedDescendant(deque)) {
+            if (view->frame().document()->updateStyleIfNeeded())
+                didWork = true;
+            if (view->needsLayout()) {
+                view->layout();
                 didWork = true;
             }
-            return didWork;
-        };
-
-        bool didWork = false;
-
-        // Use a copy of the child frame list because it can change while updating.
-        Deque<Ref<FrameView>, 16> views;
-        views.append(*this);
-        while (!views.isEmpty()) {
-            auto view = views.takeFirst();
-            if (updateOneFrame(view.get()))
-                didWork = true;
-            appendRenderedChildren(view.get(), views);
-        }
-
-        return didWork;
+        }
+        if (!didWork)
+            break;
+    }
+
+#if !ASSERT_DISABLED
+    auto needsStyleRecalc = [&] {
+        DescendantsDeque deque;
+        while (auto view = nextRenderedDescendant(deque)) {
+            auto* document = view->frame().document();
+            if (document && document->childNeedsStyleRecalc())
+                return true;
+        }
+        return false;
     };
 
-    for (unsigned i = 0; i < maxUpdatePasses; ++i) {
-        if (!updateOnce())
-            break;
-    }
-
-    // FIXME: Unclear why it's appropriate to skip this assertion for non-main frames.
-    // The need for this may be obsolete and a leftover from when this fucntion was
-    // implemented by recursively calling itself.
-    ASSERT(!frame().isMainFrame() || !needsStyleRecalcOrLayout());
+    auto needsLayout = [&] {
+        DescendantsDeque deque;
+        while (auto view = nextRenderedDescendant(deque)) {
+            if (view->needsLayout())
+                return true;
+        }
+        return false;
+    };
+#endif
+
+    ASSERT(!needsStyleRecalc());
+    ASSERT(!needsLayout());
 }
 

trunk/Source/WebCore/page/FrameView.h

@@ -122 +122 @@
     WEBCORE_EXPORT void setNeedsLayout();
     void setViewportConstrainedObjectsNeedLayout();
-
-    bool needsStyleRecalcOrLayout(bool includeSubframes = true) const;
 
     bool needsFullRepaint() const { return m_needsFullRepaint; }
@@ -607 +605 @@
     const HashSet<Widget*>& widgetsInRenderTree() const { return m_widgetsInRenderTree; }
 
-    typedef Vector<Ref<FrameView>, 16> FrameViewList;
-    FrameViewList renderedChildFrameViews() const;
-
     void addTrackedRepaintRect(const FloatRect&);