Changeset 266634 in webkit

Timestamp:

Sep 4, 2020 1:17:21 PM (4 years ago)

Author:

sihui_liu@apple.com

Message:

Webpages flash when switching between windows
​https://bugs.webkit.org/show_bug.cgi?id=216012

Reviewed by Tim Horton.

Source/WebCore/PAL:

• pal/spi/cocoa/QuartzCoreSPI.h:

Source/WebKit:

Based on patches from Tim Horton. To avoid flash, we need to make sure view to be selected updates its content
before UI process commits its transaction. We did this by making UI process block until receiving a reply
DidUpdateActivityState from web process. However, web process did not make sure the reply would be sent after
normal rendering update and corresponding transasction commit. Instead, it flushed transactions in progress and
replied. To fix this, now we make web process reply in transaction commit handler after rendering update, if
there is an active transaction.

In the switching case, view to be unselected will detach from root layer in its web process, which makes its
content empty. This change is independent from the UI process commit, so we want this to happen after UI process
commits (which submits the view hierachy change). Otherwise, empty content(white flash) on the unselected view
will be displayed. To fix this, we let UI process send the activity state change message in transaction commit
handler, if there is an active transaction.

• UIProcess/Cocoa/WebPageProxyCocoa.mm:

(WebKit::WebPageProxy::scheduleActivityStateUpdate):

• UIProcess/WebPageProxy.cpp:

(WebKit::WebPageProxy::setSuppressVisibilityUpdates):
(WebKit::WebPageProxy::activityStateDidChange):
(WebKit::WebPageProxy::dispatchActivityStateChange):

• UIProcess/WebPageProxy.h:
• WebProcess/WebPage/mac/TiledCoreAnimationDrawingArea.h:
• WebProcess/WebPage/mac/TiledCoreAnimationDrawingArea.mm:

(WebKit::TiledCoreAnimationDrawingArea::TiledCoreAnimationDrawingArea):
(WebKit::TiledCoreAnimationDrawingArea::updateRendering):
(WebKit::TiledCoreAnimationDrawingArea::handleActivityStateChangeCallbacks):
(WebKit::TiledCoreAnimationDrawingArea::handleActivityStateChangeCallbacksIfNeeded):
(WebKit::TiledCoreAnimationDrawingArea::activityStateDidChange):
(WebKit::TiledCoreAnimationDrawingArea::didUpdateActivityStateTimerFired): Deleted.

Location:

trunk/Source

Files:

• WebCore/PAL/ChangeLog (modified) (1 diff)
• WebCore/PAL/pal/spi/cocoa/QuartzCoreSPI.h (modified) (1 diff)
• WebKit/ChangeLog (modified) (1 diff)
• WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm (modified) (2 diffs)
• WebKit/UIProcess/WebPageProxy.cpp (modified) (3 diffs)
• WebKit/UIProcess/WebPageProxy.h (modified) (2 diffs)
• WebKit/WebProcess/WebPage/mac/TiledCoreAnimationDrawingArea.h (modified) (4 diffs)
• WebKit/WebProcess/WebPage/mac/TiledCoreAnimationDrawingArea.mm (modified) (3 diffs)

trunk/Source/WebCore/PAL/ChangeLog

@@ +1 @@
+2020-09-04  Sihui Liu  <sihui_liu@apple.com>
+
+        Webpages flash when switching between windows
+        https://bugs.webkit.org/show_bug.cgi?id=216012
+
+        Reviewed by Tim Horton.
+
+        * pal/spi/cocoa/QuartzCoreSPI.h:
+
 2020-08-25  Alex Christensen  <achristensen@webkit.org>
 

trunk/Source/WebCore/PAL/pal/spi/cocoa/QuartzCoreSPI.h

@@ -144 +144 @@
 + (CATransactionPhase)currentPhase;
 + (void)synchronize;
++ (uint32_t)currentState;
 @end
 

