determinePrimarySnapshottedPlugIn() should only traverse visible Frames
authorsimon.fraser@apple.com <simon.fraser@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 13 Feb 2015 19:04:09 +0000 (19:04 +0000)
committersimon.fraser@apple.com <simon.fraser@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 13 Feb 2015 19:04:09 +0000 (19:04 +0000)
https://bugs.webkit.org/show_bug.cgi?id=141547
Part of rdar://problem/18445733.

Reviewed by Anders Carlsson.
Source/WebCore:

There's an expectation from clients that FrameView::updateLayoutAndStyleIfNeededRecursive()
updates layout in all frames, but it uses the widget tree, so only hits frames
that are parented via renderers (i.e. not display:none frames or their descendants).

Moving towards a future where we remove Widgets, fix by adding a FrameTree
traversal function that only finds rendered frames (those with an ownerRenderer).

Not testable.

* page/FrameTree.cpp:
(WebCore::FrameTree::firstRenderedChild):
(WebCore::FrameTree::nextRenderedSibling):
(WebCore::FrameTree::traverseNextRendered):
(printFrames):
* page/FrameTree.h:
* page/FrameView.cpp:
(WebCore::FrameView::updateLayoutAndStyleIfNeededRecursive):

Source/WebKit2:

Use FrameTree::traverseNextRendered() to avoid doing things in unrendered frames
which are not guaranteed to have been laid out.

* WebProcess/WebPage/WebPage.cpp:
(WebKit::WebPage::determinePrimarySnapshottedPlugIn):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@180062 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog
Source/WebCore/WebCore.exp.in
Source/WebCore/page/FrameTree.cpp
Source/WebCore/page/FrameTree.h
Source/WebCore/page/FrameView.cpp
Source/WebKit2/ChangeLog
Source/WebKit2/WebProcess/WebPage/WebPage.cpp

index 2268d81..03d9ece 100644 (file)
@@ -1,3 +1,29 @@
+2015-02-12  Simon Fraser  <simon.fraser@apple.com>
+
+        determinePrimarySnapshottedPlugIn() should only traverse visible Frames
+        https://bugs.webkit.org/show_bug.cgi?id=141547
+        Part of rdar://problem/18445733.
+
+        Reviewed by Anders Carlsson.
+
+        There's an expectation from clients that FrameView::updateLayoutAndStyleIfNeededRecursive()
+        updates layout in all frames, but it uses the widget tree, so only hits frames
+        that are parented via renderers (i.e. not display:none frames or their descendants).
+        
+        Moving towards a future where we remove Widgets, fix by adding a FrameTree 
+        traversal function that only finds rendered frames (those with an ownerRenderer).
+        
+        Not testable.
+
+        * page/FrameTree.cpp:
+        (WebCore::FrameTree::firstRenderedChild):
+        (WebCore::FrameTree::nextRenderedSibling):
+        (WebCore::FrameTree::traverseNextRendered):
+        (printFrames):
+        * page/FrameTree.h:
+        * page/FrameView.cpp:
+        (WebCore::FrameView::updateLayoutAndStyleIfNeededRecursive):
+
 2015-02-13  Alexey Proskuryakov  <ap@apple.com>
 
         TimerBase::m_heapInsertionOrder calculation is racy
index 37dfb33..7844b47 100644 (file)
@@ -2145,6 +2145,7 @@ __ZNK7WebCore9FloatSize6isZeroEv
 __ZNK7WebCore9FrameTree10childCountEv
 __ZNK7WebCore9FrameTree12traverseNextEPKNS_5FrameE
 __ZNK7WebCore9FrameTree14isDescendantOfEPKNS_5FrameE
+__ZNK7WebCore9FrameTree20traverseNextRenderedEPKNS_5FrameE
 __ZNK7WebCore9FrameTree20traverseNextWithWrapEb
 __ZNK7WebCore9FrameTree24traversePreviousWithWrapEb
 __ZNK7WebCore9FrameTree3topEv
index 417a02f..0f827b7 100644 (file)
@@ -356,6 +356,68 @@ Frame* FrameTree::traverseNext(const Frame* stayWithin) const
     return nullptr;
 }
 
