Bug 219995

Created attachment 416469 [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 https://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

Comment on attachment 416469 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=416469&action=review I like it! > Source/WebKit/ChangeLog:8 > + No new tests (OOPS!). Constructing a test for this will be a lot harder than just implementing the API, but it's required to land new WebKit API. Something very simple in WebViewTest.cpp should suffice. > Source/WebKit/UIProcess/API/gtk/WebKitWebView.h:594 > + const gchar * const *patternList); gchar* (the * hangs on the type) > Source/WebKit/UIProcess/API/gtk/WebKitWebViewGtk.cpp:469 > +void webkit_web_view_set_cors_disabling_patterns(WebKitWebView* webView, const gchar * const *patternList) Needs a doc comment with introspection annotations and a Since: 2.32 tag. Also, remember that in WebKit (and most C++ projects) the * hangs on the type, not the variable name. So it's: const gchar* const * patternList > Source/WebKit/UIProcess/API/gtk/WebKitWebViewGtk.cpp:477 > + for (size_t i = 0; patternList[i]; ++i) We normally use: i++ (Unless there is really some good reason to use prefix increment, which in this case, there is not.) > Source/WebKit/UIProcess/API/gtk/WebKitWebViewGtk.cpp:480 > + auto& page = *webkitWebViewBaseGetPage(reinterpret_cast<WebKitWebViewBase*>(webView)); I was going to say: you can use a GObject cast: WEBKIT_WEB_VIEW_BASE(webView) But: actually you can't, because WebKitWebViewBase doesn't exist for WPE port. So you'd need an #if PLATFORM(GTK) condition. Then I got confused: why is the WPE EWS green? Finally, I noticed you added this to WebKitWebViewGtk.cpp. OK, well, there is nothing GTK-specific here. So move this to Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp instead. Then, use getPage(webView) to get the page without having to type WebKitWebViewBase directly, and now this will work for WPE. And lastly, modify Source/WebKit/UIProcess/API/wpe/WebKitWebView.h. > Source/WebKit/UIProcess/API/gtk/docs/webkit2gtk-4.0-sections.txt:293 > +webkit_web_view_set_cors_disabling_patterns Needs to be added to WPE docs as well.

Comment on attachment 416469 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=416469&action=review >> Source/WebKit/UIProcess/API/gtk/WebKitWebView.h:594 >> + const gchar * const *patternList); > > gchar* (the * hangs on the type) Oops, that's totally wrong because WebKit code style does not apply in the public headers. You had this correct already.

Created attachment 416474 [details] Patch

Updated version with fixed review findings, EXCEPT test.

Comment on attachment 416474 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=416474&action=review r- because we need an API test. Thanks Jan-Michael! > Source/WebKit/ChangeLog:8 > + No new tests (OOPS!). Remove this line. > Source/WebKit/ChangeLog:13 > + * UIProcess/API/gtk/WebKitWebView.h: > + * UIProcess/API/gtk/WebKitWebViewGtk.cpp: > + (webkit_web_view_set_cors_disabling_patterns): > + * UIProcess/API/gtk/docs/webkit2gtk-4.0-sections.txt: You should regenerate the changelog, since there are more files modified. > Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:4717 > + * webkit_web_view_set_cors_disabling_patterns: what about webkit_web_view_set_cors_allow_list()? > Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:4719 > + * @patterns: a NULL-terminated list of patterns. (array zero-terminated=1) (element-type utf8) (transfer none) (nullable): a %NULL-terminated list of patterns > Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:4721 > + * Sets @pattern for which CORS checks are disabled in @web_view. I think we need more documentation here. It's not clear to me what a valid pattern is (check UserContentURLPattern::parse()). > Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:4729 > + if (!patterns || !g_strv_length(const_cast<char**>(patterns))) I don't think we should return early in this case. Setting an empty list is the only way to reset the patterns. That's what updateCORSDisablingPatterns() does. I think we should mark patterns as nullable and document that if it's NULL or empty the patterns will be reset. I think this is consistent with webkit_user_script_new().

Created attachment 416511 [details] Patch

Updated version with fixed review findings, again without test as earlyoom kills my test api enabled webkit build...

Comment on attachment 416511 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=416511&action=review > Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:4735 > + if (!allowList) > + allowListVector = Vector<String>(); This is not needed, the vector has already been constructed in previous line.

(In reply to Carlos Garcia Campos from comment #7) > Comment on attachment 416474 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=416474&action=review > > [...] > > > Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:4717 > > + * webkit_web_view_set_cors_disabling_patterns: > > what about webkit_web_view_set_cors_allow_list()? I also like _set_cors_allow_list() better, as it is more consistent having a name used elsewhere in the public API =) > > Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:4729 > > + if (!patterns || !g_strv_length(const_cast<char**>(patterns))) > > I don't think we should return early in this case. Setting an empty list is > the only way to reset the patterns. That's what > updateCORSDisablingPatterns() does. I think we should mark patterns as > nullable and document that if it's NULL or empty the patterns will be reset. > I think this is consistent with webkit_user_script_new(). Agreed as well.

Created attachment 416521 [details] Patch

(By the way, you can take my previous comment as a second review agreeing on the API addition, I think!)

Comment on attachment 416521 [details] Patch r- for now so we don't forget that we need to implement the test. (The API itself looks good.)

Comment on attachment 416521 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=416521&action=review > Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:4725 > + * Sets @allow_list for which CORS (Cross-Origin Resource Sharing) checks are disabled in @web_view. > + * Passing a %NULL as @allow_list implies that no URIs are disabled for CORS checks. > + * URI patterns must be of the form `[protocol]://[host]/[path]`, where the > + * *host* and *path* components can contain the wildcard character (`*`) to > + * represent zero or more other characters. We should clarify that requests *to* resources that match the allowed patterns bypass CORS, not requests *from* resources that match the allowed patterns. Otherwise, it's not clear which. :)

Comment on attachment 416521 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=416521&action=review > Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:4727 > + * Since: 2.32 Let's not forget to update this to 2.34 now > Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitWebView.cpp:1488 > +static void testWebViewCORSAllowList(WebViewTest* test, gconstpointer) > +{ > +} I will think about how to write a test for this.

Comment on attachment 416521 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=416521&action=review > Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:4717 > + * webkit_web_view_set_cors_allow_list: I'm also going to change this to allowlist rather than allow_list, since it recently became common to use it as one word. >> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:4725 >> + * represent zero or more other characters. > > We should clarify that requests *to* resources that match the allowed patterns bypass CORS, not requests *from* resources that match the allowed patterns. Otherwise, it's not clear which. :) I've expanded this documentation a bit: * Sets the @allowlist for which * [Cross-Origin Resource Sharing](https://developer.mozilla.org/en-US/docs/Web/HTTP/CORS) * checks are disabled in @web_view. Passing a %NULL as @allowlist * implies that no URIs are disabled for CORS checks. URI patterns must * be of the form `[protocol]://[host]/[path]`, where the *host* and * *path* components may contain the wildcard character (`*`) to * represent zero or more other characters. * * Disabling CORS checks permits resources from other origins to load * allowlisted resources. It does not permit the allowlisted resources * to load resources from other origins. * * If this function is called multiple times, only the allowlist set by * the most recent call will be effective. >> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitWebView.cpp:1488 >> +} > > I will think about how to write a test for this. Going to try to write a test now....

Created attachment 430490 [details] Patch

Committed r278456 (238476@main): <https://commits.webkit.org/238476@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 430490 [details].