trunk/Source/WebKit/ChangeLog

@@ +1 @@
+2020-09-04  Sihui Liu  <sihui_liu@apple.com>
+
+        Webpages flash when switching between windows
+        https://bugs.webkit.org/show_bug.cgi?id=216012
+
+        Reviewed by Tim Horton.
+
+        Based on patches from Tim Horton. To avoid flash, we need to make sure view to be selected updates its content 
+        before UI process commits its transaction. We did this by making UI process block until receiving a reply 
+        DidUpdateActivityState from web process. However, web process did not make sure the reply would be sent after 
+        normal rendering update and corresponding transasction commit. Instead, it flushed transactions in progress and 
+        replied. To fix this, now we make web process reply in transaction commit handler after rendering update, if
+        there is an active transaction.
+
+        In the switching case, view to be unselected will detach from root layer in its web process, which makes its 
+        content empty. This change is independent from the UI process commit, so we want this to happen after UI process
+        commits (which submits the view hierachy change). Otherwise, empty content(white flash) on the unselected view 
+        will be displayed. To fix this, we let UI process send the activity state change message in transaction commit 
+        handler, if there is an active transaction.
+
+        * UIProcess/Cocoa/WebPageProxyCocoa.mm:
+        (WebKit::WebPageProxy::scheduleActivityStateUpdate):
+        * UIProcess/WebPageProxy.cpp:
+        (WebKit::WebPageProxy::setSuppressVisibilityUpdates):
+        (WebKit::WebPageProxy::activityStateDidChange):
+        (WebKit::WebPageProxy::dispatchActivityStateChange):
+        * UIProcess/WebPageProxy.h:
+        * WebProcess/WebPage/mac/TiledCoreAnimationDrawingArea.h:
+        * WebProcess/WebPage/mac/TiledCoreAnimationDrawingArea.mm:
+        (WebKit::TiledCoreAnimationDrawingArea::TiledCoreAnimationDrawingArea):
+        (WebKit::TiledCoreAnimationDrawingArea::updateRendering):
+        (WebKit::TiledCoreAnimationDrawingArea::handleActivityStateChangeCallbacks):
+        (WebKit::TiledCoreAnimationDrawingArea::handleActivityStateChangeCallbacksIfNeeded):
+        (WebKit::TiledCoreAnimationDrawingArea::activityStateDidChange):
+        (WebKit::TiledCoreAnimationDrawingArea::didUpdateActivityStateTimerFired): Deleted.
+
 2020-09-04  Per Arne Vollan  <pvollan@apple.com>
 

trunk/Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm

@@ -46 +46 @@
 #import <WebCore/LocalCurrentGraphicsContext.h>
 #import <WebCore/NotImplemented.h>
+#import <WebCore/RunLoopObserver.h>
 #import <WebCore/SearchPopupMenuCocoa.h>
 #import <WebCore/TextAlternativeWithRange.h>
 #import <WebCore/ValidationBubble.h>
 #import <pal/spi/cocoa/NEFilterSourceSPI.h>
+#import <pal/spi/cocoa/QuartzCoreSPI.h>
 #import <wtf/BlockPtr.h>
 #import <wtf/SoftLinking.h>
@@ -488 +490 @@
 #endif // HAVE(QUICKLOOK_THUMBNAILING)
 
