Changeset 232416 in webkit

Timestamp:

Jun 1, 2018, 1:58:27 PM (8 years ago)

Author:

Chris Dumez

Message:

Regression(r230876): Swipe navigation snapshot may get removed too early
​https://bugs.webkit.org/show_bug.cgi?id=186168
<rdar://problem/39743617>

Reviewed by Tim Horton.

The swipe navigation snapshot would get removed too early when receiving a paint
event after requesting a history navigation but before the provisional load has
actually started. This is because of the asynchronous navigation policy decision
which occurs after requesting to navigate. To address the issue, we now start
listening for events only after the provisional load has started.

• UIProcess/API/Cocoa/WKWebView.mm:

(-[WKWebView _didStartProvisionalLoadForMainFrame]):

• UIProcess/API/Cocoa/WKWebViewInternal.h:
• UIProcess/Cocoa/ViewGestureController.cpp:

(WebKit::ViewGestureController::didStartProvisionalLoadForMainFrame):
(WebKit::ViewGestureController::didReachMainFrameLoadTerminalState):
(WebKit::ViewGestureController::didSameDocumentNavigationForMainFrame):

• UIProcess/Cocoa/ViewGestureController.h:
• UIProcess/ios/PageClientImplIOS.mm:

(WebKit::PageClientImpl::didStartProvisionalLoadForMainFrame):

• UIProcess/ios/ViewGestureControllerIOS.mm:

(WebKit::ViewGestureController::endSwipeGesture):

• UIProcess/mac/PageClientImplMac.h:
• UIProcess/mac/PageClientImplMac.mm:

(WebKit::PageClientImpl::didStartProvisionalLoadForMainFrame):

• UIProcess/mac/ViewGestureControllerMac.mm:

(WebKit::ViewGestureController::endSwipeGesture):

Location:

trunk/Source/WebKit

Files:

• ChangeLog (modified) (1 diff)
• UIProcess/API/Cocoa/WKWebView.mm (modified) (1 diff)
• UIProcess/API/Cocoa/WKWebViewInternal.h (modified) (1 diff)
• UIProcess/Cocoa/ViewGestureController.cpp (modified) (3 diffs)
• UIProcess/Cocoa/ViewGestureController.h (modified) (2 diffs)
• UIProcess/ios/PageClientImplIOS.mm (modified) (1 diff)
• UIProcess/ios/ViewGestureControllerIOS.mm (modified) (1 diff)
• UIProcess/mac/PageClientImplMac.h (modified) (1 diff)
• UIProcess/mac/PageClientImplMac.mm (modified) (1 diff)
• UIProcess/mac/ViewGestureControllerMac.mm (modified) (1 diff)

trunk/Source/WebKit/ChangeLog

@@ +1 @@
+2018-06-01  Chris Dumez  <cdumez@apple.com>
+
+        Regression(r230876): Swipe navigation snapshot may get removed too early
+        https://bugs.webkit.org/show_bug.cgi?id=186168
+        <rdar://problem/39743617>
+
+        Reviewed by Tim Horton.
+
+        The swipe navigation snapshot would get removed too early when receiving a paint
+        event after requesting a history navigation but before the provisional load has
+        actually started. This is because of the asynchronous navigation policy decision
+        which occurs after requesting to navigate. To address the issue, we now start
+        listening for events only after the provisional load has started.
+
+        * UIProcess/API/Cocoa/WKWebView.mm:
+        (-[WKWebView _didStartProvisionalLoadForMainFrame]):
+        * UIProcess/API/Cocoa/WKWebViewInternal.h:
+        * UIProcess/Cocoa/ViewGestureController.cpp:
+        (WebKit::ViewGestureController::didStartProvisionalLoadForMainFrame):
+        (WebKit::ViewGestureController::didReachMainFrameLoadTerminalState):
+        (WebKit::ViewGestureController::didSameDocumentNavigationForMainFrame):
+        * UIProcess/Cocoa/ViewGestureController.h:
+        * UIProcess/ios/PageClientImplIOS.mm:
+        (WebKit::PageClientImpl::didStartProvisionalLoadForMainFrame):
+        * UIProcess/ios/ViewGestureControllerIOS.mm:
+        (WebKit::ViewGestureController::endSwipeGesture):
+        * UIProcess/mac/PageClientImplMac.h:
+        * UIProcess/mac/PageClientImplMac.mm:
+        (WebKit::PageClientImpl::didStartProvisionalLoadForMainFrame):
+        * UIProcess/mac/ViewGestureControllerMac.mm:
+        (WebKit::ViewGestureController::endSwipeGesture):
+
 2018-06-01  Jiewen Tan  <jiewen_tan@apple.com>
 

