Closed
Bug 1453403
Opened 7 years ago
Closed 7 years ago
Use of Ci.nsIDOMEventListener should probably go away
Categories
(Thunderbird :: General, enhancement)
Thunderbird
General
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 61.0
People
(Reporter: bzbarsky, Assigned: jorgk-bmo)
Details
Attachments
(1 file)
4.76 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
There's no reason for JS-implemented things to implement QI to nsIDOMEventListener. See bug 1453345.
And it might become impossible soon anyway if I can get away with making it noscript. At that point you will start getting exceptions from uses like this.
Assignee | ||
Comment 1•7 years ago
|
||
Thanks for the heads-up. Help you please help us out here. Which usage of nsIDOMEventListener are you referring to?
https://dxr.mozilla.org/comm-central/search?q=nsIDOMEventListener&redirect=false
You said QI to nsIDOMEventListener. That would be this one:
https://dxr.mozilla.org/comm-central/rev/3494241a0ca63ee3b224b633483e1d9a7cf954a5/mail/components/preferences/applications.js#916
Code in im/ is considered dead since Instantbird is no longer maintained. SeaMonkey/suite has a few more QI's:
https://dxr.mozilla.org/comm-central/search?q=aIID.equals(Ci.nsIDOMEventListener&redirect=false
![]() |
Reporter | |
Comment 2•7 years ago
|
||
> Which usage of nsIDOMEventListener are you referring to?
Any use in https://searchfox.org/comm-central/search?q=nsIDOMEventListener&case=true&path=.js or https://searchfox.org/comm-central/search?q=nsIDOMEventListener&case=true&path=.xml (or .xul or .xml but there aren't any).
> That would be this one:
Yes, and various others like it. Basically, Ci.nsIDOMEventListener will likely become undefined.
Assignee | ||
Comment 3•7 years ago
|
||
Hmm, so the second query
https://searchfox.org/comm-central/search?q=nsIDOMEventListener&case=true&path=.xml
brings up
<implementation implements="nsIDOMEventListener">
Sorry, I have no idea what that does. What do we do with that?
![]() |
Reporter | |
Comment 4•7 years ago
|
||
You remove it. It's a no-op already.
Assignee | ||
Comment 5•7 years ago
|
||
![]() |
Reporter | |
Comment 6•7 years ago
|
||
More importantly, see bug 1453487. With that landed, Ci.nsIDOMEventListener is gone.
Assignee | ||
Comment 7•7 years ago
|
||
Oops, that's what I meant, the changeset I quoted is from bug 1453487, I pasted the wrong bug from comment #0.
We have build with bug 1453487 merged to M-C and see no failures. Anyway, I'll do a patch now.
Assignee | ||
Comment 8•7 years ago
|
||
This should do it. Note: code in im/ is dead and I'm not doing suite/ here either, hence NI for FRG.
Boris, what about:
https://dxr.mozilla.org/comm-central/rev/cc738342cb1c3f4f91009fd38f762a9249d7eb39/mail/components/preferences/applications.js#947
That needs to stay, no? handleEvent() is the default name for the listener callback, right?
Assignee: nobody → jorgk
Status: NEW → ASSIGNED
Flags: needinfo?(frgrahl)
Attachment #8967488 -
Flags: review?(bzbarsky)
![]() |
Reporter | |
Comment 9•7 years ago
|
||
> That needs to stay, no?
The function, yes. The comment, I'd change to "EventListener" instead of "nsIDOMEventListener".
![]() |
Reporter | |
Comment 10•7 years ago
|
||
Comment on attachment 8967488 [details] [diff] [review]
1453403-nsIDOMEventListener.patch
There are a bumch of uses in suite/ as well.
Attachment #8967488 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 11•7 years ago
|
||
Thanks, Boris, I'll fix the comment before landing.
Repeating comment #8:
Code in im/ is dead and I'm not doing suite/ here either, hence NI for FRG.
Comment 12•7 years ago
|
||
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/80fab3583c0e
Port bug 1453487: Remove needless implements='nsIDOMEventListener' and QI. r=bz
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•7 years ago
|
Target Milestone: --- → Thunderbird 61.0
Comment 13•7 years ago
|
||
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/866ed0fb6e90
Follow-up: remove spurious closing parenthesis. rs=bustage-fix
![]() |
||
Comment 14•7 years ago
|
||
Thanks Jorg. Put it in our master bug for later porting.
Flags: needinfo?(frgrahl)
You need to log in
before you can comment on or make changes to this bug.
Description
•