Bug 77835 – [GObject bindings] Make EventTarget interface introspectable

• Component: WebKitGTK
• Status: RESOLVED FIXED

[C Anthony]

(unsure how to select proper version ... WebkitGTK 1.6.1)

add/remove_event_listener are missing scope annotations, as reported by g-ir-scanner during build:

DerivedSources/webkit/WebKitDOMEventTarget.h:62: Warning: WebKit: webkit_dom_event_target_add_event_listener: argument handler: Missing (scope)     
annotation for callback without GDestroyNotify (valid: call, async)
DerivedSources/webkit/WebKitDOMEventTarget.h:68: Warning: WebKit: webkit_dom_event_target_remove_event_listener: argument handler: Missing (scope)
annotation for callback without GDestroyNotify (valid: call, async)

... which causes those two methods to be marked `introspection=0`, and thus prevents all DOM event handling by GObject based bindings (Python in this case)

[C Anthony]

per the error, i added the following to `DerivedSources/webkit/WebKitDOMEventTarget.h` (i realize it's the wrong spot :) ...

/**
 * webkit_dom_event_target_add_event_listener:
 * @handler: (closure userData) (scope call):
 * @userData: (closure):
 */
WEBKIT_API gboolean webkit_dom_event_target_add_event_listener([...])

/**
 * webkit_dom_event_target_remove_event_listener:
 * @handler: (scope call):
 */
WEBKIT_API gboolean webkit_dom_event_target_remove_event_listener([...])

... + regen the gir/typelib, aaaand it worked!  i don't know if call or async is correct, both *seem* to work.  the only issue i'm having now is the callback doesn't receive any data (target/event) at all ... but this gets me moving again.

[Priit Laes (IRC: plaes)]

Created attachment 125617 [details]
webkit-gdom-bindings.patch

Not sure whether it's everything that's required, but at least it makes the handlers available for gir.

[Priit Laes (IRC: plaes)]

Created attachment 125618 [details]
webkit-gir.py

Sample script for playing around :)

[Gustavo Noronha (kov)]

Comment on attachment 125617 [details]
webkit-gdom-bindings.patch

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

> Source/WebCore/ChangeLog:3
> +        (gtk-gir-dom-events) [gobject-introspection] Missing scope annotations for add/remove_event_listener

I'd drop the prefixes, or just make it [GObject bindings]?

> Source/WebCore/bindings/gobject/WebKitDOMEventTarget.h:64
> + * @handler: (scope async):

I'm not an introspection expert, but in my understanding from the docs the scope for this should actually be notified (although that's of course impossible because there's no notify as a parameter?)

async - Only valid for the duration of the first callback invokation. Can only be called once.

This sounds like it'll break if the event is triggered more than one time, doesn't it? Would be good to have advice from a GI expert on this.

[Gustavo Noronha (kov)]

Comment on attachment 125617 [details]
webkit-gdom-bindings.patch

I discussed this with Tomeu on IRC and he agrees async is surely wrong. He mentioned plans for adding a new scope type for cases like this. libgdata skipped their methods that are similar: http://mail.gnome.org/archives/commits-list/2010-September/msg13384.html I'm not sure what to do, I guess implementing that new scope type is the only way out of this particular cross-road.

[C Anthony]

can the GDestroyNotify param be added?  how can we solve this without waiting for a new type to *possibly* be implemented ... the bug discussing new scope types:

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

... linked from the documentation:

"A few more scope types are possible, but they are not yet added in lack of proper use cases. See bug 556489 for some discussion about this."
https://live.gnome.org/GObjectIntrospection/Annotations

... has been in discussion since 2008. what other options are there, eg. can it be reworked in any way, or is there a precedent?  what else uses callbacks?

proper event handlers to the DOM are absolutely critical for my uses, and i expect many others ... without them the usefulness of the GObject bindings is *massively* reduced (for higher langs anyway).

[C Anthony]

i did believe async was wrong too, and call, but i wasn't sure what else to use sans additional types.  i thought maybe there was some other magic going on ... like the add/remove functions were doing some internal tracking ... but i really don't know how all of this works.

when using async/call, nothing crashes (or even spews an error), but no arguments are being passed to the callback (*args and **kwds are always empty)

[C Anthony]

although it would be very annoying to re-add handlers after every invocation (async?), i could deal with that as a workaround until a proper solutions is found.

however, right now i'm having a problem getting any arguments *at all* passed to the handler -- even once -- eg. the target element and the event object, which for obvious reasons ... are 100% critical ...

what else can i try?  i have (closure userData) on handler, and (closure) on userData ... is that correct? and isn't userData special ({object}.set_data)?

any help appreciated ... i'm working on adapting the GObject bindings to an existing widget toolkit/runtime -- this platform can already use Trident(IE), PythonWebkit, XULRunner(xpcom/hulahop) -- once i get this running, several existing applications will be able to hammer on the implementation.

[C Anthony]

the guys on #introspection (gimpnet) said the proper way to fix, without API break, is to add a _full variant to each function that accepts a GDestroyNotify param, and use a rename annotation to mask the original.  they point out g_timeout_add/g_timeout_add_full as existing examples:

< ebassi> xtfxme: webkit should have a add_full() that takes a GDestroyNotify
< ebassi> xtfxme: then you can use (scope notify)
< ebassi> xtfxme: add/remove just won't do: the language binding would not be able to release the memory of the data passed, unless it tracked the data as a refcounted blob, using something like a Set to avoid duplicates; that introduces an interesting class of issues when threads are involved
< ebassi> xtfxme: that's why G* libraries usually have an add and an add_full variants - see for instance g_timeout_add/g_timeout_add_full
< jdahlin> xtfxme: yes, adding a _full variant + Rename annotation seems like the best thing to do, doesn't break api and makes language bindings work properly

... what do you guys think?

[C Anthony]

ebassi (from IRC #introspection) later on added this:

< xtfxme> ebassi, jdahlin: thank you that sounds like a good solution. if i understand correctly the _full variant would be identical, except with a GDestroyNotify param appended, and the rename annotation will effectively make the _full variant mask the original (to python at least)?

[...]

< ebassi> xtfxme: yes
< ebassi> xtfxme: the remove_event_listener() function would then call the DestroyNotify internally, if any was set

[...]

< ebassi> I wonder if the API can be changed, or if it needs a new vfunc in the WebKitDOMEventTargetIface
< ebassi> it's an interface, so it's possible to add a new vfunc without breaking the ABI

[C Anthony]

well i've spent all night for 3+ nights now trying to wrote the solution myself, but i'm afraid i don't understand enough.

if anyone can guide/assist i will do what i can to accomplish myself, but i'm mostly just beating my face into the wall at this point :-(

... any assistance is much appreciated.  from the input i've gotten, from both #webkitgtk+ and #introspection sides, makes me think it's incredibly simple solution for a mildly compentent C/C++ dev, but frankly, i don't 100% understand how all these virtual interfaces and whatnot are interacting.

i can see ...

python/pygi -> webkit_dom_event_target_add_event_listener -> WebKitDOMEventTargetIface (* add_event_listener) -> GObjectEventListener::addEventListener

... and i know i have to store the GDestroyNotify pointer on the GObjectEventListener, and then call it from the gobjectDestroyed member.

i read the code and i believe i understand ~95%, but im really struggling with he correct way to write it, as im a total C/C++ n00b ...

ideas?  how can we open webkit to all of us? there are a handful of functions failing, but for the most par the introspection works wonderful ... save these two methods, which are absolutely *imperative* for even light use.  i am more than willing to sink more time, but i need some pointers in the right direction, and i feel i've already bugged everyone in #introspection/#webkitgtk+ too much :-)

thanks!

[Zan Dobersek]

I've put some time into investigating this, here are the results:

1)
It seems to me that GObject introspection is bad with GCallbacks being used. I don't know for sure if my blame falls on gi or the pygobject, but it seems that because of GCallback being used, in python, this results with the callback being called with no arguments, despite being triggered as a GObjectEventListenerCallback. I've looked around Gtk+ how this is avoided there and the solution could be to define something like a WebKitDOMEventListenerFunc type of function (basically exporting the GObjectEventListenerCallback) and use it. Because of this callback functions would not need to be wrapped in G_CALLBACK when adding a listener.

