[Webkit-unassigned] [Bug 77835] [GObject bindings] EventListeners as first-class GObjects: WebKitDOMEventListener

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=77835





--- Comment #45 from Zan Dobersek <zandobersek at gmail.com>  2012-03-21 12:55:02 PST ---
View in context: https://bugs.webkit.org/attachment.cgi?id=132222&action=review

Overall, this looks pretty solid, thanks!

I'd recommend to wait for Xan's input on these changes, but IMO this looks in a pretty good state. There are plenty of style complaints, most of them are in my opinion invalid. This should be looked at, but I recommend to leave this for the very end. Also, the documentation would require some polishing, but again, I'd say it is best to leave that for after both API and implementation are solid.

> Source/WebCore/bindings/gobject/GObjectEventListener.cpp:38
> +GObjectEventListener::GObjectEventListener(GCallback handler, void* userData, GDestroyNotify destroyNotify)

GCallback -> WebKitDOMEventListenerCallback?

> Source/WebCore/bindings/gobject/GObjectEventListener.cpp:69
> +        event->currentTarget()->removeEventListener(event->type(), this, !(event->bubbles()));

You could return here if m_object doesn't exist.

> Source/WebCore/bindings/gobject/GObjectEventListener.cpp:75
> +    reinterpret_cast<GObjectEventListenerCallback>(m_handler)(G_OBJECT(eventTarget), gobjectEvent, m_userData);

If you use WebKitDOMEventListenerCallback as the type for m_handler (that is, remove the GObjectEventListenerCallback type and use WebKitDOMEventListenerCallback instead) you can avoid typecasting here.

> Source/WebCore/bindings/gobject/GObjectEventListener.cpp:87
> +GObject* GObjectEventListener::gobject(GCallback handler, gpointer userData, GDestroyNotify destroyNotify)

GCallback -> WebKitDOMEventListenerCallback?
Also, the name could be a bit more descriptive, at least ::create. Or event better, ::createGObject.

> Source/WebCore/bindings/gobject/GObjectEventListener.cpp:91
> +    coreListener->sinkListener(listener);

I don't see sinkListener function being used anywhere else but here, so perhaps you could remove the function and do both storing the pointer to newly-created GObject directly in m_gobject and calling weak_ref inside here.

> Source/WebCore/bindings/gobject/WebKitDOMCustom.cpp:69
> +webkit_dom_create_event_listener(GCallback handler, gpointer data, GDestroyNotify destroy)

GCallback -> WebKitDOMEventListenerCallback?

>> Source/WebCore/bindings/gobject/WebKitDOMCustom.h:50
>> +WEBKIT_API WebKitDOMEventListener* webkit_dom_create_event_listener(GCallback handler, gpointer data, GDestroyNotify destroy);
> 
> The parameter name "destroy" adds no information, so it should be removed.  [readability/parameter_name] [5]

GCallback -> WebKitDOMEventListenerCallback?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.