Bug 174313 – EventTarget constructor

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Domenic Denicola]

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

[Maga D. Zandaqo]

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.

[Lucas Forschler]

Mass move bugs into the DOM component.

[Radar WebKit Bug Importer]

<rdar://problem/51693619>

[Lars Knudsen]

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

[Alexey Shvayka]

Created attachment 374688 [details]
Patch

[Alexey Shvayka]

Created attachment 374692 [details]
Patch

Adjust web platform test.

[Alexey Shvayka]

Created attachment 374820 [details]
Patch

List tests in WebCore change log.

[Darin Adler]

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.

[Alexey Shvayka]

Created attachment 375541 [details]
Patch

Remove CustomConstructor() and use InternalFunction::createSubclassStructure in regular constructors.

[Alexey Shvayka]

Created attachment 375551 [details]
Patch

Avoid casting to JSCell directly.

[Darin Adler]

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.

[Darin Adler]

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?

[Jxck]

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

[Darin Adler]

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.

[Alexey Shvayka]

Created attachment 390756 [details]
Patch

Extract setSubclassStructureIfNeeded helper, handle cross-realm NewTarget, and drop 0 constant.

[Alexey Shvayka]

Created attachment 390761 [details]
Patch

Extract setSubclassStructureIfNeeded helper, handle cross-realm NewTarget, and drop 0 constant.

[Alexey Shvayka]

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

[Darin Adler]

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.

[Alexey Shvayka]

Created attachment 390868 [details]
Patch

Set reviewer, typecast result of toJSNewlyCreated, and add a case for EventTargetInterfaceType.

[Alexey Shvayka]

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

[Alexey Shvayka]

Created attachment 390869 [details]
Patch

Revert typecasting result of toJSNewlyCreated.

[Alexey Shvayka]

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

[WebKit Commit Bot]

Comment on attachment 390869 [details]
Patch

Clearing flags on attachment: 390869

Committed r256716: <https://trac.webkit.org/changeset/256716>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Saam Barati]

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?

[Saam Barati]

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?

[Darin Adler]

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.

[Alexey Shvayka]

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

[Maga D. Zandaqo]

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?

[Alexey Shvayka]

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

[Alexey Shvayka]

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