Bug 49649 - [GTK] On-demand event-listeners for DOM event signals
Summary: [GTK] On-demand event-listeners for DOM event signals
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 38576
  Show dependency treegraph
 
Reported: 2010-11-17 00:13 PST by Carlos Garcia Campos
Modified: 2011-03-18 10:26 PDT (History)
3 users (show)

See Also:


Attachments
replaces use of signals with callbacks (14.13 KB, patch)
2011-01-23 07:25 PST, David Keijser
no flags Details | Formatted Diff | Diff
now with code in the proper style (16.05 KB, patch)
2011-01-27 10:40 PST, David Keijser
no flags Details | Formatted Diff | Diff
eventlistener.diff (23.76 KB, patch)
2011-03-18 09:57 PDT, Xan Lopez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2010-11-17 00:13:23 PST
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.
Comment 1 Carlos Garcia Campos 2010-11-17 00:33:39 PST
See https://bugzilla.gnome.org/show_bug.cgi?id=635054
Comment 2 David Keijser 2011-01-23 07:25:31 PST
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.
Comment 3 Xan Lopez 2011-01-25 13:13:23 PST
(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 :)
Comment 4 Martin Robinson 2011-01-25 13:37:04 PST
(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.
Comment 5 David Keijser 2011-01-27 10:40:49 PST
Created attachment 80347 [details]
now with code in the proper style
Comment 6 Martin Robinson 2011-01-27 10:58:27 PST
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.
Comment 7 Xan Lopez 2011-03-14 13:27:16 PDT
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?
Comment 8 Martin Robinson 2011-03-14 13:33:50 PDT
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.
Comment 9 Martin Robinson 2011-03-14 13:44:00 PDT
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.
Comment 10 Xan Lopez 2011-03-18 09:57:15 PDT
Created attachment 86173 [details]
eventlistener.diff

Proposed patch. There will be some style warnings in the event target header, as usual...
Comment 11 WebKit Review Bot 2011-03-18 10:00:27 PDT
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.
Comment 12 Martin Robinson 2011-03-18 10:10:42 PDT
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.
Comment 13 Xan Lopez 2011-03-18 10:26:17 PDT
Comment on attachment 86173 [details]
eventlistener.diff

Committed as r81486 with those changes.
Comment 14 Xan Lopez 2011-03-18 10:26:38 PDT
Closing. Thanks David for the patch!