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

Status: RESOLVED FIXED

(Firefox OS Graveyard :: NFC, defect)

Description

[psiddh]

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

Comment 1

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

Attachment: 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)

Comment 2

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

Attachment: example

dump "on" + eventname to terminal when listener is added to PeerConnection

Comment 3

[Boris Zbarsky [:bzbarsky]]

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

[Boris Zbarsky [:bzbarsky]]

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.

Comment 5

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

Ok, I'll make webidl methods take DOMString and then the param should be
without on prefix.

Comment 6

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

Attachment: patch, using strings

Comment 7

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

Attachment: example v2

Comment 8

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/db836ecd7746

Comment 9

[(no longer active)]

Backed out because of build bustage:

https://hg.mozilla.org/integration/mozilla-inbound/rev/b66f95073899
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=db836ecd7746

Comment 10

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

Attachment: v3

Different method name

Comment 11

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

Attachment: example v3

Comment 12

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

Bah, looks like possible exceptions need to be cleared.
ErrorResult is so unfriendly for C++ callers :/

Comment 13

[Boris Zbarsky [:bzbarsky]]

> ErrorResult is so unfriendly for C++ callers :/

Bug 933378.

But in this case, see comment 4?

Comment 14

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

Attachment: v3b

Not pretty, but since we currently have the rather strong assertions in
ErrorResult, so have to call WouldReportJSException()

Comment 15

[Boris Zbarsky [: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.

Comment 16

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

Aha, that is nicer. Wasn't too obvious.

Comment 17

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

Attachment: compartment check

https://hg.mozilla.org/integration/mozilla-inbound/rev/48e320f58f71

Comment 18

[Boris Zbarsky [:bzbarsky]]

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.

Comment 19

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/2d7d09bda021

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/48e320f58f71
https://hg.mozilla.org/mozilla-central/rev/2d7d09bda021