Bug 184142

Created attachment 336927 [details] Patch

Comment on attachment 336927 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=336927&action=review > Source/WebKit/ChangeLog:14 > + 3 - A WKWebView is showing content from a.com, we start a load to a.com, that that redirects to b.com. Will future work will make it so a redirect from a to b then back to a will reuse the same process? > Source/WebKit/UIProcess/WebPageProxy.cpp:2397 > + m_mainFrameSetHandler = [this, protectedThis = makeRef(*this), navigation = makeRef(navigation), request = ResourceRequest { navigation.currentRequest() }]() mutable { I think the explicit call to the constructor might be unnecessary. > Source/WebKit/UIProcess/WebPageProxy.cpp:3212 > + if (m_mainFrameSetHandler) { m_mainFrameCreationHandler? > Source/WebKit/UIProcess/WebPageProxy.cpp:3855 > + Ref<WebFramePolicyListenerProxy> listener = frame->setUpPolicyListenerProxy(listenerID, PolicyListenerType::NavigationAction); auto? > Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:162 > + [(id<WKURLSchemeTaskPrivate>)task _didPerformRedirection:redirectResponse.get() newRequest:request.get()]; Wow, this SPI seems really useful! I'm glad we have it. > Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:568 > + RetainPtr<PSONScheme> handler1 = adoptNS([[PSONScheme alloc] init]); You have inconsistent use of auto in your tests.

> > Source/WebKit/ChangeLog:14 > > + 3 - A WKWebView is showing content from a.com, we start a load to a.com, that that redirects to b.com. > > Will future work will make it so a redirect from a to b then back to a will > reuse the same process? Ideally, future work will enable us to do full loading in NetworkProcess without going through WebProcess, including navigations. That would handle this point automatically, although we might want to get to it before having the full loading stuff.

(In reply to Alex Christensen from comment #3) > Comment on attachment 336927 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=336927&action=review > > > Source/WebKit/ChangeLog:14 > > + 3 - A WKWebView is showing content from a.com, we start a load to a.com, that that redirects to b.com. > > Will future work will make it so a redirect from a to b then back to a will > reuse the same process? Maybe. Not necessarily worth it though - At the a.com to b.com redirect we have to fire up a second process, but then reverting back to the a.com process is more work... CPU/Battery tradeoff... I'll file a bug tracking the idea, definitely not convinced it's a slam dunk. > > Source/WebKit/UIProcess/WebPageProxy.cpp:2397 > > + m_mainFrameSetHandler = [this, protectedThis = makeRef(*this), navigation = makeRef(navigation), request = ResourceRequest { navigation.currentRequest() }]() mutable { > > I think the explicit call to the constructor might be unnecessary. Yah it was in some places, maybe not here, will 2x check. > > > Source/WebKit/UIProcess/WebPageProxy.cpp:3212 > > + if (m_mainFrameSetHandler) { > > m_mainFrameCreationHandler? > Sure. > > Source/WebKit/UIProcess/WebPageProxy.cpp:3855 > > + Ref<WebFramePolicyListenerProxy> listener = frame->setUpPolicyListenerProxy(listenerID, PolicyListenerType::NavigationAction); > > auto? Sure. > > > Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:162 > > + [(id<WKURLSchemeTaskPrivate>)task _didPerformRedirection:redirectResponse.get() newRequest:request.get()]; > > Wow, this SPI seems really useful! I'm glad we have it. Nice! I'll CC you on the bugs I file for how it does weird things... :) > > Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:568 > > + RetainPtr<PSONScheme> handler1 = adoptNS([[PSONScheme alloc] init]); > > You have inconsistent use of auto in your tests. Tests tests shmests

(In reply to youenn fablet from comment #4) > > > Source/WebKit/ChangeLog:14 > > > + 3 - A WKWebView is showing content from a.com, we start a load to a.com, that that redirects to b.com. > > > > Will future work will make it so a redirect from a to b then back to a will > > reuse the same process? > > Ideally, future work will enable us to do full loading in NetworkProcess > without going through WebProcess, including navigations. That would handle > this point automatically, although we might want to get to it before having > the full loading stuff. ^^^^ 👍

Created attachment 336997 [details] Patch

Created attachment 337002 [details] Patch

Comment on attachment 337002 [details] Patch Clearing flags on attachment: 337002 Committed r230174: <https://trac.webkit.org/changeset/230174>

All reviewed patches have been landed. Closing bug.

iOS and macOS Debug WK2 tests are exiting early due to crashes after this change. ASSERTION FAILED: navigation->originalRequest().url() == originalRequest.url() /Volumes/Data/slave/highsierra-debug/build/Source/WebKit/UIProcess/WebPageProxy.cpp(3845) : void WebKit::WebPageProxy::decidePolicyForNavigationAction(uint64_t, const WebCore::SecurityOriginData &, uint64_t, WebKit::NavigationActionData &&, const WebKit::FrameInfoData &, uint64_t, const WebCore::ResourceRequest &, WebCore::ResourceRequest &&, uint64_t, const WebKit::UserData &) See:https://build.webkit.org/results/Apple%20High%20Sierra%20Debug%20WK2%20(Tests)/r230176%20(2692)/results.html

Reverted r230174 for reason: Caused LayoutTests to exit early with assertion failures. Committed r230191: <https://trac.webkit.org/changeset/230191>

(In reply to Ryan Haddad from comment #11) > iOS and macOS Debug WK2 tests are exiting early due to crashes after this > change. Annnnnnnnnnnd we don't have EWS for either of them. Still. To this day.

Created attachment 337046 [details] Patch

Comment on attachment 337046 [details] Patch Clearing flags on attachment: 337046 Committed r230195: <https://trac.webkit.org/changeset/230195>

All reviewed patches have been landed. Closing bug.