[webkit-reviews] review denied: [Bug 219995] [GTK] Expose setCORSDisablingPatterns : [Attachment 416469] Patch

[bugzilla-daemon at webkit.org]

Michael Catanzaro <mcatanzaro at gnome.org> has denied Jan-Michael Brummer
<jan.brummer at tabos.org>'s request for review:
Bug 219995: [GTK] Expose setCORSDisablingPatterns
https://bugs.webkit.org/show_bug.cgi?id=219995

Attachment 416469: Patch

https://bugs.webkit.org/attachment.cgi?id=416469&action=review




--- Comment #3 from Michael Catanzaro <mcatanzaro at gnome.org> ---
Comment on attachment 416469
  --> https://bugs.webkit.org/attachment.cgi?id=416469
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.