Bug 49649 – [GTK] On-demand event-listeners for DOM event signals

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Carlos Garcia Campos]

GLib signals are currently used to implement events in GObject DOM bindings. Since it's not possible to know what signals the user is going to connect, an event listener is added in every node for every signal when object is constructed. Xan proposed two possible solutions: modify glib/gobject to add a way to notify the user when a signal is connected/disconnected, or add api to webkit dom bindings so that the user can add event listeners, something like webkit_dom_event_target_add_event_listener (target, listener);
Second option is less GObject-y, so the preferred solution is the first one although it depends on whether a patch for this is accepted in gobject.

[Carlos Garcia Campos]

See https://bugzilla.gnome.org/show_bug.cgi?id=635054

[David Keijser]

Created attachment 79866 [details]
replaces use of signals with callbacks

patch replaces use of signals with manual callbacks for DOM events. This allows getting a callback in the capture phase.

[Xan Lopez]

(In reply to comment #2)
> Created an attachment (id=79866) [details]
> replaces use of signals with callbacks
> 
> patch replaces use of signals with manual callbacks for DOM events. This allows getting a callback in the capture phase.

Assuming we end up doing this the patch looks largely sane (would need to fix all the style issues, do a proper ChangeLog, etc. See http://webkit.org/coding/contributing.html).

The only debate topic would be whether we want to keep the signal-like names or just expose directly the DOM names. I think both options have their pros and cons :)

[Martin Robinson]

(In reply to comment #3)
> The only debate topic would be whether we want to keep the signal-like names or just expose directly the DOM names. I think both options have their pros and cons :)

I support a patch like this. Perhaps it could exist in tandem with the signal approach once GLib has the proper infrastructure.

In my opinion, the principle of least surprise suggests that whatever_add_event_listener should accept the same event name you pass to whateverInstance.addEventListener.

[David Keijser]

Created attachment 80347 [details]
now with code in the proper style

[Martin Robinson]

Comment on attachment 80347 [details]
now with code in the proper style

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

This seems good to me, but I think we still need to decide what approach we're taking to the API. I'm in favor of having both signals and *_add_event_listener. The signal names will be GLib-style and the *_add_event_listeners taking DOM-style event names. Signals could be added when GLib has proper support for detecting when handlers are attached.

> Source/WebCore/bindings/gobject/GObjectEventListener.cpp:74
> +    (*(void (*)(GObject*, WebKitDOMEvent*, void*))m_handler)(m_object, gobjectEvent, m_userData);

If possible this should be a static_cast or reinterpret_cast.

[Xan Lopez]

OK, I think we should start thinking about merging this.

One idea after going through the patch again: we should add a GObject interface that defines what classes can receive events, and add the add/remove listener methods there. That way we can get rid of duplicated functions for Node/DOMWindow and have a cleaner API. Makes sense?

[Martin Robinson]

Comment on attachment 80347 [details]
now with code in the proper style

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

> Source/WebCore/bindings/scripts/CodeGeneratorGObject.pm:1088
> +WEBKIT_API gboolean
> +${lowerCaseIfaceName}_add_event_listener(${className}* self, const char* eventName, GCallback handler, gboolean useCapture, gpointer user_data);

An interface may mean these will go away, but WebKit-style dictates that there should be no newline after the return type for these methods.

[Martin Robinson]

Comment on attachment 80347 [details]
now with code in the proper style

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

>> Source/WebCore/bindings/scripts/CodeGeneratorGObject.pm:1088
>> +${lowerCaseIfaceName}_add_event_listener(${className}* self, const char* eventName, GCallback handler, gboolean useCapture, gpointer user_data);
> 
> An interface may mean these will go away, but WebKit-style dictates that there should be no newline after the return type for these methods.

Sorry. Let me clarify. This particular line is okay. Our style guidelines are kind of wonky. For header files we use the style you are using here. Below, for the implementation files, we do not add the newline and use WebKit style everywhere. The GObject binding code has the wrong style many places, but we are trying to fix it as we go along.

> Source/WebCore/bindings/scripts/CodeGeneratorGObject.pm:1100
> +WEBKIT_API gboolean
> +${lowerCaseIfaceName}_add_event_listener(${className}* self, const char* eventName, GCallback handler, gboolean useCapture, gpointer user_data)
> +{
> +    ${coreSelf}
> +    return WebCore::GObjectEventListener::addEventListener(G_OBJECT (self), coreSelf, eventName, handler, useCapture, user_data);
> +}

So for instance here, you would not use the newline.

[Xan Lopez]

Created attachment 86173 [details]
eventlistener.diff

Proposed patch. There will be some style warnings in the event target header, as usual...

[WebKit Review Bot]

Attachment 86173 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/bindings/gobject/WebKitDOMEventTarget.cpp:80:  webkit_dom_event_target_add_event_listener is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/bindings/gobject/WebKitDOMEventTarget.cpp:95:  webkit_dom_event_target_remove_event_listener is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WARNING: File exempt from style guide. Skipping: "Source/WebKit/gtk/tests/testdomdomwindow.c"
Source/WebCore/bindings/gobject/WebKitDOMEventTarget.h:46:  Extra space between GCallback and handler  [whitespace/declaration] [3]
Source/WebCore/bindings/gobject/WebKitDOMEventTarget.h:47:  Extra space between gboolean and bubble  [whitespace/declaration] [3]
Source/WebCore/bindings/gobject/WebKitDOMEventTarget.h:48:  Extra space between gpointer and userData  [whitespace/declaration] [3]
Source/WebCore/bindings/gobject/WebKitDOMEventTarget.h:51:  Extra space between GCallback and handler  [whitespace/declaration] [3]
Source/WebCore/bindings/gobject/WebKitDOMEventTarget.h:52:  Extra space between gboolean and bubble  [whitespace/declaration] [3]
Source/WebCore/bindings/gobject/WebKitDOMEventTarget.h:64:  Extra space between GCallback and handler  [whitespace/declaration] [3]
Source/WebCore/bindings/gobject/WebKitDOMEventTarget.h:65:  Extra space between gboolean and bubble  [whitespace/declaration] [3]
Source/WebCore/bindings/gobject/WebKitDOMEventTarget.h:62:  The parameter name "target" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/bindings/gobject/WebKitDOMEventTarget.h:66:  Extra space between gpointer and userData  [whitespace/declaration] [3]
Source/WebCore/bindings/gobject/WebKitDOMEventTarget.h:70:  Extra space between GCallback and handler  [whitespace/declaration] [3]
Source/WebCore/bindings/gobject/WebKitDOMEventTarget.h:68:  The parameter name "target" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/bindings/gobject/WebKitDOMEventTarget.h:71:  Extra space between gboolean and bubble  [whitespace/declaration] [3]
Total errors found: 14 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Martin Robinson]

