[webkit-reviews] review denied: [Bug 173915] [GTK] Add web process API to detect when form is submitted via JavaScript : [Attachment 320458] 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 320458: Patch

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




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

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

r- because the tests need some changes, my API changes are just suggestions we
should discuss

> Source/WebKit/ChangeLog:30
> +	   Unfortunately neither of these signals are available for WPE yet,
even though there's
> +	   nothing GTK-specific about them. This is because we do not currently
build the DOM API for
> +	   WPE. We'll eventually need to decide whether or not we want to start
doing so.

We won't expose DOM bindings in WPE, the plan is to use glib JavaScriptCore
API, so we would use JSCValue (or JSCObject) instead.

> Source/WebKit/WebProcess/InjectedBundle/API/glib/WebKitWebPage.cpp:421
> +    void fireWillSubmitSignal(uint64_t signal, HTMLFormElement* formElement,
WebFrame* frame, WebFrame* sourceFrame, const Vector<std::pair<String,
String>>& values)
> +    {
> +	   WebKitFrame* webkitTargetFrame = webkitFrameGetOrCreate(frame);
> +	   WebKitFrame* webkitSourceFrame =
webkitFrameGetOrCreate(sourceFrame);
> +
> +	   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()));
> +	   }
> +
> +	   g_signal_emit(m_webPage, signals[signal], 0,
WebKit::kit(formElement), webkitTargetFrame, webkitSourceFrame,
textFieldNames.get(), textFieldValues.get());
> +    }

So, signals are exactly the same except for the name. What do you think about
exposing a single signal with an enum, similar to the load changed signal. Then
we explain when the signal will be emitted with each value. It could be called
form-submission-event or somethng like that and what we pass to the signal is
the event that happened. In case of applications that want to do the same thing
for both signal, they would only need to connect to a signal and ignore the
event parameter (this would be the ephy case, right?)

> Source/WebKit/WebProcess/InjectedBundle/API/glib/WebKitWebPage.cpp:569
> +	* @target_frame: the #WebKitWebFrame containing the form's target
> +	* @source_frame: the #WebKitWebFrame containing the form to be
submitted

It's a bit weird that target goes before source, no? Can these be NULL?

> Source/WebKit/WebProcess/InjectedBundle/API/glib/WebKitWebPage.cpp:606
> +	* This signal is generally emitted before a form will be submitted,
> +	* but *not* when a form is submitted via JavaScript. Use

generally is confusing. Is the case of JS submission the only one when this is
not emitted? Then I would make it clearer:

This signal is emitted before a form will be submitted, except when submitted
via JavaScript.

Or something similar, but making it clear when it's emitted and its' not.

> Source/WebKit/WebProcess/InjectedBundle/API/glib/WebKitWebPage.cpp:607
> +	* ::will-submit-form if you need to detect form submission more

Does ::signal-name work? don't you need to use #ClassName::signal-name? to get
a proper link?

> Source/WebKit/WebProcess/InjectedBundle/API/glib/WebKitWebPage.cpp:608
> +	* reliably. The advantage of ::will-send-submit-event is that it

I would avoid saying that the other signal is more reliable, I would just say
that this is needed only if you need to get the form data even when the form
submission is cancelled.

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebExtensions.cpp:288
> +static void testFormSubmissionResult(GVariant* result)

Since you have created a class for the test, move this to the class.

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

Ditto.

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebExtensions.cpp:308
> +    g_main_loop_quit(test->m_mainLoop);

test->quitMainLoop();

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebExtensions.cpp:310
> +    test->m_willSubmitFormCallbackExecuted = true;

I would move this before the loop_quit()

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebExtensions.cpp:349
> +    GUniquePtr<char>
extensionBusName(g_strdup_printf("org.webkit.gtk.WebExtensionTest%u",
Test::s_webExtensionID));
> +    GRefPtr<GDBusProxy> proxy =
adoptGRef(bus->createProxy(extensionBusName.get(),
> +	   "/org/webkit/gtk/WebExtensionTest",
"org.webkit.gtk.WebExtensionTest", test->m_mainLoop));
> +    GDBusConnection* connection = g_dbus_proxy_get_connection(proxy.get());
> +
> +    guint willSubmitFormCallbackID =
g_dbus_connection_signal_subscribe(connection,
> +	   nullptr,
> +	   "org.webkit.gtk.WebExtensionTest",
> +	   "WillSubmitForm",
> +	   "/org/webkit/gtk/WebExtensionTest",
> +	   nullptr,
> +	   G_DBUS_SIGNAL_FLAGS_NONE,
> +	   reinterpret_cast<GDBusSignalCallback>(willSubmitFormCallback),
> +	   test,
> +	   nullptr);
> +    g_assert(willSubmitFormCallbackID);
> +
> +    guint willSendSubmitEventCallbackID =
g_dbus_connection_signal_subscribe(connection,
> +	   nullptr,
> +	   "org.webkit.gtk.WebExtensionTest",
> +	   "WillSendSubmitEvent",
> +	   "/org/webkit/gtk/WebExtensionTest",
> +	   nullptr,
> +	   G_DBUS_SIGNAL_FLAGS_NONE,
> +	   reinterpret_cast<GDBusSignalCallback>(willSendSubmitEventCallback),
> +	   test,
> +	   nullptr);
> +    g_assert(willSendSubmitEventCallbackID);

This could be moved to the FormSubmissionTest constructor.

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebExtensions.cpp:364
> +    webkit_web_view_run_javascript(test->m_webView, submitFormScript,
nullptr, nullptr, nullptr);
> +    g_main_loop_run(test->m_mainLoop);

test->runJavaScriptAndWaitUntilFormSubmitted(submitFormScript);

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebExtensions.cpp:368
> +    // Absurdly, this event must not be emitted when the form is submitted
via JS.

Please, remove the Absurdly,

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebExtensions.cpp:381
> +

Would it be possible to also test the case of form submit cancellation?

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebExtensions.cpp:383
> +    g_dbus_connection_signal_unsubscribe(connection,
willSubmitFormCallbackID);
> +    g_dbus_connection_signal_unsubscribe(connection,
willSendSubmitEventCallbackID);

And this should be moved to the destructor.