Bug 35703

Summary: Add support for DOM Level 3 Custom Event
Product: WebKit Reporter: Kenneth Rohde Christiansen <kenneth>
Component: WebCore Misc.Assignee: Kenneth Rohde Christiansen <kenneth>
Status: CLOSED FIXED    
Severity: Normal CC: aestes, bugmail, dglazkov, diegohcg, dimich, edisson.braga, gustavo, hausmann, japhet, laszlo.gombos, ossy, sam, tnoleto, tonikitoo, webkit.review.bot, xan.lopez
Priority: P3    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: All   
Bug Depends on:    
Bug Blocks: 35784    
Attachments:
Description Flags
Patch
none
Patch 2
none
Patch 3
none
Patch 4 (rebased, w/o xcode change)
oliver: review-
Patch 5 (w/o xcode change)
none
Patch 6
none
Patch 7 (using ScriptValue)
none
Patch 8 (with xcode)
none
Patch 9 (with V8 generator change)
none
PAtch 10 (missed include)
none
Patch 11 (Let's see if mac compiles now)
none
same patch as above with fixed xcode project file koivisto: review+

Description Kenneth Rohde Christiansen 2010-03-03 14:02:38 PST
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.
Comment 1 Antonio Gomes 2010-03-03 14:05:50 PST
great work.
Comment 2 WebKit Review Bot 2010-03-03 14:23:39 PST
Attachment 49943 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/330062
Comment 3 Dimitri Glazkov (Google) 2010-03-03 14:31:00 PST
Nate, can you help Kenneth with V8 bindings?
Comment 4 Nate Chapin 2010-03-03 14:46:26 PST
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?
Comment 5 WebKit Review Bot 2010-03-03 14:58:43 PST
Attachment 49943 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/331019
Comment 6 Sam Weinig 2010-03-03 15:09:44 PST
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.
Comment 7 WebKit Review Bot 2010-03-03 15:21:52 PST
Attachment 49943 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/332027
Comment 8 Kenneth Rohde Christiansen 2010-03-04 04:22:02 PST
(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?
Comment 9 Kenneth Rohde Christiansen 2010-03-04 04:28:00 PST
(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?
Comment 10 Kenneth Rohde Christiansen 2010-03-04 05:23:09 PST
> - 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?
Comment 11 Kenneth Rohde Christiansen 2010-03-04 05:24:19 PST
Created attachment 50006 [details]
Patch 2

Add the V8 part plus the IDL
Comment 12 WebKit Review Bot 2010-03-04 05:35:55 PST
Attachment 50006 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/331473
Comment 13 Kenneth Rohde Christiansen 2010-03-04 07:12:44 PST
The V8 generator needs some changes as well. It would be nice if someone could help me with that.
Comment 14 Kenneth Rohde Christiansen 2010-03-04 13:52:15 PST
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.
Comment 15 Kenneth Rohde Christiansen 2010-03-04 13:52:50 PST
Comment on attachment 50050 [details]
Patch 3

Marking for review, so that it is build on the bots
Comment 16 Sam Weinig 2010-03-17 22:07:55 PDT
*** Bug 36269 has been marked as a duplicate of this bug. ***
Comment 17 Kenneth Rohde Christiansen 2010-03-18 11:21:01 PDT
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?
Comment 18 Nate Chapin 2010-03-18 11:32:34 PDT
(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.
Comment 19 Kenneth Rohde Christiansen 2010-03-18 11:43:17 PDT
Created attachment 51065 [details]
Patch 4 (rebased, w/o xcode change)
Comment 20 WebKit Review Bot 2010-03-18 13:11:48 PDT
Attachment 51065 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/971030
Comment 21 WebKit Review Bot 2010-03-18 15:57:26 PDT
Attachment 51065 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/1017014
Comment 22 Csaba Osztrogonác 2010-03-19 09:24:57 PDT
Attachment 51065 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/1056011
Comment 23 Oliver Hunt 2010-03-20 21:08:37 PDT
Comment on attachment 51065 [details]
Patch 4 (rebased, w/o xcode change)

r- as you appear to be missing the new files :-(
Comment 24 Kenneth Rohde Christiansen 2010-03-21 14:59:39 PDT
Created attachment 51255 [details]
Patch 5 (w/o xcode change)
Comment 25 Kenneth Rohde Christiansen 2010-03-21 15:01:02 PDT
(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?
Comment 26 WebKit Review Bot 2010-03-21 15:02:24 PDT
Attachment 51255 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/1021097
Comment 27 Kenneth Rohde Christiansen 2010-03-21 15:03:22 PDT
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
Comment 28 Csaba Osztrogonác 2010-03-21 15:05:07 PDT
Attachment 51255 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/981096
Comment 29 WebKit Review Bot 2010-03-21 15:06:38 PDT
Attachment 51255 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1020092
Comment 30 Kenneth Rohde Christiansen 2010-03-21 15:07:41 PDT
Created attachment 51256 [details]
Patch 6

Added some missing files.
Comment 31 WebKit Review Bot 2010-03-21 15:25:36 PDT
Attachment 51256 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1025115
Comment 32 Kenneth Rohde Christiansen 2010-03-21 16:23:45 PDT
Created attachment 51261 [details]
Patch 7 (using ScriptValue)
Comment 33 WebKit Review Bot 2010-03-21 16:30:12 PDT
Attachment 51261 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/961092
Comment 34 Nate Chapin 2010-03-22 08:32:29 PDT
(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.
Comment 35 Kenneth Rohde Christiansen 2010-03-22 11:44:03 PDT
> 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.
Comment 36 Kenneth Rohde Christiansen 2010-03-22 11:45:35 PDT
Created attachment 51321 [details]
Patch 8 (with xcode)
Comment 37 WebKit Review Bot 2010-03-22 12:01:47 PDT
Attachment 51321 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/977112
Comment 38 Kenneth Rohde Christiansen 2010-03-22 13:49:12 PDT
Created attachment 51346 [details]
Patch 9 (with V8 generator change)
Comment 39 Kenneth Rohde Christiansen 2010-03-22 13:52:17 PDT
Created attachment 51347 [details]
PAtch 10 (missed include)
Comment 40 Kenneth Rohde Christiansen 2010-03-23 09:12:26 PDT
Created attachment 51429 [details]
Patch 11 (Let's see if mac compiles now)
Comment 41 Antti Koivisto 2010-03-24 07:34:22 PDT
Created attachment 51496 [details]
same patch as above with fixed xcode project file
Comment 42 Antti Koivisto 2010-03-24 08:39:08 PDT
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).
Comment 43 Kenneth Rohde Christiansen 2010-03-24 10:17:36 PDT
Landed in r56445
Comment 44 Dmitry Titov 2010-03-24 16:03:06 PDT
Fixed infinite recursion in Chromium: http://trac.webkit.org/changeset/56464
Comment 45 Simon Hausmann 2010-03-26 07:16:52 PDT
cherry-pick-for-backport: <r56464>
Comment 46 Simon Hausmann 2010-03-26 08:33:40 PDT
Revision r56445 cherry-picked into qtwebkit-2.0 with commit 70067436d926bb8a208aaf76c8099221ad6e37da
Comment 47 Simon Hausmann 2010-03-26 08:33:52 PDT
Revision r56464 cherry-picked into qtwebkit-2.0 with commit d7847f95d2024082a828afd4f2dac2f492fa5072
Comment 48 Eli Grey (:sephr) 2011-06-03 09:11:53 PDT
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.
Comment 49 Alexey Proskuryakov 2011-06-03 12:26:52 PDT
Please file a new bug for that.
Comment 50 Eli Grey (:sephr) 2011-06-03 12:29:38 PDT
(In reply to comment #49)
Sure, I filed bug 62039.