Summary: | Add support for constructor syntax for Events | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Sam Weinig <sam> | ||||||||||||||
Component: | DOM | Assignee: | Sam Weinig <sam> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | annevk, dglazkov, dominicc, feherzs, kkristof, loki, ossy, podivilov, webkit.review.bot, zoltan | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
URL: | http://dvcs.w3.org/hg/domcore/raw-file/tip/Overview.html#concept-event-constructor | ||||||||||||||||
Bug Depends on: | |||||||||||||||||
Bug Blocks: | 67824 | ||||||||||||||||
Attachments: |
|
Description
Sam Weinig
2011-07-03 14:58:08 PDT
Created attachment 99582 [details]
First pass a test case.
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).
Created attachment 99593 [details]
Second pass (now with CustomEvent support)
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. 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.
Created attachment 101345 [details]
Patch
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.
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 on attachment 101345 [details] Patch Attachment 101345 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/9153244 Comment on attachment 101345 [details] Patch Attachment 101345 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9150253 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 on attachment 101345 [details] Patch Attachment 101345 [details] did not pass cr-mac-ews (chromium): Output: http://queues.webkit.org/results/9155243 Created attachment 105451 [details]
Patch
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. 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 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 (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. Committed r93951: <http://trac.webkit.org/changeset/93951> 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? 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 |