Closed Bug 1028987 Opened 6 years ago Closed 6 years ago

Don't use Task.jsm in SimpleTest.js

Categories

(Testing :: Mochitest, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla33

People

(Reporter: marco, Assigned: marco)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached patch Patch (obsolete) — Splinter Review
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
Attachment #8444489 - Flags: review?(jmaher)
(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!
Blocks: 856067
(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).
(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.
Attached patch PatchSplinter Review
The test on the constructor name works with Promise.jsm too (and the promise implementation in the addon sdk is based on Promise.jsm).
Assignee: nobody → mar.castelluccio
Attachment #8444489 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8444489 - Flags: review?(jmaher)
Marco, did you mean to cancel review on this patch? This is blocking something important.
Flags: needinfo?(mar.castelluccio)
Comment on attachment 8444781 [details] [diff] [review]
Patch

Ops, sorry...
Attachment #8444781 - Flags: review?(jmaher)
Flags: needinfo?(mar.castelluccio)
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.
Attachment #8444781 - Flags: review?(jmaher) → review+
https://hg.mozilla.org/mozilla-central/rev/9543f68db465
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Depends on: 1084001
You need to log in before you can comment on or make changes to this bug.