Attachment 341774 Details for Bug 186168 – Patch

[patch] Patch

bug-186168-20180601115114.patch (text/plain), 12.30 KB, created by Chris Dumez on 2018-06-01 11:51:15 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Chris Dumez

Created: 2018-06-01 11:51:15 PDT

Size: 12.30 KB

patch

obsolete

>Subversion Revision: 232394
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index eb58aaa9abfeca64406c826a7675c8b840d271b6..42a3cd961b9aabb229f23fbb29f54bedfd379381 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,35 @@
>+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 NOBODY (OOPS!).
>+
>+        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  Carlos Garcia Campos  <cgarcia@igalia.com>
> 
>         Unreviewed. Try to fix GTK+ build with old versions of GTK+ after r232390.
>diff --git a/Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm b/Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm
>index 6719b0056a63c94d49a865c2bb162e66d02f9d7f..d7bd8b0895e9e8903e5a62ecdc3081a60b3d591c 100644
>--- a/Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm
>+++ b/Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm
>@@ -2829,6 +2829,12 @@ - (void)_updateVisibleContentRects
>     }
> }
> 
>+- (void)_didStartProvisionalLoadForMainFrame
>+{
>+    if (_gestureController)
>+        _gestureController->didStartProvisionalLoadForMainFrame();
>+}
>+
> - (void)_didFinishLoadForMainFrame
> {
>     if (_gestureController)
>diff --git a/Source/WebKit/UIProcess/API/Cocoa/WKWebViewInternal.h b/Source/WebKit/UIProcess/API/Cocoa/WKWebViewInternal.h
>index 1be7ffb0958a9edb766a207dda8e57eef3aaf568..9e139d0ec80379ff50ac4d99062f18c2f551a11f 100644
>--- a/Source/WebKit/UIProcess/API/Cocoa/WKWebViewInternal.h
>+++ b/Source/WebKit/UIProcess/API/Cocoa/WKWebViewInternal.h
>@@ -105,6 +105,7 @@ struct PrintInfo;
> 
> - (void)_scheduleVisibleContentRectUpdate;
> 
>+- (void)_didStartProvisionalLoadForMainFrame;
> - (void)_didFinishLoadForMainFrame;
> - (void)_didFailLoadForMainFrame;
> - (void)_didSameDocumentNavigationForMainFrame:(WebKit::SameDocumentNavigationType)navigationType;
>diff --git a/Source/WebKit/UIProcess/Cocoa/ViewGestureController.cpp b/Source/WebKit/UIProcess/Cocoa/ViewGestureController.cpp
>index 8c21c2466a2db2446c3dd00be214fe09e93dc2ac..2b3ba77f31c1187a580992b601612289da62f025 100644
>--- a/Source/WebKit/UIProcess/Cocoa/ViewGestureController.cpp
>+++ b/Source/WebKit/UIProcess/Cocoa/ViewGestureController.cpp
>@@ -127,6 +127,12 @@ bool ViewGestureController::canSwipeInDirection(SwipeDirection direction) const
>     return !!backForwardList.forwardItem();
> }
> 
>+void ViewGestureController::didStartProvisionalLoadForMainFrame()
>+{
>+    if (auto provisionalLoadCallback = WTFMove(m_provisionalLoadCallback))
>+        provisionalLoadCallback();
>+}
>+
> 
> void ViewGestureController::didFirstVisuallyNonEmptyLayoutForMainFrame()
> {
>@@ -155,6 +161,12 @@ void ViewGestureController::didRestoreScrollPosition()
> 
> void ViewGestureController::didReachMainFrameLoadTerminalState()
> {
>+    if (m_provisionalLoadCallback) {
>+        m_provisionalLoadCallback = nullptr;
>+        removeSwipeSnapshot();
>+        return;
>+    }
>+
>     if (!m_snapshotRemovalTracker.eventOccurred(SnapshotRemovalTracker::MainFrameLoad))
>         return;
> 
>@@ -179,6 +191,12 @@ void ViewGestureController::didReachMainFrameLoadTerminalState()
> 
> void ViewGestureController::didSameDocumentNavigationForMainFrame(SameDocumentNavigationType type)
> {
>+    if (m_provisionalLoadCallback) {
>+        m_provisionalLoadCallback = nullptr;
>+        removeSwipeSnapshot();
>+        return;
>+    }
>+
>     bool cancelledOutstandingEvent = false;
> 
>     // Same-document navigations don't have a main frame load or first visually non-empty layout.
>diff --git a/Source/WebKit/UIProcess/Cocoa/ViewGestureController.h b/Source/WebKit/UIProcess/Cocoa/ViewGestureController.h
>index a713a34f5f54afa7c35540d8fcc800a6a954f77d..1a80494c681c00d40c43e662e3781c02b506bfe2 100644
>--- a/Source/WebKit/UIProcess/Cocoa/ViewGestureController.h
>+++ b/Source/WebKit/UIProcess/Cocoa/ViewGestureController.h
>@@ -122,6 +122,7 @@ public:
> 
>     WebCore::Color backgroundColorForCurrentSnapshot() const { return m_backgroundColorForCurrentSnapshot; }
> 
>+    void didStartProvisionalLoadForMainFrame();
>     void didFinishLoadForMainFrame() { didReachMainFrameLoadTerminalState(); }
>     void didFailLoadForMainFrame() { didReachMainFrameLoadTerminalState(); }
>     void didFirstVisuallyNonEmptyLayoutForMainFrame();
>@@ -295,6 +296,7 @@ private:
>     uint64_t m_snapshotRemovalTargetRenderTreeSize { 0 };
> #endif
> 
>+    WTF::Function<void()> m_provisionalLoadCallback;
>     SnapshotRemovalTracker m_snapshotRemovalTracker;
> };
> 
>diff --git a/Source/WebKit/UIProcess/ios/PageClientImplIOS.mm b/Source/WebKit/UIProcess/ios/PageClientImplIOS.mm
>index 6679a44bfe99d329818cb3e12031fe05a5dbc4b1..8cddc5413d533be2fb48fdd3c04385e968cf03fa 100644
>--- a/Source/WebKit/UIProcess/ios/PageClientImplIOS.mm
>+++ b/Source/WebKit/UIProcess/ios/PageClientImplIOS.mm
>@@ -245,6 +245,7 @@ void PageClientImpl::decidePolicyForGeolocationPermissionRequest(WebFrameProxy&
> 
> void PageClientImpl::didStartProvisionalLoadForMainFrame()
> {
>+    [m_webView _didStartProvisionalLoadForMainFrame];
>     [m_webView _hidePasswordView];
> }
> 
>diff --git a/Source/WebKit/UIProcess/ios/ViewGestureControllerIOS.mm b/Source/WebKit/UIProcess/ios/ViewGestureControllerIOS.mm
>index 4b97e32a8c8910c54741aafe742c66b2b3752d58..642955cdc61312ed9cf3d50591a7ed85dbfe8fbb 100644
>--- a/Source/WebKit/UIProcess/ios/ViewGestureControllerIOS.mm
>+++ b/Source/WebKit/UIProcess/ios/ViewGestureControllerIOS.mm
>@@ -296,27 +296,34 @@ void ViewGestureController::endSwipeGesture(WebBackForwardListItem* targetItem,
> 
>     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);
>-        });
>-        drawingArea->hideContentUntilPendingUpdate();
>-    } else {
>+    if (!m_webPageProxy.drawingArea()) {
>         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();
>-    });
>+    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();
>+        });
>+    };
> 
>     if (ViewSnapshot* snapshot = targetItem->snapshot()) {
>         m_backgroundColorForCurrentSnapshot = snapshot->backgroundColor();
>diff --git a/Source/WebKit/UIProcess/mac/PageClientImplMac.h b/Source/WebKit/UIProcess/mac/PageClientImplMac.h
>index ed923b8cb2cb7b13a0def015608d9ab9749751cf..7f8301b51a514dd2e60d0656f8eac7cf0acca34b 100644
>--- a/Source/WebKit/UIProcess/mac/PageClientImplMac.h
>+++ b/Source/WebKit/UIProcess/mac/PageClientImplMac.h
>@@ -205,6 +205,7 @@ private:
>     NSView *activeView() const;
>     NSWindow *activeWindow() const;
> 
>+    void didStartProvisionalLoadForMainFrame() override;
>     void didFirstVisuallyNonEmptyLayoutForMainFrame() override;
>     void didFinishLoadForMainFrame() override;
>     void didFailLoadForMainFrame() override;
>diff --git a/Source/WebKit/UIProcess/mac/PageClientImplMac.mm b/Source/WebKit/UIProcess/mac/PageClientImplMac.mm
>index 1eb779ffe38593dfd6bb6dbf74429a6b61433a24..549d7a3d45a002521c55a781bc98c561f3059485 100644
>--- a/Source/WebKit/UIProcess/mac/PageClientImplMac.mm
>+++ b/Source/WebKit/UIProcess/mac/PageClientImplMac.mm
>@@ -755,6 +755,12 @@ void PageClientImpl::didRemoveNavigationGestureSnapshot()
> #endif
> }
> 
>+void PageClientImpl::didStartProvisionalLoadForMainFrame()
>+{
>+    if (auto gestureController = m_impl->gestureController())
>+        gestureController->didStartProvisionalLoadForMainFrame();
>+}
>+
> void PageClientImpl::didFirstVisuallyNonEmptyLayoutForMainFrame()
> {
>     if (auto gestureController = m_impl->gestureController())
>diff --git a/Source/WebKit/UIProcess/mac/ViewGestureControllerMac.mm b/Source/WebKit/UIProcess/mac/ViewGestureControllerMac.mm
>index e4c191c3d0975f0c271fc7816808de6b8b176394..df9c5a8f72bdf8f20f514868b4a0bad6f60aa896 100644
>--- a/Source/WebKit/UIProcess/mac/ViewGestureControllerMac.mm
>+++ b/Source/WebKit/UIProcess/mac/ViewGestureControllerMac.mm
>@@ -743,13 +743,15 @@ void ViewGestureController::endSwipeGesture(WebBackForwardListItem* targetItem,
>     m_webPageProxy.navigationGestureDidEnd(true, *targetItem);
>     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,
>     // we never show the old page content, instead showing the snapshot background color.

Actions: View | Formatted Diff | Diff

Attachments on bug 186168: 341708 | 341709 | 341721 | 341767 | 341772 | 341774 | 341775