Bug 35703 – Add support for DOM Level 3 Custom Event

• Component: WebCore Misc.
• Status: CLOSED FIXED

[Kenneth Rohde Christiansen]

Created attachment 49943 [details]
Patch

Add support for the DOM Level 3 Custom Event, as specified by http://www.w3.org/TR/DOM-Level-3-Events.

The namespace version of init is not implemented as WebKit doesn't yet support events with namespace.

[Antonio Gomes]

great work.

[WebKit Review Bot]

Attachment 49943 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/330062

[Dimitri Glazkov (Google)]

Nate, can you help Kenneth with V8 bindings?

[Nate Chapin]

This patch adds CustomEvent.idl to build files, but I don't see it actually being added.  Am I missing something?

In terms of changes in WebCore/bindings/v8/, the following changes are needed:
1. Add CustomEvent.h to DOMObjectsInclude.h
2. Add a line for CustomEvent to macros in V8Index.h
3. Add a downcast to toV8() in custom/V8EventCustom.cpp, very much analogous to the change in bindings/js/JSEventCustom.cpp.

In theory, the V8 bindings should Just Work with those changes.

One more question: I notice CustomEvent takes a DOMObject* in its constructor and as a private member.  It looks to me like that class is defined in bindings/js/JSDOMWrapper.h.  Is that right?

[WebKit Review Bot]

Attachment 49943 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/331019

[Sam Weinig]

A couple of comments.
- What is the use case for Custom Events?
- Do any other browsers support it?
- Instead of storing a DOMObject, this should probably store a protected JSValue, or a JSValue that is explicitly marked. You should also think about how this should work with other bindings.
- This probably needs at least one test that tests the garbage collection aspect of this, making sure the detail object is kept alive by the event.

[WebKit Review Bot]

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

[Kenneth Rohde Christiansen]

