Bug 240221

Created attachment 459019 [details] Patch

Comment on attachment 459019 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=459019&action=review Let's see if Carlos Garcia or Adrian have any alternative proposals for how the API should look. I was quite tempted to suggest using WebKitWebsitePolicies, but that is really designed around the decide-policy API, which this is unrelated to. > Source/WebKit/ChangeLog:13 > + Both of these are required for a complete WebExtension implementation in > + browsers such as Epiphany. For posterity: https://gitlab.gnome.org/GNOME/epiphany/-/issues/1777 > Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:29 > +#include <WebCore/ContentSecurityPolicy.h> I'm surprised style checker didn't complain about this. <> includes sort farther down, below all the "" includes. > Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:1476 > + * In practice this limits the Content-Security-Policies that are allowed to be set. Would be interesting to see a little more information here. You know all about it because you are a CSP expert, but I had to look into ContentSecurityPolicySourceList.cpp to see that it was doing anything at all. > Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:5148 > +/** > + * webkit_web_view_set_default_content_security_policy: > + * @web_view: a #WebKitWebView > + * @policy: (nullable): a Content-Security-Policy > + * > + * This configures created #WebKitWebView's to use @policy as its initial > + * Content-Security-Policy as if it were set by an HTTP header. OK, but the GObject property is construct only, so why does this exist? Either the property should not be construct only, or there should be no setter. I think construct-only is probably the way to go, because potential for error is high otherwise. The new policy will only take effect for a new WebPage, but page swapping is opaque to API users and very confusing. I bet 95% of developers using WebKit don't understand this. And it would probably be extremely unusual that you would ever want to call this function, right? > Source/WebKit/UIProcess/API/gtk/WebKitWebView.h:225 > + * WebKitWebExtensionMode: Sadly we hit terminology hell here. WebKitWebExtension is totally unrelated to WebExtensions. And here you have the first WebKit API for WebExtensions. Calling it anything other than WebKitWebExtensionMode would be weird, but it's also absurdly confusing because it's not related to WebKitWebExtension. Maybe it is OK because bug #226665 proposes to fix this in the GTK 4 and new WPE APIs. Will be really confusing in the old APIs, though. At least add a sentence to document that this is for WebExtensions and not WebKitWebExtensions. > Source/WebKit/UIProcess/API/gtk/WebKitWebView.h:228 > + * @WEBKIT_WEB_EXTENSION_MODE_MANIFESTV2: For a ManifestV2 extension. > + * @WEBKIT_WEB_EXTENSION_MODE_MANIFESTV3: For a ManifestV3 extension. I was hoping we could do without this enum entirely if we limit ourselves to only supporting Manifest v3, but I see that NONE really needs to be the default? I suppose enum is best, that way we're covered whenever manifest v4 rolls around. > Source/WebKit/UIProcess/API/wpe/WebKitWebView.h:638 > +WEBKIT_API void > +webkit_web_view_set_web_extension_mode (WebKitWebView *web_view, > + WebKitWebExtensionMode mode); Indentation error here > Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitWebView.cpp:1720 > + // Create a new web view with a policy that blocks eval(). > + auto webView = Test::adoptView(g_object_new(WEBKIT_TYPE_WEB_VIEW, > + "default-content-security-policy", "script-src 'self'", nullptr)); > + > + // Ensure JavaScript still functions. > + javascriptResult = test->runJavaScriptAndWaitUntilFinished("'allowed'", &error.outPtr(), webView.get()); > + g_assert_nonnull(javascriptResult); > + g_assert_no_error(error.get()); > + GUniquePtr<char> value(WebViewTest::javascriptResultToCString(javascriptResult)); > + g_assert_cmpstr(value.get(), ==, "allowed"); > + webkit_javascript_result_unref(javascriptResult); > + > + // Then ensure eval is blocked. > + javascriptResult = test->runJavaScriptAndWaitUntilFinished("eval('allowed')", &error.outPtr(), webView.get()); > + g_assert_null(javascriptResult); > + g_assert_error(error.get(), WEBKIT_JAVASCRIPT_ERROR, WEBKIT_JAVASCRIPT_ERROR_SCRIPT_FAILED); Please also test to ensure eval works when you *don't* use default-content-security-policy. And test webkit_web_view_set_default_content_security_policy()/webkit_web_view_get_default_content_security_policy(). If you retain webkit_web_view_set_default_content_security_policy(), you should test to see what happens when you change it after the web view is created. We need tests for WebKitWebExtensionMode too.

