1080466 - Add a `setTimeout` for Promise

Status: RESOLVED FIXED

(Toolkit :: Async Tooling, defect)

Description

[David Teller [:Yoric] - still alive but not very active]

Let's provide a version of Timer.jsm's `setTimeout` for `Promise`.

Comment 1

[David Teller [:Yoric] - still alive but not very active]

The code lives here: http://dxr.mozilla.org/mozilla-central/source/toolkit/modules/Timer.jsm?from=Timer.jsm#l1

The objective of this bug is to extend Timer.jsm with a new function `promiseTimeout(delay)` that returns a `Promise` resolved after `delay` milliseconds. Internally, this can be implemented with `setTimeout` (already defined in the same module).

This is a simple bug if you understand `Promise`.

Comment 2

[:Irving Reid (No longer working on Firefox)]

Hmm. Without prejudice as to choice of good function names, I was thinking something more like:

PromiseUtils.resolveOrTimeout(aPromise, delay, ?message)

that would return a Promise that resolves/rejects with the result of aPromise if it settles before 'delay' milliseconds pass, or rejects with a "Timed Out: message" error if aPromise does not settle within 'delay' ms.

Comment 3

[Akshendra Pratap Singh]

Please clarify if I am wrong.
What you want is that a new function is added to times.jsm that takes a delay and Promise as arguments. 
And after the delay, calls the resolve method of the Promise object.
or 
Something on the lines of Comment #2, check if the Promise is settled after the delay of time or not. If not then rejects with the error message.

Comment 4

[David Teller [:Yoric] - still alive but not very active]

Let's do what's Irving suggested in Comment 2. Once we have a first version, we will be able to revisit things if we find out that it's not exactly what we want.

Comment 5

[Akshendra Pratap Singh]

When I add the function in Timer.jsm, I get the following error
> TypeError: window.resolveOrTimeout is not a function
Any suggestions?

Comment 6

[David Teller [:Yoric] - still alive but not very active]

Why would you want to use `window.resolveOrTimeout`? Indeed, there is no such function.

Comment 7

[Akshendra Pratap Singh]

(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #6)
> Why would you want to use `window.resolveOrTimeout`? Indeed, there is no
> such function.

I added a function named resolveOrTimeout in Timer.jsm, how do I access it. I thought that the other function declared in that module were accessible through the window object. So, I tried window.resolveOrTimeout.

Comment 8

[Akshendra Pratap Singh]

Attachment: 1080466.patch

Comment 9

[Akshendra Pratap Singh]

Attachment: 1080466.patch

Comment 10

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8507861 [details] [diff] [review]
1080466.patch

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

Let's try something simpler.
Also, you should add a test to http://dxr.mozilla.org/mozilla-central/source/toolkit/modules/tests/xpcshell/test_timer.js to make sure that your function works.

::: toolkit/modules/Timer.jsm
@@ +7,5 @@
>  /**
>   * JS module implementation of nsIDOMJSWindow.setTimeout and clearTimeout.
>   */
>  
> +this.EXPORTED_SYMBOLS = ["setTimeout", "clearTimeout", "awesome"];

"awesome"? Did you forget changing something here, by any chance?

@@ +14,5 @@
>  const Ci = Components.interfaces;
>  const Cu = Components.utils;
>  
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +Cu.import("resource://gre/modules/Services.jsm");

You don't use Services.jsm, so don't import it.

@@ +21,5 @@
> +const STATUS_RESOLVED = 1;
> +const STATUS_REJECTED = 2;
> +
> +const salt = Math.floor(Math.random() * 100);
> +const N_INTERNALS = "{private:internals:" + salt + "}";

You don't need to hit the internals of Promise for such a simple function.

@@ +71,5 @@
> +          }); 
> +      }
> +    });
> +  return resultingPromise;
> +}
\ No newline at end of file

You can do something much, much, much simpler, that doesn't need to hit the internals of `Promise` – especially since these internals are going to change soon.

I believe that you only need
- one call to `setTimeout`;
- one call to `new Promise()`;
- one call to `Promise.race`.

Comment 11

[Akshendra Pratap Singh]

> this.resolveOrTimeout = function resolveOrTimeout(aPromise, aMilliseconds, aMessage) {
>   return new Promise(function(resolve, reject) {
>     Promise.race([aPromise]).then(resolve, reject);
>     this.setTimeout(
>       function() {
>         reject(Error("Timeout: " + aMessage));
>       }, aMilliseconds);
>   });
> }
I am using this function and this seems to work, tried the internals of the function seperately. But when I call the function the received object rejects with an empty object as response. Is there something wrong with the function?

