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



Firefox OS
4 years ago
4 years ago


(Reporter: psiddh, Assigned: smaug)


Firefox Tracking Flags

(Not tracked)



(6 attachments, 2 obsolete attachments)



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


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

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]

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]

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.
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

Comment 9

4 years ago
Backed out because of build bustage:
Created attachment 8339170 [details] [diff] [review]

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]

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]

Hmm.  Maybe the right thing is to change 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
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.
Blocks: 943939
Last Resolved: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.