Closed Bug 1453345 Opened 2 years ago Closed 2 years ago

Make nsIDOMEventListener builtinclass

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

Attachments

(6 files, 1 obsolete file)

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...
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?
Flags: needinfo?(nchen)
Nope, we should use wrappedJSObject.
Flags: needinfo?(nchen)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
MozReview-Commit-ID: I5oYAYaA6CV
Attachment #8967068 - Flags: review?(bugs)
MozReview-Commit-ID: Db0v6GZ0deo
Attachment #8967069 - Flags: review?(bugs)
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
Attachment #8967070 - Flags: review?(bugs)
Attachment #8967067 - Flags: review?(nchen) → review+
Comment on attachment 8967070 [details] [diff] [review]
part 6.  Mark nsIDOMEventListener builtinclass

because
Attachment #8967070 - Flags: review?(bugs) → review+
Attachment #8967069 - Flags: review?(bugs) → review+
Attachment #8967066 - Flags: review?(bugs) → review+
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
Attachment #8967065 - Flags: review?(bugs) → review+
> Indentation in CreateDynamicFrameEventFilter arguments

Silly tabs.  Fixed.
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.
> 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.
Attachment #8967068 - Attachment is obsolete: true
Attachment #8967068 - Flags: review?(bugs)
Attachment #8967100 - Flags: review?(bugs) → review+
Blocks: 1453487
Pushed by bzbarsky@mozilla.com:
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 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'.
Priority: -- → P3
> 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>
You need to log in before you can comment on or make changes to this bug.