(In reply to comment #6)
> A couple of comments.
> - What is the use case for Custom Events?
> - Do any other browsers support it?
> - Instead of storing a DOMObject, this should probably store a protected
> JSValue, or a JSValue that is explicitly marked. You should also think about
> how this should work with other bindings.
> - This probably needs at least one test that tests the garbage collection
> aspect of this, making sure the detail object is kept alive by the event.

Great, thanks for the feedback, Sam.

Custom Events are nice for application development and so far people have been emulating them using YUI or jQuery .bind().

Basically I'm implementing CustomEvent as it serves our needs and because I didn't want to implement the event part of the W3C Widgets 1.0 spec as it has a lot of issues and is in flux.

I will do the JSValue change.

Can you give me some hints on how to test the garbage collection?

[Kenneth Rohde Christiansen]

(In reply to comment #4)
> This patch adds CustomEvent.idl to build files, but I don't see it actually
> being added.  Am I missing something?

I much have misses it when I redid the patch.

> In terms of changes in WebCore/bindings/v8/, the following changes are needed:
> 1. Add CustomEvent.h to DOMObjectsInclude.h
> 2. Add a line for CustomEvent to macros in V8Index.h
> 3. Add a downcast to toV8() in custom/V8EventCustom.cpp, very much analogous to
> the change in bindings/js/JSEventCustom.cpp.
> 
> In theory, the V8 bindings should Just Work with those changes.

Great! I'll do it! Thanks for the explanation.

> One more question: I notice CustomEvent takes a DOMObject* in its constructor
> and as a private member.  It looks to me like that class is defined in
> bindings/js/JSDOMWrapper.h.  Is that right?

DOMObject is defined in JSDOMWrapper.h, yes. Maybe we need some more V8 changes because of this?

[Kenneth Rohde Christiansen]

> - Instead of storing a DOMObject, this should probably store a protected
> JSValue, or a JSValue that is explicitly marked. You should also think about
> how this should work with other bindings.

Storing a JSValue won't that break this for V8?

[Kenneth Rohde Christiansen]

Created attachment 50006 [details]
Patch 2

Add the V8 part plus the IDL

[WebKit Review Bot]

Attachment 50006 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/331473

[Kenneth Rohde Christiansen]

The V8 generator needs some changes as well. It would be nice if someone could help me with that.

[Kenneth Rohde Christiansen]

Created attachment 50050 [details]
Patch 3

Use JSValue directly.

The generator code could be improved a bit, and JSValue will also not work for Chromium, so I will attempt using ScriptValue instead.

Comments appreciated.

[Kenneth Rohde Christiansen]

Comment on attachment 50050 [details]
Patch 3

Marking for review, so that it is build on the bots

[Sam Weinig]

*** Bug 36269 has been marked as a duplicate of this bug. ***

[Kenneth Rohde Christiansen]

I am trying to rebase the patch to resume working on it, but the V8 bits has
changed.

--
1. Add CustomEvent.h to DOMObjectsInclude.h
2. Add a line for CustomEvent to macros in V8Index.h
--

Neither DOMObjectsInclude.h nor V8Index.h exists anymore. Can anyone give me a
hint on how to handle this in V8 land?

[Nate Chapin]

(In reply to comment #17)
> I am trying to rebase the patch to resume working on it, but the V8 bits has
> changed.
> 
> --
> 1. Add CustomEvent.h to DOMObjectsInclude.h
> 2. Add a line for CustomEvent to macros in V8Index.h
> --
> 
> Neither DOMObjectsInclude.h nor V8Index.h exists anymore. Can anyone give me a
> hint on how to handle this in V8 land?

Yeah, as of the last week, those parts are generated, no manual changes required.  Sorry for not warning you.

[Kenneth Rohde Christiansen]

Created attachment 51065 [details]
Patch 4 (rebased, w/o xcode change)

[WebKit Review Bot]

Attachment 51065 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/971030

[WebKit Review Bot]

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

[Csaba Osztrogonác]

Attachment 51065 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/1056011

[Oliver Hunt]

Comment on attachment 51065 [details]
Patch 4 (rebased, w/o xcode change)

r- as you appear to be missing the new files :-(

[Kenneth Rohde Christiansen]

Created attachment 51255 [details]
Patch 5 (w/o xcode change)

[Kenneth Rohde Christiansen]

(In reply to comment #24)
> Created an attachment (id=51255) [details]
> Patch 5 (w/o xcode change)

The V8 generator might need some changes. Could you test Nate?

[WebKit Review Bot]

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

[Kenneth Rohde Christiansen]

The patch passes
http://samples.msdn.microsoft.com/ietestcenter/domevents/domevents_harness.htm?url=./customevent.html


Test Description: Create and fire CustomEvent using methods: createEvent,
initEvent, dispatchEvent.

Test passes if the word "PASS" appears below.

Test result:
PASS

[Csaba Osztrogonác]

Attachment 51255 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/981096

[WebKit Review Bot]

Attachment 51255 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1020092

[Kenneth Rohde Christiansen]

Created attachment 51256 [details]
Patch 6

Added some missing files.

[WebKit Review Bot]

Attachment 51256 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1025115

[Kenneth Rohde Christiansen]

Created attachment 51261 [details]
Patch 7 (using ScriptValue)

[WebKit Review Bot]

Attachment 51261 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/961092

[Nate Chapin]

(In reply to comment #25)
> (In reply to comment #24)
> > Created an attachment (id=51255) [details] [details]
> > Patch 5 (w/o xcode change)
> 
> The V8 generator might need some changes. Could you test Nate?

I'll double check the details today, but yes, it looks like CodeGeneratorV8.pm will need some minor changes.

[Kenneth Rohde Christiansen]

> I'll double check the details today, but yes, it looks like CodeGeneratorV8.pm
> will need some minor changes.

Thanks Nate! Feel free to ping me online.

[Kenneth Rohde Christiansen]

Created attachment 51321 [details]
Patch 8 (with xcode)

[WebKit Review Bot]

Attachment 51321 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/977112

[Kenneth Rohde Christiansen]

Created attachment 51346 [details]
Patch 9 (with V8 generator change)

[Kenneth Rohde Christiansen]

Created attachment 51347 [details]
PAtch 10 (missed include)

[Kenneth Rohde Christiansen]

Created attachment 51429 [details]
Patch 11 (Let's see if mac compiles now)

[Antti Koivisto]

Created attachment 51496 [details]
same patch as above with fixed xcode project file

[Antti Koivisto]

Comment on attachment 51496 [details]
same patch as above with fixed xcode project file

r=me but please clean up the ChangeLog before landing. It has an unrelated change (from my tree when i updated the xcode project file).

[Kenneth Rohde Christiansen]

Landed in r56445

[Dmitry Titov]

Fixed infinite recursion in Chromium: http://trac.webkit.org/changeset/56464

[Simon Hausmann]

cherry-pick-for-backport: <r56464>

[Simon Hausmann]

Revision r56445 cherry-picked into qtwebkit-2.0 with commit 70067436d926bb8a208aaf76c8099221ad6e37da

[Simon Hausmann]

Revision r56464 cherry-picked into qtwebkit-2.0 with commit d7847f95d2024082a828afd4f2dac2f492fa5072

[Eli Grey (:sephr)]

This should be reopened as the CustomEvent interface itself should be exposed on the view as a property "CustomEvent", much like "Event", "UIEvent", etc. It's not that important but it's useful for thing such as extending CustomEvent to emulate Mozilla's DataContainerEvent, which is like CustomEvent, except with custom properties too.

[Alexey Proskuryakov]

Please file a new bug for that.

[Eli Grey (:sephr)]

(In reply to comment #49)
Sure, I filed bug 62039.