Bug 199114 - WebPageProxy::loadData should accept ShouldOpenExternalURLsPolicy
Summary: WebPageProxy::loadData should accept ShouldOpenExternalURLsPolicy
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jiewen Tan
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-06-21 12:28 PDT by Jiewen Tan
Modified: 2019-06-21 17:27 PDT (History)
8 users (show)

See Also:


Attachments
Patch (35.35 KB, patch)
2019-06-21 12:41 PDT, Jiewen Tan
no flags Details | Formatted Diff | Diff
Patch (39.60 KB, patch)
2019-06-21 14:14 PDT, Jiewen Tan
youennf: review+
Details | Formatted Diff | Diff
Patch for landing (53.34 KB, patch)
2019-06-21 15:20 PDT, Jiewen Tan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jiewen Tan 2019-06-21 12:28:55 PDT
WebPageProxy::loadData should accept ShouldOpenExternalURLsPolicy.
Comment 1 Jiewen Tan 2019-06-21 12:29:19 PDT
<rdar://problem/51671674>
Comment 2 Jiewen Tan 2019-06-21 12:41:46 PDT
Created attachment 372640 [details]
Patch
Comment 3 Geoffrey Garen 2019-06-21 12:51:36 PDT
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.
Comment 4 youenn fablet 2019-06-21 13:04:40 PDT
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.
Comment 5 Geoffrey Garen 2019-06-21 13:06:04 PDT
> > 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!
Comment 6 Jiewen Tan 2019-06-21 14:02:14 PDT
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. : )
Comment 7 Jiewen Tan 2019-06-21 14:14:12 PDT
Created attachment 372649 [details]
Patch
Comment 8 youenn fablet 2019-06-21 14:28:07 PDT
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.
Comment 9 Jiewen Tan 2019-06-21 14:43:23 PDT
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!
Comment 10 youenn fablet 2019-06-21 14:46:46 PDT
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&
Comment 11 Jiewen Tan 2019-06-21 14:54:44 PDT
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&.
Comment 12 Jiewen Tan 2019-06-21 15:20:27 PDT
Created attachment 372657 [details]
Patch for landing
Comment 13 WebKit Commit Bot 2019-06-21 15:47:23 PDT
Comment on attachment 372657 [details]
Patch for landing

Clearing flags on attachment: 372657

Committed r246701: <https://trac.webkit.org/changeset/246701>
Comment 14 Aakash Jain 2019-06-21 15:51:45 PDT
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
Comment 15 Jiewen Tan 2019-06-21 16:45:59 PDT
(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.
Comment 16 Jiewen Tan 2019-06-21 17:27:11 PDT
https://trac.webkit.org/changeset/246704/webkit