Remove uses of Promise.jsm from Thunderbird

RESOLVED FIXED in Thunderbird 57.0

Status

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jorgk, Assigned: Paenglab)

Tracking

(Depends on: 1 bug)

52 Branch
Thunderbird 57.0

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

2 years ago
According to bug 1378173 comment #9 we can just remove Promise.jsm imports, so all of these in C-C:
https://dxr.mozilla.org/comm-central/search?q=regexp%3Aimport.*promise.jsm&redirect=false

Let's to this before M-C remove Promise.jsm

We should do a try run after that.
(Assignee)

Comment 1

2 years ago
Created attachment 8896242 [details] [diff] [review]
removePromise.patch

Removed all uses of promise.jsm in C-C. The most in calendar, then in mailnews. One was in suite in a test. Also removed it.
Assignee: nobody → richard.marti
Status: NEW → ASSIGNED
Attachment #8896242 - Flags: review?(philipp)
Attachment #8896242 - Flags: review?(jorgk)
(Reporter)

Comment 3

2 years ago
You should have run the Xpcshell tests as well.
(Reporter)

Comment 5

2 years ago
No need to repeat Mozmill :-(, but thanks anyway.
(Reporter)

Comment 6

2 years ago
Comment on attachment 8896242 [details] [diff] [review]
removePromise.patch

Thanks, this is what I asked for. We need to land this before M-C remove Promise.jsm.
Attachment #8896242 - Flags: review?(jorgk) → review+
(Reporter)

Comment 7

2 years ago
More precisely: We need to land this before M-C remove Promise.jsm in bug 1378173.
Depends on: 1378173
Comment on attachment 8896242 [details] [diff] [review]
removePromise.patch

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

::: suite/common/places/tests/head_common.js
@@ -29,5 @@
>                                    "resource://gre/modules/FileUtils.jsm");
>  XPCOMUtils.defineLazyModuleGetter(this, "NetUtil",
>                                    "resource://gre/modules/NetUtil.jsm");
> -XPCOMUtils.defineLazyModuleGetter(this, "Promise",
> -                                  "resource://gre/modules/Promise.jsm");

I would revert this change, as some suite tests do seem to use Promise.defer: http://searchfox.org/comm-central/search?q=Promise.defer&case=true&path=suite
(Reporter)

Comment 9

2 years ago
Created attachment 8897386 [details] [diff] [review]
removePromise.patch (v2).

Removed suite/ hunk, thanks Florian!
Attachment #8896242 - Attachment is obsolete: true
Attachment #8896242 - Flags: review?(philipp)
Attachment #8897386 - Flags: review?(philipp)
(Reporter)

Updated

2 years ago
Attachment #8897386 - Flags: review+
Duplicate of this bug: 1390525

Comment 11

2 years ago
(In reply to Florian Quèze [:florian] [:flo] from comment #8)
> ::: suite/common/places/tests/head_common.js
> @@ -29,5 @@
> >                                    "resource://gre/modules/FileUtils.jsm");
> >  XPCOMUtils.defineLazyModuleGetter(this, "NetUtil",
> >                                    "resource://gre/modules/NetUtil.jsm");
> > -XPCOMUtils.defineLazyModuleGetter(this, "Promise",
> > -                                  "resource://gre/modules/Promise.jsm");
> 
> I would revert this change, as some suite tests do seem to use
> Promise.defer:
> http://searchfox.org/comm-central/search?q=Promise.defer&case=true&path=suite

How can you keep loading Promise.jsm if the file is going away (unless I misunderstand the bug) ?
In bug 1378173 there is mention that you can use PromiseUtils.defer() instead of Promise.defer() so I'd think this line should be changed to loading PromiseUtils.jsm instead of Promise.jsm and adapt the calls.
(Reporter)

Comment 12

2 years ago
If we land the patch now with the hunk for SM, we will break SM. If Promise.jsm goes away and we then land the patch, it doesn't matter, since SM will be broken either way. Maybe best not touch SM in this case so that they can use the appropriate replacement.
Attachment #8897386 - Flags: review?(philipp) → review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 13

2 years ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/421dc4935bc3
Remove uses of Promise.jsm. r=philipp,jorgk
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
(Reporter)

Updated

2 years ago
Target Milestone: --- → Thunderbird 57.0
(Reporter)

Comment 14

2 years ago
FRG, you need to take some action here for SM. Replace Promise.jsm with PromiseUtils.jsm and check the usage of .defer.
Flags: needinfo?(frgrahl)
Tests in SeaMonkey are broken for a long time. ewong is now trying to get them going again. Seems to be only one occurrence in tests so I will just let it stay in for now. Fixing all the broken tests later will be fun... not :)
Flags: needinfo?(frgrahl)
You need to log in before you can comment on or make changes to this bug.