Bug 63878 - Add support for constructor syntax for Events
Summary: Add support for constructor syntax for Events
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sam Weinig
URL: http://dvcs.w3.org/hg/domcore/raw-fil...
Keywords:
Depends on:
Blocks: 67824
  Show dependency treegraph
 
Reported: 2011-07-03 14:58 PDT by Sam Weinig
Modified: 2011-09-08 18:05 PDT (History)
10 users (show)

See Also:


Attachments
First pass a test case. (3.06 KB, text/html)
2011-07-03 14:59 PDT, Sam Weinig
no flags Details
First pass at an implementation (16.73 KB, patch)
2011-07-03 15:02 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Second pass (now with CustomEvent support) (41.62 KB, patch)
2011-07-03 21:03 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Third pass (44.81 KB, patch)
2011-07-05 14:08 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (97.50 KB, patch)
2011-07-19 11:03 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (28.73 KB, patch)
2011-08-28 16:30 PDT, Sam Weinig
oliver: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Weinig 2011-07-03 14:58:08 PDT
The latest DOM Core draft has new syntax for constructing and initializing DOM events in a  much more succinct manner (new Event('eventType', { bubbles: true, cancelable: false })).
Comment 1 Sam Weinig 2011-07-03 14:59:08 PDT
Created attachment 99582 [details]
First pass a test case.
Comment 2 Sam Weinig 2011-07-03 15:02:42 PDT
Created attachment 99583 [details]
First pass at an implementation

I am throwing up my first pass at a implementation so I don't lose it. I currently has the following issues:
- It is not auto generating support for IDL dictionaries, which I think we should eventually.
- It only supports the Event base class (so new CustomEvent, new MouseEvent don't work).
- JSC only
(Probably other issues too).
Comment 3 Sam Weinig 2011-07-03 21:03:09 PDT
Created attachment 99593 [details]
Second pass (now with CustomEvent support)
Comment 4 Sam Weinig 2011-07-03 21:22:06 PDT
Now I have to decide how far I want to go without auto generating.  The pattern has become pretty clear to me; we need to generate the fillFoo() function from the "dictionary" Web IDL interfaces.  

It might make sense to find a better a way to share the default values for properties between the Event and EventInit classes (since they could get out of sync) or maybe EventInit should just store whether a particular property has been set.
Comment 5 Sam Weinig 2011-07-05 14:08:25 PDT
Created attachment 99744 [details]
Third pass

Add a few more Event subclasses and refactor to avoid adding so many more new files.  I am liking this general direction right now, may stick with it.
Comment 6 Sam Weinig 2011-07-19 11:03:03 PDT
Created attachment 101345 [details]
Patch
Comment 7 WebKit Review Bot 2011-07-19 11:05:48 PDT
Attachment 101345 [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/dom/WheelEvent.h:32:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/WebCore/dom/PageTransitionEvent.h:33:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/WebCore/bindings/js/JSEventConstructors.cpp:200:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/WebCore/bindings/js/JSEventConstructors.cpp:123:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/bindings/js/JSEventConstructors.cpp:128:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/bindings/js/JSEventConstructors.cpp:133:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/bindings/js/JSEventConstructors.cpp:138:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/dom/UIEventWithKeyState.h:31:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/WebCore/dom/OverflowEvent.h:33:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/WebCore/dom/WebKitAnimationEvent.h:33:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/WebCore/dom/CompositionEvent.h:34:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/WebCore/dom/MouseRelatedEvent.h:32:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/WebCore/dom/ErrorEvent.h:39:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/WebCore/dom/MouseEvent.h:32:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/WebCore/dom/MutationEvent.h:32:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/WebCore/dom/WebKitTransitionEvent.h:33:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 16 in 75 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 8 Sam Weinig 2011-07-19 11:15:27 PDT
I decide to use macros to create a DSL for these constructors.  A V8 specific implementation would be easy to add for someone who knew how.

I am posting this to see how well it builds, it needs additional tests and probably breaking up a bit (and maybe some enable guards).
Comment 9 Gyuyoung Kim 2011-07-19 11:16:35 PDT
Comment on attachment 101345 [details]
Patch

Attachment 101345 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/9153244
Comment 10 WebKit Review Bot 2011-07-19 11:33:17 PDT
Comment on attachment 101345 [details]
Patch

Attachment 101345 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9150253
Comment 11 Darin Adler 2011-07-19 11:57:42 PDT
Comment on attachment 101345 [details]
Patch

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

> Source/WebCore/bindings/generic/EventConstructors.h:31
> +#define INSTANTIAT_INIT_CONSTRUCTOR_FOR_EVENT(DICTIONARY_START, DICTIONARY_END, FILL_PARENT_PROPERTIES, FILL_PROPERTY) \

Typo throughout: INSTANTIAT instead of INSTANTIATE.

I also don’t completely understand what an “init constructor” is.
Comment 12 WebKit Review Bot 2011-07-19 12:07:40 PDT
Comment on attachment 101345 [details]
Patch

Attachment 101345 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/9155243
Comment 13 Sam Weinig 2011-08-28 16:30:27 PDT
Created attachment 105451 [details]
Patch
Comment 14 Sam Weinig 2011-08-28 16:31:59 PDT
Since there is now some movement on the V8 side to get this in, I am going to finish this off.  I am uploading a version with just support for Event, and the test case.
Comment 15 WebKit Review Bot 2011-08-28 16:33:04 PDT
Attachment 105451 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/WebCore/bindings/js/JSEventConstructors.cpp:99:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 16 Oliver Hunt 2011-08-28 16:41:19 PDT
Comment on attachment 105451 [details]
Patch

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

> Source/WebCore/bindings/js/JSEventConstructors.cpp:55
> +        // FIXME: Not all conversions require this exception check.

does this really warrant a fixme? it's a single indirect read that doesn't cause any harm.  If anything you should have another exception check before the convertValue call -- getValue can execute arbitrary js so can throw
Comment 17 Sam Weinig 2011-08-28 16:43:07 PDT
(In reply to comment #16)
> (From update of attachment 105451 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=105451&action=review
> 
> > Source/WebCore/bindings/js/JSEventConstructors.cpp:55
> > +        // FIXME: Not all conversions require this exception check.
> 
> does this really warrant a fixme? it's a single indirect read that doesn't cause any harm.  If anything you should have another exception check before the convertValue call -- getValue can execute arbitrary js so can throw

You are probably right.  I'll remove it.
Comment 18 Sam Weinig 2011-08-28 16:58:31 PDT
Committed r93951: <http://trac.webkit.org/changeset/93951>
Comment 19 Csaba Osztrogonác 2011-08-29 02:18:27 PDT
It made 3 tests fail on the Qt bot:
http://build.webkit.org/results/Qt%20Linux%20Release/r93951%20%2836880%29/results.html

fast/dom/constructed-objects-prototypes.html
fast/dom/dom-constructors.html
fast/events/constructors/event-constructors.html

I cc-ed Szeged gardeners of monday. Could you check it?
Comment 20 Pavel Podivilov 2011-08-29 04:58:06 PDT
These tests started to fail on all bots:

Regressions: Unexpected text diff mismatch : (13)
  fast/dom/constructed-objects-prototypes.html = TEXT
  fast/dom/dom-constructors.html = TEXT
  fast/events/constructors/event-constructors.html = TEXT
  fast/loader/local-CSS-from-local.html = TEXT
  fast/loader/local-JavaScript-from-local.html = TEXT
  fast/loader/local-image-from-local.html = TEXT