RESOLVED FIXED 207384
[GTK][WPE] Expose allowTopNavigationToDataURL 
https://bugs.webkit.org/show_bug.cgi?id=207384
Summary [GTK][WPE] Expose allowTopNavigationToDataURL
Lauro Moura
Reported 2020-02-07 07:42:44 PST
bug206962/r255961 introduced a new loading behavior blocking by default the top frame from navigating to data URL's. This behavior can be controlled with the `allowTopNavigationToDataURLs` settings. This caused GTK's TestUIClient::allow-modal-dialogs test to timeout.
Attachments
Patch (11.64 KB, patch)
2020-02-07 07:49 PST, Lauro Moura 		no flags
DetailsFormatted DiffDiff
Patch (11.64 KB, patch)
2020-02-07 09:53 PST, Lauro Moura 		no flags
DetailsFormatted DiffDiff
Updated patch after aperez comments (12.01 KB, patch)
2020-02-09 20:02 PST, Lauro Moura 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Lauro Moura
Comment 1 2020-02-07 07:49:05 PST
Created attachment 390084 [details] Patch
EWS Watchlist
Comment 2 2020-02-07 07:50:07 PST
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
Lauro Moura
Comment 3 2020-02-07 07:57:38 PST
Comment on attachment 390084 [details] Patch Removing the review, as it'll have to be updated with correct API versioning.
Lauro Moura
Comment 4 2020-02-07 09:53:35 PST
Created attachment 390093 [details] Patch
Adrian Perez
Comment 5 2020-02-09 12:21:38 PST
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
Lauro Moura
Comment 6 2020-02-09 20:02:56 PST
Created attachment 390224 [details] Updated patch after aperez comments
WebKit Commit Bot
Comment 7 2020-02-09 20:47:20 PST
Comment on attachment 390224 [details] Updated patch after aperez comments Clearing flags on attachment: 390224 Committed r256109: <https://trac.webkit.org/changeset/256109>
WebKit Commit Bot
Comment 8 2020-02-09 20:47:22 PST
All reviewed patches have been landed. Closing bug.
Carlos Garcia Campos
Comment 9 2020-02-10 00:19:55 PST
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.
Adrian Perez
Comment 10 2020-02-10 00:51:38 PST
(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.
Adrian Perez
Comment 11 2020-02-10 01:28:23 PST
(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
Carlos Garcia Campos
Comment 12 2020-02-10 01:32:30 PST
(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.
Note You need to log in before you can comment on or make changes to this bug.