| Summary: | [GTK][WPE] Expose allowTopNavigationToDataURL | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Lauro Moura <lmoura> | ||||||||
| Component: | WebKitGTK | Assignee: | Lauro Moura <lmoura> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | aperez, berto, bugs-noreply, cgarcia, commit-queue, ews-watchlist, gustavo | ||||||||
| Priority: | P2 | ||||||||||
| Version: | Other | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| Bug Depends on: | |||||||||||
| Bug Blocks: | 207454 | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Lauro Moura
2020-02-07 07:42:44 PST
Created attachment 390084 [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 Comment on attachment 390084 [details]
Patch
Removing the review, as it'll have to be updated with correct API versioning.
Created attachment 390093 [details]
Patch
Comment on attachment 390093 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=390093&action=review Patch looks great, thanks for submitting it! I think we should land this, and it only needs a couple of small bits in the API documentation. Feel free to land it after taking care of them. > Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:1449 > + * Whether or not the top frame is allowed to navigate to data URLS. Typo: s/URLS/URLs It would be nicer to have a couple of lines additional lines here in the API documentation explaining the reason for the default (disabled), and telling why some application which embeds a WebKitWebView might want to enable it. For some cues as to why disabling top-level data:// loads be default is good, there's this Mozilla blog post: https://blog.mozilla.org/security/2017/11/27/blocking-top-level-navigations-data-urls-firefox-59/ As for when to enable it, I think that the main use-case is when an application which is *NOT* a generic web browser embeds a WebKitWebView widget and wants to load data:// URLs using webkit_web_view_load_uri() to provide content to be displayed that is controlled by the application. > Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:1457 > + _("Whether or not top frame navigation is allowed to data URLS"), Typo: s/URLS/URLs Created attachment 390224 [details]
Updated patch after aperez comments
Comment on attachment 390224 [details] Updated patch after aperez comments Clearing flags on attachment: 390224 Committed r256109: <https://trac.webkit.org/changeset/256109> All reviewed patches have been landed. Closing bug. Comment on attachment 390224 [details] Updated patch after aperez comments View in context: https://bugs.webkit.org/attachment.cgi?id=390224&action=review > Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:1454 > + * Since: 2.28 r255961 hasn't been merged in 2.28 branch. I think it's better to leave this for 2.30, it's probably too late in the cycle for a change in behavior like this. > Tools/TestWebKitAPI/Tests/WebKitGLib/TestUIClient.cpp:537 > + webkit_settings_set_allow_top_navigation_to_data_urls(settings, TRUE); > > test->loadHtml("<html><body onload=\"window.showModalDialog('data:text/html,<html><body/><script>window.close();</script></html>')\"></body></html>", 0); > test->waitUntilMainLoopFinishes(); Maybe it would be better to rework this test not to use a data URL. On the other hand it's good to test that the setting actually works, so I'm not sure. (In reply to Carlos Garcia Campos from comment #9) > Comment on attachment 390224 [details] > Updated patch after aperez comments > > View in context: > https://bugs.webkit.org/attachment.cgi?id=390224&action=review > > > Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:1454 > > + * Since: 2.28 > > r255961 hasn't been merged in 2.28 branch. I think it's better to leave this > for 2.30, it's probably too late in the cycle for a change in behavior like > this. I think we could still merge it to the 2.28 branch with the default value for the property set to TRUE, which would keep the existing behavior for 2.28 and also would allow web browsers (Epiphany, Midori, etc.) to set it to FALSE much sooner without accidentally breaking applications at this point of the development cycle. We will also need a small follow-up patch to always allow data:// URLs in the inspector, which I will be posting soon. (In reply to Adrian Perez from comment #10) > [...] > > We will also need a small follow-up patch to always allow data:// URLs > in the inspector, which I will be posting soon. Done in bug #207454 (In reply to Adrian Perez from comment #10) > (In reply to Carlos Garcia Campos from comment #9) > > Comment on attachment 390224 [details] > > Updated patch after aperez comments > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=390224&action=review > > > > > Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:1454 > > > + * Since: 2.28 > > > > r255961 hasn't been merged in 2.28 branch. I think it's better to leave this > > for 2.30, it's probably too late in the cycle for a change in behavior like > > this. > > I think we could still merge it to the 2.28 branch with the default > value for the property set to TRUE, which would keep the existing > behavior for 2.28 and also would allow web browsers (Epiphany, Midori, > etc.) to set it to FALSE much sooner without accidentally breaking > applications at this point of the development cycle. Ok, let's do that. > We will also need a small follow-up patch to always allow data:// URLs > in the inspector, which I will be posting soon. |