Implement EventTarget constructor

• Component: DOM
• Status: NEW ---

[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?