Bug 173915 – [GTK] Add web process API to detect when form is submitted via JavaScript

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Cédric Bellegarde]

Not sure if summary is correct but when trying to login on:
https://riot.im/app/#/login

submit-form is not emited by WebKitGTK.

[Alice Mikhaylenko]

onsubmit DOM event also doesn't get emitted. See attachment for a testcase

[Alice Mikhaylenko]

Created attachment 318007 [details]
Test case

[Alice Mikhaylenko]

Created attachment 318009 [details]
Test case

Sorry, corrected a mistake

[Michael Catanzaro]

From https://developer.mozilla.org/en-US/docs/Web/API/HTMLFormElement/submit:

"""
The form's onsubmit event handler (for example, onsubmit="return false;") will not be triggered when invoking this method from Gecko-based applications. In general, it is not guaranteed to be invoked by HTML user agents.

Reference: http://lists.w3.org/Archives/Public/www-dom/2012JanMar/0011.html
"""

We will probably never be able to change this behavior, unless Firefox or Chrome changes first, since it could break websites that expect onsubmit to not be called.

This is a problem for Epiphany because we rely on this event to detect password form submission when deciding whether or not to save passwords. This is causing us to fail to save passwords in exalm's test case above, and also on important websites like accounts.google.com. We need to come up with some way to detect from the web extension when a form is submitted via JavaScript, or we won't be able to fix password saving on these important websites.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #4) 
> This is a problem for Epiphany because we rely on this event to detect
> password form submission when deciding whether or not to save passwords.

Specifically, Epiphany attaches this event listener in web_page_form_controls_associated (in ephy-web-extension.c):

      webkit_dom_event_target_add_event_listener (WEBKIT_DOM_EVENT_TARGET (form), "submit",
                                                  G_CALLBACK (form_submitted_cb), FALSE,
                                                  web_page);

But this event is not emitted in the case where the form is submitted by JavaScript. :/ And it is not clear how we can work around this limitation.

[Michael Catanzaro]

I'm working on a patch that exposes API::InjectedBundle::FormClient::willSubmitForm, same way we currently expose API::InjectedBundle::FormClient::didAssociateFormControls. I don't know if it will work, but I bet it will.

