Domenic Denicola Reporter
	Description • 7 years ago

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

	Aryeh Gregor (:ayg) (no longer with Mozilla)
	Comment 1 • 7 years ago

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.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 2 • 7 years ago

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

	Aryeh Gregor (:ayg) (no longer with Mozilla)
	Comment 3 • 7 years ago

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

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 4 • 7 years ago

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

	Aryeh Gregor (:ayg) (no longer with Mozilla)
	Comment 5 • 7 years ago

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.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 6 • 7 years ago

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

	Aryeh Gregor (:ayg) (no longer with Mozilla)
	Comment 7 • 7 years ago

 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

:(

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 8 • 7 years ago

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

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 9 • 7 years ago

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

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 10 • 7 years ago

MozReview-Commit-ID: 4xrSSqXna7F

	Ben Kelly [:bkelly, not reviewing]
	Comment 11 • 7 years ago

Does this also make EventTarget extendable like:

class Whatever extends EventTarget {
}

	Ben Kelly [:bkelly, not reviewing]
	Comment 12 • 7 years ago

Nevermind.  I see your dev-platform mail now.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 13 • 7 years ago

> Does this also make EventTarget extendable

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

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 14 • 7 years ago

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

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 15 • 7 years ago

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.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 16 • 7 years ago

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

Thanks!

	Pulsebot
	Comment 17 • 7 years ago

Pushed by bzbarsky@mozilla.com:
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

	Cosmin Sabou [:CosminS]
	Comment 18 • 7 years ago
bugherder

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

	Chris Mills [:cmills]
	Comment 19 • 6 years ago

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!

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 20 • 6 years ago

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.

	Chris Mills [:cmills]
	Comment 21 • 6 years ago

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