1379688 - Implement EventTarget constructor

Status: RESOLVED FIXED

(Core :: DOM: Events, enhancement, P3)

Description

[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

Comment 1

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

I thought this would be trivial, but actually EventTarget has a bunch of pure virtual functions.  I don't know how much refactoring would be necessary for us to implement this.  I assume it wouldn't be too hard, though.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

EventTarget wouldn't be used to implement the concrete object, but DOMEventTargetHelper.

Comment 3

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

WrapObject is still virtual, what should I do about that?

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Implement that in DOMEventTargetHelper and make it use EventTargetBinding.
Or possibly add a new class which extends DETH and implement WrapObject there. That might be clearer.
class name could be possibly something like ConstructibleEventTarget

Comment 5

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

error: no member named 'Wrap' in namespace 'mozilla::dom::EventTargetBinding'

I don't see it in EventTargetBinding.h.  In, e.g., TouchEventBinding.h I do see a Wrap() method defined.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Is that because of http://searchfox.org/mozilla-central/rev/0f16d437cce97733c6678d29982a6bcad49f817b/dom/bindings/Bindings.conf#324

Comment 7

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

 0:05.02 Traceback (most recent call last):
 0:05.02   File "/usr/lib/python2.7/runpy.py", line 174, in _run_module_as_main
 0:05.03     "__main__", fname, loader, pkg_name)
 0:05.03   File "/usr/lib/python2.7/runpy.py", line 72, in _run_code
 0:05.03     exec code in run_globals
 0:05.03   File "/mnt/ssd/checkouts/gecko/python/mozbuild/mozbuild/action/webidl.py", line 19, in <mo
dule>
 0:05.03     sys.exit(main(sys.argv[1:]))
 0:05.03   File "/mnt/ssd/checkouts/gecko/python/mozbuild/mozbuild/action/webidl.py", line 15, in mai
n
 0:05.03     manager.generate_build_files()
 0:05.03   File "/mnt/ssd/checkouts/gecko/dom/bindings/mozwebidlcodegen/__init__.py", line 273, in generate_build_files
 0:05.03     written, deps = self._generate_build_files_for_webidl(filename)
 0:05.03   File "/mnt/ssd/checkouts/gecko/dom/bindings/mozwebidlcodegen/__init__.py", line 479, in _generate_build_files_for_webidl
 0:05.03     root = CGBindingRoot(self._config, binding_stem, filename)
 0:05.03   File "/mnt/ssd/checkouts/gecko/dom/bindings/Codegen.py", line 14346, in __init__
 0:05.03     cgthings.extend([CGDescriptor(x) for x in descriptors])
 0:05.03   File "/mnt/ssd/checkouts/gecko/dom/bindings/Codegen.py", line 12829, in __init__
 0:05.03     properties = PropertyArrays(descriptor)
 0:05.03   File "/mnt/ssd/checkouts/gecko/dom/bindings/Codegen.py", line 2827, in __init__
 0:05.03     self.methods = MethodDefiner(descriptor, "Methods", static=False)
 0:05.03   File "/mnt/ssd/checkouts/gecko/dom/bindings/Codegen.py", line 2396, in __init__
 0:05.03     self.descriptor.name)
 0:05.03 TypeError: QueryInterface is only supported on interfaces that are concrete and all of whose ancestors are abstract: Attr

:(

Comment 8

[Boris Zbarsky [:bzbarsky]]

So just for my future reference... That QueryInterface support check dates back to bug 869195.

At the time we needed it because of a combination of several things:

1)  Lots of hasXPConnectImpls interfaces (though EventTarget is still hasXPConnectImpls, sadly).
2)  WebIDL quickstubs that would clobber XPConnect bits with WebIDL bits on XPConnect-impl
    objects for quickstubbed interfaces.  There are no more WebIDL quickstubs.
3)  The WebIDL QueryInterface not really working with non-webidl objects.  This is still true.

There were complications around Window and Element at the time, and we ended up putting QueryInterface on rootmost non-abstract things, and adding that check that's failing in comment 7 to make sure no one messes it up.

Since then, QueryInterface has become opt-in, not defined on everything that's rootmost non-abstract, and we more or less preserved the previous behavior by making sure you can only opt in on things that would have gotten it before.  But in the opt-in world, I think we can loosen this check up or remove it entirely.  Need to think about it a bit.  Of course if bug 888600 got fixed we'd be done with hasXPConnectImpls too, and that would make this even simpler...

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Remove some restrictions on whether an interface that implements QueryInterface can have a non-abstract ancestor