2)
As seen in in comments #9 and #10 a solution might be adding a _full version of the function with a GDestroyNotify argument. I'd recommend avoiding that and putting the GDestroyNotify argument into the existing function and accepting a possible NULL value. This would of course cause API break, but I don't really see a need for two separate functions.
I'm not yet very fond of the introspection system, but, at least when using gi in python, it seems that the GDestroyNotify argument is completely ignored. This doesn't really affect the solution apart from an idiomatic point of view, it might not even be a problem, but as of now it bugs me.

3)
After (possibly) adding GDestroyNotify argumnet to add_event_listener, the callback must be properly pointed to its closure via annotations that introspection then picks up. Anthony posted a patch[1] in #webkitgtk+ that does that. I haven't tried it, I rather reordered arguments so that WebKitDOMEventListenerFunc, gpointer to user data and GDestroyNotify were put together at the end of the arguments list - it works as well, but it breaks conformance with how the addEventListener function is defined in DOM. Of course, the use of annotations (as it was done in the patch) is the right way.

4)
All this still leaves webkit_dom_event_target_remove_event_listener unintrospectable. The problem is that introspection demands the callback to be scoped (as it is expected to be used), but this really isn't the situation here - event listeners with that callback are being removed, so the introspection shouldn't really care about the scope. I haven't yet found a way do convince introspection of this, might not even be supported.


[1] http://paste.pocoo.org/show/548725/

[Zan Dobersek]

Created attachment 126566 [details]
WIP patch

This is just a patch to show how the problem might be solved.

A GDestroyNotify argument is added to the add_event_listener function. It may be NULL, so this is taken into account when destroying the event listener. Changes are welcomed by introspection, making this function introspectable and properly working in python bindings.

In remove_event_listener, the notified scope is annotated for the handler argument. Still, I've done some basic testing but could'n get this function to work properly, so this should be further inspected.

Also changed are calls in tests to avoid compilation breaks.

[C Anthony]

all hail Zan! bringer of light to that which was once dark! peace and happiness aplenty! progress and ideas abound!

:) woo hoo!  with your patch i can indeed now receive arguments! i can also confirm the inability to remove handlers (though i still think the correct scope for remove_event_listener is `call`) ... however, you're patch confirms i did get most of what was happening (i *almost* had it during my testing), and now i have a good base to branch from, thanks!

i intended to detail several ideas/tests, but alas, i'll return tomorrow ...

but first, one thought. here is a diff of the generated GIRs:

http://dpaste.com/hold/701377/

... as suspected prior, the GIR contains no information about what args the GCallback will receive (generic?), and in the sources GCallback itself is defined as `typedef void (*GCallback) (void)` ...

so, while i'm not sure why introspection doesn't pick up the cast to `GObjectEventListenerCallback`, maybe we can try:

@handler: (type GObjectEventListenerCallback) [...]

... or, does this make sense:

typedef GCallback GObjectEventListenerCallback ([...]);

.. but maybe that's redundant/ridiculous.  like i said, i'm a n00b :-) but i think i have a bunch if good ideas to try, and hopefully get something landed ASAP.

i'll be hitting this hard tomorrow -- will return with results.

PS. to those with black pens and over-sized checks, i happen to be an prominent and well-respected nobody; i recommend Zan for swift promotion and obscene raise.  special mention to Priit and Gustavo for help in IRC ... even if they WONTFIX ;-)

thanks much everyone; tune in tomorrow for the shocking conclusion!

[Zan Dobersek]

(In reply to comment #14)
> but first, one thought. here is a diff of the generated GIRs:
> 
> http://dpaste.com/hold/701377/
> 
> ... as suspected prior, the GIR contains no information about what args the GCallback will receive (generic?), and in the sources GCallback itself is defined as `typedef void (*GCallback) (void)` ...
> 
> so, while i'm not sure why introspection doesn't pick up the cast to `GObjectEventListenerCallback`, maybe we can try:
> 
> @handler: (type GObjectEventListenerCallback) [...]
> 
> ... or, does this make sense:
> 
> typedef GCallback GObjectEventListenerCallback ([...]);
> 
> .. but maybe that's redundant/ridiculous.  like i said, i'm a n00b :-) but i think i have a bunch if good ideas to try, and hopefully get something landed ASAP.

With the changes I'm recommending the GCallback-typed function is no longer used.  Rather than that, I've typedefed the WebKitDOMEventListenerFunc (the DOMEventListenerFunc callback in the .gir). It is basically the GObjectEventListenerCallback typedef, but moved into the header so it is available in the API. Because of that it can be then used in add_ and remove_event_listener instead of GCallback. This fixes the problem of missing arguments as it seems that pygobject doesn't care about GCallbacks that are actually different callbacks but typecasted into a GCallback, which results in those callbacks being called with no arguments at all. Again, that's at least what I think is going on, so I may very well be wrong.

About the remove_event_listener - True, the 'call' scope is more appropriate. This still doesn't fix the issue of the function not working at all.
The conditions under which the listener is removed require that the pointers to the callbacks that are passed through in add_event_listener and remove_event_listener are the same. I've put a 'print hex(id(self.on_close))' line into the test case, printing the location of the self.on_close function. With some output in add_ and remove_event_listener it becomes evident that handlers passed through those two functions never equal the output of the line added in the test case, thus it is impossible to remove an event listener after it's added. Again, I'm thinking this might be pygobject's fault. I'll look into that when possible.

[C Anthony]

Created attachment 126669 [details]
annotation to override parsed C type

this patch demonstrates using an annotation to override the parsed C type instead of actually changing it everywhere.  AFAICT, pygobject just needs to know ahead of time what the callback should receive, and by default GCallback receives nothing (and this seems to match the intended way to use/register a GCallback .. by casting at last second).  this same idea is used in the Telepathy lib.

with this patch i am able to properly receive events with arguments, however the remove_ method still of course fails.  in other libraries i see the remove_ variant using a gpointer instead of a GCallback ... maybe we can pass both add_ and remove_ this way?  im checking with the #introspection people to see how we can solve this -- i'm curious if there is some way to weakref the callback?  ideally, if the callback is GC'ed in the runtime the listener should simply be removed (i found some hinting to this mechanism in my travels).  maybe this would work when passing gpointers (like userData)?

however, most libraries seem to be side-stepping the issue altogether, with add_ returning a unique/opaque key (some incremented number, whatever), and remove_ accepting that key -- no reference to the actual callback is needed.  while this isn't 100% true to DOM API, imo the inability to enumerate the active listeners and manage them whatsoever is a major shortcoming anyway ...

i'll continue trying to get remove_ to work, but this seems an unobtrusive way to get add_ working.

thanks!

[C Anthony]

also, the `notify` arg is missing in ^^^^^ patch; main purpose is to demonstrate the use of `(type [...])` annotation.

... and any reason not to use a GClosure there? if we don't do the _full variant at all, it might be nicer to bindings.

[C Anthony]

Created attachment 126674 [details]
test case (stable listener id)

[C Anthony]

Comment on attachment 126674 [details]
test case (stable listener id)

after being completely baffled for about 2-3 hours (and i still don't really understand why) ... i realized that the callback id() was changing after every use (as you alluded Zan) because i was using an instance method as a callback.

i don't really know why it's an issue, but it's not just add/remove, or even webkit ... connecting to a gsignal does the same thing! i think it was happening because there were still live references to the callback after it ended, and a new one was created each time?

no idea.  this file uses dedicated callback classes which don't seem to be affected ... and i can store a ref to them just fine, very odd.

i think this will allow us to get a handle to the real callback, but i think it needs to be passed as a raw pointer, otherwise pygobject [i think] creates a new callback.

[C Anthony]

[for implementation RFC, please see end!]

... i have spent everyday on this since reporting -- tracing webkit/gobject, rebuilding the appropriate libs with debug symbols, stepping the marshal call chain in [py]gobject ...

... i'm happy to report that i've walked the complete py->webkit->py process, can now write C/C++ with [reasonable] success, and even had to deal with some oddities arising from using C within C++ compiler ...

... BUT ENOUGH ABOUT MY QUEST :-), i have successfully called remove_event_handler from python!  YAY!

no suitable patch ATM (tomorrow), but here are the 5-steps to freedom:

1) adapt add/remove_event_listener to use GClosure
2) write factory function: (add to iface?)

GClosure * [...]_create_event_listener (GCallback*, [...]);

3) ^^^ MUST allocate in C (transfer full)
4) pass returned (or any) GClosure to add/remove_event_listener
5) PROFIT!!!

