This feature allows authors to create their own instances of EventTarget. Such instances do not participate in a tree. Spec: https://dom.spec.whatwg.org/#dom-eventtarget-eventtarget Tests: - http://w3c-test.org/dom/events/EventTarget-constructible.any.html - http://w3c-test.org/dom/events/EventTarget-constructible.any.worker.html Spec discussion: - https://github.com/whatwg/dom/issues/441 - https://github.com/whatwg/dom/pull/467
Chrome and Firefox have both implemented this feature, and Edge rumored to be discontinued soon, WebKit/Safari is the only major modern browser that lacks this feature.
Mass move bugs into the DOM component.
<rdar://problem/51693619>
This feature makes it possible to easily do ES6 modules that can act as services (think Angular 'service'), which has a great value for especially enterprise apps done in vanilla JS. Example: class _MyService extends EventTarget { ... this.dispatchEvent(new CustomEvent('new-data', {detail: data})); ... }; export const MyService = new _MyService(); . In my current team, we are making an (internal) app in a larger enterprise, where we need to have a small payload, fast execution and rendering - using lit-element and vanilla JS. In order to support iDevices, we will have to polyfill this, which is a bit sad ;)
Created attachment 374688 [details] Patch
Created attachment 374692 [details] Patch Adjust web platform test.
Created attachment 374820 [details] Patch List tests in WebCore change log.
Comment on attachment 374820 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=374820&action=review I like most everything about the patch except for the class name. Could we call it EventTargetConstructible or EventTargetConcrete or something that is more specific than "Impl"? > Source/WebCore/dom/EventTargetImpl.cpp:30 > +#include <wtf/IsoMallocInlines.h> Don’t need this since it’s also in the header. > Source/WebCore/dom/EventTargetImpl.h:41 > + ~EventTargetImpl() = default; Why is this needed? Won’t this compile fine without this and work properly? > Source/WebCore/dom/EventTargetImpl.h:45 > + using RefCounted::deref; > +private: Normally we’d leave a blank line before "private". > Source/WebCore/dom/EventTargetImpl.h:46 > + EventTargetImpl(ScriptExecutionContext&); Should have "explicit" here.
Created attachment 375541 [details] Patch Remove CustomConstructor() and use InternalFunction::createSubclassStructure in regular constructors.
Created attachment 375551 [details] Patch Avoid casting to JSCell directly.
Comment on attachment 375541 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=375541&action=review Looks good. > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:7223 > + push(@$outputArray, " auto newTarget = state->newTarget();\n"); > + push(@$outputArray, " if (newTarget != castedThis) {\n"); > + push(@$outputArray, " auto* jsCell = jsObject.asCell();\n"); > + push(@$outputArray, " auto* subclassStructure = InternalFunction::createSubclassStructure(state, newTarget, jsCell->structure());\n"); > + push(@$outputArray, " RETURN_IF_EXCEPTION(throwScope, { });\n"); > + push(@$outputArray, " jsCell->setStructure(vm, subclassStructure);\n"); > + push(@$outputArray, " }\n"); This is a lot of new code unconditionally that is usually not used; seems a bit inelegant. Also, there’s a slightly messy conversion to JSValue, then to JSCell*; that’s because toJSNewlyCreated returns a JSValue. I would expect we could make this a bit more elegant and find a way to make this part of a template function instead of generating so much code here. We typically try not to have the code generator write out functions if they can be templates instead. > Source/WebCore/bindings/scripts/InFilesCompiler.pm:234 > + print F " ${suffix} = 0,\n"; I don’t understand why this 0 constant is a good idea. Change log does not make it clear. > Source/WebCore/dom/make_event_factory.pl:138 > - print F " }\n"; > + print F " default:\n"; Why this change to move the code inside the switch? Seems slightly less elegant to me, arbitrary change that’s not an improvement.
Comment on attachment 375551 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=375551&action=review > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:7219 > + push(@$outputArray, " auto* jsObject = asObject(jsValue);\n"); Same comment applies here: this code assumes that the newly created value is an object. Is that guaranteed? Can it be null? Maybe it could be something different if there was an exception?
Do they have any updates here ? I wanna extend native EventTarget for my own class like other browser too. if I can do some work for help to solve this, please tell me. Jxck
Next step is for someone to revise this patch based on the comments I offered on 8/5 or respond to them explaining why not. You’re welcome to do it yourself if you’d like to become a WebKit contributor.
Created attachment 390756 [details] Patch Extract setSubclassStructureIfNeeded helper, handle cross-realm NewTarget, and drop 0 constant.
Created attachment 390761 [details] Patch Extract setSubclassStructureIfNeeded helper, handle cross-realm NewTarget, and drop 0 constant.
(In reply to Jxck from comment #13) > Do they have any updates here ? Thanks for the ping! I've totally forgot about this one, sorry. (In reply to Darin Adler from comment #11) > Why this change to move the code inside the switch? Seems slightly less > elegant to me, arbitrary change that’s not an improvement. Without this change, compilation fails because EventTargetInterfaceType is not handled in `switch`. We have EventTargetInterfaceType to avoid spreading EventTargetConrete and ConcreteEventTargetInterfaceType around code base. An alternative to this change would be to hack around %parsedEvents by adding EventTargetInterfaceType manually and introduce a new parameter, which seems even less elegant. (In reply to Darin Adler from comment #12) > Same comment applies here: this code assumes that the newly created value is > an object. Is that guaranteed? Can it be null? Maybe it could be something > different if there was an exception? I've added exception check before `setSubclassStructureIfNeeded` (for ConstructorMayThrowException) and null check for `newTarget` just in case.
Comment on attachment 390761 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=390761&action=review > Source/WebCore/bindings/js/JSDOMWrapperCache.h:204 > +template<typename DOMClass> inline void setSubclassStructureIfNeeded(JSC::JSGlobalObject* lexicalGlobalObject, JSC::CallFrame* callFrame, JSC::JSValue jsValue) I suggest changing the JSValue argument to a JSObject* argument. The code that knows the value is an object is presumably at the call site; it’s best to have the typecast where the knowledge is rather than elsewhere. > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:7366 > + push(@$outputArray, " auto jsValue = toJSNewlyCreated<${IDLType}>(" . join(", ", @constructionConversionArguments) . ");\n"); Seems like toJSNewlyCreated should return a JSObject* instead of a JSValue if it’s always going to be a JSObject. Don’t have to make that change now, but it’s better than returning something and knowing you can typecast back. > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:7368 > + push(@$outputArray, " setSubclassStructureIfNeeded<${implType}>(lexicalGlobalObject, callFrame, jsValue);\n"); Looks like this might slow down all other cases just to make the EventTarget case work. Maybe only a tiny bit, though. Is there some way to avoid that? > Source/WebCore/dom/make_event_factory.pl:138 > + print F " default:\n"; I would suggest instead adding a case for EventTargetInterfaceType here? print F " case EventTargetInterfaceType:\n"; print F " break;\n"; It’s nice to get a warning if we accidentally have any unhandled type, and this approach preserves it and also make the change to the script slightly simpler/smaller.
Created attachment 390868 [details] Patch Set reviewer, typecast result of toJSNewlyCreated, and add a case for EventTargetInterfaceType.
(In reply to Darin Adler from comment #18) > Looks like this might slow down all other cases just to make the EventTarget > case work. Maybe only a tiny bit, though. Is there some way to avoid that? Please note that all DOM constructors can be subclassed, not just EventTarget. I've made this explicit in ChangeLog. I believe second common case is subclassing events (CustomEvent mostly). Please see new tests passing in wpt/dom/events/Event-subclasses-constructors.html. We inline `setSubclassStructureIfNeeded` and it returns early for most of the calls; I am not sure what else can we do here. (In reply to Darin Adler from comment #18) > I would suggest instead adding a case for EventTargetInterfaceType here? Perfect, thank you!
Created attachment 390869 [details] Patch Revert typecasting result of toJSNewlyCreated.
(In reply to Darin Adler from comment #18) > Seems like toJSNewlyCreated should return a JSObject* instead of a JSValue > if it’s always going to be a JSObject. Don’t have to make that change now, > but it’s better than returning something and knowing you can typecast back. I've added a FIXME comment, yet typecasting result of toJSNewlyCreated (I used relatively safe .getObject()) causes mass EWS crashes w/o logs.
Comment on attachment 390869 [details] Patch Clearing flags on attachment: 390869 Committed r256716: <https://trac.webkit.org/changeset/256716>
All reviewed patches have been landed. Closing bug.
Comment on attachment 390869 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=390869&action=review > Source/WebCore/bindings/js/JSDOMWrapperCache.h:211 > + using WrapperClass = typename JSDOMWrapperConverterTraits<DOMClass>::WrapperClass; Can we static assert this inherits from InternalFunction?
Comment on attachment 390869 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=390869&action=review > Source/WebCore/bindings/js/JSDOMWrapperCache.h:220 > + jsObject->setStructure(vm, subclassStructure); I’m pretty confused what’s going on here. You create an object, and then might set its structure to something completely different. This feels quite wrong. Why aren’t we constructing the object with this structure? > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:7366 > + # FIXME: toJSNewlyCreated should return JSObject* instead of JSValue. Is there a bug for this? What are the implications here? Why are we currently returning JSValue?
Comment on attachment 390869 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=390869&action=review >> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:7366 >> + # FIXME: toJSNewlyCreated should return JSObject* instead of JSValue. > > Is there a bug for this? > > What are the implications here? Why are we currently returning JSValue? I believe there are no "real" implications. It’s a code tidiness, use types that are as specific as possible, thing.
(In reply to Saam Barati from comment #25) > Can we static assert this inherits from InternalFunction? Yes, we can add an assert. (In reply to Saam Barati from comment #26) > I’m pretty confused what’s going on here. You create an object, and then > might set its structure to something completely different. This feels quite > wrong. Why aren’t we constructing the object with this structure? The perfect place for InternalFunction::createSubclassStructure would be `getDOMStructure`, yet this would require passing `newTarget` via `toJSNewlyCreated` and `createWrapper`, which would result in massive code change (we have quite a few custom `toJSNewlyCreated`s). We can also avoid invoking `getCachedDOMStructure` and `cacheWrapper` there because caching doesn't make much sense for subclasses. If we decide to do that, we can also simplify `toJSNewlyCreated` signature by avoiding passing `throwScope` as an argument.
The ticket status has been set to resolved on February 16, so has it landed in TP already? Am I correct to assume that EventTarget constructor will be supported in the next version of Safari?
*** Bug 172867 has been marked as a duplicate of this bug. ***
*** Bug 167791 has been marked as a duplicate of this bug. ***