Skip to content

Commit

Permalink
Merge r221596 - Follow up FrameView::updateLayoutAndStyleIfNeededRecu…
Browse files Browse the repository at this point in the history 
…rsive 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.
  • Loading branch information
@darinadler @carlosgcampos
darinadler authored and carlosgcampos committed Oct 16, 2017
1 parent e4349e0 commit 295cfa7
Show file tree
Hide file tree
Showing 3 changed files with 75 additions and 71 deletions.
27 changes: 27 additions & 0 deletions Source/WebCore/ChangeLog
View file
@@ -1,3 +1,30 @@
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-06 Tomas Popela <tpopela@redhat.com>

Missing break in URLParser
Expand Down
114 changes: 48 additions & 66 deletions Source/WebCore/page/FrameView.cpp
View file
Expand Up @@ -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.
Expand Down Expand Up @@ -4574,30 +4555,9 @@ void FrameView::paintOverhangAreas(GraphicsContext& context, const IntRect& hori
ScrollView::paintOverhangAreas(context, horizontalOverhangArea, verticalOverhangArea, dirtyRect);
}

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
// passes equal to the number of levels of DOM nesting. That is why this number is large.
Expand All @@ -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<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);
}
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<Ref<FrameView>, 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
Expand Down
5 changes: 0 additions & 5 deletions Source/WebCore/page/FrameView.h
View file
Expand Up @@ -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);
Expand Down Expand Up @@ -606,9 +604,6 @@ class FrameView final : public ScrollView {

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
Expand Down

0 comments on commit 295cfa7

Please sign in to comment.