1028987 - Don't use Task.jsm in SimpleTest.js

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Description

[Marco Castelluccio [:marco]]

Attachment: Patch

See bug 1000512 comment 10 and following.

The proposed patch uses promises instead of Task.jsm.
The return value of the cleanup function is treated as a promise if it has a then property that is a function. Is there a better way to do it?

Green on try: https://tbpl.mozilla.org/?tree=Try&rev=d37125dc272b

Comment 1

[Bobby Holley (:bholley)]

(In reply to Marco Castelluccio [:marco] from comment #0)
> Created attachment 8444489 [details] [diff] [review]
> Patch
> 
> See bug 1000512 comment 10 and following.
> 
> The proposed patch uses promises instead of Task.jsm.
> The return value of the cleanup function is treated as a promise if it has a
> then property that is a function. Is there a better way to do it?

You can do .constructor.name == "Promise". At present you could also do "instanceof Promise" because Promise is implemented in WebIDL, but that would break if/when Promise moves into the JS engine (which it should, per-spec).

Thanks for doing this!

Comment 2

[Marco Castelluccio [:marco]]

(In reply to Bobby Holley (:bholley) from comment #1)
> (In reply to Marco Castelluccio [:marco] from comment #0)
> > Created attachment 8444489 [details] [diff] [review]
> > Patch
> > 
> > See bug 1000512 comment 10 and following.
> > 
> > The proposed patch uses promises instead of Task.jsm.
> > The return value of the cleanup function is treated as a promise if it has a
> > then property that is a function. Is there a better way to do it?
> 
> You can do .constructor.name == "Promise". At present you could also do
> "instanceof Promise" because Promise is implemented in WebIDL, but that
> would break if/when Promise moves into the JS engine (which it should,
> per-spec).
> 
> Thanks for doing this!

Great! I thought this wouldn't work with the other promise implementations that we have in the tree.
I'll update the patch shortly (jmaher feel free to review this, the updated patch will be basically the same).

Comment 3

[Bobby Holley (:bholley)]

(In reply to Marco Castelluccio [:marco] from comment #2)
> (In reply to Bobby Holley (:bholley) from comment #1)
> > (In reply to Marco Castelluccio [:marco] from comment #0)
> > > Created attachment 8444489 [details] [diff] [review]
> > > Patch
> > > 
> > > See bug 1000512 comment 10 and following.
> > > 
> > > The proposed patch uses promises instead of Task.jsm.
> > > The return value of the cleanup function is treated as a promise if it has a
> > > then property that is a function. Is there a better way to do it?
> > 
> > You can do .constructor.name == "Promise". At present you could also do
> > "instanceof Promise" because Promise is implemented in WebIDL, but that
> > would break if/when Promise moves into the JS engine (which it should,
> > per-spec).
> > 
> > Thanks for doing this!
> 
> Great! I thought this wouldn't work with the other promise implementations
> that we have in the tree.

Oh, I don't know if it would work with Promise.jsm - would need to test.

Comment 4

[Marco Castelluccio [:marco]]

Attachment: Patch

The test on the constructor name works with Promise.jsm too (and the promise implementation in the addon sdk is based on Promise.jsm).

Comment 5

[Bobby Holley (:bholley)]

Marco, did you mean to cancel review on this patch? This is blocking something important.

Comment 6

[Marco Castelluccio [:marco]]

Comment on attachment 8444781 [details] [diff] [review]
Patch

Ops, sorry...

Comment 7

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8444781 [details] [diff] [review]
Patch

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

reading this code, it looks as though it is doing the right thing.  Please ensure this is well tested on try server.

Comment 8

[Marco Castelluccio [:marco]]

https://tbpl.mozilla.org/?tree=Try&rev=57d5164f7003

https://hg.mozilla.org/integration/mozilla-inbound/rev/9543f68db465

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/9543f68db465