From 295cfa73b6f9a21d68bc7f4eb8e5ef06c600f27f Mon Sep 17 00:00:00 2001 From: Darin Adler Date: Mon, 16 Oct 2017 08:33:53 +0000 Subject: [PATCH] Merge r221596 - 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. --- Source/WebCore/ChangeLog | 27 +++++++ Source/WebCore/page/FrameView.cpp | 114 +++++++++++++----------------- Source/WebCore/page/FrameView.h | 5 -- 3 files changed, 75 insertions(+), 71 deletions(-) diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog index ba1df7489675..fbb0981c57cb 100644 --- a/Source/WebCore/ChangeLog +++ b/Source/WebCore/ChangeLog @@ -1,3 +1,30 @@ +2017-09-03 Darin Adler + + 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-06 Tomas Popela Missing break in URLParser diff --git a/Source/WebCore/page/FrameView.cpp b/Source/WebCore/page/FrameView.cpp index ef0f6c2f037b..2e5752b2051c 100644 --- a/Source/WebCore/page/FrameView.cpp +++ b/Source/WebCore/page/FrameView.cpp @@ -3150,25 +3150,6 @@ bool FrameView::layoutPending() const return m_layoutTimer.isActive(); } -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 { // This can return true in cases where the document does not have a body yet. @@ -4574,30 +4555,9 @@ void FrameView::paintOverhangAreas(GraphicsContext& context, const IntRect& hori ScrollView::paintOverhangAreas(context, horizontalOverhangArea, verticalOverhangArea, dirtyRect); } -static void appendRenderedChildren(FrameView& view, Deque, 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 element determines // what it needs to load a subframe, a second pass is needed. That requires update // passes equal to the number of levels of DOM nesting. That is why this number is large. @@ -4610,40 +4570,62 @@ void FrameView::updateLayoutAndStyleIfNeededRecursive() AnimationUpdateBlock animationUpdateBlock(&frame().animation()); - auto updateOnce = [this] { - auto updateOneFrame = [] (FrameView& view) { - bool didWork = view.frame().document()->updateStyleIfNeeded(); - if (view.needsLayout()) { - view.layout(); - didWork = true; + using DescendantsDeque = Deque, 16>; + auto nextRenderedDescendant = [this] (DescendantsDeque& descendantsDeque) -> RefPtr { + 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); } - return didWork; - }; + if (descendantsDeque.isEmpty()) + return nullptr; + } + return descendantsDeque.first().ptr(); + }; + for (unsigned i = 0; i < maxUpdatePasses; ++i) { bool didWork = false; - - // Use a copy of the child frame list because it can change while updating. - Deque, 16> views; - views.append(*this); - while (!views.isEmpty()) { - auto view = views.takeFirst(); - if (updateOneFrame(view.get())) + DescendantsDeque deque; + while (auto view = nextRenderedDescendant(deque)) { + if (view->frame().document()->updateStyleIfNeeded()) + didWork = true; + if (view->needsLayout()) { + view->layout(); didWork = true; - appendRenderedChildren(view.get(), views); + } } + if (!didWork) + break; + } - return didWork; +#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; - } + auto needsLayout = [&] { + DescendantsDeque deque; + while (auto view = nextRenderedDescendant(deque)) { + if (view->needsLayout()) + return true; + } + return false; + }; +#endif - // 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()); + ASSERT(!needsStyleRecalc()); + ASSERT(!needsLayout()); } bool FrameView::qualifiesAsVisuallyNonEmpty() const diff --git a/Source/WebCore/page/FrameView.h b/Source/WebCore/page/FrameView.h index 4d4ef9afe70c..04ead091b3ba 100644 --- a/Source/WebCore/page/FrameView.h +++ b/Source/WebCore/page/FrameView.h @@ -122,8 +122,6 @@ class FrameView final : public ScrollView { WEBCORE_EXPORT void setNeedsLayout(); void setViewportConstrainedObjectsNeedLayout(); - bool needsStyleRecalcOrLayout(bool includeSubframes = true) const; - bool needsFullRepaint() const { return m_needsFullRepaint; } WEBCORE_EXPORT bool renderedCharactersExceed(unsigned threshold); @@ -606,9 +604,6 @@ class FrameView final : public ScrollView { const HashSet& widgetsInRenderTree() const { return m_widgetsInRenderTree; } - typedef Vector, 16> FrameViewList; - FrameViewList renderedChildFrameViews() const; - void addTrackedRepaintRect(const FloatRect&); // exposedRect represents WebKit's understanding of what part