Flaky IntersectionObserver web platform tests involving style updates
authorajuma@chromium.org <ajuma@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 17 Oct 2018 13:52:32 +0000 (13:52 +0000)
committerajuma@chromium.org <ajuma@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 17 Oct 2018 13:52:32 +0000 (13:52 +0000)
https://bugs.webkit.org/show_bug.cgi?id=189091

Reviewed by Simon Fraser.

Source/WebCore:

Update intersection observations when flushing layers from the WebProcess
to the UIProcess to make the timing of these updates more predictable, and
more consistent with the IntersectionObserver spec, since the spec expects
these updates to happen as part of the "Update the rendering" step in the
HTML EventLoop.

Getting a similar approach to work with WK1 seems to add more complexity than it's
worth, since flushes don't happen for scrolls handled by platform widgets, and
flushes for other invalidations only happen when in compositing mode.

The only remaining timer-driven intersection observation update is for handling
the initial observation on a newly added target, which needs to happen even if
there are no changes triggering a flush.

Tested by the following tests no longer being flaky:
    imported/w3c/web-platform-tests/intersection-observer/bounding-box.html
    imported/w3c/web-platform-tests/intersection-observer/display-none.html
    imported/w3c/web-platform-tests/intersection-observer/containing-block.html

* dom/Document.cpp:
(WebCore::Document::resolveStyle):
(WebCore::Document::updateIntersectionObservations):
(WebCore::Document::scheduleForcedIntersectionObservationUpdate):
(WebCore::Document::scheduleIntersectionObservationUpdate): Deleted.
* dom/Document.h:
* page/FrameView.cpp:
(WebCore::FrameView::flushCompositingStateForThisFrame):
(WebCore::FrameView::viewportContentsChanged):
* page/IntersectionObserver.cpp:
(WebCore::IntersectionObserver::observe):
* page/Page.cpp:
(WebCore::Page::Page):
(WebCore::Page::willDisplayPage):
(WebCore::Page::addDocumentNeedingIntersectionObservationUpdate):
(WebCore::Page::updateIntersectionObservations):
(WebCore::Page::scheduleForcedIntersectionObservationUpdate):
* page/Page.h:

Source/WebKit:

Add a WebPage::willDisplayPage bottleneck that gets called when flushing layers
or, in the non-composited case, when rendering the page into a bitmap.

* WebProcess/WebPage/AcceleratedDrawingArea.cpp:
(WebKit::AcceleratedDrawingArea::updateBackingStoreState):
* WebProcess/WebPage/CoordinatedGraphics/CoordinatedLayerTreeHost.cpp:
(WebKit::CoordinatedLayerTreeHost::layerFlushTimerFired):
* WebProcess/WebPage/DrawingAreaImpl.cpp:
(WebKit::DrawingAreaImpl::display):
* WebProcess/WebPage/RemoteLayerTree/RemoteLayerTreeDrawingArea.mm:
(WebKit::RemoteLayerTreeDrawingArea::flushLayers):
* WebProcess/WebPage/WebPage.cpp:
(WebKit::WebPage::willDisplayPage):
* WebProcess/WebPage/WebPage.h:
* WebProcess/WebPage/mac/TiledCoreAnimationDrawingArea.mm:
(WebKit::TiledCoreAnimationDrawingArea::flushLayers):

LayoutTests:

Remove expectation for tests that are no longer flaky.

Skip IntersectionObserver tests on WK1.

* TestExpectations:
* platform/mac-wk1/TestExpectations:

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

18 files changed:
LayoutTests/ChangeLog
LayoutTests/TestExpectations
LayoutTests/platform/mac-wk1/TestExpectations
Source/WebCore/ChangeLog
Source/WebCore/dom/Document.cpp
Source/WebCore/dom/Document.h
Source/WebCore/page/FrameView.cpp
Source/WebCore/page/IntersectionObserver.cpp
Source/WebCore/page/Page.cpp
Source/WebCore/page/Page.h
Source/WebKit/ChangeLog
Source/WebKit/WebProcess/WebPage/AcceleratedDrawingArea.cpp
Source/WebKit/WebProcess/WebPage/CoordinatedGraphics/CoordinatedLayerTreeHost.cpp
Source/WebKit/WebProcess/WebPage/DrawingAreaImpl.cpp
Source/WebKit/WebProcess/WebPage/RemoteLayerTree/RemoteLayerTreeDrawingArea.mm
Source/WebKit/WebProcess/WebPage/WebPage.cpp
Source/WebKit/WebProcess/WebPage/WebPage.h
Source/WebKit/WebProcess/WebPage/mac/TiledCoreAnimationDrawingArea.mm

