Remove uses of Promise.jsm from Thunderbird

RESOLVED FIXED in Thunderbird 57.0

Status

Thunderbird
General
RESOLVED FIXED
3 months ago
3 months ago

People

(Reporter: Jorg K [Almost not working on Thunderbird (some bustage-fix only) due to non-renewal of contract], Assigned: Paenglab)

Tracking

(Depends on: 1 bug)

52 Branch
Thunderbird 57.0

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

19.54 KB, patch
Fallen
: review+
Jorg K [Almost not working on Thunderbird (some bustage-fix only) due to non-renewal of contract]
: review+
Details | Diff | Splinter Review
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

3 months 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)
(Assignee)

Comment 2

3 months ago
Try build: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=17e47ccf07990762b9ec6b69948381ba97be9439
You should have run the Xpcshell tests as well.
(Assignee)

Comment 4

3 months ago
Try with xpcshell: https://hg.mozilla.org/try-comm-central/rev/1f0a5bf3abc22e5ecb98f95ae2368e86a350fadc
No need to repeat Mozmill :-(, but thanks anyway.
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+
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
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)
Attachment #8897386 - Flags: review+
Duplicate of this bug: 1390525

Comment 11

3 months 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.
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

3 months ago
Keywords: checkin-needed

Comment 13

3 months 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: 3 months ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 57.0
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.