... as the object is pre-allocated by C, then marshaled back to Python with pre-existing pointer, it is properly recognized by removeEventHandler()!

i really like the GClosure approach because it handles several details for you, and allows for identical signatures on add/remove_event_listener.  also, pygobject still allows you to pass a regular callable [wrapped, *unremoveable*], or passthru a native GClosure [removeable].

... see:

https://mail.gnome.org/archives/python-hackers-list/2012-February/msg00028.html

... for more information why (could possible be fixed, but unlikely, and in some cases not possible).  GClosure is the proper type for the job.

------------------------------------------

[THE RFC-esque SECTION]

while the above works, it's awkward at best -- segfaulty at worst -- to work with the native GClosure from language bindings.  i propose these solutions:

A) [preferred] CREATE WebKitDOMEventListener TYPE

this type will accept a single GClosure/GCallback, and can be subsequently bound to (PLURAL?! ;-) events and/or DOMNodes.  stores an internal ref to bound nodes (via get/set_data?).  has method to [un]bind per DOMEventTarget, or all targets.  could be added to DOMEventTarget Iface for implicit self passing?  GClosure lifetime linked to listener lifetime == auto destroy/cleanup for everyone! hooray!

ADDTL: if we use GClosure, C API users to would not *have* to use WebKitDOMEventListener.  low overhead for all -- bindings could link one high-level callable to several/all WebKit-level handlers (my use case actually *prefers* a SINGLE entry point).

B) [good] STORE gpointer TO LISTENER OR HANDLER ON DOMEventTarget

may require *_full variants, or new generic function that accept gpointers.  store pointer to GObjectListener on DOMEventTarget, eg ...

g_object_set_data(target, {key-from-user-or-???}, &listener)

C) [unknown] USE SOME OTHER WAY BASED ON get/set_data()

Python, and likely(?) other introspect-capable langs, can access pointers stored via g_object_set_data() very easily ... Python can't actually do anything with it (which is why WebKitDOMEventListener is the best choice IMO), but it could be used to unbind related listeners.

D) [non-optimal] CREATE *_full VARIANTS (RETURN/ACCEPT)ING OPAQUE KEYS

this is really more of a workaround than anything else, and breaks the expected API defined by (W3C?) DOM, alas, it's better than no event handling at all:

int webkit_dom_event_target_add_event_handler_full ([...]) {
    gboolean ret = webkit_dom_event_target_add_event_handler([...]);
    if (ret) return (int) &handler;
    return ret;
}

return handler address for use as unique key (or ???, if advantageous); pass same key to *_remove_event_handler_full.  rename by annotation to trim _full suffix.

------------------------------------------

with the exception of (D), the others maintain current return types.  i reeeeallly really like A, for maaaany many reasons -- primarily because it's the most integrated/natural -- the other feel pretty increasingly hacky ... also, (A) would be generally useful for *ALL* users, AAAND all objects are properly linked/chained/destroyed/GC'ed without our help.

... thoughts??? i want to begin writing the solution immediately in the AM, and would love some feedback ASAP!

apologies for the length, i've been at this for days on end, and wanted to be thorough ... thanks much! you guys are the bestest :-)

[C Anthony]

Created attachment 127839 [details]
[hastily] implements add/remove_event_handler in a binding friendly manner, breaks current API

note: against 1.6.3, i will update my tree if need be ... probably still applies
note: used EventHandler for now -- EventListener triggers generation from IDL file (see below)

this is a working patch, but incomplete (leaks/etc).  however i wanted to post it now to be sure i'm moving in the right direction (i'm slow, this took forever :-).

break current API, but IMO, API is wrong -- handleEvent isnt suppose to take 3 params (DOMNode,DOMEvent/userData), it's only suppose to take one: DOMEvent. the original DOMNode/sender is already available on the DOMEvent object as the `target` or `src_element` property.

i think i can come up with a way to maintain current API, but also support the old, by using some form of _full, or similar, but internally the GObjectEventListener would need to convert ... is this worth it?

also, as stated above, DOMEventListener triggers the generation of WebKitDOMEventListener.(h|cpp) from the corresponding IDL file ... however, looking it over, it *almost* looks correct.  it looks like it might be possible to tweak CodeGenerator to make WebKitDOMEventListener wrap *WebCore::GObjectEventListener* objects instead (basically adding `GObject` in the right places)

i really want to allow a single Listener object to be assigned to unlimited Target objects -- this is both efficient and useful.  i can then add a method to "exchange" the callback contained by the Listener with a new one ... this lets you add listener objects to targets, and change the callback for all of them in one shot, something like:

WEBKIT_DOM_EVENT_HANDLER(m_handler)->handle_event(gobjectEvent)

... where handle_event is the actual callback passed during Listener creation, or updated at anytime.

thoughts?  in a bit of a hurry, apologies if somewhat incomplete ...

[Zan Dobersek]

(In reply to comment #21)
> i think i can come up with a way to maintain current API, but also support the old, by using some form of _full, or similar, but internally the GObjectEventListener would need to convert ... is this worth it?
> 

I don't think there's any sense in keeping the current API, because it's not idiomatically parallel to the DOM specification.

If it is presumed that instead of a GCallback or WebKitDOMEventHandler a proper binding to the EventListener interface is introduced (as it is planned), API should reflect the DOM specification perfectly, probably like this:

EventTarget:
http://www.w3.org/TR/DOM-Level-3-Events/#interface-EventTarget

addEventListener(DOMString type, EventListener listener, boolean useCapture) ->
webkit_dom_event_target_add_event_listener(WebKitDOMEventTarget* target, gchar* type, WebKitDOMEventListener* listener, gboolean useCapture)

removeEventListener(DOMString type, EventListener listener, boolean useCapture) ->
webkit_dom_event_target_remove_event_listener(WebKitDOMEventTarget* target, gchar* type, WebKitDOMEventListener* listener, gboolean useCapture)

dispatchEvent(Event event) ->
webkit_dom_event_target_dispatch_event(WebKitDOMEventTarget* target, WebKitDOMEvent* event)

EventListener:
http://www.w3.org/TR/DOM-Level-3-Events/#events-EventListener

To create a WebKitDOMEventListener:
webkit_dom_event_listener_create(WebKitDOMEventListenerFunc callback, gpointer data, GDestroyNotify notify)
WebKitDOMEventListenerFunc:
typedef void (*WebKitDOMEventListenerFunc)(GObject*, WebKitDOMEvent*, void*);
(the same as GObjectEventListenerCallback)

handleEvent(Event event) ->
webkit_dom_event_listener_handle_event(WebKitDOMEventListener* self, WebKitDOMEvent* event)

Most of your work in the latest patch already has this covered, it just requires some more polishing.

> also, as stated above, DOMEventListener triggers the generation of WebKitDOMEventListener.(h|cpp) from the corresponding IDL file ... however, looking it over, it *almost* looks correct.  it looks like it might be possible to tweak CodeGenerator to make WebKitDOMEventListener wrap *WebCore::GObjectEventListener* objects instead (basically adding `GObject` in the right places)
> 

WebKitDOMEventListener.* files should probably be manually written to properly execute the handleEvent method and to properly call the notify callback when necessary. To avoid triggering the autogeneration, the build rules for these files should follow the ones for WebKitDOMEventTarget* files in Source/WebCore/bindings/gobject/GNUmakefile.am.

[C Anthony]

cool, i was having the same thoughts ...

... and i'm getting this down pretty well.  i think the solution i'm working on will be the best long term -- i've almost completely eliminated GObjectEventListener (except as the backing WebCore coreObject for EventListener) and there is a good chance of using the IDL/CodeGenerator.

when a GObjectEventListener is destroyed (last remove) it decrefs the corresponding EventListener.  the GNotifyDestroy is not called until the EventListener itself is destroyed (eg, from python/whatever, providing no handles still exist on EventTargets).  if the empty EventListener later tries to rebind to another EventTarget a new GObjectEventListener/coreObject will be dynamically allocated for it.

the GObjectEventListener is essentially only a small proxy now, and it no longer stores the m_target/m_object (EventTargets) because ...

a) the EventTarget is always available on the Event object itself

b) it breaks established semantics from specs and JS (prevents EventListener from binding to multiple EventTargets)

... these changes should allow for unlimited binding and simpler cleanup.

lastly, IMO at least, _create_event_listener() should be on the WebKitDOMDocument, right next to _create_event() ... this can be done in the WebKitDOMCustom files.  two reasons ...

