731746 - Allow chrome JS object to implement EventTarget

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[[:fabrice] Fabrice Desré]

Several DOM APIs are being implemented in JS and need to expose objects implementing EventTarget. This means that we have either to 
- write a robust implementation in JS
- get a way to expose the c++ implementatino to chrome JS

Jonas was in favor of the second solution iirc.

Comment 1

[Boris Zbarsky [:bzbarsky]]

The first solution is not compatible with the way the new DOM bindings will work, so I'm 100% certain you want option 2...

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

In order to allow instantiating Event objects manually and dispatching them at EventTarget we'll need a C++ implementation yeah. Likewise to get the prototype chain correct so that someone can do EventTarget.prototype.myfunction = ...

For everything else I think a pure JS implementation would work. But we couldn't get it to be perfect no.

Comment 3

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

I pretty strongly against option 1. We should have fewer event target implementations, not
more. (and implementing EventTarget in JS would most probably affect to performance also
elsewhere.)

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

So we just need to figure out a way to expose the C++ implementation...

Comment 5

[Philipp von Weitershausen [:philikon]]

Will this work also include allowing chrome JS components to implement their own event objects? E.g. CallEvent for telephony, etc. If not, I'll file a separate but for that.

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

I don't think so. This is about allowing DOM-implemented JS objects to fire events properly.

Comment 7

[Philipp von Weitershausen [:philikon]]

