Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Allow chrome JS object to implement EventTarget

RESOLVED FIXED in mozilla23

Status

()

Core
DOM: Events
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: fabrice, Assigned: bz)

Tracking

(Blocks: 1 bug)

Other Branch
mozilla23
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(blocking-kilimanjaro:+, blocking-basecamp:-)

Details

Attachments

(4 attachments, 3 obsolete attachments)

(Reporter)

Description

6 years ago
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.
(Assignee)

Comment 1

6 years ago
The first solution is not compatible with the way the new DOM bindings will work, so I'm 100% certain you want option 2...
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

6 years ago
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.)
So we just need to figure out a way to expose the C++ implementation...
(Reporter)

Updated

5 years ago
Blocks: 743032
(Reporter)

Updated

5 years ago
Blocks: 733573
Assignee: nobody → anygregor
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.
I don't think so. This is about allowing DOM-implemented JS objects to fire events properly.
(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.

Updated

5 years ago
blocking-basecamp: --- → +

Comment 8

5 years ago
(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

5 years ago
(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?
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

5 years ago
I don't think we are blocked by this. Please renom if you disagree.
blocking-basecamp: + → ---
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
blocking-basecamp: --- → ?
Blocks: 781647
Nice to have but not a blocker.
blocking-basecamp: ? → -

Comment 14

5 years ago
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.
(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.
(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.
Assignee: anygregor → schien

Updated

5 years ago
Blocks: 779500

Updated

5 years ago
Duplicate of this bug: 782509
Created attachment 654168 [details] [diff] [review]
add JSDOMEventTargetHelper, WIP v1

introduce nsJSDOMEventTargetHelper that allows JS component to delegate nsIDOMEventTarget operations.
Attachment #654168 - Flags: feedback?(fabrice)
(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.
blocking-basecamp: - → ?
(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?
(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.
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.
blocking-basecamp: ? → -
blocking-kilimanjaro: --- → +
> 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...
No longer blocks: 779500
We should just use the forwarding-to-JS stuff Andrew is going to implement this quarter, if this is still necessary.
Well, DOMEventTarget itself must be implemented in C++, but hopefully other parts of some
interface can be implemented using forward-to-JS.
(Assignee)

Comment 26

5 years ago
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...
I'm not working on this bug currently, feel free to take it. :)
Assignee: schien → nobody
Depends on: 827486

Updated

5 years ago
Blocks: 842531
I can start looking at this after I finish bug 827486.  bz thinks it won't be too much work beyond that.
(Assignee)

Comment 29

4 years ago
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?
(Assignee)

Updated

4 years ago
Depends on: 852219
Blocks: 823512
Summary: Allow chrome JS object to implement nsIDOMEventTarget → Allow chrome JS object to implement EventTarget
Blocks: 850430
(Assignee)

Comment 30

4 years ago
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.
Attachment #741440 - Flags: review?(bugs)
(Assignee)

Updated

4 years ago
Assignee: nobody → bzbarsky
(Assignee)

Comment 31

4 years ago
Created attachment 741441 [details] [diff] [review]
part 2.  Change JS-implemented webidl codegen to pass an nsPIDOMWindow, not an nsISupports, to the object constructor.
Attachment #741441 - Flags: review?(continuation)
(Assignee)

Comment 32

4 years ago
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.
Attachment #741442 - Flags: review?(continuation)
(Assignee)

Comment 33

4 years ago
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.
Attachment #741443 - Flags: review?(continuation)
(Assignee)

Comment 34

4 years ago
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.  ;)
Whiteboard: [need review]
Attachment #654168 - Attachment is obsolete: true
Attachment #654168 - Flags: feedback?(fabrice)
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?
Attachment #741441 - Flags: review?(continuation) → review+
(Assignee)

Comment 36

4 years ago
> Should this be protected

Might not be a bad idea, yeah...
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
Attachment #741440 - Flags: review?(bugs)
(Assignee)

Comment 38

4 years ago
Created attachment 741513 [details] [diff] [review]
Updated part 1
Attachment #741513 - Flags: review?(bugs)
(Assignee)

Updated

4 years ago
Attachment #741440 - Attachment is obsolete: true
(Assignee)

Comment 39

4 years ago
Created attachment 741514 [details] [diff] [review]
Updated part 3
Attachment #741514 - Flags: review?(continuation)
(Assignee)

Updated

4 years ago
Attachment #741442 - Attachment is obsolete: true
Attachment #741442 - Flags: review?(continuation)

Updated

4 years ago
Attachment #741513 - Flags: review?(bugs) → review+
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?
Attachment #741514 - Flags: review?(continuation) → review+
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.
Attachment #741443 - Flags: review?(continuation) → review+
(Assignee)

Comment 42

4 years ago
> 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.
(Assignee)

Comment 43

4 years ago
   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
Flags: in-testsuite?
Whiteboard: [need review]
Target Milestone: --- → mozilla23
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
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Is there any docs or code that I can point people at for how to use this stuff?
(Assignee)

Comment 46

4 years ago
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.
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.
(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?
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.
As far as I can tell it doesn't actually correctly inherit the C++ EventTarget implementation though.
(Assignee)

Comment 51

4 years ago
It does, in fact.  I'll update the documentation to make this clearer tomorrow.
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.
(Assignee)

Comment 53

4 years ago
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.
bz++! Great stuff!
You need to log in before you can comment on or make changes to this bug.