Created attachment 459086 [details] Patch

Comment on attachment 459086 [details] Patch Better, thanks. I would still provide getter functions for the new properties, not because it's reuired, but because it's conventional. Omitting the setters makes sense since they're construct only. Nice tests. I think the changes to WebViewTest are OK. No doubt some functions of WebViewTest might not work if using the new optional parameters, but presumably that is obvious enough that it won't be a problem? Let's see what other reviewers think.

Created attachment 459180 [details] Patch

Comment on attachment 459180 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=459180&action=review OK, r=me. Needs a second GTK/WPE reviewer, since it adds new API. > Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:5116 > + * Returns: the #WebKitWebExtensionMode > + * Since: 2.38 Need a blank line here. > Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:5132 > + * Returns: (nullable): The default policy or %NULL > + * Since: 2.38 And here. > Tools/TestWebKitAPI/glib/WebKitGLib/WebViewTest.h:40 > - virtual void loadHtml(const char* html, const char* baseURI); > + virtual void loadHtml(const char* html, const char* baseURI, WebKitWebView* = nullptr); I see you copied a new pattern introduced in bug #213884. I would check with cgarcia to be sure he's OK with these changes to WebViewTest.

Created attachment 459239 [details] Patch

Pull request: https://github.com/WebKit/WebKit/pull/978

Committed r295679 (251684@main): <https://commits.webkit.org/251684@main> Reviewed commits have been landed. Closing PR #978 and removing active labels.