(In reply to Blake Kaplan (:mrbkap) from comment #6)
> I don't think so. This is about allowing DOM-implemented JS objects to fire
> events properly.

Fair. But we should at least be able to create *trusted* vanilla events (via document createEvent), though ideally being able to define event subtypes would also be good. Filed bug 759579.

Comment 8

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

(In reply to Philipp von Weitershausen [:philikon] from comment #5)
> Will this work also include allowing chrome JS components to implement their
> own event objects? E.g. CallEvent for telephony, etc. If not, I'll file a
> separate but for that.
Once Bug 765163 is fixed, implementing events in JS is hopefully never needed.
Most of the new events are so trivial that the code generator can handle them just fine.

Comment 9

[David Dahl :ddahl]

(In reply to Olli Pettay [:smaug] (limited connectivity July 26-29) from comment #8)
> (In reply to Philipp von Weitershausen [:philikon] from comment #5)
> > Will this work also include allowing chrome JS components to implement their
> > own event objects? E.g. CallEvent for telephony, etc. If not, I'll file a
> > separate but for that.
> Once Bug 765163 is fixed, implementing events in JS is hopefully never
> needed.
> Most of the new events are so trivial that the code generator can handle
> them just fine.

I noticed in bug 765163 that jsval args are not handled by the event code generator, see https://bugzilla.mozilla.org/show_bug.cgi?id=765163#c1 - will this bug allow JS privileged nsIDOMEventTargets to handle jsval args?

Comment 10

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

This bug is about *EventTarget* objects, bug 765163 was about *Event* objects.
If you need jsval values in Event objects (which you almost never actually should need), file a new bug.

Comment 11

[Andreas Gal :gal]

I don't think we are blocked by this. Please renom if you disagree.

Comment 12

[Jason Smith [:jsmith]]

Per IRC conversations with a few other folks, I think the best course of action if there is disagreement on whether this blocks or not is to do the following:

- Move the blocking-basecamp flag to ? for re-evaluation
- Indicate a rationale for why you disagree

Comment 13

[Andrew Overholt [:overholt]]

Nice to have but not a blocker.

Comment 14

[StevenLee[:slee]]

Hi all,

Thinker proposed a method by implementing EventTarget in JS, see 
https://bugzilla.mozilla.org/show_bug.cgi?id=779500#c26. How do you
think? 

Hi gwagner, do you start working on this bug? If not, can I re-assigned this bug? We need this patch in Bug 779500. Thanks.

Comment 15

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

(In reply to StevenLee from comment #14)
> Hi all,
> 
> Thinker proposed a method by implementing EventTarget in JS, see 
> https://bugzilla.mozilla.org/show_bug.cgi?id=779500#c26. How do you
> think? 
I think that approach could work as a temporary solution.
However since it doesn't put EventTarget to the right place in inheritance chain,
with WebIDL bindings prototype chain won't be correct, AFAIK.

Comment 16

[Gregor Wagner [:gwagner]]

(In reply to StevenLee from comment #14)
> 
> Hi gwagner, do you start working on this bug? If not, can I re-assigned this
> bug? We need this patch in Bug 779500. Thanks.

Go ahead. I am currently not working on it.

Comment 18

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: add JSDOMEventTargetHelper, WIP v1

introduce nsJSDOMEventTargetHelper that allows JS component to delegate nsIDOMEventTarget operations.

Comment 19

[Justin Lebar (not reading bugmail)]

(In reply to Andreas Gal :gal from comment #11)
> I don't think we are blocked by this. Please renom if you disagree.

This bug is intended to be used in the WebFM API.  That API will be exposed to non-certified content (including web content), so it should be designed to meet a high level of correctness.

If we don't complete this bug, then we'll have to do what's suggested in bug 779500 comment 24 in order to have an addEventListener() function on the WebFM interface.  smaug doesn't approve of that approach.

Alternatively, we could implement WebFM in C++.  That would require rewriting a fair bit of code, but the resultant API would be correct.

I don't have an opinion on which approach is best; I think we should take the path of least resistance.  It's not clear to me what that path is.

Comment 20

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Justin Lebar [:jlebar] from comment #19)
> Alternatively, we could implement WebFM in C++.  That would require
> rewriting a fair bit of code, but the resultant API would be correct.

Could we make the C++ layer a shim around the existing JS implementation?

Comment 21

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

(There is a plan to fix this properly but I guess we're talking about temporary solutions here).

We could wrap JS inside some C++ stuff, but one would need to expose right kind of classinfo and
JS part would have to be aware of the C++ object so that it could dispatch events to the right
object.

Comment 22

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Justin: While I agree that APIs exposed to content should have a high level of quality, I think this is a shortcoming we can live with in an initial release. I.e. I wouldn't hold release for this.

Comment 23

[Justin Lebar (not reading bugmail)]

> Could we make the C++ layer a shim around the existing JS implementation?

I guess so?  And we even generate macros for forwarding from one implementation of an interface to another, so perhaps it would be simple...

Comment 24

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

We should just use the forwarding-to-JS stuff Andrew is going to implement this quarter, if this is still necessary.

Comment 25

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

Well, DOMEventTarget itself must be implemented in C++, but hopefully other parts of some
interface can be implemented using forward-to-JS.

Comment 26

[Boris Zbarsky [:bzbarsky]]

Indeed.  We'll need a small C++ EventTarget class no matter what, but we can make the forward-to-js code create a binding that inherits from that...

Comment 27

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

I'm not working on this bug currently, feel free to take it. :)

Comment 28

[Andrew McCreight [:mccr8]]

I can start looking at this after I finish bug 827486.  bz thinks it won't be too much work beyond that.

Comment 29

[Boris Zbarsky [:bzbarsky]]

The one thing I'm not sure about yet is how to expose the EventTarget to the chrome code.  Would it make sense to stick a property like __DOMObject__ on the chrome side of JS-implemented WebIDL objects that points to the content-side object?  Or can we make a nicer API somehow?

Comment 30

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Add a constructor on nsDOMEventTargetHelper that takes a window and an unused JSObject*, for use by JS-implemented event targets.

Comment 31

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Change JS-implemented webidl codegen to pass an nsPIDOMWindow, not an nsISupports, to the object constructor.

Comment 32

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Change JS-implemented webidl codegen to always invoke the parent constructor if there is a parent interface.

Comment 33

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. When wrapping a JS-implemented webidl object, define the new object as a property on the implementing object.

Comment 34

[Boris Zbarsky [:bzbarsky]]

So with those patches, if I give my JS-implemented object (which implements a MyNumber interface; I stole it from attachment 720058 [details] [diff] [review]) a method like this:

    sendEvent: function(win) {
        var e = new win.Event("foopy");
        this.__DOM_IMPL__.dispatchEvent(e);
    },

and have content script like this:

  var obj = new MyNumber();
  obj.addEventListener("foopy", function(e) { alert(e); });
  obj.sendEvent(window);

then everything works peachy.

I filed 865377 to make it so you don't have to pass in a window in cases like this, ever; with that fixed, something like:

    sendEvent: function() {
        var e = new this.__DOM_IMPL__.global.Event("foopy");
        this.__DOM_IMPL__.dispatchEvent(e);
    },

would work.

Oh, and all this only works for WebIDL objects, but I think that's fine.  ;)

Comment 35

[Andrew McCreight [:mccr8]]

Comment on attachment 741440 [details] [diff] [review]
part 1.  Add a constructor on nsDOMEventTargetHelper that takes a window and an unused JSObject*, for use by JS-implemented event targets.

Review of attachment 741440 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/events/src/nsDOMEventTargetHelper.h
@@ +29,5 @@
> +    : mParentObject(nullptr)
> +    , mOwnerWindow(nullptr)
> +    , mHasOrHasHadOwnerWindow(false)
> +  {}
> +  nsDOMEventTargetHelper(JSObject* /* unused */, nsPIDOMWindow* aWindow)

Should this be protected so it is only used by WebIDL things that inherit from it?

Comment 36

[Boris Zbarsky [:bzbarsky]]

> Should this be protected

Might not be a bad idea, yeah...

Comment 37

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

Comment on attachment 741440 [details] [diff] [review]
part 1.  Add a constructor on nsDOMEventTargetHelper that takes a window and an unused JSObject*, for use by JS-implemented event targets.

New patch coming

Comment 38

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated part 1

Comment 39

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated part 3

Comment 40

[Andrew McCreight [:mccr8]]

Comment on attachment 741514 [details] [diff] [review]
Updated part 3

Review of attachment 741514 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/bindings/Codegen.py
@@ +8447,5 @@
>          parentInterface = descriptor.interface.parent
>          while parentInterface:
>              if parentInterface.isJSImplemented():
> +                baseConstructors.insert(
> +                    0, "%s(aJSImplObject, aParent)" % parentClass )

You are just doing an insert instead of a list append here, right?

Comment 41

[Andrew McCreight [:mccr8]]

Comment on attachment 741443 [details] [diff] [review]
part 4.  When wrapping a JS-implemented webidl object, define the new object as a property on the implementing object.

Review of attachment 741443 [details] [diff] [review]:
-----------------------------------------------------------------

Neat.

Comment 42

[Boris Zbarsky [:bzbarsky]]

> You are just doing an insert instead of a list append here, right?

Yes.  The constructor for the base class has to come before the property initializers.

Comment 43

[Boris Zbarsky [:bzbarsky]]

   https://hg.mozilla.org/integration/mozilla-inbound/rev/796d4d1042ba
   https://hg.mozilla.org/integration/mozilla-inbound/rev/69ca7cb14709
   https://hg.mozilla.org/integration/mozilla-inbound/rev/b84fe2eb6138
   https://hg.mozilla.org/integration/mozilla-inbound/rev/3255b8c57e83

Comment 44

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/796d4d1042ba
https://hg.mozilla.org/mozilla-central/rev/69ca7cb14709
https://hg.mozilla.org/mozilla-central/rev/b84fe2eb6138
https://hg.mozilla.org/mozilla-central/rev/3255b8c57e83

Comment 45

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Is there any docs or code that I can point people at for how to use this stuff?

Comment 46

[Boris Zbarsky [:bzbarsky]]

https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#Implementing_WebIDL_using_Javascript is the closest we have to docs so far.  Code... is not in the tree yet, but should be soon.

Comment 47

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

I have a larger patch I hope to upload to Bug 823512 today that uses it, which will hopefully land by FF23. I used Comment 34 here as a guide. For dispatching simple events it is all you need. If you're making your own event subclasses you need that patch first.

Comment 48

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to Boris Zbarsky (:bz) from comment #46)
> https://developer.mozilla.org/en-US/docs/Mozilla/
> WebIDL_bindings#Implementing_WebIDL_using_Javascript is the closest we have
> to docs so far.  Code... is not in the tree yet, but should be soon.

This doesn't mention anything about how you inherit/extend the C++ base classes that implement EventTarget. Wasn't that the point of this bug?

Comment 49

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

The "Part 1" attachment 748497 [details] [diff] [review] in bug 823512 has several webidl subclasses of Event, and mozRTCPeerConnection itself is derived off of EventTarget. Fwiw.

Comment 50

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

As far as I can tell it doesn't actually correctly inherit the C++ EventTarget implementation though.

Comment 51

[Boris Zbarsky [:bzbarsky]]

It does, in fact.  I'll update the documentation to make this clearer tomorrow.

Comment 52

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I don't see how it forwards the .onfoo setter to .addEventListener the same way that IMPL_EVENT_HANDLER does. But if it does, please include that in the description.

Comment 53

[Boris Zbarsky [:bzbarsky]]

Jonas, take a look at https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#Inheriting_from_interfaces_implemented_in_C.2B.2B and let me know what you think?

For what it's worth, I considered automating onfoo completely based on some heuristics for detecting an event handler, but decided the explicit opt-in (with similar overhead to IMPL_EVENT_HANDLER) was probably better through lack of magic.

Comment 54

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

bz++! Great stuff!