1) non-standard, but looks/feels more natural to me -- people already know _create_event()'s location well

2) in higher languages at least, creating my constructor is currently broken, likely for all objects.  eg, in python, WebKit.DOMEvent() and WebKit.DOMEventListener() result in GObjects with no backing WebCore object, and fails or segfaults when used.  this could possibly be fixed with a late check in _constructor() or maybe _init()

anyways, i'll have a patch up soon enough; hopefully we can kill this bug quickly and i can get back to the actual project that brought me here :-)

[C Anthony]

Created attachment 131002 [details]
Implements EventListeners by leveraging existing CodeGenerator

^^^^^ by "soon" i meant 3 weeks of "carefully crafting with love [and tears]".

(apologies if somewhat terse ... 2am, want slee... will elaborate as needed later)

this patch implements WebKitEventListener via the existing GObjectCodeGenerator; with the exception of one small tweak to generate the correct signature for `handleEvent` no other changes to the generator were required (others were made for unrelated reasons, however).

patch is complete, save:

) changing the comments as needed
) making the test cases work instead of commenting them out

patch still against 1.6.3 because i've been in a tight feedback loop, and my ccache is blisteringly hot. if it won't apply let me know, i will pull ... (any method to avoid 1GiB+ download??)

side note: there are continuous leaks when using *any* _create() method (any method annotated with `ReturnsNew` in the IDL files).  these functions must be marked `(transfer full)` otherwise they get an extra ref and live forever.  The WebKitEventListener will handle this properly.

DOM spec doesn't allow listeners to be enumerated -- so for example, in JS, if you do something like this:

window.addEventListener('click', function(){console.log("whoopsie daisy")}, false)

... you get in a situation where the browser own the only ref, and there is *no way* to stop it.  while the same can happen with GObject, i took a slightly different "lazy unbind" approach -- if the GObject half is destroyed (from python, unref(), etc) *without* properly removing itself from bound EventTargets, the WebCore half will "silently" (CRIT warn) drop incoming events and remove itself from each EventTarget as they come.  best case, they all fire, and the WebCore half is properly destroyed.  worst case, uhm, idk, it gets destroyed maybe when the DOM gets released? beats me :-) better than nothing.

i implemented this several different ways on my quest for supreme understanding. at one point i was using a single array of GValues -- it worked well -- they know how to destroy their boxed objects additionally create clean separation from C++ objects.  i borrowed some 3 line static functions from Telepathy to allocate GValues for the array ... i'm rambling now, but if there were any more than two GObjects, a hashmap (datalist?) keyed on the address might worthwhile, but since its only `m_listener` and `m_handler` ... meh.

^^^^^ i went that path trying to support GCallback and GClosure in a stupidly generic and "optimized" way.  if not obvious from the patch, API supports GClosure only (for good reason IMO).

thanks guys! look forward to review.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files']" exit_code: 1
Total errors found: 0 in 0 files


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

[C Anthony]

^^^^^ what does that mean?  diff died or the style checker died?

now that i think about it, i have a feeling the `\%attributes` hunk wil conflict with trunk (trunk has quotes around the value ... converted to string!)

any chance this could be merged:

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

... i think the generator is dropping attributes.  the patch is valid but i will double check submission stuff and rebase onto master later tonight.

[C Anthony]

... also, the GObjectEventListener was nearly 100% rewritten -- the diff is very hard to follow and makes it look super complex.  i will try to reorder some stuff and get a better diff.

[C Anthony]

Created attachment 131302 [details]
Event Listeners as first class GObjects.

this patch is an improvement on the last, and updated to r110399 (today-ish IIRC)

things changed considerably since my original working tree -- i had to touch many more files just to *allow* the IDL to be processed ... if i could get some feedback ASAP that would be fantastic as im having difficulty keeping up with the changes.

... who else needs to know or should be involved?  i want to be sure my work, or others, are not in conflict.

testcases will be fixed once patch design is approved.
comments will be set to whatever you guys want.

PLEASE see the `handleEvent` except added to CodeGeneratorGObject.pm ... i dont know how to handle this.  i tried using `Supplement`, ie. modifying IDL etc ... not straight forward.  right now both the external and internal impls share the same $interfaceName identifiers, so there is no clean way (AFAICS) for me to operate as a "EventListener" externally (GObject/API), and a GObjectEventlistener internally (WebCore) ...

all in all, i think it's pretty solid, and it does everything as advertised.  will post a little demo app soon.

thoughts?

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files']" exit_code: 1
Total errors found: 0 in 0 files


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

[C Anthony]

oh yeah, and i enabled the attribute listeners just because it could be done, but im not actually sure if this is desired, and likely incomplete still.  it does compile/work however (eg, on<HANDLER>)

i can disable this if need be.

thanks!

[Philippe Normand]

CCing Xan our in-house GObject DOM bindings guru.

[C Anthony]

ill update the patch tonight with test cases completed, and Changelog (using Tools/Scripts/webkit-patch and friends)

in case it wasn't clear, i enabled the attribute-based handlers; this almost doubles the size of the GIR file (~2600l -> 4300+) and adds *many* new properties to all the objects.

personally, i find the attributes based ones convenient, *and* there is never a risk of accidentally "losing" your handler.  this works (python):

listener = doc.create_event_listener(_onload_cb)
window.set_onload(listener)

... perfectly! albeit this:

listener = doc.create_event_listener(_onload_cb)
window.props.onload = listener

... does not.  IIRC it complained about wrong GType ID, then segfaulted at some point. however, it's seems likey this is a pygobject problem since it work just fine the other way; will dig more tonight when i create the testcases in C.

the question is ...

*do we want to expose attribute handlers?*