Comment 12

[David Teller [:Yoric] - still alive but not very active]

Your code seems to work for me. How are you testing it?

Note, by the way, that when you call `Promise.race(list)`, you obtain a promise that is resolved/rejected once the first promise of `list` is resolved/rejected. Here, your list consists in a single promise, so there is no race. In other words, if `aPromise` is a `Promise`, `Promise.race([aPromise])` is basically the same thing as `aPromise`.

Comment 13

[David Teller [:Yoric] - still alive but not very active]

By the way, in the future, please use "Need more information from" to make sure that I see your message. Otherwise, it risks getting lost in the ~800 e-mails/day I receive.

Comment 14

[Akshendra Pratap Singh]

I tried it using the following code in an html file and that worked
https://pastebin.mozilla.org/6862416

But when I receive the object in the test file like below
https://pastebin.mozilla.org/6862432
In this case it get rejected with an empty object.

Comment 15

[Akshendra Pratap Singh]

Need info is not working.

Comment 16

[David Teller [:Yoric] - still alive but not very active]

Well, the `do_check_eq` will fail because `Error` is not a string, but other than that, it looks liike the test should work. What's the output?

Note: needinfo worked nicely.

Comment 17

[Akshendra Pratap Singh]

The problem is that the received promise seems to reject instantly, it does not wait at all and that too with an empty object.
It tries to compare something like {} == "Sorry", which obviously fails.
I even tried something very simple like just providing a resolve or reject after a delay inside the resolveOrTimeout(), again the result does not seem to get propogated to the object received in the test file, it only gets rejected with an empty object.

Comment 18

[Akshendra Pratap Singh]

The received object gets instantly rejected with Error (oops the empty object was actually an error object) ie a TypeError and says "This is undefined".

Comment 19

[David Teller [:Yoric] - still alive but not very active]

I assume that "the received object" is `p1` in https://pastebin.mozilla.org/6862432, right?
If so, why don't you test with a promise that never resolves, e.g. `new Promise(resolve => {})` or equivalently `new Promise(function() {})`?

Comment 20

[Akshendra Pratap Singh]

"the received object" is not p1, sorry for my bad choice of words. Its the Promise that is returned from the resolveOrTimeout() function. This object gets rejected instantly with a TypeError: This is not defined.
Ans as you said, I also tried with the Promise that does not resolves at all. Same problem.

Comment 21

[Akshendra Pratap Singh]

do_test_pending();
  imported.resolveOrTimeout(p1, 300, "Sorry").then(null,
    function(error) {
      do_check_eq(error.name + error.message,"Timeout: Sorry");
	do_test_finished();			
	});

error.name + error.message; // this says TypeError  this is undefined
Ans this happens instantly no matter how much delay is pass.

Comment 22

[David Teller [:Yoric] - still alive but not very active]

Ah, ok. In that case, the error message contains your answer: `this` is not defined. Apparently, you are trying to execute code that uses `this` in a context where it is not defined.

To fix this, you can either capture `this.setTimeout` in a global variable `setTimeout`, or switch to the arrow syntax for your anonymous functions, e.g.

new Promise((resolve, reject) => {
  // Here, `this` is the same as outside the call to `new Promise`.
});

Comment 23

[David Teller [:Yoric] - still alive but not very active]

I suggest using the arrow syntax, btw.

Comment 24

[Akshendra Pratap Singh]

Attachment: 1080466.patch

:) Thanks for the help.

Comment 25

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8510268 [details] [diff] [review]
1080466.patch

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

Good first version. I realize that this function should probably go to a new module `PromiseUtils.jsm`, in the same directory as `Timer.jsm`. Also, let's move the tests to a new file `test_promiseutils.js`.
You will need to update moz.build and xpcshell.ini so that they build your new files.

Also, don't forget to add a commit message, e.g. "Bug 1080466: Implement resolveOrTimeout;r=yoric"

