711126 - Extend Promise.jsm

Status: RESOLVED FIXED

(Core :: General, enhancement)

Description

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

Attachment: 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 Teller [:Yoric] - still alive but not very active]

Attachment: 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]]

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

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

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 Teller [:Yoric] - still alive but not very active]

(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 Teller [:Yoric] - still alive but not very active]

(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 Teller [:Yoric] - still alive but not very active]

> > 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 Teller [:Yoric] - still alive but not very active]

Attachment: Utilities for promises

Comment 9

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

Attachment: Logging uncaught rejections

Comment 10

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

Attachment: Utilities for promises

Comment 11

[Joe Walker [:jwalker]]

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 Teller [:Yoric] - still alive but not very active]

Attachment: Utilities for promises

Comment 13

[Joe Walker [:jwalker]]

Comment on attachment 591033 [details] [diff] [review]
Utilities for promises

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

Thanks.

Comment 14

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

Attachment: Utilities for promises

Comment 15

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

Attachment: Companion testsuite

Comment 16

[Joe Walker [:jwalker]]

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

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 Teller [:Yoric] - still alive but not very active]

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 Teller [:Yoric] - still alive but not very active]

Comment on attachment 586031 [details] [diff] [review]
Logging uncaught rejections

Moved this to bug 720628.

Comment 20

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

Attachment: Companion testsuite

Comment 21

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

Attachment: Utilities for promises

Comment 22

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

Attachment: Companion testsuite

Comment 24

[Joe Walker [:jwalker]]

Attachment: promise changes

Comment 25

[Joe Walker [:jwalker]]

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

Clearly s/path/patch/g in the above comment.

Comment 27

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

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 Teller [:Yoric] - still alive but not very active]

Attachment: 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 Teller [:Yoric] - still alive but not very active]

Attachment: 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)]

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 Teller [:Yoric] - still alive but not very active]

Attachment: Utilities for promises

Oops - and thanks for the review

Comment 32

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 600381 [details] [diff] [review]
Companion testsuite

r+ with a successful try run.

Comment 33

[Joe Walker [:jwalker]]

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 Teller [:Yoric] - still alive but not very active]

See bug 720628

Comment 35

[Joe Walker [:jwalker]]

Ah thanks. r+

Comment 36

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

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 Teller [:Yoric] - still alive but not very active]

Attachment: Utilities for promises

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0975cabb34e6

Comment 39

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/0975cabb34e6