Summary: | First piece of process swapping on navigation | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Brady Eidson <beidson> | ||||||||
Component: | WebKit Misc. | Assignee: | Brady Eidson <beidson> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | aestes, cdumez, commit-queue, dbates, ews-watchlist, japhet, mcatanzaro | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | Other | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 183962 | ||||||||||
Attachments: |
|
Description
Brady Eidson
2018-03-15 10:54:38 PDT
Created attachment 335898 [details]
Patch
Created attachment 335906 [details]
Patch
Comment on attachment 335906 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=335906&action=review > Source/WebCore/loader/FrameLoader.cpp:1407 > + SetForScope<bool> currentLoadShouldCheckNavigationPolicyGuard(m_currentLoadShouldCheckNavigationPolicy, 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). (In reply to Andy Estes from comment #4) > Comment on attachment 335906 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=335906&action=review > > > Source/WebCore/loader/FrameLoader.cpp:1407 > > + SetForScope<bool> currentLoadShouldCheckNavigationPolicyGuard(m_currentLoadShouldCheckNavigationPolicy, request.shouldCheckNavigationPolicy()); > > Can shouldCheckNavigationPolicy be a property of DocumentLoader to avoid > needing a SetForScope thing here? Unique DocumentLoaders can go through multiple rounds of policy checks, as well, so something similar would still be needed. > > > Source/WebKit/UIProcess/API/APINavigation.h:40 > > + Navigation(const Navigation&) = delete; > > WTF_MAKE_NONCOPYABLE? Sure > > > 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). Will try this. Created attachment 336164 [details]
Patch
(In reply to Brady Eidson from comment #6) > Created attachment 336164 [details] > Patch Will cq+ after EWS Comment on attachment 336164 [details] Patch Clearing flags on attachment: 336164 Committed r229778: <https://trac.webkit.org/changeset/229778> Fixed? |