Bug 189012 – Make WebPageProxy always have a API::NavigationClient instead of always having a API::LoaderClient and API::PolicyClient

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Alex Christensen]

Make WebPageProxy always have a API::NavigationClient instead of always having a API::LoaderClient and API::PolicyClient

[Alex Christensen]

Created attachment 348209 [details]
Patch

[Alex Christensen]

Created attachment 349925 [details]
Patch

[Alex Christensen]

Created attachment 349933 [details]
Patch

[EWS Watchlist]

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

[Alex Christensen]

Created attachment 349942 [details]
Patch

[youenn fablet]

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.

[Alex Christensen]

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.

[Alex Christensen]

Created attachment 350036 [details]
Patch

[Alex Christensen]

http://trac.webkit.org/r236154

[Radar WebKit Bug Importer]

<rdar://problem/44574543>