index bc2797c..4410f20 100644 (file)
@@ -1,3 +1,17 @@
+2018-10-17  Ali Juma  <ajuma@chromium.org>
+
+        Flaky IntersectionObserver web platform tests involving style updates
+        https://bugs.webkit.org/show_bug.cgi?id=189091
+
+        Reviewed by Simon Fraser.
+
+        Remove expectation for tests that are no longer flaky.
+
+        Skip IntersectionObserver tests on WK1.
+
+        * TestExpectations:
+        * platform/mac-wk1/TestExpectations:
+
 2018-10-17  Charlie Turner  <cturner@igalia.com>
 
         [EME] media/encrypted-media/mock-MediaKeySystemAccess.html crashes in CDM::createInstance
index 4b6adae..0744509 100644 (file)
@@ -2842,10 +2842,6 @@ webkit.org/b/175609 imported/w3c/web-platform-tests/IndexedDB/idbcursor-continue
 webkit.org/b/175609 imported/w3c/web-platform-tests/IndexedDB/idbdatabase-deleteObjectStore-exception-order.htm [ Pass Failure ]
 webkit.org/b/175609 imported/w3c/web-platform-tests/IndexedDB/idbobjectstore_getAllKeys.html [ Pass Failure ]
 
-webkit.org/b/189091 imported/w3c/web-platform-tests/intersection-observer/bounding-box.html [ Pass Failure ]
-webkit.org/b/189091 imported/w3c/web-platform-tests/intersection-observer/display-none.html [ Pass Failure ]
-webkit.org/b/189091 imported/w3c/web-platform-tests/intersection-observer/containing-block.html [ Pass Failure ]
-
 webkit.org/b/186848 imported/w3c/web-platform-tests/FileAPI/blob/Blob-slice.html [ Pass Failure ]
 webkit.org/b/179176 svg/wicd/test-rightsizing-a.xhtml [ Pass Failure ]
 
index 37b9bd0..b1d84fd 100644 (file)
@@ -524,7 +524,9 @@ webkit.org/b/179775 imported/w3c/web-platform-tests/xhr/firing-events-http-no-co
 
 webkit.org/b/172044 [ Debug ] imported/w3c/web-platform-tests/IndexedDB/open-request-queue.html [ Pass Timeout ]
 
-webkit.org/b/189731 intersection-observer/no-document-leak.html [ Pass Timeout ]
+# Not supported on WK1
+imported/w3c/web-platform-tests/intersection-observer [ Skip ]
+intersection-observer [ Skip ]
 
 webkit.org/b/180898 accessibility/mac/AOM-events.html [ Skip ]
 webkit.org/b/183023 accessibility/mac/AOM-events-all.html [ Skip ]
