[webkit-reviews] review denied: [Bug 173915] [GTK] Add web process API to detect when form is submitted via JavaScript : [Attachment 318447] Patch

[bugzilla-daemon at webkit.org]

Carlos Garcia Campos <cgarcia at igalia.com> has denied Michael Catanzaro
<mcatanzaro at igalia.com>'s request for review:
Bug 173915: [GTK] Add web process API to detect when form is submitted via
JavaScript
https://bugs.webkit.org/show_bug.cgi?id=173915

Attachment 318447: Patch

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




--- Comment #13 from Carlos Garcia Campos <cgarcia at igalia.com> ---
Comment on attachment 318447
  --> https://bugs.webkit.org/attachment.cgi?id=318447
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=318447&action=review

> Source/WebKit/WebProcess/InjectedBundle/API/glib/WebKitWebPage.cpp:61
> +#include "WebKitDOMHTMLFormElement.h"
> +#include "WebKitDOMHTMLFormElementPrivate.h"

WebKitDOMHTMLFormElementPrivate.h already includes WebKitDOMHTMLFormElement.h,
so you only need to include the private one.

> Source/WebKit/WebProcess/InjectedBundle/API/glib/WebKitWebPage.cpp:389
> +	   GRefPtr<WebKitFrame> webkitFrame =
adoptGRef(webkitFrameCreate(frame));
> +	   GRefPtr<WebKitFrame> webkitSourceFrame =
adoptGRef(webkitFrameCreate(sourceFrame));

This is not how frames should be created, one of those could probably be the
main frame, we should ensure that webkit_web_page_get_main_frame() == frame and
that's not going to happen here. Same if frame and source frame are the same,
here we would be using two different pointers. You should use
webkitFrameGetOrCreate instead.

> Source/WebKit/WebProcess/InjectedBundle/API/glib/WebKitWebPage.cpp:396
> +	   GRefPtr<GPtrArray> textFieldNames =
adoptGRef(g_ptr_array_new_full(values.size(), g_free));
> +	   GRefPtr<GPtrArray> textFieldValues =
adoptGRef(g_ptr_array_new_full(values.size(), g_free));
> +	   for (auto& pair : values) {
> +	       g_ptr_array_add(textFieldNames.get(),
g_strdup(pair.first.utf8().data()));
> +	       g_ptr_array_add(textFieldValues.get(),
g_strdup(pair.second.utf8().data()));
> +	   }

This are key-value pairs, right? why not using a GHashTable? It would be
consistent with what we do in the UI process, see
webkit_form_submission_request_get_text_fields(). If there can be several
elements with the same name, then we should change the UI process API,
deprecating the current method and adding a new one using arrays or whatever.

> Source/WebKit/WebProcess/InjectedBundle/API/glib/WebKitWebPage.cpp:559
> +	* @source_frame: the source #WebKitWebFrame

the source #WebKitWebFrame? what's that?

> Source/WebKit/WebProcess/InjectedBundle/API/glib/WebKitWebPage.cpp:571
> +	* Since: 2.18

I'm not sure we want to add new API like this at this point of the cycle, we
are frozen now.

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebExtensions.cpp:280
> +static void willSubmitFormCallback(GDBusConnection*, const char*, const
char*, const char*, const char*, GVariant* result, WebViewTest* test)

test is unused here.

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebExtensions.cpp:286
> +    g_assert(!strcmp(concatenatedTextFieldNames, "foobar"));
> +    g_assert(!strcmp(concatenatedTextFieldValues, "firstsecond"));

g_assert_cmpstr

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebExtensions.cpp:306
> +	   test,

I don't think we need to pass any data.

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebExtensions.cpp:313
> +	   "</form>", 0);

0 -> nullptr

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebExtensions.cpp:341
> +    g_assert(willSubmitFormCallbackExecuted);
> +

willSubmitFormCallbackExecuted = false;

> Tools/TestWebKitAPI/Tests/WebKitGLib/WebExtensionTest.cpp:338
> +static void willSubmitFormCallback(WebKitWebPage* webPage,
WebKitDOMHTMLFormElement* formElement, WebKitFrame* frame, WebKitFrame*
sourceFrame, GPtrArray* textFieldNames, GPtrArray* textFieldValues,
WebKitWebExtension* extension)

formElement, frame and sourceFrame are unused here. You should check them here,
at least that they are not nullptr, but ideally that they are what we expect,
for example, the frame in this case I assume is the main frame.

> Tools/TestWebKitAPI/Tests/WebKitGLib/WebExtensionTest.cpp:499
> -	   case FormControlsAssociatedSignal:
>  #if PLATFORM(GTK)
> +	   case FormControlsAssociatedSignal:

Doesn't this introduce compile warnings in WPE?