Notify JS implemented Event Target when an event listener is added / removed

RESOLVED FIXED

Status

Firefox OS
NFC
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: psiddh, Assigned: smaug)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments, 2 obsolete attachments)

(Reporter)

Description

4 years ago
User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Ubuntu Chromium/30.0.1599.114 Chrome/30.0.1599.114 Safari/537.36

Steps to reproduce:

Whenever a Gaia application adds / removes event listeners , JS implementation of EventTargets should be notified
(Reporter)

Updated

4 years ago
Blocks: 933136
Assignee: nobody → bugs
Created attachment 8338881 [details] [diff] [review]
patch

Avoid adding more callback magic to codegen, but just hack this into
DETH and require webidl interface to implement 
[ChromeOnly] void eventListenerAdded(nsIAtom? aEventNameWithOnPrefix);
[ChromeOnly] void eventListenerRemoved(nsIAtom? aEventNameWithOnPrefix);

Passing nsIAtom isn't super nice for js impl, but since we normally
don't have this stuff called, I'd prefer to not convert the param to
string (nor remove on-prefix)
Attachment #8338881 - Flags: review?(bzbarsky)
Created attachment 8338882 [details] [diff] [review]
example

dump "on" + eventname to terminal when listener is added to PeerConnection
> Passing nsIAtom isn't super nice for js impl

How about we just special-case it in BindingUtils.h like we do nsIVariant and convert it to a string JS value at that point?
Comment on attachment 8338881 [details] [diff] [review]
patch

OK, I think I've convinced myself that you shouldn't end up with an exception that needs cleanup on rv in these cases (because your compartment is null and because the callback codegen won't throw strings that need deallocating).

So r=me if we either make nsIAtom convert to string or if you just make this take a DOMString and use nsDependentAtomString, which should be pretty cheap.
Attachment #8338881 - Flags: review?(bzbarsky) → review+
Ok, I'll make webidl methods take DOMString and then the param should be
without on prefix.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Created attachment 8338963 [details] [diff] [review]
patch, using strings
Attachment #8338881 - Attachment is obsolete: true
Created attachment 8338966 [details] [diff] [review]
example v2
Attachment #8338882 - Attachment is obsolete: true
https://hg.mozilla.org/integration/mozilla-inbound/rev/db836ecd7746

Comment 9

4 years ago
Backed out because of build bustage:

https://hg.mozilla.org/integration/mozilla-inbound/rev/b66f95073899
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=db836ecd7746
Created attachment 8339170 [details] [diff] [review]
v3

Different method name
Created attachment 8339173 [details] [diff] [review]
example v3
Bah, looks like possible exceptions need to be cleared.
ErrorResult is so unfriendly for C++ callers :/
> ErrorResult is so unfriendly for C++ callers :/

Bug 933378.

But in this case, see comment 4?
Created attachment 8339238 [details] [diff] [review]
v3b

Not pretty, but since we currently have the rather strong assertions in
ErrorResult, so have to call WouldReportJSException()
Attachment #8339238 - Flags: review?(bzbarsky)
Comment on attachment 8339238 [details] [diff] [review]
v3b

Hmm.  Maybe the right thing is to change http://hg.mozilla.org/mozilla-central/file/34f431863037/dom/bindings/CallbackObject.cpp#l169 to be:

   if ((mCompartment && mExceptionHandling == eRethrowContentExceptions) ||
       mExceptionHandling == eRethrowExceptions) {

because ShouldRethrowException always returns false if !mCompartment.

r=me either way, but seems like that would be nicer API-wise.
Attachment #8339238 - Flags: review?(bzbarsky) → review+
Aha, that is nicer. Wasn't too obvious.
Created attachment 8339319 [details] [diff] [review]
compartment check

https://hg.mozilla.org/integration/mozilla-inbound/rev/48e320f58f71
No, that's wrong.  eRethrowExceptions rethrows unconditionally, no matter what mCompartment is.  So the mCompartment check should be attached only to the eRethrowContentExceptions case, as in comment 15.
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d7d09bda021
Blocks: 943939
https://hg.mozilla.org/mozilla-central/rev/48e320f58f71
https://hg.mozilla.org/mozilla-central/rev/2d7d09bda021
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.