+void WebPageProxy::scheduleActivityStateUpdate()
+{
+    bool hasScheduledObserver = m_activityStateChangeDispatcher->isScheduled();
+    bool hasActiveCATransaction = [CATransaction currentState];
+
+    if (hasScheduledObserver && hasActiveCATransaction) {
+        ASSERT(m_hasScheduledActivityStateUpdate);
+        m_hasScheduledActivityStateUpdate = false;
+        m_activityStateChangeDispatcher->invalidate();
+    }
+
+    if (m_hasScheduledActivityStateUpdate)
+        return;
+    m_hasScheduledActivityStateUpdate = true;
+
+    // If there is an active transaction, we need to dispatch the update after the transaction is committed,
+    // to avoid flash caused by web process setting root layer too early.
+    // If there is no active transaction, likely there is no root layer change or change is committed,
+    // then schedule dispatch on runloop observer to collect changes in the same runloop cycle before dispatching.
+    if (hasActiveCATransaction) {
+        [CATransaction addCommitHandler:[weakThis = makeWeakPtr(*this)] {
+            auto protectedThis = makeRefPtr(weakThis.get());
+            if (!protectedThis)
+                return;
+
+            protectedThis->dispatchActivityStateChange();
+        } forPhase:kCATransactionPhasePostCommit];
+        return;
+    }
+
+    m_activityStateChangeDispatcher->schedule();
+}
+
 } // namespace WebKit
 

trunk/Source/WebKit/UIProcess/WebPageProxy.cpp

@@ -1857 +1857 @@
     if (!m_suppressVisibilityUpdates) {
 #if PLATFORM(COCOA)
-        m_activityStateChangeDispatcher->schedule();
+        scheduleActivityStateUpdate();
 #else
         dispatchActivityStateChange();
@@ -1909 +1909 @@
         return;
     }
-    m_activityStateChangeDispatcher->schedule();
+    scheduleActivityStateUpdate();
 #else
     UNUSED_PARAM(dispatchMode);
@@ -1938 +1938 @@
 {
 #if PLATFORM(COCOA)
-    m_activityStateChangeDispatcher->invalidate();
+    if (m_activityStateChangeDispatcher->isScheduled())
+        m_activityStateChangeDispatcher->invalidate();
+    m_hasScheduledActivityStateUpdate = false;
 #endif
 

trunk/Source/WebKit/UIProcess/WebPageProxy.h

@@ -899 +899 @@
     bool scrollPerformanceDataCollectionEnabled() const { return m_scrollPerformanceDataCollectionEnabled; }
     RemoteLayerTreeScrollingPerformanceData* scrollingPerformanceData() { return m_scrollingPerformanceData.get(); }
+
+    void scheduleActivityStateUpdate();
 #endif // PLATFORM(COCOA)
 
@@ -2721 +2723 @@
     std::unique_ptr<RemoteLayerTreeScrollingPerformanceData> m_scrollingPerformanceData;
     bool m_scrollPerformanceDataCollectionEnabled { false };
+
+    bool m_hasScheduledActivityStateUpdate { false };
 #endif
     UserObservablePageCounter::Token m_pageIsUserObservableCount;

trunk/Source/WebKit/WebProcess/WebPage/mac/TiledCoreAnimationDrawingArea.h

@@ -85 +85 @@
 
     void activityStateDidChange(OptionSet<WebCore::ActivityState::Flag> changed, ActivityStateChangeID, const Vector<CallbackID>&) override;
-    void didUpdateActivityStateTimerFired();
 
     void attachViewOverlayGraphicsLayer(WebCore::GraphicsLayer*) override;
@@ -109 +108 @@
     void sendEnterAcceleratedCompositingModeIfNeeded() override;
     void sendDidFirstLayerFlushIfNeeded();
+    void handleActivityStateChangeCallbacksIfNeeded();
+    void handleActivityStateChangeCallbacks();
 
     void adjustTransientZoom(double scale, WebCore::FloatPoint origin) override;
@@ -152 +153 @@
     WebCore::FloatPoint m_transientZoomOrigin;
 
-    RunLoop::Timer<TiledCoreAnimationDrawingArea> m_sendDidUpdateActivityStateTimer;
     Vector<CallbackID> m_nextActivityStateChangeCallbackIDs;
     ActivityStateChangeID m_activityStateChangeID { ActivityStateChangeAsynchronous };
@@ -170 +170 @@
     bool m_needsSendEnterAcceleratedCompositingMode { true };
     bool m_needsSendDidFirstLayerFlush { true };
+    bool m_shouldHandleActivityStateChangeCallbacks { false };
 };
 

