David Teller [:Yoric] - still alive but not very active Reporter
	Description • 10 years ago

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

	David Teller [:Yoric] - still alive but not very active Reporter
	Comment 1 • 10 years ago

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`.

	:Irving Reid (No longer working on Firefox)
	Comment 2 • 10 years ago

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.

	Akshendra Pratap Singh Assignee
	Comment 3 • 10 years ago

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.

	David Teller [:Yoric] - still alive but not very active Reporter
	Comment 4 • 10 years ago

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.

	Akshendra Pratap Singh Assignee
	Comment 5 • 10 years ago

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

	David Teller [:Yoric] - still alive but not very active Reporter
	Comment 6 • 10 years ago

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

	Akshendra Pratap Singh Assignee
	Comment 7 • 10 years ago

(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.

	David Teller [:Yoric] - still alive but not very active Reporter
	Comment 10 • 10 years ago

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`.

	Akshendra Pratap Singh Assignee
	Comment 11 • 10 years ago

> 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?

	David Teller [:Yoric] - still alive but not very active Reporter
	Comment 12 • 10 years ago

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`.

	David Teller [:Yoric] - still alive but not very active Reporter
	Comment 13 • 10 years ago

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.

	Akshendra Pratap Singh Assignee
	Comment 14 • 10 years ago

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.

	Akshendra Pratap Singh Assignee
	Comment 15 • 10 years ago

Need info is not working.

	David Teller [:Yoric] - still alive but not very active Reporter
	Comment 16 • 10 years ago

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.

	Akshendra Pratap Singh Assignee
	Comment 17 • 10 years ago

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.

	Akshendra Pratap Singh Assignee
	Comment 18 • 10 years ago

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".

	David Teller [:Yoric] - still alive but not very active Reporter
	Comment 19 • 10 years ago

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() {})`?

	Akshendra Pratap Singh Assignee
	Comment 20 • 10 years ago

"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.

	Akshendra Pratap Singh Assignee
	Comment 21 • 10 years ago

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.

	David Teller [:Yoric] - still alive but not very active Reporter
	Comment 22 • 10 years ago

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`.
});

	David Teller [:Yoric] - still alive but not very active Reporter
	Comment 23 • 10 years ago

I suggest using the arrow syntax, btw.

	Akshendra Pratap Singh Assignee
	Comment 24 • 10 years ago

:) Thanks for the help.

	David Teller [:Yoric] - still alive but not very active Reporter
	Comment 25 • 10 years ago

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.

	David Teller [:Yoric] - still alive but not very active Reporter
	Comment 27 • 10 years ago

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.

	Akshendra Pratap Singh Assignee
	Comment 28 • 10 years ago

Can you please assign this to me?

	David Teller [:Yoric] - still alive but not very active Reporter
	Comment 29 • 10 years ago

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.

	Akshendra Pratap Singh Assignee
	Comment 30 • 10 years ago

*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?

	Akshendra Pratap Singh Assignee
	Comment 31 • 10 years ago

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.

	Mark Hammond [:markh] [:mhammond]
	Comment 33 • 10 years ago

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

	David Teller [:Yoric] - still alive but not very active Reporter
	Comment 34 • 10 years ago

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.

	David Teller [:Yoric] - still alive but not very active Reporter
	Comment 36 • 10 years ago

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.

	David Teller [:Yoric] - still alive but not very active Reporter
	Comment 38 • 10 years ago

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.

	David Teller [:Yoric] - still alive but not very active Reporter
	Comment 40 • 10 years ago

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?

	Akshendra Pratap Singh Assignee
	Comment 41 • 10 years ago

Fingers Crossed :)

	David Teller [:Yoric] - still alive but not very active Reporter
	Comment 42 • 10 years ago

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

	David Teller [:Yoric] - still alive but not very active Reporter
	Comment 43 • 10 years ago

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

	David Teller [:Yoric] - still alive but not very active Reporter
	Comment 44 • 10 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 45 • 10 years ago

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

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 46 • 10 years ago

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