(In reply to Cédric Bellegarde from comment #0)
> Not sure if summary is correct but when trying to login on:
> https://riot.im/app/#/login
> 
> submit-form is not emited by WebKitGTK.

So... I think this is a WONTFIX as that's just how browsers work. :/ It's hard to change how old DOM APIs work due to high risk of breaking websites. We'd likely wind up with code improperly running twice on some websites if we were to change this.

[Michael Catanzaro]

Created attachment 318232 [details]
Patch

[Michael Catanzaro]

Hey Carlos Garcia, does this approach look sane?

I still need to write an API test. Also not sure what is the difference between @frame and @source_frame.

[Build Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #8)
> Hey Carlos Garcia, does this approach look sane?

Yes.

> I still need to write an API test. Also not sure what is the difference
> between @frame and @source_frame.

[Michael Catanzaro]

Are you OK with naming the signal "will-submit-form"? We don't use will- or did- prefixes anywhere else in our public API, because we normally expose only did- callbacks and rename them to past tense (e.g. STARTED, REDIRECTED, COMMITTED, FINISHED). But in this case there is no did- callback we can use, only a will-.

[Michael Catanzaro]

Created attachment 318447 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 318447 [details]
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?

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #11)
> Are you OK with naming the signal "will-submit-form"? We don't use will- or
> did- prefixes anywhere else in our public API, because we normally expose
> only did- callbacks and rename them to past tense (e.g. STARTED, REDIRECTED,
> COMMITTED, FINISHED). But in this case there is no did- callback we can use,
> only a will-.

We have two kind of signals mostly, 

 - notifications: something has happened or is going to happen. The signal is just a notification it doesn't allow the user to prevent the event from happening.

 - actions: we request the user the perform an action, the user can normally do the action, or not. When not doing the action the user can also decide whether to fallback to the default implementation or prevent the action from happening.

We use past tense for notifications because all of them are about things that has already happened. So, I think it makes sense to use the future for things that are going to happen.

[Michael Catanzaro]

(In reply to Carlos Garcia Campos from comment #13)
> WebKitDOMHTMLFormElementPrivate.h already includes
> WebKitDOMHTMLFormElement.h, so you only need to include the private one.

OK.

> 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.

OK.

> 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.

I'm not sure if there can be several elements with the same name (probably), but there can definitely be several elements without a name (empty string) so a HashTable is not going to work. We should investigate the UI process API indeed.

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

Wasn't sure, like you suggested in chat, it makes sense if frame is the submission target of the form and source_frame is the frame containing the form.

> > 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.

2.20 it is!

> > 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.

Good catch. Turns out unused parameter warnings are disabled.

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

OK.

> > Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebExtensions.cpp:306
> > +        test,
> 
> I don't think we need to pass any data.

We don't, it should be removed. We did earlier in development of the patch when I was quitting the main loop from that callback.

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

Right.

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

OK.

> > 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.

Hm, this is slightly tricky because, except for sendRequestCallback, WebExtensionTest is currently written in a very generic manner. It's not great to check the frames in willSubmitFormCallback because other UI process tests using WebExtensionTests could also submit forms. Now, I could add a conditional and do some tests based on the conditional. E.g. if formElement.id equals something in particular to indicate the test that's running, then add some asserts. I think I'll do that, because it's the best I can come up with. But the disadvantage is the asserts could easily be disabled by accident when modifying the test in the other file.

> > Tools/TestWebKitAPI/Tests/WebKitGLib/WebExtensionTest.cpp:499
> > -        case FormControlsAssociatedSignal:
> >  #if PLATFORM(GTK)
> > +        case FormControlsAssociatedSignal:
> 
> Doesn't this introduce compile warnings in WPE?

Oh good catch, I was thinking that these enum values would not be declared on WPE, but they are. Probably best to add another #if up above.

[Michael Catanzaro]

So willSubmitForm is apparently not completely reliable. Let's expose willSendSubmitEvent as well.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #16)
> So willSubmitForm is apparently not completely reliable. Let's expose
> willSendSubmitEvent as well.

There are two reasons we need to expose willSendSubmitEvent as well:

 * For unknown reasons, willSubmitForm is not always called before submitting a form... yeah. I wonder if some websites catch and block the submit event and then manually grab form values from JS. Not sure.

 * For dumb reasons, some websites intentionally clear the form values when handling the submit event, probably as an attempt to block browser password managers from saving the password. Unfortunately willSendSubmitEvent occurs before willSubmitForm, so willSubmitForm is too late to grab the password and stop this. (Of course, if the website submits the form via JavaScript, then willSendSubmitEvent will never be called, so it doesn't matter that willSubmitForm is later in that case.)

So the robust solution is for browsers to connect to both willSendSubmitEvent and willSubmitForm, and just ignore willSubmitForm if willSendSubmitEvent was received. We could hack around this in the GTK+ API layer and try to provide just a single signal, but that's never worked out well for us in the past, so I favor exposing them both.

[Michael Catanzaro]

The forthcoming patch depends on the patch in bug #176719 for the test to pass, but it's not needed to build and run.

[Michael Catanzaro]

Also I still need to deprecate and replace the broken UI process API. That will be another bug.

[Michael Catanzaro]

Created attachment 320458 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 320458 [details]
Patch

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

I'm fine with this, but I still find confusing these signals

> Source/WebKit/WebProcess/InjectedBundle/API/glib/WebKitWebPage.cpp:613
> +     * This signal is generally emitted before a form will be submitted,
> +     * but *not* when a form is submitted via JavaScript. Use
> +     * ::will-submit-form if you need to detect form submission more
> +     * reliably. The advantage of ::will-send-submit-event is that it
> +     * occurs before the DOM submit event, where form submission could
> +     * be cancelled. Some websites may cancel form submission in an
> +     * attempt to prevent browsers from detecting the submission, e.g.
> +     * when a website wants to subvert the browser's password manager.
> +     * ::will-send-submit-event is useful if you need to detect this.

So, could it happen that both are emitted in some cases? I still find quit confusing having this both signals, is ephy going to connect to both and do the same thing? Would it be possible to emit the same signal instead?

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebExtensions.cpp:380
> +    g_assert(test->m_willSubmitFormCallbackExecuted);
> +    g_assert(test->m_willSendSubmitEventCallbackExecuted);

So, in this case both are emitted, is the user expected to detect this to avoid doing the same thing twice?

[Michael Catanzaro]

(In reply to Carlos Garcia Campos from comment #21)
> So, could it happen that both are emitted in some cases? I still find quit
> confusing having this both signals, is ephy going to connect to both and do
> the same thing?

Yes.

> Would it be possible to emit the same signal instead?

Yes, it's possible, but quite complex. We would need to add a way to "tag" a form submission in WebCore in order to detect that will-send-submit-event has already been emitted *for a particular form submission* (not just a particular form). Then at the GLib API layer we will be able to translate it to a will-submit-form event. But there is another disadvantage to this: the web page could decide to cancel form submission in the submit event callback. So the application will not be able to distinguish between an actual unstoppable form submission and a possible future form submission. Then you might think that we should expose only will-submit-form and not will-send-submit-event, but we think we really need will-send-submit-event to work around websites that try to sabotage password saving.

> > Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebExtensions.cpp:380
> > +    g_assert(test->m_willSubmitFormCallbackExecuted);
> > +    g_assert(test->m_willSendSubmitEventCallbackExecuted);
> 
> So, in this case both are emitted, is the user expected to detect this to
> avoid doing the same thing twice?

Yes, that's exactly what Epiphany does.

[Carlos Garcia Campos]

Comment on attachment 320458 [details]
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.

[Michael Catanzaro]

(In reply to Carlos Garcia Campos from comment #23)
> We won't expose DOM bindings in WPE, the plan is to use glib JavaScriptCore
> API, so we would use JSCValue (or JSCObject) instead.

Yeah... note the date on the patch. :P

> 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?)

Good idea. I didn't think of that. Let's do it.

> > 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_frame can definitely not be NULL.

I'll test target_frame to figure that out.

> > 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.

OK. I think that's the only case it's not emitted.

> > 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?

I don't know. I'm sure I copied it from somewhere, and I'd presume it works from within the same class, but I'll investigate to be sure.

> > 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.

OK.

> > Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebExtensions.cpp:381
> > +
> 
> Would it be possible to also test the case of form submit cancellation?

I'll try.

[Michael Catanzaro]

(In reply to Carlos Garcia Campos from comment #23) 
> We won't expose DOM bindings in WPE, the plan is to use glib JavaScriptCore
> API, so we would use JSCValue (or JSCObject) instead.

Rewritten.

> 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.

Done.
 
> > 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?

Flipped.

@source_frame can never be NULL, because it's emitting the event.

@target_frame will never be NULL either. If unspecified, it will be the same frame as @source_frame. Documented.

> > 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.

Rewritten.

> > 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?

I didn't test it, so I changed to #ClassName::signal-name to be safe.

> > 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.

Rewritten.

> > 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.

Done.

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

Done.

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

Done.

> > Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebExtensions.cpp:310
> > +    test->m_willSubmitFormCallbackExecuted = true;
> 
> I would move this before the loop_quit()

Done.

> This could be moved to the FormSubmissionTest constructor.

Done.

> > 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);

Created.

> > 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,

Done.

> > Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebExtensions.cpp:381
> > +
> 
> Would it be possible to also test the case of form submit cancellation?

Hmm, I'm not sure. It's certainly not easy. We can cancel the form submission with some JS in onsubmit, but then where do we check that willSubmitForm was never emitted? There's no place where this can be tested, so it would require waiting for some arbitrary timeout, which I don't want to do. So unless you have any suggestions, I'm going to say no.

> > 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.

Done.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #25) 
> > 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.
> 
> Done.

Also this required running glib-mkenums for the web extension headers, which we were not previously doing. And that has resulted in this patch also adding GType goo for WebKitConsoleMessageLevel and WebKitConsoleMessageSource. There's no way to properly annotate that with @Since I'm afraid, but that's a very minor issue IMO.

[Michael Catanzaro]

Created attachment 330165 [details]
Patch

[Michael Catanzaro]

Created attachment 330167 [details]
Patch

[EWS Watchlist]

Comment on attachment 330167 [details]
Patch

Attachment 330167 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/5815682

New failing tests:
imported/w3c/web-platform-tests/service-workers/service-worker/ServiceWorkerGlobalScope/update.https.html

[EWS Watchlist]

Created attachment 330169 [details]
Archive of layout-test-results from ews107 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Carlos Garcia Campos]

Comment on attachment 330167 [details]
Patch

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

> Source/WebKit/WebProcess/InjectedBundle/API/glib/WebKitWebPage.cpp:592
> +     * WebKitWebPage::form-submission-event:

I would keep the name from previous patch, will-submit-form. form-submission-event sounds like something that has already happened, like button_press_event or key_press_event. Possible events happen during the form submission process.

> Source/WebKit/WebProcess/InjectedBundle/API/glib/WebKitWebPage.cpp:595
> +     * @type: a #WebKitFormSubmissionEventType indicating the current

And instead of event, maybe step is less confusing, because will send submit event already contains the word event.

> Source/WebKit/WebProcess/InjectedBundle/API/gtk/WebKitWebPage.h:50
> + * WebKitFormSubmissionEventType:

Why Type suffix?

[Michael Catanzaro]

(In reply to Carlos Garcia Campos from comment #31)
> > Source/WebKit/WebProcess/InjectedBundle/API/glib/WebKitWebPage.cpp:592
> > +     * WebKitWebPage::form-submission-event:
> 
> I would keep the name from previous patch, will-submit-form.
> form-submission-event sounds like something that has already happened, like
> button_press_event or key_press_event. Possible events happen during the
> form submission process.

We can certainly bikeshed over the name... the problem with will-submit-form is that it corresponds to one of the two possible events, and that is confusing. Especially when the event type will be WILL_SEND_SUBMIT_EVENT, which can cancel form submission entirely. So will-submit-form is not a good name, unless we go back to using two separate signals instead of one. (But I liked your suggestion to merge the two signals into one.)

How about WebKitWebPage::form-submission-step? That matches your next suggestion:

> > Source/WebKit/WebProcess/InjectedBundle/API/glib/WebKitWebPage.cpp:595
> > +     * @type: a #WebKitFormSubmissionEventType indicating the current
> 
> And instead of event, maybe step is less confusing, because will send submit
> event already contains the word event.

WebKitFormSubmissionEventType -> WebKitFormSubmissionStep sounds good to me. Yes, I agree that using Event twice was confusing, but I couldn't think of a better way. "Step" fixes that.

> > Source/WebKit/WebProcess/InjectedBundle/API/gtk/WebKitWebPage.h:50
> > + * WebKitFormSubmissionEventType:
> 
> Why Type suffix?

It's not really needed. Let's go with WebKitFormSubmissionStep.

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #32)
> (In reply to Carlos Garcia Campos from comment #31)
> > > Source/WebKit/WebProcess/InjectedBundle/API/glib/WebKitWebPage.cpp:592
> > > +     * WebKitWebPage::form-submission-event:
> > 
> > I would keep the name from previous patch, will-submit-form.
> > form-submission-event sounds like something that has already happened, like
> > button_press_event or key_press_event. Possible events happen during the
> > form submission process.
> 
> We can certainly bikeshed over the name...

I don't think this is bikeshedding, naming is key in public APIs. We need to ensure that names are consistent with existing APIs and as less confusing as possible.

> the problem with will-submit-form
> is that it corresponds to one of the two possible events, and that is
> confusing.

You are right.

> Especially when the event type will be WILL_SEND_SUBMIT_EVENT,
> which can cancel form submission entirely. So will-submit-form is not a good
> name, unless we go back to using two separate signals instead of one. (But I
> liked your suggestion to merge the two signals into one.)

We can use 

 - WILL_SEND_DOM_EVENT: it clarifies that the event is the DOM one and we don't need to use SUBMIT because it's redundant with the signal name

 - WILL_COMPLETE: we avoid using SUBMIT here too.

> How about WebKitWebPage::form-submission-step?

Same problem, sounds like the given step has already happened, we really need the will here.

> That matches your next
> suggestion:
> 
> > > Source/WebKit/WebProcess/InjectedBundle/API/glib/WebKitWebPage.cpp:595
> > > +     * @type: a #WebKitFormSubmissionEventType indicating the current
> > 
> > And instead of event, maybe step is less confusing, because will send submit
> > event already contains the word event.
> 
> WebKitFormSubmissionEventType -> WebKitFormSubmissionStep sounds good to me.
> Yes, I agree that using Event twice was confusing, but I couldn't think of a
> better way. "Step" fixes that.
> 
> > > Source/WebKit/WebProcess/InjectedBundle/API/gtk/WebKitWebPage.h:50
> > > + * WebKitFormSubmissionEventType:
> > 
> > Why Type suffix?
> 
> It's not really needed. Let's go with WebKitFormSubmissionStep.

[Michael Catanzaro]

I agree the naming is important. I like your suggestion, so I'll change it to WebKitWebPage::will-submit-form, WebKitFormSubmissionStep, WEBKIT_FORM_SUBMISSION_WILL_SEND_DOM_EVENT, and WEBKIT_FORM_SUBMISSION_WILL_COMPLETE. OK?

[Michael Catanzaro]

Created attachment 330338 [details]
Patch

[EWS Watchlist]

Comment on attachment 330338 [details]
Patch

Attachment 330338 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/5896489

New failing tests:
fast/mediastream/MediaStream-MediaElement-setObject-null.html

[EWS Watchlist]

Created attachment 330347 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Carlos Garcia Campos]

Comment on attachment 330338 [details]
Patch

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

> Source/WebKit/WebProcess/InjectedBundle/API/glib/WebKitWebPage.cpp:595
> +     * @type: a #WebKitFormSubmissionEventType indicating the current

@step
#WebKitFormSubmissionStep

> Source/WebKit/WebProcess/InjectedBundle/API/glib/WebKitWebPage.cpp:597
> +     * @source_frame: the #WebKitWebFrame containing the form to be

#WebKitWebFrame -> #WebKitFrame

> Source/WebKit/WebProcess/InjectedBundle/API/glib/WebKitWebPage.cpp:599
> +     * @target_frame: the #WebKitWebFrame containing the form's target,

#WebKitWebFrame -> #WebKitFrame

> Source/WebKit/WebProcess/InjectedBundle/API/glib/WebKitWebPage.cpp:607
> +     * submission. @type indicates the current stage of form submission.

@step

[Michael Catanzaro]

Committed r226366: <https://trac.webkit.org/changeset/226366>

[Michael Catanzaro]

Committed r226376: <https://trac.webkit.org/changeset/226376>