Last Comment Bug 711126 - Extend Promise.jsm
: Extend Promise.jsm
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: General (show other bugs)
: unspecified
: All All
: -- enhancement (vote)
: mozilla14
Assigned To: David Rajchenbach-Teller [:Yoric] (please use "needinfo")
:
Mentors:
: 659300 (view as bug list)
Depends on: 720628
Blocks: JSScheduleAPI 724819 735477
  Show dependency treegraph
 
Reported: 2011-12-15 09:30 PST by David Rajchenbach-Teller [:Yoric] (please use "needinfo")
Modified: 2012-12-04 11:59 PST (History)
8 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Utilities for promises (5.25 KB, patch)
2011-12-15 09:30 PST, David Rajchenbach-Teller [:Yoric] (please use "needinfo")
no flags Details | Diff | Review
Promises with progress (2.38 KB, patch)
2011-12-15 09:51 PST, David Rajchenbach-Teller [:Yoric] (please use "needinfo")
jwalker: feedback+
Details | Diff | Review
Utilities for promises (2.93 KB, patch)
2012-01-05 03:42 PST, David Rajchenbach-Teller [:Yoric] (please use "needinfo")
no flags Details | Diff | Review
Logging uncaught rejections (1.93 KB, patch)
2012-01-05 03:43 PST, David Rajchenbach-Teller [:Yoric] (please use "needinfo")
jwalker: feedback+
Details | Diff | Review
Utilities for promises (3.09 KB, patch)
2012-01-24 01:13 PST, David Rajchenbach-Teller [:Yoric] (please use "needinfo")
jwalker: review+
Details | Diff | Review
Utilities for promises (3.08 KB, patch)
2012-01-24 01:47 PST, David Rajchenbach-Teller [:Yoric] (please use "needinfo")
jwalker: review+
Details | Diff | Review
Utilities for promises (3.12 KB, patch)
2012-01-25 02:46 PST, David Rajchenbach-Teller [:Yoric] (please use "needinfo")
jwalker: review+
Details | Diff | Review
Companion testsuite (2.29 KB, patch)
2012-01-25 02:47 PST, David Rajchenbach-Teller [:Yoric] (please use "needinfo")
jwalker: review+
Details | Diff | Review
Companion testsuite (2.27 KB, patch)
2012-02-03 09:59 PST, David Rajchenbach-Teller [:Yoric] (please use "needinfo")
no flags Details | Diff | Review
Utilities for promises (3.08 KB, patch)
2012-02-03 10:00 PST, David Rajchenbach-Teller [:Yoric] (please use "needinfo")
no flags Details | Diff | Review
Companion testsuite (2.38 KB, patch)
2012-02-03 10:12 PST, David Rajchenbach-Teller [:Yoric] (please use "needinfo")
no flags Details | Diff | Review
promise changes (24.51 KB, patch)
2012-02-13 14:32 PST, Joe Walker [:jwalker] (needinfo me or ping on irc)
no flags Details | Diff | Review
Utilities for promises (5.06 KB, patch)
2012-02-24 07:00 PST, David Rajchenbach-Teller [:Yoric] (please use "needinfo")
rcampbell: review-
Details | Diff | Review
Companion testsuite (3.33 KB, patch)
2012-02-24 07:01 PST, David Rajchenbach-Teller [:Yoric] (please use "needinfo")
rcampbell: review+
Details | Diff | Review
Utilities for promises (5.04 KB, patch)
2012-02-27 11:47 PST, David Rajchenbach-Teller [:Yoric] (please use "needinfo")
rcampbell: review+
Details | Diff | Review
Utilities for promises (5.09 KB, patch)
2012-03-14 10:18 PDT, David Rajchenbach-Teller [:Yoric] (please use "needinfo")
no flags Details | Diff | Review

Description David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2011-12-15 09:30:49 PST
Created attachment 582009 [details] [diff] [review]
Utilities for promises

Now that Promise.jsm is on its way to become public, I attach a few patches to make client life easier. Namely, a counterpart to |finally|, a counterpart to |catch| and trivial stuff.
Comment 1 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2011-12-15 09:51:12 PST
Created attachment 582013 [details] [diff] [review]
Promises with progress

