Summary: | Make WebPageProxy always have a API::NavigationClient instead of always having a API::LoaderClient and API::PolicyClient | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alex Christensen <achristensen> | ||||||||||||
Component: | New Bugs | Assignee: | Alex Christensen <achristensen> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | aestes, berto, cgarcia, ews-watchlist, gustavo, mcatanzaro, webkit-bug-importer, youennf | ||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Attachments: |
|
Description
Alex Christensen
2018-08-27 15:09:32 PDT
Created attachment 348209 [details]
Patch
Created attachment 349925 [details]
Patch
Created attachment 349933 [details]
Patch
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API Created attachment 349942 [details]
Patch
Comment on attachment 349942 [details] Patch Can the change log contain some explanations about the change of order between calls to NavigationClient and LoaderClient/PolicyClient. In the case of navigation loads, it makes sense to me NavigationClient should be called even if LoaderClient is set. View in context: https://bugs.webkit.org/attachment.cgi?id=349942&action=review > Source/WebKit/UIProcess/WebPageProxy.cpp:3492 > m_loaderClient->didStartProvisionalLoadForFrame(*this, *frame, navigation.get(), m_process->transformHandlesToObjects(userData.object()).get()); Isn't it a behavioral change here? Previously, whenever we had a m_navigationClient, we were never calling m_loaderClient->didStartProvisionalLoadForFrame. Shouldn't we revert the test so that we continue calling didStartProvisionalNavigation for main frames and call didStartProvisionalLoadForFrame for other frames if loader client is there? > Source/WebKit/UIProcess/WebPageProxy.cpp:3525 > + m_navigationClient->didReceiveServerRedirectForProvisionalNavigation(*this, navigation.get(), m_process->transformHandlesToObjects(userData.object()).get()); Ditto here and below. > Source/WebKit/UIProcess/WebPageProxy.cpp:6922 > + if (RefPtr<API::Data> keyData = m_navigationClient->webCryptoMasterKey(*this)) auto here and below. Comment on attachment 349942 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=349942&action=review >> Source/WebKit/UIProcess/WebPageProxy.cpp:3492 >> m_loaderClient->didStartProvisionalLoadForFrame(*this, *frame, navigation.get(), m_process->transformHandlesToObjects(userData.object()).get()); > > Isn't it a behavioral change here? > Previously, whenever we had a m_navigationClient, we were never calling m_loaderClient->didStartProvisionalLoadForFrame. > Shouldn't we revert the test so that we continue calling didStartProvisionalNavigation for main frames and call didStartProvisionalLoadForFrame for other frames if loader client is there? This is not a meaningful change in behavior. All clients with a LoaderClient do not use a NavigationClient. Preferring the LoaderClient just makes us not break those legacy clients until they migrate to using WKPageSetPageNavigationClient. Modern clients use a NavigationClient, and this is making that the default without breaking legacy software. Created attachment 350036 [details]
Patch
|