index 8b3123f..aa46b77 100644 (file)
@@ -1,3 +1,48 @@
+2018-10-17  Ali Juma  <ajuma@chromium.org>
+
+        Flaky IntersectionObserver web platform tests involving style updates
+        https://bugs.webkit.org/show_bug.cgi?id=189091
+
+        Reviewed by Simon Fraser.
+
+        Update intersection observations when flushing layers from the WebProcess
+        to the UIProcess to make the timing of these updates more predictable, and
+        more consistent with the IntersectionObserver spec, since the spec expects
+        these updates to happen as part of the "Update the rendering" step in the
+        HTML EventLoop.
+
+        Getting a similar approach to work with WK1 seems to add more complexity than it's
+        worth, since flushes don't happen for scrolls handled by platform widgets, and
+        flushes for other invalidations only happen when in compositing mode.
+
+        The only remaining timer-driven intersection observation update is for handling
+        the initial observation on a newly added target, which needs to happen even if
+        there are no changes triggering a flush.
+
+        Tested by the following tests no longer being flaky:
+            imported/w3c/web-platform-tests/intersection-observer/bounding-box.html
+            imported/w3c/web-platform-tests/intersection-observer/display-none.html
+            imported/w3c/web-platform-tests/intersection-observer/containing-block.html
+
+        * dom/Document.cpp:
+        (WebCore::Document::resolveStyle):
+        (WebCore::Document::updateIntersectionObservations):
+        (WebCore::Document::scheduleForcedIntersectionObservationUpdate):
+        (WebCore::Document::scheduleIntersectionObservationUpdate): Deleted.
+        * dom/Document.h:
+        * page/FrameView.cpp:
+        (WebCore::FrameView::flushCompositingStateForThisFrame):
+        (WebCore::FrameView::viewportContentsChanged):
+        * page/IntersectionObserver.cpp:
+        (WebCore::IntersectionObserver::observe):
+        * page/Page.cpp:
+        (WebCore::Page::Page):
+        (WebCore::Page::willDisplayPage):
+        (WebCore::Page::addDocumentNeedingIntersectionObservationUpdate):
+        (WebCore::Page::updateIntersectionObservations):
+        (WebCore::Page::scheduleForcedIntersectionObservationUpdate):
+        * page/Page.h:
+
 2018-10-17  Charlie Turner  <cturner@igalia.com>
 
         [EME] Sanity check key ID length in the keyids init data format
index 582c394..9927e07 100644 (file)
@@ -510,7 +510,6 @@ Document::Document(Frame* frame, const URL& url, unsigned documentClasses, unsig
     , m_documentClasses(documentClasses)
     , m_eventQueue(*this)
 #if ENABLE(INTERSECTION_OBSERVER)
-    , m_intersectionObservationUpdateTimer(*this, &Document::updateIntersectionObservations)
     , m_intersectionObserversNotifyTimer(*this, &Document::notifyIntersectionObserversTimerFired)
 #endif
     , m_loadEventDelayTimer(*this, &Document::loadEventDelayTimerFired)
@@ -1929,8 +1928,8 @@ void Document::resolveStyle(ResolveStyleType type)
         // Usually this is handled by post-layout.
         if (!frameView.needsLayout()) {
             frameView.frame().selection().scheduleAppearanceUpdateAfterStyleChange();
-            if (m_needsIntersectionObservationUpdate)
-                scheduleIntersectionObservationUpdate();
+            if (m_needsForcedIntersectionObservationUpdate)
+                page()->scheduleForcedIntersectionObservationUpdate(*this);
         }
 
         // As a result of the style recalculation, the currently hovered element might have been
@@ -7645,7 +7644,6 @@ static void computeIntersectionRects(FrameView& frameView, IntersectionObserver&
 
 void Document::updateIntersectionObservations()
 {
-    ASSERT(m_needsIntersectionObservationUpdate);
     auto* frameView = view();
     if (!frameView)
         return;
@@ -7654,7 +7652,7 @@ void Document::updateIntersectionObservations()
     if (needsLayout || hasPendingStyleRecalc())
         return;
 
-    m_needsIntersectionObservationUpdate = false;
+    m_needsForcedIntersectionObservationUpdate = false;
 
     for (auto observer : m_intersectionObservers) {
         bool needNotify = false;
@@ -7719,13 +7717,15 @@ void Document::updateIntersectionObservations()
         m_intersectionObserversNotifyTimer.startOneShot(0_s);
 }
 
-void Document::scheduleIntersectionObservationUpdate()
+void Document::scheduleForcedIntersectionObservationUpdate()
 {
-    if (m_intersectionObservers.isEmpty() || m_intersectionObservationUpdateTimer.isActive())
+    ASSERT(!m_intersectionObservers.isEmpty());
+    if (m_needsForcedIntersectionObservationUpdate)
         return;
 
-    m_needsIntersectionObservationUpdate = true;
-    m_intersectionObservationUpdateTimer.startOneShot(0_s);
+    m_needsForcedIntersectionObservationUpdate = true;
+    if (auto* page = this->page())
+        page->scheduleForcedIntersectionObservationUpdate(*this);
 }
 
 void Document::notifyIntersectionObserversTimerFired()
index 5e44874..6531d17 100644 (file)
@@ -1394,8 +1394,8 @@ public:
     void addIntersectionObserver(IntersectionObserver&);
     void removeIntersectionObserver(IntersectionObserver&);
     unsigned numberOfIntersectionObservers() const { return m_intersectionObservers.size(); }
+    void scheduleForcedIntersectionObservationUpdate();
     void updateIntersectionObservations();
-    void scheduleIntersectionObservationUpdate();
 #endif
 
 #if ENABLE(MEDIA_STREAM)
@@ -1825,10 +1825,6 @@ private:
 #if ENABLE(INTERSECTION_OBSERVER)
     Vector<WeakPtr<IntersectionObserver>> m_intersectionObservers;
     Vector<WeakPtr<IntersectionObserver>> m_intersectionObserversWithPendingNotifications;
-
-    // FIXME: Schedule intersection observation updates in a way that fits into the HTML
-    // EventLoop. See https://bugs.webkit.org/show_bug.cgi?id=160711.
-    Timer m_intersectionObservationUpdateTimer;
     Timer m_intersectionObserversNotifyTimer;
 #endif
 
@@ -2017,7 +2013,7 @@ private:
 #endif
 
 #if ENABLE(INTERSECTION_OBSERVER)
-    bool m_needsIntersectionObservationUpdate { false };
+    bool m_needsForcedIntersectionObservationUpdate { false };
 #endif
 
 #if ENABLE(MEDIA_STREAM)
index 074b938..9cb3f16 100644 (file)
@@ -1000,6 +1000,7 @@ bool FrameView::flushCompositingStateForThisFrame(const Frame& rootFrameForFlush
 #endif
 
     renderView->compositor().flushPendingLayerChanges(&rootFrameForFlush == m_frame.ptr());
+
     return true;
 }
 
@@ -2008,8 +2009,12 @@ void FrameView::viewportContentsChanged()
     });
 
 #if ENABLE(INTERSECTION_OBSERVER)
-    if (auto* document = frame().document())
-        document->scheduleIntersectionObservationUpdate();
+    if (auto* document = frame().document()) {
+        if (document->numberOfIntersectionObservers()) {
+            if (auto* page = frame().page())
+                page->addDocumentNeedingIntersectionObservationUpdate(*document);
+        }
+    }
 #endif
 }
 