trunk/Source/WebKit/WebProcess/WebPage/mac/TiledCoreAnimationDrawingArea.mm

@@ -74 +74 @@
 TiledCoreAnimationDrawingArea::TiledCoreAnimationDrawingArea(WebPage& webPage, const WebPageCreationParameters& parameters)
     : DrawingArea(DrawingAreaTypeTiledCoreAnimation, parameters.drawingAreaIdentifier, webPage)
-    , m_sendDidUpdateActivityStateTimer(RunLoop::main(), this, &TiledCoreAnimationDrawingArea::didUpdateActivityStateTimerFired)
     , m_isPaintingSuspended(!(parameters.activityState & ActivityState::IsVisible))
 {
@@ -493 +492 @@
 
         sendDidFirstLayerFlushIfNeeded();
+        handleActivityStateChangeCallbacksIfNeeded();
         invalidateRenderingUpdateRunLoopObserver();
     }
+}
+
+void TiledCoreAnimationDrawingArea::handleActivityStateChangeCallbacks()
+{
+    if (!m_shouldHandleActivityStateChangeCallbacks)
+        return;
+    m_shouldHandleActivityStateChangeCallbacks = false;
+
+    if (m_activityStateChangeID != ActivityStateChangeAsynchronous)
+        m_webPage.send(Messages::WebPageProxy::DidUpdateActivityState());
+
+    for (auto& callbackID : m_nextActivityStateChangeCallbackIDs)
+        m_webPage.send(Messages::WebPageProxy::VoidCallback(callbackID));
+    m_nextActivityStateChangeCallbackIDs.clear();
+
+    m_activityStateChangeID = ActivityStateChangeAsynchronous;
+}
+
+void TiledCoreAnimationDrawingArea::handleActivityStateChangeCallbacksIfNeeded()
+{
+    if (!m_shouldHandleActivityStateChangeCallbacks)
+        return;
+
+    // If there is no active transaction, likely there is no layer change or change is committed,
+    // perform the callbacks immediately, which may unblock UI process.
+    if (![CATransaction currentState]) {
+        handleActivityStateChangeCallbacks();
+        return;
+    }
+
+    [CATransaction addCommitHandler:[weakThis = makeWeakPtr(*this)] {
+        if (!weakThis)
+            return;
+
+        auto protectedPage = makeRef(weakThis->m_webPage);
+        auto* drawingArea = static_cast<TiledCoreAnimationDrawingArea*>(protectedPage->drawingArea());
+        ASSERT(weakThis.get() == drawingArea);
+        if (drawingArea != weakThis.get())
+            return;
+
+        drawingArea->handleActivityStateChangeCallbacks();
+    } forPhase:kCATransactionPhasePostCommit];
 }
 
@@ -510 +552 @@
 
     if (m_activityStateChangeID != ActivityStateChangeAsynchronous || !m_nextActivityStateChangeCallbackIDs.isEmpty())
-        m_sendDidUpdateActivityStateTimer.startOneShot(0_s);
-}
-
-void TiledCoreAnimationDrawingArea::didUpdateActivityStateTimerFired()
-{
-    [CATransaction flush];
-
-    if (m_activityStateChangeID != ActivityStateChangeAsynchronous)
-        m_webPage.send(Messages::WebPageProxy::DidUpdateActivityState());
-
-    for (const auto& callbackID : m_nextActivityStateChangeCallbackIDs)
-        m_webPage.send(Messages::WebPageProxy::VoidCallback(callbackID));
-
-    m_nextActivityStateChangeCallbackIDs.clear();
-    m_activityStateChangeID = ActivityStateChangeAsynchronous;
+        m_shouldHandleActivityStateChangeCallbacks = true;
+
+    scheduleRenderingUpdate();
 }