Closed Bug 1379688 Opened 3 years ago Closed 2 years ago

Implement EventTarget constructor

Categories

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

55 Branch
enhancement

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: d, Assigned: bzbarsky)

References

Details

(Keywords: dev-doc-complete)

Attachments

(2 files)

Priority: -- → P3
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.
EventTarget wouldn't be used to implement the concrete object, but DOMEventTargetHelper.
WrapObject is still virtual, what should I do about that?
Flags: needinfo?(bugs)
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
Flags: needinfo?(bugs)
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.
Flags: needinfo?(bugs)
 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

:(
Flags: needinfo?(bugs)
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...
Depends on: 888600
Flags: needinfo?(bugs) → needinfo?(bzbarsky)
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
Attachment #8929803 - Flags: review?(bugs)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
MozReview-Commit-ID: 4xrSSqXna7F
Attachment #8929804 - Flags: review?(bugs)
Flags: needinfo?(bzbarsky)
Does this also make EventTarget extendable like:

class Whatever extends EventTarget {
}
Nevermind.  I see your dev-platform mail now.
> Does this also make EventTarget extendable

Just for the record, yes.  And there are are web platform tests that test that.
And also for the record, anything that has a constructor in our webidl automatically becomes extendable.  ;)
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.
Attachment #8929803 - Flags: review?(bugs) → review+
Comment on attachment 8929804 [details] [diff] [review]
part 2.  Make the EventTarget interface constructible

Thanks!
Attachment #8929804 - Flags: review?(bugs) → review+
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
https://hg.mozilla.org/mozilla-central/rev/55753b766fd5
https://hg.mozilla.org/mozilla-central/rev/312db92828f8
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
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!
Flags: needinfo?(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.
Flags: needinfo?(bzbarsky)
(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().
You need to log in before you can comment on or make changes to this bug.