index 797ea47..cfdaa44 100644 (file)
@@ -156,7 +156,7 @@ void IntersectionObserver::observe(Element& target)
     auto* document = trackingDocument();
     if (!hadObservationTargets)
         document->addIntersectionObserver(*this);
-    document->scheduleIntersectionObservationUpdate();
+    document->scheduleForcedIntersectionObservationUpdate();
 }
 
 void IntersectionObserver::unobserve(Element& target)
index b909cb2..cf82c28 100644 (file)
@@ -242,6 +242,9 @@ Page::Page(PageConfiguration&& pageConfiguration)
     , m_storageNamespaceProvider(*WTFMove(pageConfiguration.storageNamespaceProvider))
     , m_userContentProvider(*WTFMove(pageConfiguration.userContentProvider))
     , m_visitedLinkStore(*WTFMove(pageConfiguration.visitedLinkStore))
+#if ENABLE(INTERSECTION_OBSERVER)
+    , m_intersectionObservationUpdateTimer(*this, &Page::updateIntersectionObservations)
+#endif
     , m_sessionID(PAL::SessionID::defaultSessionID())
 #if ENABLE(VIDEO)
     , m_playbackControlsManagerUpdateTimer(*this, &Page::playbackControlsManagerUpdateTimerFired)
@@ -979,6 +982,13 @@ void Page::didFinishLoad()
         m_performanceMonitor->didFinishLoad();
 }
 