Attaching a proposal for extending promises with progress listeners.

This is more to jumpstart discussion on exactly what we would need than to land this second patch.
Comment 2 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-12-16 09:31:17 PST
Comment on attachment 582013 [details] [diff] [review]
Promises with progress

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

I like the changes.

The big question to me is - do we aim to maintain web compatibility or do we fork. It feels as though currently there isn't a significant advantage to forking.

::: browser/devtools/shared/Promise.jsm
@@ +144,5 @@
> + * @param {*} aData The value to pass to handlers.
> + * @return {Promise} |this|
> + */
> +Promise.prototype.progress = function(aData) {
> +  for each(let handler in this._onProgressHandlers) {

Promise as is stands is used on the web in GCLI, so either we fork, or we use:

  for (var i = 0; i < this._onProgressHandlers.length; i++) {

The latter feels like it's not a significant burden to me?
Comment 3 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-12-16 09:35:40 PST
Comment on attachment 582009 [details] [diff] [review]
Utilities for promises

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

Thanks!

::: browser/devtools/shared/Promise.jsm
@@ +214,5 @@
>    return groupPromise;
>  };
> +
> +/**
> + * Create a promise that is already resolved.

I didn't include these because you can do:

  return new Promise().resolve(x);

and

  return new Promise().reject(x);

@@ +232,5 @@
> +  return promise;
> +};
> +
> +/**
> + * Handle errors.

Neither of these function are ones I've ever wanted. Am I right in thinking that you have a concrete place where you want to do this, or is this more theoretical?
Comment 4 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-12-16 09:41:56 PST
Comment on attachment 582013 [details] [diff] [review]
Promises with progress

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

Back for more!

We don't specify the type of the progress data. I think that's probably OK - however commonly it will be a percentage complete or similar - I wonder how often a progress listener will be given an arbitrary promise to 'watch' and want some idea of what it's getting.

I have one use-case for this, and it would need some guarantee of the semantics of the progress data. But that could be convention...

::: browser/devtools/shared/Promise.jsm
@@ +159,5 @@
> + *
> + * @param {Function} aHandler A function to inform in case of progress.
> + * @return {Promise} |this|
> + */
> +Promise.prototype.stream = function(aHandler)

Wouldn't onProgress be a better name? stream() doesn't jump to mind as the right name.
Comment 5 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2011-12-16 10:53:05 PST
(In reply to Joe Walker from comment #2)
> Comment on attachment 582013 [details] [diff] [review]
> Promises with progress
> 
> Review of attachment 582013 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I like the changes.
> 
> The big question to me is - do we aim to maintain web compatibility or do we
> fork. It feels as though currently there isn't a significant advantage to
> forking.

I have no objection to keeping it compatible.

> ::: browser/devtools/shared/Promise.jsm
> @@ +144,5 @@
> > + * @param {*} aData The value to pass to handlers.
> > + * @return {Promise} |this|
> > + */
> > +Promise.prototype.progress = function(aData) {
> > +  for each(let handler in this._onProgressHandlers) {
> 
> Promise as is stands is used on the web in GCLI, so either we fork, or we
> use:
> 
>   for (var i = 0; i < this._onProgressHandlers.length; i++) {
> 
> The latter feels like it's not a significant burden to me?

I have no issue with this.
Comment 6 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2011-12-16 10:56:09 PST
(In reply to Joe Walker from comment #3)
> Comment on attachment 582009 [details] [diff] [review]
> Utilities for promises
> 
> Review of attachment 582009 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks!
> 
> ::: browser/devtools/shared/Promise.jsm
> @@ +214,5 @@
> >    return groupPromise;
> >  };
> > +
> > +/**
> > + * Create a promise that is already resolved.
> 
> I didn't include these because you can do:
> 
>   return new Promise().resolve(x);
> 
> and
> 
>   return new Promise().reject(x);

Good point. Let's forget about these.

> @@ +232,5 @@
> > +  return promise;
> > +};
> > +
> > +/**
> > + * Handle errors.
> 
> Neither of these function are ones I've ever wanted. Am I right in thinking
> that you have a concrete place where you want to do this, or is this more
> theoretical?

I am actually using them. If you are patient, you can find an example at:
http://hg.mozilla.org/users/dteller_mozilla.com/perf.api.file/file/da51dea6fe93/tmp.reorderme

(search "makeUsing" - with these changes, the code is much clearer)


> We don't specify the type of the progress data. I think that's probably OK -
> however commonly it will be a percentage complete or similar - I wonder how
> often a progress listener will be given an arbitrary promise to 'watch' and
> want some idea of what it's getting.

From what I understand, e10s is planning using something-like-promises-with-progress to monitor cross-process events. This would need investigations.

> I have one use-case for this, and it would need some guarantee of the
> semantics of the progress data. But that could be convention...

I am not sure I understand.

> ::: browser/devtools/shared/Promise.jsm
> @@ +159,5 @@
> > + *
> > + * @param {Function} aHandler A function to inform in case of progress.
> > + * @return {Promise} |this|
> > + */
> > +Promise.prototype.stream = function(aHandler)
> 
> Wouldn't onProgress be a better name? stream() doesn't jump to mind as the
> right name.

I have no issue with |onProgress|.
Comment 7 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2011-12-17 04:36:57 PST
> > Neither of these function are ones I've ever wanted. Am I right in thinking
> > that you have a concrete place where you want to do this, or is this more
> > theoretical?
> 
> I am actually using them. If you are patient, you can find an example at:
> http://hg.mozilla.org/users/dteller_mozilla.com/perf.api.file/file/
> da51dea6fe93/tmp.reorderme
> 
> (search "makeUsing" - with these changes, the code is much clearer)

Or something more readable at
https://gist.github.com/1481186
Comment 8 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-01-05 03:42:10 PST
Created attachment 586030 [details] [diff] [review]
Utilities for promises
Comment 9 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-01-05 03:43:15 PST
Created attachment 586031 [details] [diff] [review]
Logging uncaught rejections
Comment 10 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-01-24 01:13:54 PST
Created attachment 591023 [details] [diff] [review]
Utilities for promises
Comment 11 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-01-24 01:43:42 PST
Comment on attachment 591023 [details] [diff] [review]
Utilities for promises

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

Please could you s/let/var/g so we can continue to have something that will work largely unchanged in a browser?
Comment 12 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-01-24 01:47:28 PST
Created attachment 591033 [details] [diff] [review]
Utilities for promises
Comment 13 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-01-24 02:08:33 PST
Comment on attachment 591033 [details] [diff] [review]
Utilities for promises

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

Thanks.
Comment 14 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-01-25 02:46:45 PST
Created attachment 591399 [details] [diff] [review]
Utilities for promises
Comment 15 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-01-25 02:47:58 PST
Created attachment 591400 [details] [diff] [review]
Companion testsuite
Comment 16 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-01-25 03:17:21 PST
Comment on attachment 591400 [details] [diff] [review]
Companion testsuite

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

::: browser/devtools/shared/test/browser_promise_basic.js
@@ +226,5 @@
> +      throw new Error();
> +    }
> +  ).chainPromise(//Promise rejected, should not be executed
> +    function() {
> +      ok(false, "This should not be executed");

Not an important point, just something to think about - you could return something random here:

return 'should be ignored';

Just to check that it is ignored.
Also the following line has a incorrect indentation.

Reviews can pick over minor details though, so I'm totally happy for you to ignore both comments.
Comment 17 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-01-25 03:27:27 PST
Comment on attachment 591399 [details] [diff] [review]
Utilities for promises

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

Point for discussion:

  var complete = false;
  var p = new Promise();

  p.then(function() {
    complete = true;
  });

  p.resolve();

  console.log(complete);

Question: what gets logged? The current implementation prints true (IIRC)

But it's implementation dependent - it requires:
- resolve to happen before the the log, in most cases resolve is not so close to the readout
- the implementation of then to be asynchronous

Q takes the (IMHO) reasonable opinion that the point of Promise is to handle asynchronisity, so it should be predictably asynch, and not allow the user to make assumptions (which could later be false) that something is synchronous.

The solution would be to make p.resolve/etc use setTimeout before calling the callbacks.

What do you think?
Comment 18 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-01-25 03:30:12 PST
Actually, I have implemented asynchronicity as a subclass of |Promise|, called |Schedule| (see bug JSScheduleAPI). I would be glad to merge (back) |Promise| and |Schedule|, if there is any interest.
Comment 19 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-02-03 09:58:47 PST
Comment on attachment 586031 [details] [diff] [review]
Logging uncaught rejections

Moved this to bug 720628.
Comment 20 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-02-03 09:59:11 PST
Created attachment 594225 [details] [diff] [review]
Companion testsuite
Comment 21 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-02-03 10:00:35 PST
Created attachment 594226 [details] [diff] [review]
Utilities for promises
Comment 22 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-02-03 10:12:06 PST
Created attachment 594232 [details] [diff] [review]
Companion testsuite
Comment 23 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-02-13 14:30:36 PST
*** Bug 659300 has been marked as a duplicate of this bug. ***
Comment 24 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-02-13 14:32:49 PST
Created attachment 596789 [details] [diff] [review]
promise changes
Comment 25 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-02-13 14:39:14 PST
Here are the main promise bugs, and their status:

* Bug 711126 - Extend Promise.jsm
  [https://bugzilla.mozilla.org/show_bug.cgi?id=711126]
  * Attachment 594226 [details] [diff] - trap/always
    [https://bug711126.bugzilla.mozilla.org/attachment.cgi?id=594226]
    -> Included in path 596789
  * Attachment 594232 [details] [diff] - tests for trap/always
    [https://bug711126.bugzilla.mozilla.org/attachment.cgi?id=594232]
    -> Included in path 596789

* Bug 720628 - Improve error reporting of Promise.jsm
  [https://bugzilla.mozilla.org/show_bug.cgi?id=720628]
  * Attachment 591035 [details] [diff] - Error reporting
    [https://bug720628.bugzilla.mozilla.org/attachment.cgi?id=591035]
    -> Included in path 596789
    -> Marked depending on bug 711126

* Bug 659300 - GCLI's promise should enable progress feedback
  [https://bugzilla.mozilla.org/show_bug.cgi?id=659300]
  -> Marked duplicate of Bug 711126

* Bug 708984 - Promote Promise.jsm from developer tools to browser/platform 
  [https://bugzilla.mozilla.org/show_bug.cgi?id=708984]
  -> Needs work

In addition the latest patch completes asynchronously, and as if to prove that it needs doing, it uncovered a bug in the template test suite.

There is one more thing that needs doing before we push - we need to fix a memory leak. I've narrowed it down to browser_promise_basic.js / testTrap(). But I'm at a loss to know what's up. Might you have chance to have a look?
Comment 26 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-02-13 14:40:01 PST
Clearly s/path/patch/g in the above comment.
Comment 27 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-02-16 03:57:11 PST
Comment on attachment 596789 [details] [diff] [review]
promise changes

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

::: browser/devtools/shared/Promise.jsm
@@ +157,5 @@
> +    Promise._error(data);
> +    var frame;
> +    if (this._trace) {
> +      Promise._error("Original trace");
> +      Promise._error(this._trace);

Could we group both as one line?
Promise._error("Original trace", this._trace)

@@ +165,5 @@
> +      Promise._error("Printing original stack");
> +      for (frame = data.stack; frame; frame = frame.caller) {
> +        Promise._error(frame);
> +      }
> +    }

I know that I am the one who introduced these lines, but on second thought, we should probably protect all of this within a try / catch block, in case |frame.caller| is undefined or otherwise weird.

@@ +192,5 @@
>  
> +    // Call all the handlers, and then delete them
> +    list.forEach(function(handler) {
> +      handler.call(null, this._value);
> +    }, this);

A question comes to me: perhaps we should try/catch around that call? This would improve the robustness of Promise.

@@ +301,5 @@
> + * Minimal debugging.
> + */
> +Promise.prototype.toString = function() {
> +  return "[Promise " + this._id + "]";
> +};

Let's also return the status here.

@@ +348,5 @@
> +
> +/**
> + * This implementation of promise also runs in a browser.
> + * Promise._setTimeout allows us to have |then()| act asynchronously
> + */

I do not understand that comment.

@@ +381,5 @@
> +                          .createInstance(Components.interfaces.nsITimer);
> +    timer.initWithCallback(new TimerCallback(callback), delay, timer.TYPE_ONE_SHOT);
> +  };
> +})();
> +

I do not understand the above function call. Won't this be invoked both in web and chrome context? If so, what is the point?

Shouldn't we do some feature detection (e.g. is |Components.utils| defined) to determine how to initialize Promise._error and Promise._setTimeout?

@@ +390,5 @@
> + */
> +Promise._error = function(message) {
> +  dump(message);
> +  dump("\n");
> +};

I do not think that this definition of |Promise._error| is compatible with some of the uses of |Promise._error| above, which pass several arguments.
Comment 28 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-02-24 07:00:06 PST
Created attachment 600380 [details] [diff] [review]
Utilities for promises

Attaching an updated version. This integrates an extended version of function |Promise._error|, as introduced by Joe, as well as my own suggestions to his patch. For the moment, I have not attempted to introduce |setTimeout|. Also, I introduce a condition |Promise.Debug._debug| that determines whether all the debugging information should be maintained. This should be nicer than commenting out code.
Comment 29 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-02-24 07:01:23 PST
Created attachment 600381 [details] [diff] [review]
Companion testsuite

Attaching a version of the suite that runs the set of tests once in non-debug mode and once in debug mode.
Comment 30 Rob Campbell [:rc] (:robcee) 2012-02-27 10:57:03 PST
Comment on attachment 600380 [details] [diff] [review]
Utilities for promises

+ * If the console is available, this method uses |console.warn|. Otherwise,
+ * this methods falls back to |dump|.

nit: this method (singluar) falls back to |dump|

+  Promise._error = function() {
+    var args = Array.prototype.splice.call(arguments);

this'll return an empty array. Did you mean Array.prototype.slice?

+      dump(arguments[i]);
+      dump(" ");

you can combine these by just adding + " ", e.g.,

dump(arguments[i] + " ");

otherwise ok by me. r- for the splice/splice thing.
Comment 31 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-02-27 11:47:28 PST
Created attachment 600994 [details] [diff] [review]
Utilities for promises

Oops - and thanks for the review
Comment 32 Rob Campbell [:rc] (:robcee) 2012-03-05 05:39:11 PST
Comment on attachment 600381 [details] [diff] [review]
Companion testsuite

r+ with a successful try run.
Comment 33 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-03-09 01:53:59 PST
Comment on attachment 600381 [details] [diff] [review]
Companion testsuite

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

::: browser/devtools/shared/test/browser_promise_basic.js
@@ +281,5 @@
> +  switch (configurationTestComplete) {
> +  case 0:
> +    info("Finished run in non-debug mode");
> +    configurationTestComplete = 1;
> +    Promise.Debug.setDebug(true);

My copy of Promise doesn't have a Promise.Debug - please could you point me at what I should be reading?
Comment 34 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-03-09 02:03:29 PST
See bug 720628
Comment 35 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-03-09 04:49:45 PST
Ah thanks. r+
Comment 36 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-03-13 15:23:49 PDT
Comment on attachment 600381 [details] [diff] [review]
Companion testsuite

Thanks Joe.

For better clarity, moving this patch to bug 735477 and launching one last TryServer test.
Comment 37 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-03-14 10:18:34 PDT
Created attachment 605830 [details] [diff] [review]
Utilities for promises
Comment 38 Ryan VanderMeulen [:RyanVM] 2012-03-14 15:03:07 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/0975cabb34e6
Comment 39 Marco Bonardo [::mak] 2012-03-15 08:18:28 PDT
https://hg.mozilla.org/mozilla-central/rev/0975cabb34e6

Note You need to log in before you can comment on or make changes to this bug.