Bug 40847 – [GTK] Fix DOM event dispatch

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

[Xan Lopez]

Patches coming.

[Xan Lopez]

Created attachment 59136 [details]
eventlisteres.diff

Fix eventlistener creation.

[Xan Lopez]

Created attachment 59138 [details]
unittestdispatch.diff

Unit test event dispatch.

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/bindings/gobject/WebKitDOMObject.cpp:25:  prop_id is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/bindings/gobject/WebKitDOMObject.cpp:34:  prop_id is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 2 in 3 files


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

[Martin Robinson]

Looks good to me!

Only one suggestion: Perhaps move the coreObject->ref() into the GObject itself. Since the object is responsible for the deref(), it makes sense for it to do the ref() as well.

You've exposed addEventListener to the bindings (I think). Is that part of some later work, or required for this patch?

[Xan Lopez]

(In reply to comment #4)
> Looks good to me!
> 
> Only one suggestion: Perhaps move the coreObject->ref() into the GObject itself. Since the object is responsible for the deref(), it makes sense for it to do the ref() as well.

Yeah, I actually planned to do this, but the issue is you cannot know the type of the coreObject in WebKitDOMObject, where that code lives, so you cannot do the casting we do in the wrap method to call ->ref(). On the other hand I don't think that's really needed, I suppose if you cast it to some really low level class where ref/deref are defined it should be more than enough.

> 
> You've exposed addEventListener to the bindings (I think). Is that part of some later work, or required for this patch?

I actually didn't, they are blacklisted in CodeGeneratorGObject. I just removed the guards that were there so that the Node wrapper correctly adds its event listeners.

[Martin Robinson]

(In reply to comment #5)
> Yeah, I actually planned to do this, but the issue is you cannot 
> know the type of the coreObject in WebKitDOMObject, where that code
> lives, so you cannot do the casting we do in the wrap method to call ->ref().

You're right, of course. I don't think there is shared reference counted
class between them. The only approach that I can think of is a vmethod
in the GObject. Not sure if it is worth it here.

[WebKit Review Bot]

Attachment 59138 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/3299372

[Gustavo Noronha (kov)]

Comment on attachment 59136 [details]
eventlisteres.diff

This looks very good to me. The only conceptual problem I have is with the property - I don't think we should expose a core object in public API (which a property ends up being). Having said that, I understand it would be difficult to deal with this in another way, and approve of the method. Time for GObject to grow private/protected properties? =)

[Xan Lopez]

Comment on attachment 59136 [details]
eventlisteres.diff

Landed as r62004.

[Xan Lopez]

Comment on attachment 59138 [details]
unittestdispatch.diff

This needs a bit of updating, removing from the queue.

(In reply to comment #10)
> (From update of attachment 59138 [details])
> This needs a bit of updating, removing from the queue.

I have added an updated patch which builds against the latest commit (apart from the constant changing Changelog)

Created attachment 62119 [details]
Updated patch

[Xan Lopez]

Created attachment 63244 [details]
uievents.diff

Updated patch. Adds a custom kit method for Event* instead off doing an ad-hoc create_event in Document, it's less code \o/

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WARNING: File exempt from style guide. Skipping: "WebKit/gtk/tests/testdomdomwindow.c"
WebCore/bindings/gobject/WebKitDOMEventTargetPrivate.h:25:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/bindings/gobject/WebKitDOMEventTargetPrivate.h:28:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 2 in 11 files


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

[Xan Lopez]

Created attachment 63255 [details]
uievents.diff

[Gustavo Noronha (kov)]

Comment on attachment 63255 [details]
uievents.diff

Seems sane.

[Xan Lopez]

Comment on attachment 63255 [details]
uievents.diff

Thank you, landed as r64580.

[Xan Lopez]

All patches landed, closing bug.