1453345 - Make nsIDOMEventListener builtinclass

Status: RESOLVED FIXED

(Core :: DOM: Events, enhancement, P3)

Description

[Boris Zbarsky [:bzbarsky]]

There's only one place left where we actually use XPCWrappedJS for nsIDOMEventListener.  We should switch it to using a dom::EventListener instead.

There's also only one place where we use XPCWrappedNative for nsIDOMEventListener: the return value of nsISessionStoreUtils::CreateDynamicFrameEventFilter.  I'm not quite sure yet whether we can get rid of that and hence make nsIDOMEventListener noscript...

Comment 1

[Boris Zbarsky [:bzbarsky]]

Actually, there are two places.  The one I knew about (CreateDynamicFrameEventFilter) and the one in geckoview's  GeckoViewPrompt.js which is used like so:

 Cc["@mozilla.org/prompter;1"].getService(Ci.nsIDOMEventListener)

Is there a reason this was done like that?  We would really like to move away from having two different kinds of scripted event listeners, and and this is blocking that.  Given that PromptFactory does the wrappedJSObject thing anyway, is there any reason we can't use:

  Cc["@mozilla.org/prompter;1"].getService(Ci.nsISupports).wrappedJSObject

here and bypass several layers of xpconnect slowdown to boot?

Comment 2

[(inactive) Jim Chen [:jchen] [:darchons]]

Nope, we should use wrappedJSObject.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Switch session storage content scripts to not rely on XPCWrappedJS implementing nsIDOMEventListener

MozReview-Commit-ID: LPYKxngcXJw

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Remove the eWrappedJSListener listener type, now that we shouldn't have them anymore

MozReview-Commit-ID: LycJUIJm5p9

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Switch geckoview's prompt setup to not rely on XPCWrappedJS implementing nsIDOMEventListener

MozReview-Commit-ID: 3CKiDtpQePU

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Stop using XPCWrappedJS implementing nsIDOMEventListener in EventListenerInfo

MozReview-Commit-ID: I5oYAYaA6CV

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: part 5. Remove pointless JS implemenations of QI to nsIDOMEventListener

MozReview-Commit-ID: Db0v6GZ0deo

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: part 6. Mark nsIDOMEventListener builtinclass

We can't make it non-scriptable, becuse we have _one_ case in which we actually
expose a C++ event listener to script: the return value of
nsISessionStoreUtils.createDynamicFrameEventFilter.

MozReview-Commit-ID: KTv2WsvGN52

Comment 9

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

Comment on attachment 8967070 [details] [diff] [review]
part 6.  Mark nsIDOMEventListener builtinclass

because

Comment 10

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

Comment on attachment 8967065 [details] [diff] [review]
part 1.  Switch session storage content scripts to not rely on XPCWrappedJS implementing nsIDOMEventListener

Indentation in CreateDynamicFrameEventFilter arguments

Comment 11

[Boris Zbarsky [:bzbarsky]]

> Indentation in CreateDynamicFrameEventFilter arguments

Silly tabs.  Fixed.

Comment 12

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

Comment on attachment 8967068 [details] [diff] [review]
part 4.  Stop using XPCWrappedJS implementing nsIDOMEventListener in EventListenerInfo

I'm having trouble to see why this doesn't affect to reporting native listeners.

Comment 13

[Boris Zbarsky [:bzbarsky]]

> I'm having trouble to see why this doesn't affect to reporting native listeners.

It does!  Try caught this too.  Posting a new patch here in a few mins once I verify it works right.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Stop using XPCWrappedJS implementing nsIDOMEventListener in EventListenerInfo

MozReview-Commit-ID: I5oYAYaA6CV

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/81d403264f0c
part 1.  Switch session storage content scripts to not rely on XPCWrappedJS implementing nsIDOMEventListener.  r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7d33dbd5c48
part 2.  Switch geckoview's prompt setup to not rely on XPCWrappedJS implementing nsIDOMEventListener.  r=jchen
https://hg.mozilla.org/integration/mozilla-inbound/rev/57eb117b9246
part 3.  Remove the eWrappedJSListener listener type, now that we shouldn't have them anymore.  r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/a5f419dfebd5
part 4.  Stop using XPCWrappedJS implementing nsIDOMEventListener in EventListenerInfo.  r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb2cb334ba39
part 5.  Remove pointless JS implemenations of QI to nsIDOMEventListener.  r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c2716f85b46
part 6.  Mark nsIDOMEventListener builtinclass.  r=smaug

Comment 16

[Peter Van der Beken [:peterv]]

Comment on attachment 8967069 [details] [diff] [review]
part 5.  Remove pointless JS implemenations of QI to nsIDOMEventListener

Review of attachment 8967069 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/places/content/bookmarkProperties.js
@@ +377,3 @@
>        return this;
>  
>      throw Cr.NS_NOINTERFACE;

BTW, you don't need a QI implementation for just nsISupports, nsXPCWrappedJS will return the right thing itself.

::: browser/components/preferences/in-content/main.js
@@ +1294,5 @@
>  
>  
>    // nsISupports
>  
> +  QueryInterface: XPCOMUtils.generateQI([Ci.nsIObserver]),

Same for just nsIObserver, because it's marked 'function'.

Comment 17

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/81d403264f0c
https://hg.mozilla.org/mozilla-central/rev/b7d33dbd5c48
https://hg.mozilla.org/mozilla-central/rev/57eb117b9246
https://hg.mozilla.org/mozilla-central/rev/a5f419dfebd5
https://hg.mozilla.org/mozilla-central/rev/eb2cb334ba39
https://hg.mozilla.org/mozilla-central/rev/1c2716f85b46

Comment 18

[Boris Zbarsky [:bzbarsky]]

> BTW, you don't need a QI implementation for just nsISupports

Ah, I was wondering about that...

> Same for just nsIObserver, because it's marked 'function'.

That affects how non-function objects implementing the interface behave?  <sigh>