::: toolkit/modules/Timer.jsm
@@ +40,5 @@
>      gTimeoutTable.delete(aId);
>    }
>  }
> +
> +this.resolveOrTimeout = function resolveOrTimeout(aPromise, aMilliseconds, aMessage) {

This function needs documentation.

@@ +44,5 @@
> +this.resolveOrTimeout = function resolveOrTimeout(aPromise, aMilliseconds, aMessage) {
> +  return new Promise((resolve, reject) => {
> +    aPromise.then(resolve, reject);
> +    this.setTimeout(
> +      function() {

Let's use the arrow syntax here, too.
this.setTimeout(() => reject(...), aMilliseconds)

::: toolkit/modules/tests/xpcshell/test_timer.js
@@ +26,5 @@
>    do_check_true(timeout2 > 0, "setTimeout returns a positive number");
>    do_check_neq(timeout1, timeout2, "Calling setTimeout again returns a different value");
>  
>    imported.clearTimeout(timeout1);
> +

Your tests should go to a new function, e.g. `function test_resolveOrTimeout`.

@@ +27,5 @@
>    do_check_neq(timeout1, timeout2, "Calling setTimeout again returns a different value");
>  
>    imported.clearTimeout(timeout1);
> +
> +  // test for the resolveOrTimeout : case where the passed Promise is not resolved within the pasesed time 

Nit: Please remove whitespace at the end of the line.

@@ +34,5 @@
> +  		imported.setTimeout(
> +  			function() {
> +  				resolve("p1 is resolved");
> +  			}, 10000);
> +  	});

These days, we prefer the arrow syntax for anonymous functions, e.g.

let p1 = new Promise(resolve =>
  imported.setTimeout(
    () => resolve("p1 is resolved"),
    10000
  )
)

@@ +36,5 @@
> +  				resolve("p1 is resolved");
> +  			}, 10000);
> +  	});
> +
> +  do_test_pending();

Once you have moved your tests to a new file, I suggest you use `add_task` to chain your asynchronous tests, rather than do_test_pending/do_test_finished, this will be much more readable and robust.

