Last Comment Bug 731746 - Allow chrome JS object to implement EventTarget
: Allow chrome JS object to implement EventTarget
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Events (show other bugs)
: Other Branch
: All All
: -- normal (vote)
: mozilla23
Assigned To: Boris Zbarsky [:bz] (still a bit busy)
:
:
Mentors:
: 782509 (view as bug list)
Depends on: 827486 852219
Blocks: 743032 733573 781647 823512 842531 850430
  Show dependency treegraph
 
Reported: 2012-02-29 13:00 PST by [:fabrice] Fabrice Desré
Modified: 2013-05-14 12:18 PDT (History)
38 users (show)
bzbarsky: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
-


Attachments
add JSDOMEventTargetHelper, WIP v1 (18.95 KB, patch)
2012-08-22 04:15 PDT, Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)
no flags Details | Diff | Splinter Review
part 1. Add a constructor on nsDOMEventTargetHelper that takes a window and an unused JSObject*, for use by JS-implemented event targets. (1.66 KB, patch)
2013-04-24 11:14 PDT, Boris Zbarsky [:bz] (still a bit busy)
no flags Details | Diff | Splinter Review
part 2. Change JS-implemented webidl codegen to pass an nsPIDOMWindow, not an nsISupports, to the object constructor. (2.08 KB, patch)
2013-04-24 11:14 PDT, Boris Zbarsky [:bz] (still a bit busy)
continuation: review+
Details | Diff | Splinter Review
part 3. Change JS-implemented webidl codegen to always invoke the parent constructor if there is a parent interface. (2.35 KB, patch)
2013-04-24 11:15 PDT, Boris Zbarsky [:bz] (still a bit busy)
no flags Details | Diff | Splinter Review
part 4. When wrapping a JS-implemented webidl object, define the new object as a property on the implementing object. (1.81 KB, patch)
2013-04-24 11:15 PDT, Boris Zbarsky [:bz] (still a bit busy)
continuation: review+
Details | Diff | Splinter Review
Updated part 1 (1.64 KB, patch)
2013-04-24 14:12 PDT, Boris Zbarsky [:bz] (still a bit busy)
bugs: review+
Details | Diff | Splinter Review
Updated part 3 (2.36 KB, patch)
2013-04-24 14:13 PDT, Boris Zbarsky [:bz] (still a bit busy)
continuation: review+
Details | Diff | Splinter Review

Description [:fabrice] Fabrice Desré 2012-02-29 13:00:35 PST
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 [:bz] (still a bit busy) 2012-02-29 14:30:53 PST
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 2012-02-29 14:37:44 PST
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] (reviewing overload) 2012-03-01 12:47:49 PST
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 2012-03-01 16:23:11 PST
So we just need to figure out a way to expose the C++ implementation...
Comment 5 Philipp von Weitershausen [:philikon] 2012-05-29 12:40:10 PDT
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) 2012-05-29 13:01:21 PDT
I don't think so. This is about allowing DOM-implemented JS objects to fire events properly.
Comment 7 Philipp von Weitershausen [:philikon] 2012-05-29 16:43:39 PDT
(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] (reviewing overload) 2012-07-19 12:08:02 PDT
(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 2012-07-29 20:39:41 PDT
(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] (reviewing overload) 2012-07-30 01:54:51 PDT
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 2012-08-08 09:34:25 PDT
I don't think we are blocked by this. Please renom if you disagree.
Comment 12 Jason Smith [:jsmith] 2012-08-08 16:11:14 PDT
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] 2012-08-13 10:12:28 PDT
Nice to have but not a blocker.
Comment 14 StevenLee[:slee] 2012-08-13 22:54:37 PDT
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] (reviewing overload) 2012-08-13 23:03:31 PDT
(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] 2012-08-14 00:20:40 PDT
(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 17 Thinker Li [:sinker] PTO during Sep 22 ~ 30 2012-08-15 01:01:19 PDT
*** Bug 782509 has been marked as a duplicate of this bug. ***
Comment 18 Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) 2012-08-22 04:15:21 PDT
Created attachment 654168 [details] [diff] [review]
add JSDOMEventTargetHelper, WIP v1

