[webkit-reviews] review granted: [Bug 184142] Process swapping on navigation needs to handle server redirects : [Attachment 336927] Patch

[bugzilla-daemon at webkit.org]

Alex Christensen <achristensen at apple.com> has granted Brady Eidson
<beidson at apple.com>'s request for review:
Bug 184142: Process swapping on navigation needs to handle server redirects
https://bugs.webkit.org/show_bug.cgi?id=184142

Attachment 336927: Patch

https://bugs.webkit.org/attachment.cgi?id=336927&action=review




--- Comment #3 from Alex Christensen <achristensen at apple.com> ---
Comment on attachment 336927
  --> https://bugs.webkit.org/attachment.cgi?id=336927
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.