Bug 199114 – WebPageProxy::loadData should accept ShouldOpenExternalURLsPolicy

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Jiewen Tan]

WebPageProxy::loadData should accept ShouldOpenExternalURLsPolicy.

[Jiewen Tan]

<rdar://problem/51671674>

[Jiewen Tan]

Created attachment 372640 [details]
Patch

[Geoffrey Garen]

Comment on attachment 372640 [details]
Patch

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

> Source/WebKit/ChangeLog:11
> +        This patch teaches WebPageProxy::loadData to propagate ShouldOpenExternalURLsPolicy policy,
> +        and then utilize it in RedirectSOAuthorizationSession. Therefore, navigations after successful
> +        interceptions will be able to propagate this information from the last navigation.

What's the policy implemented here? From where does loadData propagate ShouldOpenExternalURLsPolicy? Or does it always just set it to Allow?

> Source/WebKit/UIProcess/WebPageProxy.cpp:1214
> +    loadParameters.shouldOpenExternalURLsPolicy = (uint64_t)shouldOpenExternalURLsPolicy;

This should be static_cast.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/TestSOAuthorization.mm:286
> +    configureSOAuthorizationWebView(webView.get(), delegate.get(), true);

true/false as arguments to functions are not terribly readable. I'd suggest using an enum, or putting the true value in a named local variable.

[youenn fablet]

Comment on attachment 372640 [details]
Patch

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

>> Source/WebKit/ChangeLog:11
>> +        interceptions will be able to propagate this information from the last navigation.
> 
> What's the policy implemented here? From where does loadData propagate ShouldOpenExternalURLsPolicy? Or does it always just set it to Allow?

I think the flow is as follows:
- a page load gets intercepted, this page load has an associated ShouldOpenExternalURLsPolicy.
- SOAuthorization intercepts the load, will do the SO process and will finish the navigation by using loadData.
- This loadData is a continuation of the intercepted page load and should have the same policy

> Source/WebKit/UIProcess/Cocoa/SOAuthorization/RedirectSOAuthorizationSession.mm:92
> +        pagePtr->loadData(IPC::DataReference(static_cast<const uint8_t*>(data.bytes), data.length), "text/html"_s, "UTF-8"_s, response.url().string(), nullptr, shouldOpenExternalURLsPolicy(navigationActionPtr->shouldOpenExternalSchemes()));

navigationAction stores the policy, should we instead just get it directly.
This will make sure we use exactly the same, be it ShouldOpenExternalURLsPolicy::ShouldAllowExternalSchemes or ShouldOpenExternalURLsPolicy::ShouldAllow.

>> Source/WebKit/UIProcess/WebPageProxy.cpp:1214
>> +    loadParameters.shouldOpenExternalURLsPolicy = (uint64_t)shouldOpenExternalURLsPolicy;
> 
> This should be static_cast.

We could make loadParameters.shouldOpenExternalURLsPolicy a ShouldOpenExternalURLsPolicy and use the enum decoder/encoder.
That will remove the need for the two static_cast.

[Geoffrey Garen]

> > This should be static_cast.
> 
> We could make loadParameters.shouldOpenExternalURLsPolicy a
> ShouldOpenExternalURLsPolicy and use the enum decoder/encoder.
> That will remove the need for the two static_cast.

Sounds even better!

[Jiewen Tan]

Comment on attachment 372640 [details]
Patch

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

Thanks Geoff and Youenn for reviewing this patch.

>>> Source/WebKit/ChangeLog:11
>>> +        interceptions will be able to propagate this information from the last navigation.
>> 
>> What's the policy implemented here? From where does loadData propagate ShouldOpenExternalURLsPolicy? Or does it always just set it to Allow?
> 
> I think the flow is as follows:
> - a page load gets intercepted, this page load has an associated ShouldOpenExternalURLsPolicy.
> - SOAuthorization intercepts the load, will do the SO process and will finish the navigation by using loadData.
> - This loadData is a continuation of the intercepted page load and should have the same policy

Thanks Youenn for explaining it for me. I should write it in a more comprehensive way.

>> Source/WebKit/UIProcess/Cocoa/SOAuthorization/RedirectSOAuthorizationSession.mm:92
>> +        pagePtr->loadData(IPC::DataReference(static_cast<const uint8_t*>(data.bytes), data.length), "text/html"_s, "UTF-8"_s, response.url().string(), nullptr, shouldOpenExternalURLsPolicy(navigationActionPtr->shouldOpenExternalSchemes()));
> 
> navigationAction stores the policy, should we instead just get it directly.
> This will make sure we use exactly the same, be it ShouldOpenExternalURLsPolicy::ShouldAllowExternalSchemes or ShouldOpenExternalURLsPolicy::ShouldAllow.

We could definitely do that. I like it better!

>>> Source/WebKit/UIProcess/WebPageProxy.cpp:1214
>>> +    loadParameters.shouldOpenExternalURLsPolicy = (uint64_t)shouldOpenExternalURLsPolicy;
>> 
>> This should be static_cast.
> 
> We could make loadParameters.shouldOpenExternalURLsPolicy a ShouldOpenExternalURLsPolicy and use the enum decoder/encoder.
> That will remove the need for the two static_cast.

Sounds right.

>> Tools/TestWebKitAPI/Tests/WebKitCocoa/TestSOAuthorization.mm:286
>> +    configureSOAuthorizationWebView(webView.get(), delegate.get(), true);
> 
> true/false as arguments to functions are not terribly readable. I'd suggest using an enum, or putting the true value in a named local variable.