This restriction was put in place back when we automatically added
QueryInterface to all rootmost non-abstract interfaces.  At the time, we needed
to make sure it did NOT end up on EventTarget, because then webidl quickstubs
would replace the QI impl on non-webidl EventTargets with the WebIDL one, which
would not work for them.

Since then, we have removed WebIDL quickstubs and we now explicitly list which
interfaces get QueryInterface, so this check is no longer needed.

MozReview-Commit-ID: 5B13ymdyLp3

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Make the EventTarget interface constructible

MozReview-Commit-ID: 4xrSSqXna7F

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

Does this also make EventTarget extendable like:

class Whatever extends EventTarget {
}

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

Nevermind.  I see your dev-platform mail now.

Comment 13

[Boris Zbarsky [:bzbarsky]]

> Does this also make EventTarget extendable

Just for the record, yes.  And there are are web platform tests that test that.

Comment 14

[Boris Zbarsky [:bzbarsky]]

And also for the record, anything that has a constructor in our webidl automatically becomes extendable.  ;)

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8929803 [details] [diff] [review]
part 1.  Remove some restrictions on whether an interface that implements QueryInterface can have a non-abstract ancestor

Ahaa, I found interface LegacyQueryInterface.

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8929804 [details] [diff] [review]
part 2.  Make the EventTarget interface constructible

Thanks!

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/55753b766fd5
part 1.  Remove some restrictions on whether an interface that implements QueryInterface can have a non-abstract ancestor.  r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/312db92828f8
part 2.  Make the EventTarget interface constructible.  r=smaug

Comment 18

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/55753b766fd5
https://hg.mozilla.org/mozilla-central/rev/312db92828f8

Comment 19

[Chris Mills [:cmills]]

I've documented this by adding a new page for the constructor, and adding a note to the Fx59 rel notes:

https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/EventTarget
https://developer.mozilla.org/en-US/Firefox/Releases/59#DOM

I'm really not convinced that my example is any good on the ref page. Can you give me a quick code snippet showing how you'd be most likely to use this?

Thanks!

Comment 20

[Boris Zbarsky [:bzbarsky]]

Chris,

"EventTarget.EventTarget()" is really odd.  I'd expect the docs to look more like those for https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest or https://developer.mozilla.org/en-US/docs/Web/API/Websocket or something....

In any case, referring to it as "EventTarget.EventTarget()" in the rel notes seems really odd.  It should just say "The EventTarget constructor has been implemented".

As far as examples, I would do something like this:

  class MyEventTarget extends EventTarget {
    constructor(mySecret) {
      this._secret = mySecret;
    }

    get secret { return this._secret; }
  };

  var myEventTarget = new MyEventTarget(5);
  var value = myEventTarget.secret;  // == 5
  myEventTarget.addEventListener("something", function(e) {
    this.secret = e.detail;
  });
  var event = new CustomEvent("something", { detail: 7 });
  myEventTarget.dispatchEvent(event);
  var newValue = myEventTarget.secret; // == 7

or so.

Comment 21

[Chris Mills [:cmills]]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #20)
> Chris,
> 
> "EventTarget.EventTarget()" is really odd.  I'd expect the docs to look more
> like those for
> https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest or
> https://developer.mozilla.org/en-US/docs/Web/API/Websocket or something....
> 
> In any case, referring to it as "EventTarget.EventTarget()" in the rel notes
> seems really odd.  It should just say "The EventTarget constructor has been
> implemented".
> 
> As far as examples, I would do something like this:
> 
>   class MyEventTarget extends EventTarget {
>     constructor(mySecret) {
>       this._secret = mySecret;
>     }
> 
>     get secret { return this._secret; }
>   };
> 
>   var myEventTarget = new MyEventTarget(5);
>   var value = myEventTarget.secret;  // == 5
>   myEventTarget.addEventListener("something", function(e) {
>     this.secret = e.detail;
>   });
>   var event = new CustomEvent("something", { detail: 7 });
>   myEventTarget.dispatchEvent(event);
>   var newValue = myEventTarget.secret; // == 7
> 
> or so.

Cool, thanks for your help Boris. I've cleaned up the example a bit and added it in, and improved the confusing references to EventTarget().