1378173 - Remove Promise.jsm

Status: RESOLVED FIXED

(Toolkit :: Async Tooling, task, P3)

Description

[Marco Castelluccio [:marco]]

It's now only used in mobile/*, devtools/*, addon-sdk/* and some tests.
We should stop using it and remove it from the tree.

Comment 1

[Florian Quèze [:florian]]

(In reply to Marco Castelluccio [:marco] from comment #0)
> It's now only used in mobile/*, devtools/*, addon-sdk/* and some tests.
> We should stop using it and remove it from the tree.

This should wait until after 57, as removing it now would most likely break lots of add-ons.

Comment 2

[Kris Maglione [:kmag]]

Can we replace it with a stub for DOM promises in the mean time? That'd mean a lot less code to ship and load.

Comment 3

[:Paolo Amadini]

(In reply to Kris Maglione [:kmag] from comment #2)
> Can we replace it with a stub for DOM promises in the mean time? That'd mean
> a lot less code to ship and load.

We would still have to load the module first, so I don't think it will provide a substantial benefit. Depending on how this is done it may even add more C++/JS boundary traversals.

Comment 4

[Kris Maglione [:kmag]]

(In reply to :Paolo Amadini from comment #3)
> We would still have to load the module first, so I don't think it will
> provide a substantial benefit.

We'd still have a lot less code to load, though, especially since the bulk of
the module code is currently loaded into the module scope via loadSubScript.

> Depending on how this is done it may even add more C++/JS boundary
> traversals.

I doubt that, given how Promise.jsm stack saving and uncaught rejection
handling works (and which mostly relies on XPConnect). But that doesn't
especially worry me either way. WebIDL bindings are extremely fast.

Comment 5

[:Paolo Amadini]

I don't think that loading less code will necessarily make a difference, and we've already removed most imports of the module from the Firefox front-end. It seems to me that simply removing the remaining imports of Promise.jsm from mozilla-central would be a better use of our development time.

I suspect that replacing the uncaught rejection detection system would require test changes as well. I'm not opposed to reviewing a patch that passes all tests, including support for the way the module is loaded in the Add-on SDK and the devtools debugger. However, it might be easier and more useful to remove the imports.

If we wait until the Add-on SDK is removed from mozilla-central, it's also one less folder to worry about.

Comment 6

[Kris Maglione [:kmag]]

(In reply to :Paolo Amadini from comment #5)
> I don't think that loading less code will necessarily make a difference, and
> we've already removed most imports of the module from the Firefox front-end.
> It seems to me that simply removing the remaining imports of Promise.jsm
> from mozilla-central would be a better use of our development time.

Every extra script we load during startup is measurably expensive. At least the script preloader saves us from a lot of the overhead of synchronously reading and decoding the bytecode, but the overhead is still there. And the memory and performance overhead of using the Promise.jsm code is not trivial either.

I agree that we should focus on removing the in-tree imports, but I think that getting the Promise.jsm code out of the tree should be a priority, whether it happens before or after those removals.

> I suspect that replacing the uncaught rejection detection system would
> require test changes as well. I'm not opposed to reviewing a patch that
> passes all tests, including support for the way the module is loaded in the
> Add-on SDK and the devtools debugger. However, it might be easier and more
> useful to remove the imports.

DOM promises already have their own uncaught rejection handling, with their own tests, so I'm not especially worried about this.

The devtools debugger is going to have to switch to using native promises, regardless, so that's a separate issue.

Comment 7

[Jorg K (CEST = GMT+2)]

Promise.jsm is used quite a bit in comm-central (calendar, mailnews), so it there a replacement, or does C-C need to adopt it?

Comment 8

[:Paolo Amadini]

Code using "new Promise" can just use the native JavaScript promise implementation, while for Promise.defer() we have the equivalent PromiseUtils.defer(), although this module will eventually be removed too. See bug 1362882 and bug 1034599 for examples of the conversions we made in mozilla-central.

You'll need to adopt Promise.jsm only if you want to support legacy add-ons that use it.

Comment 9

[Florian Quèze [:florian]]

(In reply to Jorg K (GMT+2) from comment #7)
> Promise.jsm is used quite a bit in comm-central (calendar, mailnews), so it
> there a replacement, or does C-C need to adopt it?

It doesn't seem to be used: http://searchfox.org/comm-central/search?q=Promise.defer&case=true

But to actually answer your question: old style Promise.jsm promises were created using Promise.defer. We switched away from that using scripts in bug 1362882. For the few cases where we actually needed a deferred object, PromiseUtils.jsm can be used instead (that's what we did by hand in bug 1034599).

If you never use Promise.defer, you can just remove the Promise.jsm imports, like I did in bug 1368456 (there's a script you can re-use). Unless you are worried about add-on compat of course...

Comment 10

[Jorg K (CEST = GMT+2)]

Personally, I'm not so worried about add-on compatibility. IMHO, add-on authors should maintain add-ons and make adjustments where necessary.
 
> If you never use Promise.defer, you can just remove the Promise.jsm imports,
> like I did in bug 1368456
And that's it? Nothing else? I can do that ;-)

Comment 11

[Richard Marti (:Paenglab)]

Some files are using PromiseUtils.jsm too. Is this also planned to remove or is this something different?

Comment 12

[Florian Quèze [:florian]]

(In reply to Richard Marti (:Paenglab) from comment #11)
> Some files are using PromiseUtils.jsm too. Is this also planned to remove or
> is this something different?

We have no plan to remove PromiseUtils.jsm in the near future.

Comment 13

[Mark Banner (:standard8)]

Attachment: Bug 1378173 - Remove Promise.jsm.

Comment 14

[Mark Banner (:standard8)]

As Nicolas has been very generous in removing Promise.jsm from devtools (almost landed), I took a quick look at removing it from the tests - which looks like it hasn't been too hard to do.

Try run is here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=22004136cc0efb0e4bc05ef3ee12d582f8676e42

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c49a4fa60c95
Remove Promise.jsm. r=Gijs

Comment 16

[Marco Castelluccio [:marco]]

Nice to see this cleanup happen!

Comment 17

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/c49a4fa60c95

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/c4e633f5ea59
Port bug 1378173 - Remove Promise.jsm. rs=bustage-fix DONTBUILD