Bug 174313 - EventTarget constructor
Summary: EventTarget constructor
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: Safari Technology Preview
Hardware: All All
: P2 Normal
Assignee: Alexey Shvayka
URL:
Keywords: InRadar, WebExposed
Depends on:
Blocks:
 
Reported: 2017-07-10 09:34 PDT by Domenic Denicola
Modified: 2020-04-17 15:03 PDT (History)
24 users (show)

See Also:


Attachments
Patch (22.81 KB, patch)
2019-07-23 11:11 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (27.13 KB, patch)
2019-07-23 12:19 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (27.43 KB, patch)
2019-07-24 15:47 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (53.63 KB, patch)
2019-08-05 11:50 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (53.72 KB, patch)
2019-08-05 14:09 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (62.25 KB, patch)
2020-02-14 05:24 PST, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (65.76 KB, patch)
2020-02-14 07:11 PST, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (66.14 KB, patch)
2020-02-15 08:57 PST, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (66.06 KB, patch)
2020-02-15 11:13 PST, Alexey Shvayka
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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

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
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
<rdar://problem/51693619>
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.

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

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

List tests in WebCore change log.
Comment 8 Darin Adler 2019-07-24 18:39:10 PDT
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.
Comment 9 Alexey Shvayka 2019-08-05 11:50:09 PDT
Created attachment 375541 [details]
Patch

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

Avoid casting to JSCell directly.
Comment 11 Darin Adler 2019-08-05 18:54:53 PDT
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 12 Darin Adler 2019-08-05 18:55:55 PDT
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?
Comment 13 Jxck 2020-02-12 22:51:22 PST
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
Comment 14 Darin Adler 2020-02-13 14:23:13 PST
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.
Comment 15 Alexey Shvayka 2020-02-14 05:24:09 PST Comment hidden (obsolete)
Comment 16 Alexey Shvayka 2020-02-14 07:11:50 PST
Created attachment 390761 [details]
Patch

Extract setSubclassStructureIfNeeded helper, handle cross-realm NewTarget, and drop 0 constant.
Comment 17 Alexey Shvayka 2020-02-14 07:47:32 PST
(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 18 Darin Adler 2020-02-14 15:07:56 PST
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.
Comment 19 Alexey Shvayka 2020-02-15 08:57:31 PST
Created attachment 390868 [details]
Patch

Set reviewer, typecast result of toJSNewlyCreated, and add a case for EventTargetInterfaceType.
Comment 20 Alexey Shvayka 2020-02-15 09:04:18 PST
(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!
Comment 21 Alexey Shvayka 2020-02-15 11:13:12 PST
Created attachment 390869 [details]
Patch

Revert typecasting result of toJSNewlyCreated.
Comment 22 Alexey Shvayka 2020-02-15 11:17:30 PST
(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 23 WebKit Commit Bot 2020-02-16 17:41:32 PST
Comment on attachment 390869 [details]
Patch

Clearing flags on attachment: 390869

Committed r256716: <https://trac.webkit.org/changeset/256716>
Comment 24 WebKit Commit Bot 2020-02-16 17:41:34 PST
All reviewed patches have been landed.  Closing bug.
Comment 25 Saam Barati 2020-03-05 22:22:22 PST
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 26 Saam Barati 2020-03-05 22:31:02 PST
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 27 Darin Adler 2020-03-06 09:56:50 PST
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.
Comment 28 Alexey Shvayka 2020-03-06 15:41:44 PST
(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.
Comment 29 Maga D. Zandaqo 2020-03-13 10:31:19 PDT
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?