@@ +37,5 @@
> +  			}, 10000);
> +  	});
> +
> +  do_test_pending();
> +  imported.resolveOrTimeout(p1, 300, "Sorry").then(null,

Once you have moved to `add_task`, you will be able to write

try {
  yield imported.resolveOrTimeout(...);
  ...
} catch (ex) {
  ...
}

@@ +40,5 @@
> +  do_test_pending();
> +  imported.resolveOrTimeout(p1, 300, "Sorry").then(null,
> +		function(error) {
> +			do_check_eq(error,"Timeout: Sorry");
> +			do_test_finished();			

Nit: whitespace.

@@ +43,5 @@
> +			do_check_eq(error,"Timeout: Sorry");
> +			do_test_finished();			
> +		});
> +
> +  // test for the resolveOrTimeout : case where the passed Promise is resolved within the pasesed time 

Nit: whitespace.

@@ +56,5 @@
> +  do_test_pending();
> +  imported.resolveOrTimeout(p2, 300, "Sorry").then(
> +		function(val) {
> +			do_check_eq(val, "p2 is resolved");
> +			do_test_finished();			

Nit: whitespace.

Comment 26

[Akshendra Pratap Singh]

Attachment: 1080466.patch

Comment 27

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8510394 [details] [diff] [review]
1080466.patch

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

Good code. Now, let's build on that :)

::: toolkit/modules/PromiseUtils.jsm
@@ +1,1 @@
> +/* */

Please add the standard headers: https://www.mozilla.org/MPL/headers/

@@ +1,5 @@
> +/* */
> +
> +"use strict"
> +
> +this.EXPORTED_SYMBOLS = ["resolveOrTimeout"];

Let's put everything in an object PromiseUtils and export that symbol instead.

@@ +4,5 @@
> +
> +this.EXPORTED_SYMBOLS = ["resolveOrTimeout"];
> +
> +let imported = {};
> +Components.utils.import("resource://gre/modules/Timer.jsm", imported); 

Just import to `this`. Also, please remove the whitespace at the end of the line.

@@ +8,5 @@
> +Components.utils.import("resource://gre/modules/Timer.jsm", imported); 
> +
> +/* resolveOrTimeout returns a Promise that will get settled with the result of the "aPromise"
> + * IF "aPromise" settles within the "aMillisecond" delay that is passed
> + * Otherwise the returned Promise will get rejected with a message "Timeout: " + aMessage. */

Let's rephrase, reformat and take the opportunity to change `aMessage` into something a bit more generic.

/**
 * A simple timeout mechanism.
 *
 * Example:
 * resolveOrTimeout(myModule.shutdown(), 1000, new Error("The module took too long to shutdown"));
 *
 * @param {Promise} promise The Promise that should resolve/reject quickly.
 * @param {number} delay A delay after which to stop waiting for `aPromise`, in milliseconds.
 * @param {function} rejection If `aPromise` hasn't resolved/rejected after `delay`,
 * a value used to construct the rejection.
 *
 * @return {Promise} A promise that behaves as `promise`, if `promise` is
 * resolved/rejected within `delay` ms, or rejects with `rejection()` otherwise.
 */

Also, please rename `aPromise`, `aMilliseconds`, `aMessage` - prefixing with "a" is a style we now try to avoid in new code.

@@ +12,5 @@
> + * Otherwise the returned Promise will get rejected with a message "Timeout: " + aMessage. */
> +this.resolveOrTimeout = function resolveOrTimeout(aPromise, aMilliseconds, aMessage) {
> +  return new Promise((resolve, reject) => {
> +    aPromise.then(resolve, reject);
> +    imported.setTimeout(() => reject("Timeout: " + aMessage), aMilliseconds);

You will need to change the value passed to `reject()` to match the above documentation.

::: toolkit/modules/tests/xpcshell/test_PromiseUtils.js
@@ +1,1 @@
> +"use strict";

Please add the public domain boilerplate
https://www.mozilla.org/MPL/headers/

@@ +1,4 @@
> +"use strict";
> +
> +let impPromiseUtils = {};
> +let impTimer = {};

You don't need these. Just import to `this`.

@@ +7,5 @@
> +
> +// Tests for PromiseUtils.jsm
> +function run_test() {
> +  add_task(test_Promise1);
> +  add_task(test_Promise2);

function run_test() {
  run_next_test();
}

@@ +12,5 @@
> +  run_next_test();
> +} 
> +
> +// This generator tests for the case when the passed Promise("p1" here) settels after the delay passed to resolveOrTimeout
> +function test_Promise1() {

The preferred style is
add_task(function* test_foo() {
});

Also, the name of this test is not very clear. This should probably be `test_timeout`.

@@ +17,5 @@
> +  let p1 = new Promise((resolve, reject) => impTimer.setTimeout(() => resolve("p1 is resolved"), 10000));
> +  try {
> +  	yield impPromiseUtils.resolveOrTimeout(p1, 300, "Sorry");
> +  }
> +  catch(ex) {

Could you make this `} catch (ex) {`?

@@ +18,5 @@
> +  try {
> +  	yield impPromiseUtils.resolveOrTimeout(p1, 300, "Sorry");
> +  }
> +  catch(ex) {
> +  	do_check_eq(ex, "Timeout: Sorry", "Timeout occured because the passed promise does not settles within time");

These days, we use `Assert.equal` instead of `do_check_eq`.

@@ +19,5 @@
> +  	yield impPromiseUtils.resolveOrTimeout(p1, 300, "Sorry");
> +  }
> +  catch(ex) {
> +  	do_check_eq(ex, "Timeout: Sorry", "Timeout occured because the passed promise does not settles within time");
> +  }

This is not going to test what you want, as you don't make sure that your code actually launches an exception.
You should use `Assert.rejects`, as defined here: http://dxr.mozilla.org/mozilla-central/search?q=proto.rejects&case=true

@@ +29,5 @@
> +  try {
> +  	yield impPromiseUtils.resolveOrTimeout(p2, 300, "Sorry").then((val) => do_check_eq(val, "p2 is resolved", "The values of p2 is here because p2 settled within time"));
> +  }
> +  catch(ex) {}
> +}
\ No newline at end of file

I would like the following tests:
- when `promise` resolves quickly, the result resolves quickly;
- when `promise` rejects quickly, the result rejects quickly;
- when `promise` never resolves/rejects, the result rejects with `rejection()` – let's try this when `rejection()` returns a string, `undefined` or an object;
- when `promise` never resolves/rejects and `rejection()` throws an Error, the result rejects with that error;
- when we pass something other than a promise as first argument, we get a TypeError;
- when we pass something other than a number as second argument, we get a TypeError;
- when we pass something other than a function as third argument, we get a TypeError.

Comment 28

[Akshendra Pratap Singh]

Can you please assign this to me?

Comment 29

[David Teller [:Yoric] - still alive but not very active]

Done.
Oh, I forgot: let's make the third argument optional. If it not provided, we should throw `new Error("Promise timeout")` in case of timeout.

Comment 30

[Akshendra Pratap Singh]

*rejection()* is going to a function, which will return the value to be passed as the reason of rejection. And if this *rejection()* throws an error then it will be the reason of rejection.

Is what I understoodd correct?

Comment 31

[Akshendra Pratap Singh]

Attachment: 1080466.patch

About using Assert.rejects, can you explain a little more why it should be used. Because all the tests are running just fine even without it.

Comment 32

[Akshendra Pratap Singh]

Attachment: 1080466.patch

Comment 33

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8511495 [details] [diff] [review]
1080466.patch

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

A drive-by - bug in general, I'm not clear on the use-case for this bug.  I've seen the implementation of comment 1 in a number of tests, but I'm yet to come across code that uses this second pattern (and best I can tell, this new function isn't generally suitable for code which wants comment 1).  Is there an identified use-case, or is this just a matter of "just incase we need it in the future it will be ready"?

::: toolkit/modules/PromiseUtils.jsm
@@ +30,5 @@
> +    }
> +
> +    // throw a TypeError if <delay> is not a number
> +    if (typeof delay != "number" || delay < 0) {
> +      throw new TypeError("< resolveOrTimeout(promise, delay[, rejection] > 'delay' must be a positive number");  

nit: a fair but of trailing whitespace in this patch.

@@ +35,5 @@
> +    }
> +
> +    var rejectionReason = new Error("Promise Timeout");
> +    // if <rejection> provided but is not a function then throw a TypeError 
> +    if (arguments.length > 2 && typeof rejection != "function") {

can't we just say "if rejection && typeof..." (or an explicit check for undefined if that floats your boat)?

Then something like:

rejection = rejection || () => new Error("...");

But - even then, we are unconditionally and immediately calling the rejection function if one is passed - why not just have the optional argument be the rejection reason itself?

@@ +50,5 @@
> +    }
> +
> +    return new Promise((resolve, reject) => {
> +      promise.then(resolve, reject);
> +      setTimeout(() => reject(rejectionReason), delay);

I'd expect to see the timer killed on resolution.

@@ +51,5 @@
> +
> +    return new Promise((resolve, reject) => {
> +      promise.then(resolve, reject);
> +      setTimeout(() => reject(rejectionReason), delay);
> +      });

this line needs a dedent

Comment 34

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8511495 [details] [diff] [review]
1080466.patch

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

This gets better with each version.
You should really double-check your tests, though, most of them don't do what you expect.

::: toolkit/modules/PromiseUtils.jsm
@@ +22,5 @@
> +   *
> +   * @return {Promise} A promise that behaves as `promise`, if `promise` is
> +   * resolved/rejected within `delay` ms, or rejects with `rejection()` otherwise.
> +   */
> +  resolveOrTimeout : (promise, delay, rejection) => {

Nit: Generally, we keep the => notation for anonymous functions, so I'd suggest using `function(promise, delay, rejection)` here.

@@ +33,5 @@
> +    if (typeof delay != "number" || delay < 0) {
> +      throw new TypeError("< resolveOrTimeout(promise, delay[, rejection] > 'delay' must be a positive number");  
> +    }
> +
> +    var rejectionReason = new Error("Promise Timeout");

You should avoid `var` in our code, and rather use `let`.
Also, don't create `rejectionReason` if you don't need it, as creating exceptions is rather expensive. Rather, use a default argument for `rejection`, e.g.
`function (promise, delay, rejection = new Error("Promise Timeout"))`

This will help you simplify your code.

@@ +39,5 @@
> +    if (arguments.length > 2 && typeof rejection != "function") {
> +      throw new TypeError("< resolveOrTimeout(promise, delay[, rejection] > 'rejection' must be a function"); 
> +    }
> +    // or other wise find the reason for rejection using the funtion passed
> +    else if(arguments.length > 2){

Nit: `} else {`

@@ +50,5 @@
> +    }
> +
> +    return new Promise((resolve, reject) => {
> +      promise.then(resolve, reject);
> +      setTimeout(() => reject(rejectionReason), delay);

Ah, right, as Mark mentioned: please call `clearTimeout` on resolution of `promise`. This will not change the result, but it will clear up memory, etc.

::: toolkit/modules/tests/xpcshell/test_PromiseUtils.js
@@ +8,5 @@
> +
> +// Tests for PromiseUtils.jsm
> +function run_test() {
> +  run_next_test();
> +} 

Nit: whitespace.

@@ +12,5 @@
> +} 
> +
> +/* This generator tests for the case when first argument to resolveOrTimeout
> + * that should be a Promise is not a promise. */
> +add_task(function test_argument_not_promise() {

You are using a deprecated syntax. These days, it's `function*`.

@@ +16,5 @@
> +add_task(function test_argument_not_promise() {
> +  try {
> +  	yield PromiseUtils.resolveOrTimeout("string", 200);
> +  }
> +  catch(ex) {

Nit: `} catch (ex) {`.

@@ +18,5 @@
> +  	yield PromiseUtils.resolveOrTimeout("string", 200);
> +  }
> +  catch(ex) {
> +  	Assert.equal(ex.message, "< resolveOrTimeout(promise, delay[, rejection] > 'promise' must be a Promise object"
> +              , "The first argument to resolveOrTimeout is not a promise");

If no exception is raised, this test will also pass.
To make sure that the exception is raised, you should use `Assert.throws`.

@@ +20,5 @@
> +  catch(ex) {
> +  	Assert.equal(ex.message, "< resolveOrTimeout(promise, delay[, rejection] > 'promise' must be a Promise object"
> +              , "The first argument to resolveOrTimeout is not a promise");
> +  }
> +});

No need to split all these TypeError tests across so many tasks, just gather them in `function* test_wrong_arguments`.

@@ +57,5 @@
> +    yield PromiseUtils.resolveOrTimeout(p, 200);
> +  }
> +  catch(ex) {
> +    Assert.equal(ex.message, "Promise Timeout"
> +              , "The third argument is not given and the returned promise is rejected");

If `resolveOrTimeout` doesn't reject, this test will also pass.
To make sure that the rejection happens, you should use `Assert.rejects`.

@@ +70,5 @@
> +    yield PromiseUtils.resolveOrTimeout(p, 200);
> +  }
> +  catch(ex) {
> +    Assert.equal(ex, "Promise is resolved"
> +              , "The passed promise resolves quickly");

That test doesn't make sense.

Comment 35

[Akshendra Pratap Singh]

Attachment: 1080466.patch

Comment 36

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8512576 [details] [diff] [review]
1080466.patch

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

::: toolkit/modules/PromiseUtils.jsm
@@ +22,5 @@
> +   *
> +   * @return {Promise} A promise that behaves as `promise`, if `promise` is
> +   * resolved/rejected within `delay` ms, or rejects with `rejection()` otherwise.
> +   */
> +  resolveOrTimeout : function(promise, delay, rejection)  {

Let's not propagate undefined stuff, so that should be `rejection = null`.

::: toolkit/modules/tests/xpcshell/test_PromiseUtils.js
@@ +17,5 @@
> +  let p = new Promise((resolve, reject) => {});
> +
> +  // for the first argument
> +  try {
> +    yield Assert.throws (() => PromiseUtils.resolveOrTimeout("string", 200));

That's not how `Assert.throws` works.
See http://dxr.mozilla.org/mozilla-central/source/toolkit/components/places/tests/bookmarks/test_bookmarks_fetch.js#12 for some examples of using Assert.throws.

@@ +30,5 @@
> +  } catch(ex) {
> +    Assert.equal(ex.name + " : " + ex.message, "second argument <delay> must be a positive number"
> +                      , "The second argument to resolveOrTimeout is not a number");
> +  }
> +  

Nit: whitespace.

Comment 37

[Akshendra Pratap Singh]

Attachment: 1080466.patch

Comment 38

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8512696 [details] [diff] [review]
1080466.patch

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

We're getting there :)

::: toolkit/modules/tests/xpcshell/test_PromiseUtils.js
@@ +11,5 @@
> +  run_next_test();
> +}
> +
> +/* This generator tests for the case when first argument to resolveOrTimeout
> + * are not of correct type */

Nit: Can you remove "This generator" from all your comments?
Also, this comment is not true anymore.

@@ +16,5 @@
> +add_task(function* test_wrong_arguments() {
> +  let p = new Promise((resolve, reject) => {});
> +
> +  // for the first argument
> +  Assert.throws (() => PromiseUtils.resolveOrTimeout("string", 200), /first argument <promise> must be a Promise object/);

Nit: No whitespace after `throws`.

@@ +36,5 @@
> +/* This generator tests for the case when the passed promise resolves quickly
> + * In that case the returned promise also resolves with the same value */
> +add_task(function* test_resolve_quickly() {
> +  let p = new Promise((resolve, reject) => setTimeout(() => resolve("Promise is resolved"), 20));
> +  PromiseUtils.resolveOrTimeout(p, 200).then((val) => Assert.equal(val, "Promise is resolved"));

Your code should work, but I would prefer the following, which depends less on our sophisticated machinery for catching uncaught rejections.

let result = yield PromiseUtils.resolveOrTimeout(p, 200);
Assert.equal(result, "Promise is resolved");

@@ +43,5 @@
> +/* This generator tests for the case when the passed promise rejects quickly
> + * In that case the returned promise also rejects with the same value */
> +add_task(function* test_reject_quickly() {
> +  let p = new Promise((resolve, reject) => setTimeout(() => reject("Promise is rejected"), 20));
> +  PromiseUtils.resolveOrTimeout(p, 200).then(null, (val) => Assert.equal(val, "Promise is rejected"));

No, this won't work.
Here, you should use `Assert.rejects`.

@@ +46,5 @@
> +  let p = new Promise((resolve, reject) => setTimeout(() => reject("Promise is rejected"), 20));
> +  PromiseUtils.resolveOrTimeout(p, 200).then(null, (val) => Assert.equal(val, "Promise is rejected"));
> +});
> +
> +/* This generator tests for the case when the passed promise doesn't settles

Nit: "doesn't settle".

@@ +50,5 @@
> +/* This generator tests for the case when the passed promise doesn't settles
> + * ans rejection returns a string */
> +add_task(function* test_rejection_function() {
> +  let p = new Promise((resolve, reject) => {});
> +  

Nit: Whitespace.

@@ +59,5 @@
> +    }));
> +  } catch(ex) {
> +    Assert.equal(ex, "Rejection returned a string"
> +              , "The rejection function returned a string");
> +  }

Since you're already using `Assert.rejects`, this `try/catch` doesn't make sense. Same thing below.

@@ +60,5 @@
> +  } catch(ex) {
> +    Assert.equal(ex, "Rejection returned a string"
> +              , "The rejection function returned a string");
> +  }
> +  

Nit: Whitespace.

Comment 39

[Akshendra Pratap Singh]

Attachment: 1080466.patch

Comment 40

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8512762 [details] [diff] [review]
1080466.patch

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

Almost there :)

::: toolkit/modules/tests/xpcshell/test_PromiseUtils.js
@@ +23,5 @@
> +  Assert.throws(() => PromiseUtils.resolveOrTimeout(p, 200, "string"), /third optional argument <rejection> must be a function/);
> +});
> +
> +/* Tests for the case when the optional third argument is not provided
> + * In that case the retrned promise rejects with a default Error */

Nit: "the returned promise"

@@ +26,5 @@
> +/* Tests for the case when the optional third argument is not provided
> + * In that case the retrned promise rejects with a default Error */
> +add_task(function* test_optional_third_argument() {
> +  let p = new Promise((resolve, reject) => {});
> +  Assert.rejects(PromiseUtils.resolveOrTimeout(p, 200), /Promise Timeout/);

`Assert.rejects` returns a Promise, so you need to call `yield` here and in every other call to `Assert.rejects`.

@@ +45,5 @@
> +  Assert.rejects(PromiseUtils.resolveOrTimeout(p, 200), /Promise is rejected/);
> +});
> +
> +/* Tests for the case when the passed promise doesn't settle
> + * ans rejection returns a string */

Nit: "and rejection". Also, the comment isn't true anymore.

@@ +56,5 @@
> +
> +  // for rejection returning object
> +  Assert.rejects(PromiseUtils.resolveOrTimeout(p, 200, () => {
> +    return {Name:"Promise"};
> +  }));

Does that work?

Comment 41

[Akshendra Pratap Singh]

Attachment: 1080466_setTimout_for_promise.patch

Fingers Crossed :)

Comment 42

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8514310 [details] [diff] [review]
1080466_setTimout_for_promise.patch

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

This looks good to me \o/
Let's run this through the Try server:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=8b17608191ef

Comment 43

[David Teller [:Yoric] - still alive but not very active]

By the way, once this bug has landed, you should take a look at bug 1093021, which is more work based on Promise.

Comment 44

[David Teller [:Yoric] - still alive but not very active]

This looks good to me. Let's land that patch.

Comment 45

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/26bc4268f868

Comment 46

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/26bc4268f868