Looks like this introduced some new clang warnings: [1662/1856] Building CXX object Tools/TestWebKitAPI/glib/CMakeFiles/TestAuthentication.dir/__/Tests/WebKitGLib/TestAuthentication.cpp.o In file included from /app/webkit/Tools/TestWebKitAPI/Tests/WebKitGLib/TestAuthentication.cpp:21: /app/webkit/Tools/TestWebKitAPI/glib/WebKitGLib/LoadTrackingTest.h:42:10: warning: 'LoadTrackingTest::loadHtml' hides overloaded virtual function [-Woverloaded-virtual] void loadHtml(const char* html, const char* baseURI); ^ /app/webkit/Tools/TestWebKitAPI/glib/WebKitGLib/WebViewTest.h:40:18: note: hidden overloaded virtual function 'WebViewTest::loadHtml' declared here: different number of parameters (3 vs 2) virtual void loadHtml(const char* html, const char* baseURI, WebKitWebView* = nullptr); ^ 1 warning generated. [1663/1856] Building CXX object Tools/TestWebKitAPI/glib/CMakeFiles/WebKitGLibAPITestsCore.dir/WebKitGLib/LoadTrackingTest.cpp.o In file included from /app/webkit/Tools/TestWebKitAPI/glib/WebKitGLib/LoadTrackingTest.cpp:21: /app/webkit/Tools/TestWebKitAPI/glib/WebKitGLib/LoadTrackingTest.h:42:10: warning: 'LoadTrackingTest::loadHtml' hides overloaded virtual function [-Woverloaded-virtual] void loadHtml(const char* html, const char* baseURI); ^ /app/webkit/Tools/TestWebKitAPI/glib/WebKitGLib/WebViewTest.h:40:18: note: hidden overloaded virtual function 'WebViewTest::loadHtml' declared here: different number of parameters (3 vs 2) virtual void loadHtml(const char* html, const char* baseURI, WebKitWebView* = nullptr); ^ 1 warning generated. [1692/1856] Building CXX object Tools/TestWebKitAPI/glib/CMakeFiles/TestWebKitFindController.dir/__/Tests/WebKitGLib/TestWebKitFindController.cpp.o In file included from /app/webkit/Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitFindController.cpp:22: /app/webkit/Tools/TestWebKitAPI/glib/WebKitGLib/LoadTrackingTest.h:42:10: warning: 'LoadTrackingTest::loadHtml' hides overloaded virtual function [-Woverloaded-virtual] void loadHtml(const char* html, const char* baseURI); ^ /app/webkit/Tools/TestWebKitAPI/glib/WebKitGLib/WebViewTest.h:40:18: note: hidden overloaded virtual function 'WebViewTest::loadHtml' declared here: different number of parameters (3 vs 2) virtual void loadHtml(const char* html, const char* baseURI, WebKitWebView* = nullptr); ^ 1 warning generated. [1696/1856] Building CXX object Tools/TestWebKitAPI/glib/CMakeFiles/TestWebKitPolicyClient.dir/__/Tests/WebKitGLib/TestWebKitPolicyClient.cpp.o In file included from /app/webkit/Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitPolicyClient.cpp:22: /app/webkit/Tools/TestWebKitAPI/glib/WebKitGLib/LoadTrackingTest.h:42:10: warning: 'LoadTrackingTest::loadHtml' hides overloaded virtual function [-Woverloaded-virtual] void loadHtml(const char* html, const char* baseURI); ^ /app/webkit/Tools/TestWebKitAPI/glib/WebKitGLib/WebViewTest.h:40:18: note: hidden overloaded virtual function 'WebViewTest::loadHtml' declared here: different number of parameters (3 vs 2) virtual void loadHtml(const char* html, const char* baseURI, WebKitWebView* = nullptr); ^ 1 warning generated. [1721/1856] Building CXX object Tools/TestWebKitAPI/glib/CMakeFiles/TestLoaderClient.dir/__/Tests/WebKitGLib/TestLoaderClient.cpp.o In file included from /app/webkit/Tools/TestWebKitAPI/Tests/WebKitGLib/TestLoaderClient.cpp:24: /app/webkit/Tools/TestWebKitAPI/glib/WebKitGLib/LoadTrackingTest.h:42:10: warning: 'LoadTrackingTest::loadHtml' hides overloaded virtual function [-Woverloaded-virtual] void loadHtml(const char* html, const char* baseURI); ^ /app/webkit/Tools/TestWebKitAPI/glib/WebKitGLib/WebViewTest.h:40:18: note: hidden overloaded virtual function 'WebViewTest::loadHtml' declared here: different number of parameters (3 vs 2) virtual void loadHtml(const char* html, const char* baseURI, WebKitWebView* = nullptr); ^ 1 warning generated. [1722/1856] Building CXX object Tools/TestWebKitAPI/glib/CMakeFiles/TestSSL.dir/__/Tests/WebKitGLib/TestSSL.cpp.o In file included from /app/webkit/Tools/TestWebKitAPI/Tests/WebKitGLib/TestSSL.cpp:22: /app/webkit/Tools/TestWebKitAPI/glib/WebKitGLib/LoadTrackingTest.h:42:10: warning: 'LoadTrackingTest::loadHtml' hides overloaded virtual function [-Woverloaded-virtual] void loadHtml(const char* html, const char* baseURI); ^ /app/webkit/Tools/TestWebKitAPI/glib/WebKitGLib/WebViewTest.h:40:18: note: hidden overloaded virtual function 'WebViewTest::loadHtml' declared here: different number of parameters (3 vs 2) virtual void loadHtml(const char* html, const char* baseURI, WebKitWebView* = nullptr); ^ 1 warning generated. [1723/1856] Building CXX object Tools/TestWebKitAPI/glib/CMakeFiles/TestWebKitWebContext.dir/__/Tests/WebKitGLib/TestWebKitWebContext.cpp.o In file included from /app/webkit/Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitWebContext.cpp:22: /app/webkit/Tools/TestWebKitAPI/glib/WebKitGLib/LoadTrackingTest.h:42:10: warning: 'LoadTrackingTest::loadHtml' hides overloaded virtual function [-Woverloaded-virtual] void loadHtml(const char* html, const char* baseURI); ^ /app/webkit/Tools/TestWebKitAPI/glib/WebKitGLib/WebViewTest.h:40:18: note: hidden overloaded virtual function 'WebViewTest::loadHtml' declared here: different number of parameters (3 vs 2) virtual void loadHtml(const char* html, const char* baseURI, WebKitWebView* = nullptr); ^ 1 warning generated.

Re-opening for pull request https://github.com/WebKit/WebKit/pull/1735

Committed 251825@main (65bee18661f8): <https://commits.webkit.org/251825@main> Reviewed commits have been landed. Closing PR #1735 and removing active labels.