:-D yay! <- awesome! me too!
:-( nay. <- oh ... i, uh, agree.

[Zan Dobersek]

I'll just post some concerns about the API for starters.

> b/Source/WebCore/bindings/gobject/WebKitDOMCustom.h:49
> +WEBKIT_API WebKitDOMEventListener* webkit_dom_document_create_event_listener(WebKitDOMDocument* self, GClosure* handler);

Event listener is in no way tied to a document in the DOM spec, meaning creating it should be done through a standalone function - webkit_dom_event_listener_create.

I'm reiterating ideas previously written in comment #22 - webkit_dom_event_listener_create would take in a WebKitDOMEventListenerFunc, gpointer to a data and a GDestroyNotify callback. As far as I am concerned, this is the most optimal solution. Using a GClosure is just another unnecessary step that should be avoided if possible.

> b/Source/WebCore/bindings/gobject/WebKitDOMCustom.h:58
> +WEBKIT_API void webkit_dom_event_listener_set_handler(WebKitDOMEventListener* self, GClosure* handler);

The DOM spec does not allow these sorts of manipulations on an event listener, and I am of a very strong opinion that we shouldn't neither.

> b/Source/WebCore/bindings/gobject/WebKitDOMCustom.h:68
> +WEBKIT_API GClosure* webkit_dom_event_listener_get_handler(WebKitDOMEventListener* self);

Ditto.

[C Anthony]

(In reply to comment #33)
> I'll just post some concerns about the API for starters.
> 
> > b/Source/WebCore/bindings/gobject/WebKitDOMCustom.h:49
> > +WEBKIT_API WebKitDOMEventListener* webkit_dom_document_create_event_listener(WebKitDOMDocument* self, GClosure* handler);
> 
> Event listener is in no way tied to a document in the DOM spec, meaning creating it should be done through a standalone function - webkit_dom_event_listener_create.

ok ok ok, i did that mainly because people would already be aware of _document_create_event ... so adding _listener is a natural extension of that.

however, i'm mostly impartial, and will change it as suggested unless otherwise stated.

> I'm reiterating ideas previously written in comment #22 - webkit_dom_event_listener_create would take in a WebKitDOMEventListenerFunc, gpointer to a data and a GDestroyNotify callback. As far as I am concerned, this is the most optimal solution. Using a GClosure is just another unnecessary step that should be avoided if possible.

isn't GClosure specifically designed for exactly this? it simplifies things for bindings because an explicit type is not needed and the arguments are variable; additionally, the same [single] object can be used for C/Python/whatever, and maintains it's own ref/destroy.  i want to say it's faster for bindings too, but i could be making that up :-X

i discussed on IRC previously, and tomeu agreed with the GClosure approach, and added it would not affect performance.  one should note that the WebKitDOMEventListener will sink() the closure and assume ownership -- no need for the caller to manage it.

however, i admittedly moved to GClosure while implementing an alternative way, and never changed it back -- it's no longer a hard requirement -- if consensus is for callback/data/destroy signature i'll change it to that (i will anyway for time being)

> > b/Source/WebCore/bindings/gobject/WebKitDOMCustom.h:58
> > +WEBKIT_API void webkit_dom_event_listener_set_handler(WebKitDOMEventListener* self, GClosure* handler);
> 
> The DOM spec does not allow these sorts of manipulations on an event listener, and I am of a very strong opinion that we shouldn't neither.

i'm very much the opposite -- the DOM spec doesn't specify anything really, and EventListener is an technically only an interface ... i mean not even _create() would be permissible then, except by necessity.  by enabling the *real* handler to be swapped dynamically, at no cost or code complexity, add a LOT of power and flexibility -- isn't this useful?  imagine 200 Targets that all shared a Listener, but with varying event types ... not simple/fast to update, and expensive to update from a binding (marshall back/forth hundreds of times).  by exposing a this we can change the backing handler in a single operation from any language ...

you can think of it as a detail being exposed to the DOM implementation, NOT the DOM spec (spec doesn't undestand GCallback/etc, eg. _create(), setHandler()), so it doesn't violate anything IMO.

how about a compromise: you get callback/data/destroy sig, and i get setHandler? :-D fair?

[Zan Dobersek]

(In reply to comment #34)
> (In reply to comment #33)
> > > b/Source/WebCore/bindings/gobject/WebKitDOMCustom.h:58
> > > +WEBKIT_API void webkit_dom_event_listener_set_handler(WebKitDOMEventListener* self, GClosure* handler);
> > 
> > The DOM spec does not allow these sorts of manipulations on an event listener, and I am of a very strong opinion that we shouldn't neither.
> 
> i'm very much the opposite -- the DOM spec doesn't specify anything really, and EventListener is an technically only an interface ... i mean not even _create() would be permissible then, except by necessity.  by enabling the *real* handler to be swapped dynamically, at no cost or code complexity, add a LOT of power and flexibility -- isn't this useful?  imagine 200 Targets that all shared a Listener, but with varying event types ... not simple/fast to update, and expensive to update from a binding (marshall back/forth hundreds of times).  by exposing a this we can change the backing handler in a single operation from any language ...
> 
> you can think of it as a detail being exposed to the DOM implementation, NOT the DOM spec (spec doesn't undestand GCallback/etc, eg. _create(), setHandler()), so it doesn't violate anything IMO.
> 

DOM spec clearly specifies the EventListener interface, and that should be implemented. There are extensions to be made, such as the _create() method, but, as said, this is out of necessity. Other additions simply should not be made. DOM implementation should closely follow the spec and not deviate from it.

Given the test case of 200 targets, find the solution within the constraints of the DOM spec rather than exploiting a DOM implementation.

[C Anthony]

Created attachment 132222 [details]
Event Listeners as first class GObjects; func/data/destroy triple

[WebKit Review Bot]

Attachment 132222 [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:79:  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:92:  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:48:  Extra space between gboolean and useCapture  [whitespace/declaration] [3]
Source/WebCore/bindings/gobject/WebKitDOMEventTarget.h:52:  Extra space between gboolean and useCapture  [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:64:  The parameter name "listener" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/bindings/gobject/WebKitDOMEventTarget.h:65:  Extra space between gboolean and useCapture  [whitespace/declaration] [3]
Source/WebCore/bindings/gobject/WebKitDOMEventTarget.h:67:  The parameter name "target" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/bindings/gobject/WebKitDOMEventTarget.h:69:  The parameter name "listener" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/bindings/gobject/WebKitDOMEventTarget.h:70:  Extra space between gboolean and useCapture  [whitespace/declaration] [3]
Source/WebCore/bindings/gobject/GObjectEventListener.h:51:  The parameter name "destroyNotify" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/bindings/gobject/GObjectEventListener.h:55:  The parameter name "destroyNotify" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/bindings/gobject/GObjectEventListener.cpp:28:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/bindings/gobject/WebKitDOMCustom.cpp:30:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/bindings/gobject/WebKitDOMCustom.h:50:  The parameter name "destroy" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 15 in 13 files


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

[Gustavo Noronha (kov)]

Comment on attachment 132222 [details]
Event Listeners as first class GObjects; func/data/destroy triple

Attachment 132222 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11963061

[C Anthony]

(In reply to comment #38)
> (From update of attachment 132222 [details])
> Attachment 132222 [details] did not pass gtk-ews (gtk):
> Output: http://queues.webkit.org/results/11963061

it's not restarting properly -- it never reran the necessary GEN step to build that file -- this is a bug w/bot not patch.

[C Anthony]

Comment on attachment 132222 [details]
Event Listeners as first class GObjects; func/data/destroy triple

With great pain this patch undoes most of what the previous iteration accomplished; GClosures are replaced by GCallback/gpointer/GDestroyNotify triples.  After experienceing great pain trying to use this from Python ... please, GClosures!

I understand there are most likely issues, perhaps even basic ones, C/C++ novice of 6wks ... be nice pls :-/

[C Anthony]

(In reply to comment #33)
> I'll just post some concerns about the API for starters.

is this an indication that my code is complete {expletive deleted}? :-o

if you Zan, and others generously provide additional feedback on design/C/C++ to help me push this thru i would be most grateful ... after 6 wks of nothing else, i'm getting bored/burned, but *need* events for my projects! bah! DOM w/out events is nil ... i will do the work ... but i've implemented it a couple ways here -- and 3x more at home -- need solid path to success, and need to know why/what/must be updated!

i just need to do whatever it takes to wrap this up.  i'm a fairly competent full-time developer w/several langs, but some corners of the C/C++ realm are new and remain fuzzy ... just lmk whats wrong, and whats *really* wrong :-)

thanks to all for time/patience as this moves forward. this is key to extending the most powerful GUI engine in existence to a broad audience

/end rant-vertisement

> > b/Source/WebCore/bindings/gobject/WebKitDOMCustom.h:49
> > +WEBKIT_API WebKitDOMEventListener* webkit_dom_document_create_event_listener(WebKitDOMDocument* self, GClosure* handler);
> 
> Event listener is in no way tied to a document in the DOM spec, meaning creating it should be done through a standalone function - webkit_dom_event_listener_create.

can an EventListener from one doc listen for events in another?  in JS, mainly for DOM but even simple stuff like string comparison, things tend to get very wonky when objects mix cross-doc (doc1.String !== doc2.String, etc).  maybe it doesn't matter for these objects, idk.

... although in IRC you mentioned `ScriptExecutionContext` was a Document or Worker -- this suggests to me that `handleEvent` is in fact tied to a doc/context (otherwise how does stuff like "current propagating event" make sense?) AFAIK you never "naturally" recieve events across [i]frame, you have to first set the context/root to that window (JS again, my experience)

> I'm reiterating ideas previously written in comment #22 - webkit_dom_event_listener_create would take in a WebKitDOMEventListenerFunc, gpointer to a data and a GDestroyNotify callback. As far as I am concerned, this is the most optimal solution. Using a GClosure is just another unnecessary step that should be avoided if possible.

after doing a ton of work with this i cannot stress enough that a GClosure is the optimal and ideal solution.  i adds *minimal* work for a C developer, if really any at all, and in turn works splendidly with existing all binding tools.  in pygobject, with triples, i could not control the refcount -- it increases monotonically every time the listener fires -- no way to destroy ...

... it could very well be a bug with pygobject, or annotations, but again, these problems arise from trying to marshall + coordinate + manage 3 *independent* objects that *do not have any real relation whatsoever* ... GClosures purpose is to avoid these very issues! it does so with much less effort and code than i've written in supporting triples.

if GClosure *really* can't be accepted, then IMO it's crucial the API be split to accommodate (create/create_with_closure) because it's flat-out stable and hassle free for higher langs.  both interfaces must accept only a single parameter on creation, GCallback or GClosure, respectively, and accept a single Event on invocation (+context??..).  the `userdata` param currently in use breaks the DOM spec, and should be omitted; those who need it's facilities can simply use a GClosure.

... buuut why not just sidestep the redundancy and use GClosures exclusively?  We can whittle the original patch as needed, but this detail should persist.

> > b/Source/WebCore/bindings/gobject/WebKitDOMCustom.h:58
> > +WEBKIT_API void webkit_dom_event_listener_set_handler(WebKitDOMEventListener* self, GClosure* handler);
> 
> The DOM spec does not allow these sorts of manipulations on an event listener, and I am of a very strong opinion that we shouldn't neither.
> 
> [...]
> 
> Ditto.

as we discussed in IRC, i still maintain this would be incredibly useful for not just Python/higher langs, but *especially* C.  hotswappping N handlers with a single pointer swap is atomic and cheap ... however, at this point i'm mostly indifferent, and i do/respect understand your reasoning.

anyone else?

-------------------------------------------

the main piece i'm adament about is GClosures being used exclusively or at least prominently ... they *really do remove complexity* at every level.

also, contrary to v1, i disabled the attribute listeners because i _really_ need to get a working version of this stabilized/landed; attrib can come later if wanted...

^^^^^ and i misquoted in previous comments ... the GIR file BLIMPS to over 10x, ~40,000K lines, with attrib EventListeners enabled!  i no longer think it's worthwhile, as they are from HTML4, and events are already 100% exposed.

apologies for the length, or any perceived terseness, neither were intentional -- just a bit weary and in dire need of a couple guiding lights! *hint* *hint* ;-) ;-)

so, what can i do to land this beast?  what needs explain/explore/verify?  i implemented a couple other ways as well, one making great use of GValues to support Gclosure and GCallback on same interface, and the other using a manual ::kit() impl in WebKitDOMBindings to do some magic ... but IMO, what you see here are the cleanest, most succinct solutions, after weeks of experimentation.

please comment! thanks so much for your time!. and thanks for all the fish too -- delicious!

[Csaba Osztrogonác]

Comment on attachment 132222 [details]
Event Listeners as first class GObjects; func/data/destroy triple

r- now, because this patch made Qt EWS bots cry. After this patch they can't build patches anymore. :(((

[C Anthony]

:(In reply to comment #42)
> (From update of attachment 132222 [details])
> r- now, because this patch made Qt EWS bots cry. After this patch they can't build patches anymore. :(((

:-/ whoopsie.

the bots seem to be skipping the reGENeration stage ... aggressive caching i presume ... this is needed to create the WebKitDOMEventListener.(cpp|h) files, and update the top-level headers with that include.  most of this patch is autogenerated.

what can i do from here?  i just guidance/recommendation on how to proceed -- both approaches appear to work correctly.

comments?  would love to land this week if at all possible.

[C Anthony]

(In reply to comment #43)
> :(In reply to comment #42)
> > (From update of attachment 132222 [details] [details])
> > r- now, because this patch made Qt EWS bots cry. After this patch they can't build patches anymore. :(((
> 
> :-/ whoopsie.

after chatting with mrobinson in #webkitgtk+ i realize that i edited the QT build:

Source/WebCore/DerivedSources.pri

... :-/ i thought i needed to enable the eventlistener for everyone, which in retrospect is obvious that i do not ... GObject is GTK+ only.  derp.  sorry.
 
> what can i do from here?  i just guidance/recommendation on how to proceed -- both approaches appear to work correctly.

... i also just now realized all set* or sink* methods on GObjectEventListener are totally unnecessary; GObjectEventListener can simply call:

WebKit::kit(this)

... to get a ref to it's companion WebKitDOMEventListener!!! of course! it's so simple! i'm going to spin one last patch that does this, and creates *_with_closure variants, possibly even:

add_event_listener_with_closure
[...]

... if needed.  i think i can maintain API (event though it's hopelessly broken), and ABI if possible (i dont know what all breaks this...).

could someone pretty please confirm this as a solution?  i really really (really) want to "X" out this bug.

[Zan Dobersek]

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?

[Xan Lopez]

Comment on attachment 132222 [details]
Event Listeners as first class GObjects; func/data/destroy triple

I still need to go over this in detail (hopefully next week), but this patch breaks the API, so surely it cannot be right? It's pretty much auto r- because of that, we'll need to figure out something else...

[Zan Dobersek]

(In reply to comment #46)
> (From update of attachment 132222 [details])
> I still need to go over this in detail (hopefully next week), but this patch breaks the API, so surely it cannot be right? It's pretty much auto r- because of that, we'll need to figure out something else...

Is that a bad thing? Is there interest to get this into 1.8?

[C Anthony]

(In reply to comment #46)
> (From update of attachment 132222 [details])
> I still need to go over this in detail (hopefully next week), but this patch breaks the API, so surely it cannot be right? It's pretty much auto r- because of that, we'll need to figure out something else...

the add/remove_event_listeners currently have a `userData` param in what seems to be an arbitrary location ... i don't know how to remedy that properly?  while i have a few changes to make for other reasons, this patch brings the signatures permanently inline with the DOM spec, and allows for many other uses (eg, DOM spec allows a single EventListener to bind to unlimited EventTargets, current API breaks this, patch fixes by providing a reusable object).

i think the API is already pretty busted, and as-is, simply will never work for anyone but C consumers (ie. introspection-driven bindings like Python will never function, which is my motivation for this to begin with)

the API changes slightly, but there is very little required to update, for the most part its just creating a listener and passing it along instead of passing a raw pointer to a handler.  the signatures are only being corrected.

how does API ever change?  what was needed last year when events moved from signals to functions?  isn't the gobject bindings still green enough to allow for further development?

[Xan Lopez]

(In reply to comment #47)
> Is that a bad thing? Is there interest to get this into 1.8?

All our public API is stable once it's been released in a stable release, so yes, breaking it is a bad thing. It's probably too late for 1.8 now, but perhaps this could go in 1.8.1 since it's pretty important for bindings.

[C Anthony]

is there any timeline for this bug?

i painstakingly created some solutions, but i'm not very adept in C.  can someone please please carry this forward? event handling is 100% busted in introspected bindings, notably python, making the bindings as a whole next to useless (what good is a browser without events?)

i am in dire need of this again, after 4 months of patching ... i guess i will try again o get it merged, but i expect what takes me 2 wks probably takes a competent C developer less than a day.

status? please?

[C Anthony]

...so should i give up trying to fix this? everyone i talk to essentially gives me a "yes, the API is broken" and "but the API cannot be changed"... which amounts to ROCK \o/ HARDPLACE.

this is a TRIVIAL problem that is RENDERING THIS FABULOUS INTEGRATION NEXT TO USELESS for gobject-introspection based bindings such as PYTHON.

gobject-introspection is the BEST AND ONLY PATH python has for interfacing with Webkit.

what, what, what(?!) do i need to do/say/ask/write/patch to remedy this? i've been trying for over a year now to reach a solution... if i could simply be given INDICATION that it can somehow be fixed, i will make it HAPPEN!

thanks.

ALSO, can someone please make this higher priority that UNCONFIRMED? it is very much an issue for ANY HIGH LEVEL CONSUMER OF THIS BINDING.

[C Anthony]

this problem is supposedly "fixed" by:

https://bugs.webkit.org/show_bug.cgi?id=114980#c9

...but in reality, it's just masked, and gives the illusion of success (in fact, i was totally elated to discover it was "fixed" only to be thoroughly disappointed by smoke and mirrors).

as i said in the other bug:

> i'm also sorry if i appear crass or whatever, but i honestly don't know what
> more i can do short of physically walking to Apple and picketing outside.  if
> it seems like i'm YELLING, it's because I AM SHOUTING OBSCENITIES AS I TYPE
> THIS.

> c'mon guys, i did all the work 3 times over and implemented in multiple ways,
> just pick one. any one. i fully intend on setting a reminder to ping both of
> these bugs until i can get a concrete path forward...

> i don't need an immediate resolution, but you guys could throw me a bone and
> give me some glimmer of hope by simply ACKNOWLEDGING THE PROBLEM *OFFICIALLY*
> BY ACCEPTING THIS BUG:

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

...i know it's probably super-annoying, and i really would rather not, but i think i deserve some TRULY CONSTRUCTIVE INPUT OR RESOLUTION... i pretty much spoon-fed multiple solution onto deaf ears... with the exception of Zan Dobersek, and to some small degree others, i have gotten nothing but "oh gee, that's too bad, but we DGAF, sorry".

[Carlos Garcia Campos]

I think we can try to fix this without breaking the api/abi until we can change the api.

 - Add new methods, for example register/unregister_event_listener that receives the proper WebKitDOMEventListenerCallback + GDestroyNotify.
 - Modify the virtual functions of WebKitDOMEventTarget interface, since nobody outside WebKitDOM API should use the vfuncs directly even if they are public. We don't have padding to add new vfuncs.
 - Deprecate all the dispatch_event methods that are public, everybody should use the interface API, unless there's a good reason for those to the be public.

I don't know if there's a way for the bindings to skip the add/remove ones and expose the register/unregisiter as add/remove to follow the DOM spec. Once we can change the api/abi we can just rename the register/unregister methods as add/remove to follow the DOM spec.

[Carlos Garcia Campos]

(In reply to comment #53)
> I don't know if there's a way for the bindings to skip the add/remove ones and expose the register/unregisiter as add/remove to follow the DOM spec. 

There's Rename to: tag

[Carlos Garcia Campos]

Created attachment 214724 [details]
Another approach using a closure

This patch adds two new methods add_event_listener_with_closure and remove_event_listener_with_closure that receive a GClosure instead of a GObject. They are only to be used by bindings, that expose them as add_event_listener and remove_event_listener. This keeps the API compatible, and consistent with the DOM spec. ABI is ot broken either, because nobody could be using the virtual methods outside of WebKit, since all implementations use private API (GObjectEventListener). With this patch and changing the test case attached to this bug to use the new API:

link.add_event_listener('click', self.on_click, True)

Running the test case and clicking on the button gives:

on_click (<DOMHTMLButtonElement object at 0x12d3870 (WebKitDOMHTMLButtonElement at 0x13ac740)>, <DOMMouseEvent object at 0x15be280 (WebKitDOMMouseEvent at 0x1637930)>) {}

So, it seems to be working fine.

[WebKit Commit Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/gobject/GRefPtr.cpp', u'Source/WTF/wtf/gobject/GRefPtr.h', u'Source/WTF/wtf/gobject/GTypedefs.h', u'Source/WebCore/ChangeLog', u'Source/WebCore/bindings/gobject/GObjectEventListener.cpp', u'Source/WebCore/bindings/gobject/GObjectEventListener.h', u'Source/WebCore/bindings/gobject/WebKitDOMEventTarget.cpp', u'Source/WebCore/bindings/gobject/WebKitDOMEventTarget.h', u'Source/WebCore/bindings/scripts/CodeGeneratorGObject.pm']" exit_code: 1
Source/WebCore/bindings/gobject/WebKitDOMEventTarget.cpp:89:  webkit_dom_event_target_add_event_listener_with_closure is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/bindings/gobject/WebKitDOMEventTarget.cpp:102:  webkit_dom_event_target_remove_event_listener_with_closure is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/bindings/gobject/WebKitDOMEventTarget.h:45:  Extra space between GClosure and *handler  [whitespace/declaration] [3]
Source/WebCore/bindings/gobject/WebKitDOMEventTarget.h:45:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/gobject/WebKitDOMEventTarget.h:46:  Extra space between gboolean and bubble  [whitespace/declaration] [3]
Source/WebCore/bindings/gobject/WebKitDOMEventTarget.h:46:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/gobject/WebKitDOMEventTarget.h:49:  Extra space between GClosure and *handler  [whitespace/declaration] [3]
Source/WebCore/bindings/gobject/WebKitDOMEventTarget.h:103:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebCore/bindings/gobject/WebKitDOMEventTarget.h:104:  Extra space between GClosure and *handler  [whitespace/declaration] [3]
Source/WebCore/bindings/gobject/WebKitDOMEventTarget.h:102:  The parameter name "target" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/bindings/gobject/WebKitDOMEventTarget.h:105:  Extra space between gboolean and bubble  [whitespace/declaration] [3]
Source/WebCore/bindings/gobject/WebKitDOMEventTarget.h:122:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/gobject/WebKitDOMEventTarget.h:123:  Extra space between GClosure and *handler  [whitespace/declaration] [3]
Source/WebCore/bindings/gobject/WebKitDOMEventTarget.h:123:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/gobject/WebKitDOMEventTarget.h:121:  The parameter name "target" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/bindings/gobject/WebKitDOMEventTarget.h:124:  Extra space between gboolean and bubble  [whitespace/declaration] [3]
Source/WebCore/bindings/gobject/WebKitDOMEventTarget.h:124:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 17 in 10 files


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

[Carlos Garcia Campos]

Created attachment 214725 [details]
Updated patch including new binding test results

Forgot to update the bindings test results

[WebKit Commit Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/gobject/GRefPtr.cpp', u'Source/WTF/wtf/gobject/GRefPtr.h', u'Source/WTF/wtf/gobject/GTypedefs.h', u'Source/WebCore/ChangeLog', u'Source/WebCore/bindings/gobject/GObjectEventListener.cpp', u'Source/WebCore/bindings/gobject/GObjectEventListener.h', u'Source/WebCore/bindings/gobject/WebKitDOMEventTarget.cpp', u'Source/WebCore/bindings/gobject/WebKitDOMEventTarget.h', u'Source/WebCore/bindings/scripts/CodeGeneratorGObject.pm', u'Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestEventTarget.cpp', u'Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestNode.cpp']" exit_code: 1
Source/WebCore/bindings/gobject/WebKitDOMEventTarget.cpp:89:  webkit_dom_event_target_add_event_listener_with_closure is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/bindings/gobject/WebKitDOMEventTarget.cpp:102:  webkit_dom_event_target_remove_event_listener_with_closure is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/bindings/gobject/WebKitDOMEventTarget.h:45:  Extra space between GClosure and *handler  [whitespace/declaration] [3]
Source/WebCore/bindings/gobject/WebKitDOMEventTarget.h:45:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/gobject/WebKitDOMEventTarget.h:46:  Extra space between gboolean and bubble  [whitespace/declaration] [3]
Source/WebCore/bindings/gobject/WebKitDOMEventTarget.h:46:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/gobject/WebKitDOMEventTarget.h:49:  Extra space between GClosure and *handler  [whitespace/declaration] [3]
Source/WebCore/bindings/gobject/WebKitDOMEventTarget.h:103:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebCore/bindings/gobject/WebKitDOMEventTarget.h:104:  Extra space between GClosure and *handler  [whitespace/declaration] [3]
Source/WebCore/bindings/gobject/WebKitDOMEventTarget.h:102:  The parameter name "target" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/bindings/gobject/WebKitDOMEventTarget.h:105:  Extra space between gboolean and bubble  [whitespace/declaration] [3]
Source/WebCore/bindings/gobject/WebKitDOMEventTarget.h:122:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/gobject/WebKitDOMEventTarget.h:123:  Extra space between GClosure and *handler  [whitespace/declaration] [3]
Source/WebCore/bindings/gobject/WebKitDOMEventTarget.h:123:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/gobject/WebKitDOMEventTarget.h:121:  The parameter name "target" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/bindings/gobject/WebKitDOMEventTarget.h:124:  Extra space between gboolean and bubble  [whitespace/declaration] [3]
Source/WebCore/bindings/gobject/WebKitDOMEventTarget.h:124:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 17 in 12 files


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

[C Anthony]

hi Carlos,

thank you so much for taking some time to look into this.

i know the bindings generated by introspection are capable of exposing the *_with_closure variants as the real thing to the bound language... i was told previously that GTK does exactly this, with *_full variants (IIRC, WebKit/someone must then ship a small python file in the appropriate location indicating/handling this)

so does the patch as-is maintain A[PB]I? or is this patch the "final" or "eventual" version?  if there is any testing/verification i can do to help out, i am more than willing to do so.

[C Anthony]

oops, i missed that you found the "Rename to:" annotation, awesome!

if the goal is still to align with DOM spec, we might want to consider fixing the signature of addEventListener, which is (per the spec):

addEventListener(type, listener[, useCapture])

...vs what we have:

addEventListener(event_name, handler[, bubble])

...which i believe is actually backward; the third arg in JS is normally `false`, indicating WE WANT TO REGISTER WITH THE BUBBLE PHASE.

i'd have to double check, but i think i had to manually invert this flag when integrating with an existing framework (which expects the former, per the spec). alas, i want *something* to be merged more than anything else, so feel free to completely ignore this in the name of progress!

[Carlos Garcia Campos]

(In reply to comment #59)
> hi Carlos,
> 
> thank you so much for taking some time to look into this.
> 
> i know the bindings generated by introspection are capable of exposing the *_with_closure variants as the real thing to the bound language... i was told previously that GTK does exactly this, with *_full variants (IIRC, WebKit/someone must then ship a small python file in the appropriate location indicating/handling this)
> 
> so does the patch as-is maintain A[PB]I? or is this patch the "final" or "eventual" version?  if there is any testing/verification i can do to help out, i am more than willing to do so.

Yes, I think it should be API/ABI compatible, I have changed the vmethods that should only be used internally, but the size of the structure is the same.

[Carlos Garcia Campos]

(In reply to comment #60)
> oops, i missed that you found the "Rename to:" annotation, awesome!
> 
> if the goal is still to align with DOM spec, we might want to consider fixing the signature of addEventListener, which is (per the spec):
> 
> addEventListener(type, listener[, useCapture])
> 
> ...vs what we have:
> 
> addEventListener(event_name, handler[, bubble])
> 
> ...which i believe is actually backward; the third arg in JS is normally `false`, indicating WE WANT TO REGISTER WITH THE BUBBLE PHASE.

This is one of the problems of boolean arguments. Changing the logic wouldn't break the API either, but it could break existing apps using the bubble argument, so we can't change this until we break the API.

> i'd have to double check, but i think i had to manually invert this flag when integrating with an existing framework (which expects the former, per the spec). alas, i want *something* to be merged more than anything else, so feel free to completely ignore this in the name of progress!

[Carlos Garcia Campos]

There's another thing I don't understand about the EventTarget interface exposed in the GObject bindings. The interface is defined as:

void addEventListener(DOMString type, 
                          EventListener listener, 
                          optional boolean useCapture);
void removeEventListener(DOMString type, 
                             EventListener listener, 
                             optional boolean useCapture);
[RaisesException] boolean dispatchEvent(Event event);

And we do exactly the opposite wrt to the return values, we return a boolean from add/remove_event_listener and void in dispatch_event. But then, we expose dispatch_event as public API in objects implementing the EventTarget interface returning a boolean. I thought about deprecating all the public dispatch_event in favor of using the interface, but those public methods are the only way to get the return value of dispatchEvent.

[Carlos Garcia Campos]

(In reply to comment #62)
> (In reply to comment #60)
> > oops, i missed that you found the "Rename to:" annotation, awesome!
> > 
> > if the goal is still to align with DOM spec, we might want to consider fixing the signature of addEventListener, which is (per the spec):
> > 
> > addEventListener(type, listener[, useCapture])
> > 
> > ...vs what we have:
> > 
> > addEventListener(event_name, handler[, bubble])
> > 
> > ...which i believe is actually backward; the third arg in JS is normally `false`, indicating WE WANT TO REGISTER WITH THE BUBBLE PHASE.
> 
> This is one of the problems of boolean arguments. Changing the logic wouldn't break the API either, but it could break existing apps using the bubble argument, so we can't change this until we break the API.
> 
> > i'd have to double check, but i think i had to manually invert this flag when integrating with an existing framework (which expects the former, per the spec). alas, i want *something* to be merged more than anything else, so feel free to completely ignore this in the name of progress!

We are actually using capture, but with the wrong name in the API to make it even more confusing. The boolean argument we are receiving from the API is what we are passing to WebCore, see:

http://trac.webkit.org/browser/trunk/Source/WebCore/bindings/gobject/WebKitDOMEventTarget.cpp#L68
http://trac.webkit.org/browser/trunk/Source/WebCore/bindings/gobject/GObjectEventListener.h#L38

So, we can simply rename the argument as use_capture.

[Carlos Garcia Campos]

Created attachment 214942 [details]
Updated patch

Patch updated to change the dispatch_event public method to return a gboolean and rename bubble parameter as use_capture in all public methods.

[WebKit Commit Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/gobject/GRefPtr.cpp', u'Source/WTF/wtf/gobject/GRefPtr.h', u'Source/WTF/wtf/gobject/GTypedefs.h', u'Source/WebCore/ChangeLog', u'Source/WebCore/bindings/gobject/GObjectEventListener.cpp', u'Source/WebCore/bindings/gobject/GObjectEventListener.h', u'Source/WebCore/bindings/gobject/WebKitDOMEventTarget.cpp', u'Source/WebCore/bindings/gobject/WebKitDOMEventTarget.h', u'Source/WebCore/bindings/scripts/CodeGeneratorGObject.pm', u'Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestEventTarget.cpp', u'Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestNode.cpp']" exit_code: 1
Source/WebCore/bindings/gobject/WebKitDOMEventTarget.cpp:49:  webkit_dom_event_target_dispatch_event is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/bindings/gobject/WebKitDOMEventTarget.cpp:58:  webkit_dom_event_target_add_event_listener is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/bindings/gobject/WebKitDOMEventTarget.cpp:68:  webkit_dom_event_target_remove_event_listener is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/bindings/gobject/WebKitDOMEventTarget.cpp:77:  webkit_dom_event_target_add_event_listener_with_closure is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/bindings/gobject/WebKitDOMEventTarget.cpp:86:  webkit_dom_event_target_remove_event_listener_with_closure is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/bindings/gobject/WebKitDOMEventTarget.h:45:  Extra space between GClosure and *handler  [whitespace/declaration] [3]
Source/WebCore/bindings/gobject/WebKitDOMEventTarget.h:45:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/gobject/WebKitDOMEventTarget.h:46:  Extra space between gboolean and use_capture  [whitespace/declaration] [3]
Source/WebCore/bindings/gobject/WebKitDOMEventTarget.h:46:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/gobject/WebKitDOMEventTarget.h:46:  use_capture is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/bindings/gobject/WebKitDOMEventTarget.h:49:  Extra space between GClosure and *handler  [whitespace/declaration] [3]
Source/WebCore/bindings/gobject/WebKitDOMEventTarget.h:50:  Extra space between gboolean and use_capture  [whitespace/declaration] [3]
Source/WebCore/bindings/gobject/WebKitDOMEventTarget.h:50:  use_capture is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/bindings/gobject/WebKitDOMEventTarget.h:64:  The parameter name "target" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/bindings/gobject/WebKitDOMEventTarget.h:81:  Extra space between gboolean and use_capture  [whitespace/declaration] [3]
Source/WebCore/bindings/gobject/WebKitDOMEventTarget.h:81:  use_capture is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/bindings/gobject/WebKitDOMEventTarget.h:96:  Extra space between gboolean and use_capture  [whitespace/declaration] [3]
Source/WebCore/bindings/gobject/WebKitDOMEventTarget.h:96:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/gobject/WebKitDOMEventTarget.h:96:  use_capture is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/bindings/gobject/WebKitDOMEventTarget.h:113:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebCore/bindings/gobject/WebKitDOMEventTarget.h:114:  Extra space between GClosure and *handler  [whitespace/declaration] [3]
Source/WebCore/bindings/gobject/WebKitDOMEventTarget.h:112:  The parameter name "target" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/bindings/gobject/WebKitDOMEventTarget.h:115:  Extra space between gboolean and use_capture  [whitespace/declaration] [3]
Source/WebCore/bindings/gobject/WebKitDOMEventTarget.h:115:  use_capture is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/bindings/gobject/WebKitDOMEventTarget.h:132:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/gobject/WebKitDOMEventTarget.h:133:  Extra space between GClosure and *handler  [whitespace/declaration] [3]
Source/WebCore/bindings/gobject/WebKitDOMEventTarget.h:133:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/gobject/WebKitDOMEventTarget.h:131:  The parameter name "target" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/bindings/gobject/WebKitDOMEventTarget.h:134:  Extra space between gboolean and use_capture  [whitespace/declaration] [3]
Source/WebCore/bindings/gobject/WebKitDOMEventTarget.h:134:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/gobject/WebKitDOMEventTarget.h:134:  use_capture is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 31 in 12 files


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

[Gustavo Noronha (kov)]

Comment on attachment 214942 [details]
Updated patch

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

[Carlos Garcia Campos]

(In reply to comment #67)
> (From update of attachment 214942 [details])
> LGTM!

Thanks for the review.

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

Let me try, yes.

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

We don't want to adopt the ref in the case of closure received from add_event_listener_with_closure, so GObjectEventListener should always take a new reference.

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

Yes, I explained the situation in the mailing list asking if anybody is using dispatch_event, and nobody replied so far. If someone eventually complains we can just revert this patch before the next stable release.

[Carlos Garcia Campos]

Committed r157918: <http://trac.webkit.org/changeset/157918>