+void Page::willDisplayPage()
+{
+#if ENABLE(INTERSECTION_OBSERVER)
+    updateIntersectionObservations();
+#endif
+}
+
 bool Page::isOnlyNonUtilityPage() const
 {
     return !isUtilityPage() && nonUtilityPageCount == 1;
@@ -1120,6 +1130,32 @@ void Page::removeActivityStateChangeObserver(ActivityStateChangeObserver& observ
     m_activityStateChangeObservers.remove(&observer);
 }
 
+#if ENABLE(INTERSECTION_OBSERVER)
+void Page::addDocumentNeedingIntersectionObservationUpdate(Document& document)
+{
+    if (m_documentsNeedingIntersectionObservationUpdate.find(&document) == notFound)
+        m_documentsNeedingIntersectionObservationUpdate.append(makeWeakPtr(document));
+}
+
+void Page::updateIntersectionObservations()
+{
+    m_intersectionObservationUpdateTimer.stop();
+    for (auto document : m_documentsNeedingIntersectionObservationUpdate) {
+        if (document)
+            document->updateIntersectionObservations();
+    }
+    m_documentsNeedingIntersectionObservationUpdate.clear();
+}
+
+void Page::scheduleForcedIntersectionObservationUpdate(Document& document)
+{
+    addDocumentNeedingIntersectionObservationUpdate(document);
+    if (m_intersectionObservationUpdateTimer.isActive())
+        return;
+    m_intersectionObservationUpdateTimer.startOneShot(0_s);
+}
+#endif
+
 void Page::suspendScriptedAnimations()
 {
     m_scriptedAnimationsSuspended = true;
index 9565a6f..82b6fe6 100644 (file)
@@ -22,6 +22,7 @@
 
 #include "ActivityState.h"
 #include "DisabledAdaptations.h"
+#include "Document.h"
 #include "FindOptions.h"
 #include "FrameLoaderTypes.h"
 #include "LayoutMilestones.h"
@@ -325,6 +326,8 @@ public:
     void didStartProvisionalLoad();
     void didFinishLoad(); // Called when the load has been committed in the main frame.
 
+    WEBCORE_EXPORT void willDisplayPage();
+
     // The view scale factor is multiplied into the page scale factor by all
     // callers of setPageScaleFactor.
     WEBCORE_EXPORT void setViewScaleFactor(float);
@@ -445,6 +448,12 @@ public:
     WEBCORE_EXPORT void addActivityStateChangeObserver(ActivityStateChangeObserver&);
     WEBCORE_EXPORT void removeActivityStateChangeObserver(ActivityStateChangeObserver&);
 
+#if ENABLE(INTERSECTION_OBSERVER)
+    void addDocumentNeedingIntersectionObservationUpdate(Document&);
+    void scheduleForcedIntersectionObservationUpdate(Document&);
+    void updateIntersectionObservations();
+#endif
+
     WEBCORE_EXPORT void suspendScriptedAnimations();
     WEBCORE_EXPORT void resumeScriptedAnimations();
     bool scriptedAnimationsSuspended() const { return m_scriptedAnimationsSuspended; }
@@ -865,6 +874,14 @@ private:
 
     HashSet<ActivityStateChangeObserver*> m_activityStateChangeObservers;
 
+#if ENABLE(INTERSECTION_OBSERVER)
+    Vector<WeakPtr<Document>> m_documentsNeedingIntersectionObservationUpdate;
+
+    // FIXME: Schedule intersection observation updates in a way that fits into the HTML
+    // EventLoop. See https://bugs.webkit.org/show_bug.cgi?id=160711.
+    Timer m_intersectionObservationUpdateTimer;
+#endif
+
 #if ENABLE(RESOURCE_USAGE)
     std::unique_ptr<ResourceUsageOverlay> m_resourceUsageOverlay;
 #endif
index bb038c8..3da5d26 100644 (file)
@@ -1,3 +1,27 @@
+2018-10-17  Ali Juma  <ajuma@chromium.org>
+
+        Flaky IntersectionObserver web platform tests involving style updates
+        https://bugs.webkit.org/show_bug.cgi?id=189091
+
+        Reviewed by Simon Fraser.
+
+        Add a WebPage::willDisplayPage bottleneck that gets called when flushing layers
+        or, in the non-composited case, when rendering the page into a bitmap.
+
+        * WebProcess/WebPage/AcceleratedDrawingArea.cpp:
+        (WebKit::AcceleratedDrawingArea::updateBackingStoreState):
+        * WebProcess/WebPage/CoordinatedGraphics/CoordinatedLayerTreeHost.cpp:
+        (WebKit::CoordinatedLayerTreeHost::layerFlushTimerFired):
+        * WebProcess/WebPage/DrawingAreaImpl.cpp:
+        (WebKit::DrawingAreaImpl::display):
+        * WebProcess/WebPage/RemoteLayerTree/RemoteLayerTreeDrawingArea.mm:
+        (WebKit::RemoteLayerTreeDrawingArea::flushLayers):
+        * WebProcess/WebPage/WebPage.cpp:
+        (WebKit::WebPage::willDisplayPage):
+        * WebProcess/WebPage/WebPage.h:
+        * WebProcess/WebPage/mac/TiledCoreAnimationDrawingArea.mm:
+        (WebKit::TiledCoreAnimationDrawingArea::flushLayers):
+
 2018-10-16  Patrick Griffis  <pgriffis@igalia.com>
 
         [GTK][WPE] Fix xdg-desktop-portal permissions from a sandbox
index f752b5d..7cf8079 100644 (file)
@@ -252,6 +252,7 @@ void AcceleratedDrawingArea::updateBackingStoreState(uint64_t stateID, bool resp
         m_webPage.layoutIfNeeded();
         m_webPage.flushPendingEditorStateUpdate();
         m_webPage.scrollMainFrameIfNotAtMaxScrollPosition(scrollOffset);
+        m_webPage.willDisplayPage();
 
         if (m_layerTreeHost)
             m_layerTreeHost->sizeDidChange(m_webPage.size());
index 7092fc5..aa6ace8 100644 (file)
@@ -189,6 +189,7 @@ void CoordinatedLayerTreeHost::layerFlushTimerFired()
 
     m_coordinator.syncDisplayState();
     m_webPage.flushPendingEditorStateUpdate();
+    m_webPage.willDisplayPage();
 
     if (!m_isValid || !m_coordinator.rootCompositingLayer())
         return;
index cbe1c58..7824e5c 100644 (file)
@@ -406,6 +406,7 @@ void DrawingAreaImpl::display(UpdateInfo& updateInfo)
     if (m_layerTreeHost)
         return;
 
+    m_webPage.willDisplayPage();
     updateInfo.viewSize = m_webPage.size();
     updateInfo.deviceScaleFactor = m_webPage.corePage()->deviceScaleFactor();
 
index 3f5ceed..36c5fcf 100644 (file)
@@ -334,6 +334,7 @@ void RemoteLayerTreeDrawingArea::flushLayers()
     backingStoreCollection.willFlushLayers();
 
     m_webPage.layoutIfNeeded();
+    m_webPage.willDisplayPage();
 
     FloatRect visibleRect(FloatPoint(), m_viewSize);
     if (m_scrolledViewExposedRect)
index 99d765c..28e1908 100644 (file)
@@ -3392,6 +3392,11 @@ void WebPage::didFlushLayerTreeAtTime(MonotonicTime timestamp)
 }
 #endif
 
+void WebPage::willDisplayPage()
+{
+    m_page->willDisplayPage();
+}
+
 WebInspector* WebPage::inspector(LazyCreationPolicy behavior)
 {
     if (m_isClosed)
index 813aa40..babcb49 100644 (file)
@@ -289,6 +289,8 @@ public:
     void didFlushLayerTreeAtTime(MonotonicTime);
 #endif
 
+    void willDisplayPage();
+
     enum class LazyCreationPolicy { UseExistingOnly, CreateIfNeeded };
 
     WebInspector* inspector(LazyCreationPolicy = LazyCreationPolicy::CreateIfNeeded);
index 341796f..6ecacd3 100644 (file)
@@ -419,6 +419,7 @@ bool TiledCoreAnimationDrawingArea::flushLayers()
 
         m_webPage.layoutIfNeeded();
         m_webPage.flushPendingEditorStateUpdate();
+        m_webPage.willDisplayPage();
 
         updateIntrinsicContentSizeIfNeeded();