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:

Version:

528+ (Nightly build)

Hardware:

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+

Kenneth Rohde Christiansen

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

Attachments
Patch (20.14 KB, patch) 2010-03-03 14:02 PST, Kenneth Rohde Christiansen	no flags	Details Formatted Diff Diff
Patch 2 (24.06 KB, patch) 2010-03-04 05:24 PST, Kenneth Rohde Christiansen	no flags	Details Formatted Diff Diff
Patch 3 (28.79 KB, patch) 2010-03-04 13:52 PST, Kenneth Rohde Christiansen	no flags	Details Formatted Diff Diff
Patch 4 (rebased, w/o xcode change) (17.69 KB, patch) 2010-03-18 11:43 PDT, Kenneth Rohde Christiansen	oliver: review-	Details Formatted Diff Diff
Patch 5 (w/o xcode change) (18.52 KB, patch) 2010-03-21 14:59 PDT, Kenneth Rohde Christiansen	no flags	Details Formatted Diff Diff
Patch 6 (22.81 KB, patch) 2010-03-21 15:07 PDT, Kenneth Rohde Christiansen	no flags	Details Formatted Diff Diff
Patch 7 (using ScriptValue) (22.75 KB, patch) 2010-03-21 16:23 PDT, Kenneth Rohde Christiansen	no flags	Details Formatted Diff Diff
Patch 8 (with xcode) (27.59 KB, patch) 2010-03-22 11:45 PDT, Kenneth Rohde Christiansen	no flags	Details Formatted Diff Diff
Patch 9 (with V8 generator change) (29.65 KB, patch) 2010-03-22 13:49 PDT, Kenneth Rohde Christiansen	no flags	Details Formatted Diff Diff
PAtch 10 (missed include) (29.83 KB, patch) 2010-03-22 13:52 PDT, Kenneth Rohde Christiansen	no flags	Details Formatted Diff Diff
Patch 11 (Let's see if mac compiles now) (30.24 KB, patch) 2010-03-23 09:12 PDT, Kenneth Rohde Christiansen	no flags	Details Formatted Diff Diff
same patch as above with fixed xcode project file (27.44 KB, patch) 2010-03-24 07:34 PDT, Antti Koivisto	koivisto: review+	Details Formatted Diff Diff
Show Obsolete (11) View All Add attachment proposed patch, testcase, etc.

Antonio Gomes

Comment 1 2010-03-03 14:05:50 PST

great work.

WebKit Review Bot

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

Dimitri Glazkov (Google)

Comment 3 2010-03-03 14:31:00 PST

Nate, can you help Kenneth with V8 bindings?

Nate Chapin

Comment 4 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?

WebKit Review Bot

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

Sam Weinig

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

WebKit Review Bot

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

Kenneth Rohde Christiansen

Comment 8 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?

Kenneth Rohde Christiansen

Comment 9 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?

Kenneth Rohde Christiansen

Comment 10 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?

Kenneth Rohde Christiansen

Comment 11 2010-03-04 05:24:19 PST

Created attachment 50006 [details] Patch 2 Add the V8 part plus the IDL

WebKit Review Bot

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

Kenneth Rohde Christiansen

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

Kenneth Rohde Christiansen

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

Kenneth Rohde Christiansen

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

Sam Weinig

Comment 16 2010-03-17 22:07:55 PDT

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

Kenneth Rohde Christiansen

Comment 17 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?

Nate Chapin

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

Kenneth Rohde Christiansen

Comment 19 2010-03-18 11:43:17 PDT

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

WebKit Review Bot

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

WebKit Review Bot

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

Csaba Osztrogonác

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

Oliver Hunt

Comment 23 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 :-(

Kenneth Rohde Christiansen

Comment 24 2010-03-21 14:59:39 PDT

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

Kenneth Rohde Christiansen

Comment 25 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?

WebKit Review Bot

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

Kenneth Rohde Christiansen

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

Csaba Osztrogonác

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

WebKit Review Bot

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

Kenneth Rohde Christiansen

Comment 30 2010-03-21 15:07:41 PDT

Created attachment 51256 [details] Patch 6 Added some missing files.

WebKit Review Bot

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

Kenneth Rohde Christiansen

Comment 32 2010-03-21 16:23:45 PDT

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

WebKit Review Bot

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

Nate Chapin

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

Kenneth Rohde Christiansen

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

Kenneth Rohde Christiansen

Comment 36 2010-03-22 11:45:35 PDT

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

WebKit Review Bot

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

Kenneth Rohde Christiansen

Comment 38 2010-03-22 13:49:12 PDT

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

Kenneth Rohde Christiansen

Comment 39 2010-03-22 13:52:17 PDT

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

Kenneth Rohde Christiansen

Comment 40 2010-03-23 09:12:26 PDT

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

Antti Koivisto

Comment 41 2010-03-24 07:34:22 PDT

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

Antti Koivisto

Comment 42 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).

Kenneth Rohde Christiansen

Comment 43 2010-03-24 10:17:36 PDT

Landed in r56445

Dmitry Titov

Comment 44 2010-03-24 16:03:06 PDT

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

Simon Hausmann

Comment 45 2010-03-26 07:16:52 PDT

cherry-pick-for-backport: <r56464>

Simon Hausmann

Comment 46 2010-03-26 08:33:40 PDT

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

Simon Hausmann

Comment 47 2010-03-26 08:33:52 PDT

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

Eli Grey (:sephr)

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

Alexey Proskuryakov

Comment 49 2011-06-03 12:26:52 PDT

Please file a new bug for that.

Eli Grey (:sephr)

Comment 50 2011-06-03 12:29:38 PDT

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

Note You need to log in before you can comment on or make changes to this bug.