Closed
Bug 1388403
Opened 7 years ago
Closed 7 years ago
Remove uses of Promise.jsm from Thunderbird
Categories
(Thunderbird :: General, enhancement)
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 57.0
People
(Reporter: jorgk-bmo, Assigned: Paenglab)
References
Details
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
Fallen
:
review+
jorgk-bmo
:
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•7 years ago
|
||
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•7 years ago
|
||
Try build: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=17e47ccf07990762b9ec6b69948381ba97be9439
Reporter | ||
Comment 3•7 years ago
|
||
You should have run the Xpcshell tests as well.
Assignee | ||
Comment 4•7 years ago
|
||
Try with xpcshell: https://hg.mozilla.org/try-comm-central/rev/1f0a5bf3abc22e5ecb98f95ae2368e86a350fadc
Reporter | ||
Comment 5•7 years ago
|
||
No need to repeat Mozmill :-(, but thanks anyway.
Reporter | ||
Comment 6•7 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•7 years ago
|
||
More precisely: We need to land this before M-C remove Promise.jsm in bug 1378173.
Depends on: 1378173
Comment 8•7 years ago
|
||
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•7 years ago
|
||
Removed suite/ hunk, thanks Florian!
Attachment #8896242 -
Attachment is obsolete: true
Attachment #8896242 -
Flags: review?(philipp)
Attachment #8897386 -
Flags: review?(philipp)
Reporter | ||
Updated•7 years ago
|
Attachment #8897386 -
Flags: review+
Comment 11•7 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•7 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.
Updated•7 years ago
|
Attachment #8897386 -
Flags: review?(philipp) → review+
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 13•7 years ago
|
||
Pushed by mozilla@jorgk.com: https://hg.mozilla.org/comm-central/rev/421dc4935bc3 Remove uses of Promise.jsm. r=philipp,jorgk
Reporter | ||
Updated•7 years ago
|
Target Milestone: --- → Thunderbird 57.0
Reporter | ||
Comment 14•7 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)
Comment 15•7 years ago
|
||
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.
Description
•