| Summary: | WebPageProxy::loadData should accept ShouldOpenExternalURLsPolicy | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Jiewen Tan <jiewen_tan> | ||||||||
| Component: | WebKit Misc. | Assignee: | Jiewen Tan <jiewen_tan> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | aakash_jain, ap, commit-queue, ggaren, jiewen_tan, webkit-bot-watchers-bugzilla, webkit-bug-importer, youennf | ||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||
| Version: | WebKit Nightly Build | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Jiewen Tan
2019-06-21 12:28:55 PDT
Created attachment 372640 [details]
Patch
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 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. > > 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 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. : ) Created attachment 372649 [details]
Patch
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 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 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 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&. Created attachment 372657 [details]
Patch for landing
Comment on attachment 372657 [details] Patch for landing Clearing flags on attachment: 372657 Committed r246701: <https://trac.webkit.org/changeset/246701> 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 (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. |