+Frame* FrameTree::firstRenderedChild() const
+{
+    Frame* child = firstChild();
+    if (!child)
+        return nullptr;
+    
+    if (child->ownerRenderer())
+        return child;
+
+    while ((child = child->tree().nextSibling())) {
+        if (child->ownerRenderer())
+            return child;
+    }
+    
+    return nullptr;
+}
+
+Frame* FrameTree::nextRenderedSibling() const
+{
+    Frame* sibling = &m_thisFrame;
+
+    while ((sibling = sibling->tree().nextSibling())) {
+        if (sibling->ownerRenderer())
+            return sibling;
+    }
+    
+    return nullptr;
+}
+
+Frame* FrameTree::traverseNextRendered(const Frame* stayWithin) const
+{
+    Frame* child = firstRenderedChild();
+    if (child) {
+        ASSERT(!stayWithin || child->tree().isDescendantOf(stayWithin));
+        return child;
+    }
+
+    if (&m_thisFrame == stayWithin)
+        return nullptr;
+
+    Frame* sibling = nextRenderedSibling();
+    if (sibling) {
+        ASSERT(!stayWithin || sibling->tree().isDescendantOf(stayWithin));
+        return sibling;
+    }
+
+    Frame* frame = &m_thisFrame;
+    while (!sibling && (!stayWithin || frame->tree().parent() != stayWithin)) {
+        frame = frame->tree().parent();
+        if (!frame)
+            return nullptr;
+        sibling = frame->tree().nextRenderedSibling();
+    }
+
+    if (frame) {
+        ASSERT(!stayWithin || !sibling || sibling->tree().isDescendantOf(stayWithin));
+        return sibling;
+    }
+
+    return nullptr;
+}
+
 Frame* FrameTree::traverseNextWithWrap(bool wrap) const
 {
     if (Frame* result = traverseNext())
@@ -428,7 +490,9 @@ static void printFrames(const WebCore::Frame& frame, const WebCore::Frame* targe
     printIndent(indent);
     printf("  renderView=%p\n", view ? view->renderView() : nullptr);
     printIndent(indent);
-    printf("  document=%p (needs style recalc %d)\n", frame.document(), frame.document() ? frame.document()->needsStyleRecalc() : false);
+    printf("  ownerRenderer=%p\n", frame.ownerRenderer());
+    printIndent(indent);
+    printf("  document=%p (needs style recalc %d)\n", frame.document(), frame.document() ? frame.document()->childNeedsStyleRecalc() : false);
     printIndent(indent);
     printf("  uri=%s\n", frame.document()->documentURI().utf8().data());
 
index c54f47d..c036783 100644 (file)
@@ -56,7 +56,10 @@ namespace WebCore {
         Frame* lastChild() const { return m_lastChild; }
 
         WEBCORE_EXPORT bool isDescendantOf(const Frame* ancestor) const;
-        WEBCORE_EXPORT Frame* traverseNext(const Frame* stayWithin = 0) const;
+        
+        WEBCORE_EXPORT Frame* traverseNext(const Frame* stayWithin = nullptr) const;
+        // Rendered means being the main frame or having an ownerRenderer. It may not have been parented in the Widget tree yet (see WidgetHierarchyUpdatesSuspensionScope).
+        WEBCORE_EXPORT Frame* traverseNextRendered(const Frame* stayWithin = nullptr) const;
         WEBCORE_EXPORT Frame* traverseNextWithWrap(bool) const;
         WEBCORE_EXPORT Frame* traversePreviousWithWrap(bool) const;
         
@@ -87,6 +90,9 @@ namespace WebCore {
         Frame* scopedChild(const AtomicString& name, TreeScope*) const;
         unsigned scopedChildCount(TreeScope*) const;
 
+        Frame* firstRenderedChild() const;
+        Frame* nextRenderedSibling() const;
+
         Frame& m_thisFrame;
 
         Frame* m_parent;
@@ -104,7 +110,7 @@ namespace WebCore {
 
 #ifndef NDEBUG
 // Outside the WebCore namespace for ease of invocation from gdb.
-void showFrameTree(const WebCore::Frame*);
+WEBCORE_EXPORT void showFrameTree(const WebCore::Frame*);
 #endif
 
 #endif // FrameTree_h
index de05dc6..7a7c092 100644 (file)
@@ -3969,6 +3969,7 @@ void FrameView::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()) {
index 1476520..a471068 100644 (file)
@@ -1,3 +1,17 @@
+2015-02-12  Simon Fraser  <simon.fraser@apple.com>
+
+        determinePrimarySnapshottedPlugIn() should only traverse visible Frames
+        https://bugs.webkit.org/show_bug.cgi?id=141547
+        Part of rdar://problem/18445733.
+
+        Reviewed by Anders Carlsson.
+        
+        Use FrameTree::traverseNextRendered() to avoid doing things in unrendered frames
+        which are not guaranteed to have been laid out.
+
+        * WebProcess/WebPage/WebPage.cpp:
+        (WebKit::WebPage::determinePrimarySnapshottedPlugIn):
+
 2015-02-13  Antti Koivisto  <antti@apple.com>
 
         WorkQueue should support concurrent queues
index 31c929d..942d7eb 100644 (file)
@@ -4648,7 +4648,7 @@ void WebPage::determinePrimarySnapshottedPlugIn()
     HTMLPlugInImageElement* candidatePlugIn = nullptr;
     unsigned candidatePlugInArea = 0;
 
-    for (Frame* frame = &mainFrame; frame; frame = frame->tree().traverseNext()) {
+    for (Frame* frame = &mainFrame; frame; frame = frame->tree().traverseNextRendered()) {
         if (!frame->loader().subframeLoader().containsPlugins())
             continue;
         if (!frame->document() || !frame->view())