Attachment 362689 Details for Bug 194924 – Patch

[patch] Patch

bug-194924-20190221204043.patch (text/plain), 8.49 KB, created by Chris Dumez on 2019-02-21 20:40:45 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Chris Dumez

Created: 2019-02-21 20:40:45 PST

Size: 8.49 KB

patch

obsolete

>Subversion Revision: 241864
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index 92c66d4dcfaaa152ea88c3d785a6c4eecb9b0e8e..31cce40a055db81f82ab43effe57cd2b62cc2b83 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,27 @@
>+2019-02-21  Chris Dumez  <cdumez@apple.com>
>+
>+        REGRESSION(PSON) Scroll position is sometimes not restored on history navigation
>+        https://bugs.webkit.org/show_bug.cgi?id=194924
>+        <rdar://problem/48216125>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        When process-swapping, we would create a new WebPage in the new process, which would
>+        call restoreSessionInternal() to restore the HistoryItems based on the UIProcess'
>+        backforward list. The issue is that this session restoring would send HistoryItem
>+        updates back to the UIProcess. Without PSON, this would be unnecessary but harmless.
>+        With PSON though, this may end up overwriting values set by the previous process,
>+        such as the scroll position.
>+
>+        Address the issue by temporarily disabling the HistoryItem update notifications to
>+        the UIProcess while restoring a session.
>+
>+        * UIProcess/API/Cocoa/WKBackForwardListItem.mm:
>+        (-[WKBackForwardListItem _scrollPosition]):
>+        * UIProcess/API/Cocoa/WKBackForwardListItemPrivate.h:
>+        * WebProcess/WebPage/WebPage.cpp:
>+        (WebKit::WebPage::restoreSessionInternal):
>+
> 2019-02-20  Antti Koivisto  <antti@apple.com>
> 
>         Make programmatic frame scrolling work on iOS
>diff --git a/Source/WebKit/UIProcess/API/Cocoa/WKBackForwardListItem.mm b/Source/WebKit/UIProcess/API/Cocoa/WKBackForwardListItem.mm
>index cd818efd5e6d29a9426d188f6ea8e1179257d822..6d8d8b3dbf52da7a35497c0bf5773768f5fb3304 100644
>--- a/Source/WebKit/UIProcess/API/Cocoa/WKBackForwardListItem.mm
>+++ b/Source/WebKit/UIProcess/API/Cocoa/WKBackForwardListItem.mm
>@@ -71,6 +71,11 @@ - (CGImageRef)_copySnapshotForTesting
>     return nullptr;
> }
> 
>+- (CGPoint) _scrollPosition
>+{
>+    return CGPointMake(_item->pageState().mainFrameState.scrollPosition.x(), _item->pageState().mainFrameState.scrollPosition.y());
>+}
>+
> #pragma mark WKObject protocol implementation
> 
> - (API::Object&)_apiObject
>diff --git a/Source/WebKit/UIProcess/API/Cocoa/WKBackForwardListItemPrivate.h b/Source/WebKit/UIProcess/API/Cocoa/WKBackForwardListItemPrivate.h
>index 6238771874ce51950e74284cef818ea3ff2fba26..3d30ac8aab5fa7fb9e2ef151b904253be3b77db3 100644
>--- a/Source/WebKit/UIProcess/API/Cocoa/WKBackForwardListItemPrivate.h
>+++ b/Source/WebKit/UIProcess/API/Cocoa/WKBackForwardListItemPrivate.h
>@@ -32,6 +32,8 @@
> // For testing only.
> - (CGImageRef)_copySnapshotForTesting WK_API_AVAILABLE(macosx(10.12.3), ios(10.3));
> 
>+@property (nonatomic) CGPoint _scrollPosition WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA));
>+
> @end
> 
> #endif
>diff --git a/Source/WebKit/WebProcess/WebPage/WebPage.cpp b/Source/WebKit/WebProcess/WebPage/WebPage.cpp
>index 085d82a8e64d97ab6219948000ab9fc7a9b78fb0..af608173ffaaacb7b0365fe37a1dd49b74f38d97 100644
>--- a/Source/WebKit/WebProcess/WebPage/WebPage.cpp
>+++ b/Source/WebKit/WebProcess/WebPage/WebPage.cpp
>@@ -2702,6 +2702,8 @@ void WebPage::setNeedsFontAttributes(bool needsFontAttributes)
> 
> void WebPage::restoreSessionInternal(const Vector<BackForwardListItemState>& itemStates, WasRestoredByAPIRequest restoredByAPIRequest, WebBackForwardListProxy::OverwriteExistingItem overwrite)
> {
>+    // Since we're merely restoring HistoryItems from the UIProcess, there is no need to send HistoryItem update notifications back to the UIProcess.
>+    SetForScope<void (*)(WebCore::HistoryItem&)> bypassHistoryItemUpdateNotifications(WebCore::notifyHistoryItemChanged, [](WebCore::HistoryItem&){});
>     for (const auto& itemState : itemStates) {
>         auto historyItem = toHistoryItem(itemState);
>         historyItem->setWasRestoredFromSession(restoredByAPIRequest == WasRestoredByAPIRequest::Yes);
>diff --git a/Tools/ChangeLog b/Tools/ChangeLog
>index 4f32b37f60af1e516528fe5da88f4c1dda354310..b69a36af161d092a1d61720c17967087d146b8ed 100644
>--- a/Tools/ChangeLog
>+++ b/Tools/ChangeLog
>@@ -1,3 +1,15 @@
>+2019-02-21  Chris Dumez  <cdumez@apple.com>
>+
>+        REGRESSION(PSON) Scroll position is sometimes not restored on history navigation
>+        https://bugs.webkit.org/show_bug.cgi?id=194924
>+        <rdar://problem/48216125>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Add API test coverage.
>+
>+        * TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:
>+
> 2019-02-21  Rob Buis  <rbuis@igalia.com>
> 
>         Update MIME type parser
>diff --git a/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm b/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm
>index 37f6773b0c873257db494963ae1cfc8ba1c0e73c..0ac9b28d98d26957b73be32904ee020e276f43db 100644
>--- a/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm
>+++ b/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm
>@@ -29,6 +29,7 @@
> #import "Test.h"
> #import "TestNavigationDelegate.h"
> #import "TestWKWebView.h"
>+#import <WebKit/WKBackForwardListItemPrivate.h>
> #import <WebKit/WKContentRuleListStore.h>
> #import <WebKit/WKNavigationDelegatePrivate.h>
> #import <WebKit/WKNavigationPrivate.h>
>@@ -4794,6 +4795,8 @@ TEST(ProcessSwap, GoBackToSuspendedPageWithMainFrameIDThatIsNotOne)
>     EXPECT_EQ(pid1, pid5);
> }
> 
>+#endif // PLATFORM(MAC)
>+
> static const char* tallPageBytes = R"PSONRESOURCE(
> <!DOCTYPE html>
> <html>
>@@ -4808,11 +4811,30 @@ body {
> </style>
> </head>
> <body>
>+<script>
>+// Pages with dedicated workers do not go into page cache.
>+var myWorker = new Worker('worker.js');
>+</script>
> <a id="testLink" href="pson://www.apple.com/main.html">Test</a>
> </body>
> </html>
> )PSONRESOURCE";
> 
>+static unsigned waitUntilScrollPositionIsRestored(WKWebView *webView)
>+{
>+    unsigned scrollPosition = 0;
>+    do {
>+        [webView evaluateJavaScript:@"window.scrollY" completionHandler: [&] (id result, NSError *error) {
>+            scrollPosition = [result integerValue];
>+            done = true;
>+        }];
>+        TestWebKitAPI::Util::run(&done);
>+        done = false;
>+    } while (!scrollPosition);
>+
>+    return scrollPosition;
>+}
>+
> TEST(ProcessSwap, ScrollPositionRestoration)
> {
>     auto processPoolConfiguration = psonProcessPoolConfiguration();
>@@ -4824,8 +4846,6 @@ TEST(ProcessSwap, ScrollPositionRestoration)
>     [handler addMappingFromURLString:@"pson://www.webkit.org/main.html" toData:tallPageBytes];
>     [webViewConfiguration setURLSchemeHandler:handler.get() forURLScheme:@"PSON"];
> 
>-    [[webViewConfiguration preferences] _setUsesPageCache:NO];
>-
>     auto webView = adoptNS([[WKWebView alloc] initWithFrame:NSMakeRect(0, 0, 800, 600) configuration:webViewConfiguration.get()]);
>     auto delegate = adoptNS([[PSONNavigationDelegate alloc] init]);
>     [webView setNavigationDelegate:delegate.get()];
>@@ -4841,6 +4861,10 @@ TEST(ProcessSwap, ScrollPositionRestoration)
>     TestWebKitAPI::Util::run(&done);
>     done = false;
> 
>+    do {
>+        TestWebKitAPI::Util::sleep(0.05);
>+    } while (lroundf([[[webView backForwardList] currentItem] _scrollPosition].y) != 5000);
>+
>     [webView evaluateJavaScript:@"testLink.click()" completionHandler: nil];
> 
>     TestWebKitAPI::Util::run(&done);
>@@ -4857,15 +4881,38 @@ TEST(ProcessSwap, ScrollPositionRestoration)
>     TestWebKitAPI::Util::run(&done);
>     done = false;
> 
>+    auto scrollPosition = waitUntilScrollPositionIsRestored(webView.get());
>+    EXPECT_EQ(5000U, scrollPosition);
>+
>+    [webView evaluateJavaScript:@"scroll(0, 4000)" completionHandler: [&] (id result, NSError *error) {
>+        done = true;
>+    }];
>+    TestWebKitAPI::Util::run(&done);
>+    done = false;
>+
>+    do {
>+        TestWebKitAPI::Util::sleep(0.05);
>+    } while (lroundf([[[webView backForwardList] currentItem] _scrollPosition].y) != 4000);
>+
>+    [webView evaluateJavaScript:@"testLink.click()" completionHandler: nil];
>+
>+    TestWebKitAPI::Util::run(&done);
>+    done = false;
>+
>     [webView evaluateJavaScript:@"window.scrollY" completionHandler: [&] (id result, NSError *error) {
>-        EXPECT_EQ(5000, [result integerValue]);
>+        EXPECT_EQ(0, [result integerValue]);
>         done = true;
>     }];
>     TestWebKitAPI::Util::run(&done);
>     done = false;
>-}
> 
>-#endif // PLATFORM(MAC)
>+    [webView goBack];
>+    TestWebKitAPI::Util::run(&done);
>+    done = false;
>+
>+    scrollPosition = waitUntilScrollPositionIsRestored(webView.get());
>+    EXPECT_EQ(4000U, scrollPosition);
>+}
> 
> static NSString *blockmeFilter = @"[{\"action\":{\"type\":\"block\"},\"trigger\":{\"url-filter\":\".*blockme.html\"}}]";
>

Flags:

ggaren: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 194924: 362671 | 362681 | 362687 | 362689