trunk/Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm

@@ -2828 +2828 @@
         callback();
     }
+}
+
+- (void)_didStartProvisionalLoadForMainFrame
+{
+    if (_gestureController)
+        _gestureController->didStartProvisionalLoadForMainFrame();
 }
 

trunk/Source/WebKit/UIProcess/API/Cocoa/WKWebViewInternal.h

@@ -106 +106 @@
 - (void)_scheduleVisibleContentRectUpdate;
 
+- (void)_didStartProvisionalLoadForMainFrame;
 - (void)_didFinishLoadForMainFrame;
 - (void)_didFailLoadForMainFrame;

trunk/Source/WebKit/UIProcess/Cocoa/ViewGestureController.cpp

@@ -128 +128 @@
 }
 
+void ViewGestureController::didStartProvisionalLoadForMainFrame()
+{
+    if (auto provisionalLoadCallback = WTFMove(m_provisionalLoadCallback))
+        provisionalLoadCallback();
+}
+
 
 void ViewGestureController::didFirstVisuallyNonEmptyLayoutForMainFrame()
@@ -156 +162 @@
 void ViewGestureController::didReachMainFrameLoadTerminalState()
 {
+    if (m_provisionalLoadCallback) {
+        m_provisionalLoadCallback = nullptr;
+        removeSwipeSnapshot();
+        return;
+    }
+
     if (!m_snapshotRemovalTracker.eventOccurred(SnapshotRemovalTracker::MainFrameLoad))
         return;
@@ -180 +192 @@
 void ViewGestureController::didSameDocumentNavigationForMainFrame(SameDocumentNavigationType type)
 {
+    if (m_provisionalLoadCallback) {
+        m_provisionalLoadCallback = nullptr;
+        removeSwipeSnapshot();
+        return;
+    }
+
     bool cancelledOutstandingEvent = false;
 

trunk/Source/WebKit/UIProcess/Cocoa/ViewGestureController.h

@@ -123 +123 @@
     WebCore::Color backgroundColorForCurrentSnapshot() const { return m_backgroundColorForCurrentSnapshot; }
 
+    void didStartProvisionalLoadForMainFrame();
     void didFinishLoadForMainFrame() { didReachMainFrameLoadTerminalState(); }
     void didFailLoadForMainFrame() { didReachMainFrameLoadTerminalState(); }
@@ -296 +297 @@
 #endif
 
+    WTF::Function<void()> m_provisionalLoadCallback;
     SnapshotRemovalTracker m_snapshotRemovalTracker;
 };

trunk/Source/WebKit/UIProcess/ios/PageClientImplIOS.mm

@@ -246 +246 @@
 void PageClientImpl::didStartProvisionalLoadForMainFrame()
 {
+    [m_webView _didStartProvisionalLoadForMainFrame];
     [m_webView _hidePasswordView];
 }

trunk/Source/WebKit/UIProcess/ios/ViewGestureControllerIOS.mm

@@ -297 +297 @@
     m_webPageProxyForBackForwardListForCurrentSwipe->goToBackForwardItem(*targetItem);
 
-    if (auto drawingArea = m_webPageProxy.drawingArea()) {
-        uint64_t pageID = m_webPageProxy.pageID();
-        GestureID gestureID = m_currentGestureID;
-        drawingArea->dispatchAfterEnsuringDrawing([pageID, gestureID] (CallbackBase::Error error) {
-            if (auto gestureController = controllerForGesture(pageID, gestureID))
-                gestureController->willCommitPostSwipeTransitionLayerTree(error == CallbackBase::Error::None);
+    if (!m_webPageProxy.drawingArea()) {
+        removeSwipeSnapshot();
+        return;
+    }
+
+    m_provisionalLoadCallback = [this] {
+        if (auto drawingArea = m_webPageProxy.drawingArea()) {
+            uint64_t pageID = m_webPageProxy.pageID();
+            GestureID gestureID = m_currentGestureID;
+            drawingArea->dispatchAfterEnsuringDrawing([pageID, gestureID] (CallbackBase::Error error) {
+                if (auto gestureController = controllerForGesture(pageID, gestureID))
+                    gestureController->willCommitPostSwipeTransitionLayerTree(error == CallbackBase::Error::None);
+            });
+            drawingArea->hideContentUntilPendingUpdate();
+        } else {
+            removeSwipeSnapshot();
+            return;
+        }
+
+        // FIXME: Should we wait for VisuallyNonEmptyLayout like we do on Mac?
+        m_snapshotRemovalTracker.start(SnapshotRemovalTracker::RenderTreeSizeThreshold
+            | SnapshotRemovalTracker::RepaintAfterNavigation
+            | SnapshotRemovalTracker::MainFrameLoad
+            | SnapshotRemovalTracker::SubresourceLoads
+            | SnapshotRemovalTracker::ScrollPositionRestoration, [this] {
+                this->removeSwipeSnapshot();
         });
-        drawingArea->hideContentUntilPendingUpdate();
-    } else {
-        removeSwipeSnapshot();
-        return;
-    }
-
-    // FIXME: Should we wait for VisuallyNonEmptyLayout like we do on Mac?
-    m_snapshotRemovalTracker.start(SnapshotRemovalTracker::RenderTreeSizeThreshold
-        | SnapshotRemovalTracker::RepaintAfterNavigation
-        | SnapshotRemovalTracker::MainFrameLoad
-        | SnapshotRemovalTracker::SubresourceLoads
-        | SnapshotRemovalTracker::ScrollPositionRestoration, [this] {
-            this->removeSwipeSnapshot();
-    });
+    };
 
     if (ViewSnapshot* snapshot = targetItem->snapshot()) {

trunk/Source/WebKit/UIProcess/mac/PageClientImplMac.h

@@ -206 +206 @@
     NSWindow *activeWindow() const;
 
+    void didStartProvisionalLoadForMainFrame() override;
     void didFirstVisuallyNonEmptyLayoutForMainFrame() override;
     void didFinishLoadForMainFrame() override;

trunk/Source/WebKit/UIProcess/mac/PageClientImplMac.mm

@@ -756 +756 @@
 }
 
+void PageClientImpl::didStartProvisionalLoadForMainFrame()
+{
+    if (auto gestureController = m_impl->gestureController())
+        gestureController->didStartProvisionalLoadForMainFrame();
+}
+
 void PageClientImpl::didFirstVisuallyNonEmptyLayoutForMainFrame()
 {

trunk/Source/WebKit/UIProcess/mac/ViewGestureControllerMac.mm

@@ -744 +744 @@
     m_webPageProxy.goToBackForwardItem(*targetItem);
 
-    SnapshotRemovalTracker::Events desiredEvents = SnapshotRemovalTracker::VisuallyNonEmptyLayout
-        | SnapshotRemovalTracker::MainFrameLoad
-        | SnapshotRemovalTracker::SubresourceLoads
-        | SnapshotRemovalTracker::ScrollPositionRestoration;
-    if (renderTreeSize)
-        desiredEvents |= SnapshotRemovalTracker::RenderTreeSizeThreshold;
-    m_snapshotRemovalTracker.start(desiredEvents, [this] { this->forceRepaintIfNeeded(); });
+    m_provisionalLoadCallback = [this, renderTreeSize] {
+        SnapshotRemovalTracker::Events desiredEvents = SnapshotRemovalTracker::VisuallyNonEmptyLayout
+            | SnapshotRemovalTracker::MainFrameLoad
+            | SnapshotRemovalTracker::SubresourceLoads
+            | SnapshotRemovalTracker::ScrollPositionRestoration;
+        if (renderTreeSize)
+            desiredEvents |= SnapshotRemovalTracker::RenderTreeSizeThreshold;
+        m_snapshotRemovalTracker.start(desiredEvents, [this] { this->forceRepaintIfNeeded(); });
+    };
 
     // FIXME: Like on iOS, we should ensure that even if one of the timeouts fires,