Fixed. Maybe one day we will have a header that collects all common enums for boolean. : )

[Jiewen Tan]

Created attachment 372649 [details]
Patch

[youenn fablet]

Comment on attachment 372649 [details]
Patch

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

> Source/WebKit/Shared/LoadParameters.h:82
> +template<> struct EnumTraits<WebCore::ShouldOpenExternalURLsPolicy> {

This might be best put next to ShouldOpenExternalURLsPolicy definition.

> Source/WebKit/UIProcess/Cocoa/SOAuthorization/RedirectSOAuthorizationSession.mm:59
> +    auto* navigationActionPtr = navigationAction();

maybe: auto* navigationAction = this->navigationAction();

> Source/WebKit/UIProcess/Cocoa/SOAuthorization/RedirectSOAuthorizationSession.mm:85
>      if (response.httpStatusCode() == 200) {

Do we need that if given the above if checks?

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:1577
> +    ShouldOpenExternalURLsPolicy externalURLsPolicy = static_cast<ShouldOpenExternalURLsPolicy>(loadParameters.shouldOpenExternalURLsPolicy);

No need for static_cast.
We could even remove externalURLsPolicy.

[Jiewen Tan]

Comment on attachment 372649 [details]
Patch

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

Thanks Youenn for r+ this patch.

>> Source/WebKit/Shared/LoadParameters.h:82
>> +template<> struct EnumTraits<WebCore::ShouldOpenExternalURLsPolicy> {
> 
> This might be best put next to ShouldOpenExternalURLsPolicy definition.

Fixed.

>> Source/WebKit/UIProcess/Cocoa/SOAuthorization/RedirectSOAuthorizationSession.mm:59
>> +    auto* navigationActionPtr = navigationAction();
> 
> maybe: auto* navigationAction = this->navigationAction();

I don't know this this-> trick. Cool. Fixed.

> Source/WebKit/UIProcess/Cocoa/SOAuthorization/RedirectSOAuthorizationSession.mm:61
>      auto* pagePtr = page();

I fixed this one as well.

>> Source/WebKit/UIProcess/Cocoa/SOAuthorization/RedirectSOAuthorizationSession.mm:85
>>      if (response.httpStatusCode() == 200) {
> 
> Do we need that if given the above if checks?

We do. It could be other response codes that we should ignore.

>> Source/WebKit/WebProcess/WebPage/WebPage.cpp:1577
>> +    ShouldOpenExternalURLsPolicy externalURLsPolicy = static_cast<ShouldOpenExternalURLsPolicy>(loadParameters.shouldOpenExternalURLsPolicy);
> 
> No need for static_cast.
> We could even remove externalURLsPolicy.

You are right!

[youenn fablet]

Comment on attachment 372649 [details]
Patch

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

>>> Source/WebKit/UIProcess/Cocoa/SOAuthorization/RedirectSOAuthorizationSession.mm:85
>>>      if (response.httpStatusCode() == 200) {
>> 
>> Do we need that if given the above if checks?
> 
> We do. It could be other response codes that we should ignore.

But we do the following check above:
if ((response.httpStatusCode() != 302 && response.httpStatusCode() != 200) || !pagePtr) {
   ...
   return;
}
And it does not seem we change response within this method.
Also, it seems completeInternal could take a const ResourceResponse&

[Jiewen Tan]

Comment on attachment 372649 [details]
Patch

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

>>>> Source/WebKit/UIProcess/Cocoa/SOAuthorization/RedirectSOAuthorizationSession.mm:85
>>>>      if (response.httpStatusCode() == 200) {
>>> 
>>> Do we need that if given the above if checks?
>> 
>> We do. It could be other response codes that we should ignore.
> 
> But we do the following check above:
> if ((response.httpStatusCode() != 302 && response.httpStatusCode() != 200) || !pagePtr) {
>    ...
>    return;
> }
> And it does not seem we change response within this method.
> Also, it seems completeInternal could take a const ResourceResponse&

For sure, but then we will have to check it again to tell if it is a 200 or not. Therefore, I would prefer the current way.

completeInternal could definitely take a const ResourceResponse&.

[Jiewen Tan]

Created attachment 372657 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 372657 [details]
Patch for landing

Clearing flags on attachment: 372657

Committed r246701: <https://trac.webkit.org/changeset/246701>

[Aakash Jain]

This seems to break API tests:

on mac: WKWebView.LoadAlternateHTMLStringFromProvisionalLoadError https://ews-build.webkit.org/#/builders/3/builds/3604

on iOS: ProcessSwap.ContentExtensionBlocksMainLoadThenReloadWithoutExtensions, WKWebView.LoadAlternateHTMLStringFromProvisionalLoadError

https://ews-build.webkit.org/#/builders/9/builds/3510

[Jiewen Tan]

(In reply to Aakash Jain from comment #14)
> This seems to break API tests:
> 
> on mac: WKWebView.LoadAlternateHTMLStringFromProvisionalLoadError
> https://ews-build.webkit.org/#/builders/3/builds/3604
> 
> on iOS:
> ProcessSwap.ContentExtensionBlocksMainLoadThenReloadWithoutExtensions,
> WKWebView.LoadAlternateHTMLStringFromProvisionalLoadError
> 
> https://ews-build.webkit.org/#/builders/9/builds/3510

I should add a default value for LoadParameters::shouldOpenExternalURLsPolicy.

[Jiewen Tan]

https://trac.webkit.org/changeset/246704/webkit