[webkit-reviews] review granted: [Bug 183665] First piece of process swapping on navigation : [Attachment 335906] Patch

[bugzilla-daemon at webkit.org]

Andy Estes <aestes at apple.com> has granted Brady Eidson <beidson at apple.com>'s
request for review:
Bug 183665: First piece of process swapping on navigation
https://bugs.webkit.org/show_bug.cgi?id=183665

Attachment 335906: Patch

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




--- Comment #4 from Andy Estes <aestes at apple.com> ---
Comment on attachment 335906
  --> https://bugs.webkit.org/attachment.cgi?id=335906
Patch

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

> Source/WebCore/loader/FrameLoader.cpp:1407
> +    SetForScope<bool>
currentLoadShouldCheckNavigationPolicyGuard(m_currentLoadShouldCheckNavigationP
olicy, request.shouldCheckNavigationPolicy());

Can shouldCheckNavigationPolicy be a property of DocumentLoader to avoid
needing a SetForScope thing here?

> Source/WebKit/UIProcess/API/APINavigation.h:40
> +    Navigation(const Navigation&) = delete;

WTF_MAKE_NONCOPYABLE?

> Source/WebKit/UIProcess/WebPageProxy.cpp:2392
> +    if (m_currentNavigationPolicyListenerID &&
*m_currentNavigationPolicyListenerID == listenerID) {
> +	   m_currentNavigationPolicyListenerID = std::nullopt;
> +
> +	   if (action == PolicyAction::Use && navigation) {
> +	       auto proposedProcess =
process().processPool().processForNavigation(*this,
navigation->request().url());
> +	       if (proposedProcess.ptr() != &process()) {
> +		   action = PolicyAction::Suspend;
> +		   RunLoop::main().dispatch([this, protectedThis =
makeRef(*this), navigation = makeRef(*navigation), proposedProcess =
WTFMove(proposedProcess)]() mutable {
> +		       continueNavigationInNewProcess(navigation.get(),
WTFMove(proposedProcess));
> +		   });
> +	       }
> +	   }
> +    }

I was confused why we only now needed m_currentNavigationPolicyListenerID, and
you explained to me that it was to distinguish decidePolicyForNavigationAction
from the other policy decisions (response, new window). A more direct way to do
this might be to add a type enum to WebFramePolicyListenerProxy and check that
instead. Looks like you can get to the active listener from
WebFrameProxy::m_activeListener (and you could always assert or check that it's
listener ID equals listenerID).