[webkit-reviews] review granted: [Bug 77835] [GObject bindings] Make EventTarget interface introspectable : [Attachment 214942] Updated patch

[bugzilla-daemon at webkit.org]

Gustavo Noronha (kov) <gns at gnome.org> has granted Carlos Garcia Campos
<cgarcia at igalia.com>'s request for review:
Bug 77835: [GObject bindings] Make EventTarget interface introspectable
https://bugs.webkit.org/show_bug.cgi?id=77835

Attachment 214942: Updated patch
https://bugs.webkit.org/attachment.cgi?id=214942&action=review

------- Additional Comments from Gustavo Noronha (kov) <gns at gnome.org>
LGTM!

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


>> Source/WebCore/bindings/gobject/WebKitDOMEventTarget.cpp:49
>> -void webkit_dom_event_target_dispatch_event(WebKitDOMEventTarget* target,
WebKitDOMEvent* event, GError** error)
>> +gboolean webkit_dom_event_target_dispatch_event(WebKitDOMEventTarget*
target, WebKitDOMEvent* event, GError** error)
> 
> webkit_dom_event_target_dispatch_event is incorrectly named. Don't use
underscores in your identifier names.  [readability/naming/underscores] [4]

Can you add exceptions for these checks along with this patch? It should be
pretty straight-forward, just add it alongside WebKitDOMCustom in
Tools/Scripts/webkitpy/style/checker.py, I believe.

> Source/WebCore/bindings/gobject/WebKitDOMEventTarget.cpp:65
> +    GRefPtr<GClosure> closure = adoptGRef(g_cclosure_new(handler, userData,
0));
> +    return
WEBKIT_DOM_EVENT_TARGET_GET_IFACE(target)->add_event_listener(target,
eventName, closure.get(), useCapture);

I keep wondering if we should ditch GRefPtr here and do the adoption of the
reference for the closure in the GObjectListener constructor. Feels like
unnecessary churn but at the same time makes things more straight-forward and
obvious I guess…

>> Source/WebCore/bindings/gobject/WebKitDOMEventTarget.h:64
>> +WEBKIT_API gboolean 
webkit_dom_event_target_dispatch_event(WebKitDOMEventTarget *target,
> 
> The parameter name "target" adds no information, so it should be removed. 
[readability/parameter_name] [5]

API break, but ABI stable, no sweat from my side =P