Comment on attachment 86173 [details]
eventlistener.diff

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

Nice!

> Source/WebCore/bindings/gobject/WebKitDOMEventTarget.cpp:87
> +    WebKitDOMEventTargetIface* iface;
> +
> +    g_return_val_if_fail(WEBKIT_DOM_IS_EVENT_TARGET(target), FALSE);
> +    g_return_val_if_fail(eventName, FALSE);
> +
> +    iface = WEBKIT_DOM_EVENT_TARGET_GET_IFACE(target);

You'd save a few lines by moving the the declaration of iface to where you first use it.

> Source/WebCore/bindings/gobject/WebKitDOMEventTarget.cpp:102
> +    WebKitDOMEventTargetIface* iface;
> +
> +    g_return_val_if_fail(WEBKIT_DOM_IS_EVENT_TARGET(target), FALSE);
> +    g_return_val_if_fail(eventName, FALSE);
> +
> +    iface = WEBKIT_DOM_EVENT_TARGET_GET_IFACE(target);

Ditto.

> Source/WebCore/bindings/gobject/WebKitDOMEventTarget.h:52
> +
> +    gboolean      (* add_event_listener)(WebKitDOMEventTarget* target,
> +                                         const char          * eventName,
> +                                         GCallback             handler,
> +                                         gboolean              bubble,
> +                                         gpointer              userData);
> +    gboolean      (* remove_event_listener)(WebKitDOMEventTarget* target,
> +                                            const char          * eventName,
> +                                            GCallback             handler,
> +                                            gboolean              bubble);

The asterisk placement seems inconsistent here.

> Source/WebCore/bindings/gobject/WebKitDOMEventTarget.h:72
> +WEBKIT_API gboolean  webkit_dom_event_target_add_event_listener(WebKitDOMEventTarget *target,
> +                                                                const char           *eventName,
> +                                                                GCallback             handler,
> +                                                                gboolean              bubble,
> +                                                                gpointer              userData);
> +
> +WEBKIT_API gboolean  webkit_dom_event_target_remove_event_listener(WebKitDOMEventTarget *target,
> +                                                                   const char           *eventName,
> +                                                                   GCallback             handler,
> +                                                                   gboolean              bubble);
> +

And here as well.

[Xan Lopez]

Comment on attachment 86173 [details]
eventlistener.diff

Committed as r81486 with those changes.

[Xan Lopez]

Closing. Thanks David for the patch!