Bug 63878 – Add support for constructor syntax for Events

• Component: DOM
• Status: RESOLVED FIXED

[Sam Weinig]

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 })).

[Sam Weinig]

Created attachment 99582 [details]
First pass a test case.

[Sam Weinig]

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

[Sam Weinig]

Created attachment 99593 [details]
Second pass (now with CustomEvent support)

[Sam Weinig]

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.

[Sam Weinig]

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.

[Sam Weinig]

Created attachment 101345 [details]
Patch

[WebKit Review Bot]

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.

[Sam Weinig]

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

[Gyuyoung Kim]

Comment on attachment 101345 [details]
Patch

Attachment 101345 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/9153244

[WebKit Review Bot]

Comment on attachment 101345 [details]
Patch

Attachment 101345 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9150253

[Darin Adler]

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.

[WebKit Review Bot]

Comment on attachment 101345 [details]
Patch

Attachment 101345 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/9155243

[Sam Weinig]

Created attachment 105451 [details]
Patch

[Sam Weinig]

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.

[WebKit Review Bot]

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.

[Oliver Hunt]

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

[Sam Weinig]

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

[Sam Weinig]

Committed r93951: <http://trac.webkit.org/changeset/93951>

[Csaba Osztrogonác]

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?

[Pavel Podivilov]

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