[webkit-reviews] review granted: [Bug 229203] [WK2] Implement process-swapping based on Cross-Origin-Opener-Policy HTTP header : [Attachment 435785] Patch

[bugzilla-daemon at webkit.org]

Alex Christensen <achristensen at apple.com> has granted Chris Dumez
<cdumez at apple.com>'s request for review:
Bug 229203: [WK2] Implement process-swapping based on
Cross-Origin-Opener-Policy HTTP header
https://bugs.webkit.org/show_bug.cgi?id=229203

Attachment 435785: Patch

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




--- Comment #7 from Alex Christensen <achristensen at apple.com> ---
Comment on attachment 435785
  --> https://bugs.webkit.org/attachment.cgi?id=435785
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=435785&action=review

> Source/WebCore/loader/DocumentLoader.cpp:1042
> +    if (m_isContinuingLoadAfterResponsePolicyCheck) {

Should this be std::exchange(m_isContinuingLoadAfterResponsePolicyCheck,
false)?

> Source/WebKit/UIProcess/ProvisionalPageProxy.cpp:233
> +    auto* previousMainFrame = m_page.mainFrame();

May as well use a RefPtr here.

> Source/WebKit/UIProcess/ProvisionalPageProxy.h:131
> +	   uint64_t listenerID, const UserData&);

Can we keep all the parameters on one line?

> Source/WebKit/UIProcess/WebPageProxy.cpp:5638
> +    if (!navigation) {

Why is this needed now but wasn't before?

> Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:1221
> +    [webViewConfiguration preferences].javaScriptCanOpenWindowsAutomatically
= YES;

Is this only needed on iOS?  Could we only do it on iOS if it is?
setJavaScriptCanOpenWindowsAutomaticallyIfNecessary(webViewConfiguration)

> Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:7226
> +    // When the provisional load starts in the provisional process, kill the
WebView's processes.

I don't understand this comment.