introduce nsJSDOMEventTargetHelper that allows JS component to delegate nsIDOMEventTarget operations.
Comment 19 Justin Lebar (not reading bugmail) 2012-08-27 11:02:50 PDT
(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) 2012-08-27 11:15:41 PDT
(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] (reviewing overload) 2012-08-27 11:29:30 PDT
(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 2012-08-27 11:32:31 PDT
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) 2012-08-27 17:36:44 PDT
> 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 (⌚ UTC+1/+2) 2013-01-19 05:55:07 PST
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] (reviewing overload) 2013-01-19 10:15:28 PST
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 [:bz] (still a bit busy) 2013-01-22 12:50:59 PST
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) 2013-01-22 18:37:44 PST
I'm not working on this bug currently, feel free to take it. :)
Comment 28 Andrew McCreight [:mccr8] 2013-03-05 15:09:06 PST
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 [:bz] (still a bit busy) 2013-03-05 16:22:12 PST
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 [:bz] (still a bit busy) 2013-04-24 11:14:23 PDT
Created 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.
Comment 31 Boris Zbarsky [:bz] (still a bit busy) 2013-04-24 11:14:49 PDT
Created attachment 741441 [details] [diff] [review]
part 2.  Change JS-implemented webidl codegen to pass an nsPIDOMWindow, not an nsISupports, to the object constructor.
Comment 32 Boris Zbarsky [:bz] (still a bit busy) 2013-04-24 11:15:07 PDT
Created attachment 741442 [details] [diff] [review]
part 3.  Change JS-implemented webidl codegen to always invoke the parent constructor if there is a parent interface.
Comment 33 Boris Zbarsky [:bz] (still a bit busy) 2013-04-24 11:15:21 PDT
Created 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.
Comment 34 Boris Zbarsky [:bz] (still a bit busy) 2013-04-24 11:31:31 PDT
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] 2013-04-24 13:30:03 PDT
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 [:bz] (still a bit busy) 2013-04-24 13:45:04 PDT
> Should this be protected

Might not be a bad idea, yeah...
Comment 37 Olli Pettay [:smaug] (reviewing overload) 2013-04-24 14:02:35 PDT
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 [:bz] (still a bit busy) 2013-04-24 14:12:52 PDT
Created attachment 741513 [details] [diff] [review]
Updated part 1
Comment 39 Boris Zbarsky [:bz] (still a bit busy) 2013-04-24 14:13:15 PDT
Created attachment 741514 [details] [diff] [review]
Updated part 3
Comment 40 Andrew McCreight [:mccr8] 2013-04-24 15:39:22 PDT
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] 2013-04-24 15:46:06 PDT
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 [:bz] (still a bit busy) 2013-04-24 17:43:03 PDT
> 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 45 Jonas Sicking (:sicking) No longer reading bugmail consistently 2013-05-03 00:41:17 PDT
Is there any docs or code that I can point people at for how to use this stuff?
Comment 46 Boris Zbarsky [:bz] (still a bit busy) 2013-05-03 00:54:23 PDT
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] 2013-05-03 07:08:57 PDT
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 2013-05-12 02:18:33 PDT
(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] 2013-05-12 13:35:35 PDT
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 2013-05-12 19:44:16 PDT
As far as I can tell it doesn't actually correctly inherit the C++ EventTarget implementation though.
Comment 51 Boris Zbarsky [:bz] (still a bit busy) 2013-05-12 20:33:43 PDT
It does, in fact.  I'll update the documentation to make this clearer tomorrow.
Comment 52 Jonas Sicking (:sicking) No longer reading bugmail consistently 2013-05-12 20:49:01 PDT
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 [:bz] (still a bit busy) 2013-05-13 23:49:15 PDT
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 2013-05-14 12:18:39 PDT
bz++! Great stuff!

Note You need to log in before you can comment on or make changes to this bug.