Bug 174313

Summary: Implement EventTarget constructor
Product: WebKit Reporter: Domenic Denicola <d>
Component: DOMAssignee: Nobody <webkit-unassigned>
Status: NEW ---    
Severity: Normal CC: 709922234, aperez, cdumez, darin, denelxan, dvpdiner2, larsgk, mjs, reuben.eggar, shvaikalesh, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: Safari Technology Preview   
Hardware: Unspecified   
OS: Unspecified   
Description Flags
Patch shvaikalesh: review?, shvaikalesh: commit-queue?

Description Domenic Denicola 2017-07-10 09:34:41 PDT
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

- 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
Comment 1 Maga D. Zandaqo 2018-12-04 05:11:57 PST
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.
Comment 2 Lucas Forschler 2019-02-06 09:18:34 PST
Mass move bugs into the DOM component.
Comment 3 Radar WebKit Bug Importer 2019-06-12 20:13:05 PDT
Comment 4 Lars Knudsen 2019-06-17 15:09:43 PDT
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.


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 ;)
Comment 5 Alexey Shvayka 2019-07-23 11:11:37 PDT
Created attachment 374688 [details]
Comment 6 Alexey Shvayka 2019-07-23 12:19:04 PDT
Created attachment 374692 [details]

Adjust web platform test.
Comment 7 Alexey Shvayka 2019-07-24 15:47:29 PDT
Created attachment 374820 [details]

List tests in WebCore change log.
Comment 8 Darin Adler 2019-07-24 18:39:10 PDT
Comment on attachment 374820 [details]

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.
Comment 9 Alexey Shvayka 2019-08-05 11:50:09 PDT
Created attachment 375541 [details]

Remove CustomConstructor() and use InternalFunction::createSubclassStructure in regular constructors.
Comment 10 Alexey Shvayka 2019-08-05 14:09:36 PDT
Created attachment 375551 [details]

Avoid casting to JSCell directly.
Comment 11 Darin Adler 2019-08-05 18:54:53 PDT
Comment on attachment 375541 [details]

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 12 Darin Adler 2019-08-05 18:55:55 PDT
Comment on attachment 375551 [details]

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?