Promise.jsm is great and all but we're going to have trouble converting existing code to the JSM without some of the basic helpers that deal with a collection of promises. When converting about:newtab to Promise.jsm I was blocked by not having Promise.every() so that's what I started with. The implementation of Promise.every() in this patch adheres to the DOM spec: http://dom.spec.whatwg.org/#dom-promise-every

Comment on attachment 777458 [details] [diff] [review] Implement Promise.every() I think the "every" function can be a useful helper, and the fact that it is part of the specification makes it a candidate for implementation directly in the "Promise.jsm" module. However, we don't want a "synchronous" callback implementation here. This doesn't give any additional value to the Chrome code that's using this module. So, we should implement the function in a very simple way, on top of the other existing public functions (read "defer"), without using any of the internals.

Ok, sounds good to me. I agree with you about the sync flag, I was just sticking to the spec :)

So, is this a substitute for Promise.all from the SDK module?

(In reply to Paolo Amadini [:paolo] from comment #3) > So, is this a substitute for Promise.all from the SDK module? Yes, this should do the same.

Comment on attachment 777719 [details] [diff] [review] Implement Promise.every(), v2 Review of attachment 777719 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/modules/Promise.jsm @@ +177,5 @@ > + * Returns a promise that is resolved or rejected when all values are > + * resolved or any is rejected. > + * > + * @param aValues > + * List of promises that may be pending, resolved, or rejected. When List of -> Array of Do we support array-like objects as well? If any of the elements in the array is not a promise, what should we do? Should we forward it as a resolution value, throw an exception, or reject the returned promise? (We'll need tests for that) @@ +182,5 @@ > + * all are resolved or any is rejected, the returned promise will be > + * resolved or rejected as well. > + * > + * @return A new promise that is fulfilled when all values are resolved or > + * that is reject when any of the values are rejected. Please specify which resolution value and rejection reason the returned promise assume. @@ +188,5 @@ > + every: function (aValues) > + { > + if (!aValues.length) { > + return Promise.resolve(undefined); > + } I wonder whether we should explicitly throw a descriptive exception if the provided argument is not array-like? @@ +193,5 @@ > + > + let index = 0; > + let countdown = aValues.length; > + let deferred = Promise.defer(); > + let args = new Array(countdown); nit: args -> resolutionValues @@ +196,5 @@ > + let deferred = Promise.defer(); > + let args = new Array(countdown); > + > + for (let value of aValues) { > + let currentIndex = index++; A classic "for" loop seems simpler here. Are there reasons for using enumeration? @@ +199,5 @@ > + for (let value of aValues) { > + let currentIndex = index++; > + > + value.then(function (aArgument) { > + args[currentIndex] = aArgument; nit: aArgument -> aValue

David, can you take a look at comment 6 and tell us what you think makes most sense? Dave, if you can also look at this, we'll need super-review on the API.

sr+

(In reply to Paolo Amadini [:paolo] from comment #6) > If any of the elements in the array is not a promise, what should we do? > Should we forward it as a resolution value, throw an exception, or reject > the returned promise? (We'll need tests for that) I would forward it as a resolution value. This is consistent with the natural implementation of |every| in Task.jsm: otherImplementationOfEvery = function(promises) { return Task.spawn(function() { let values = []; for (let x of promises) { values.push((yield x)); } throw new Task.Result(values); }); }

(In reply to Paolo Amadini [:paolo] from comment #6) > If any of the elements in the array is not a promise, what should we do? > Should we forward it as a resolution value, throw an exception, or reject > the returned promise? (We'll need tests for that) I took Yoric's suggestions and forwarded the value. Added tests. > I wonder whether we should explicitly throw a descriptive exception if the > provided argument is not array-like? Added. > > + for (let value of aValues) { > > + let currentIndex = index++; > > A classic "for" loop seems simpler here. Are there reasons for using > enumeration? It's not really simpler. It's just different :) Changed it to a classic for loop anyway.

Comment on attachment 778445 [details] [diff] [review] Implement Promise.every(), v3 Review of attachment 778445 [details] [diff] [review]: ----------------------------------------------------------------- Looks OK, with the comment change below. ::: toolkit/modules/Promise.jsm @@ +184,5 @@ > + * > + * @return A new promise that is fulfilled when all values are resolved or > + * that is rejected when any of the values are rejected. Its > + * resolution value will be an array of all resolved values in the > + * given order. The reject reason will be forwarded from the first "Its resolution value will be an array of all resolved values in the given order, or undefined if aValues is an empty array." (for what it's worth, I think this inconsistent behavior with empty arrays is a bug in the specification, but we have to live with it.) Do you think we may have issues when converting code from "Promise.all" to "Promise.every" because of this behavior?

(In reply to Paolo Amadini [:paolo] from comment #11) > Do you think we may have issues when converting code from "Promise.all" to > "Promise.every" because of this behavior? I can't imagine that we have too many cases where we actually pass an empty list to Promise.all/every and expect it to have a meaningful outcome. I think we should rather stick to the spec, that should give us less problems in the future. Converting a few occurrences of the old code, if any, should not be too much work. I think it would be helpful to add this as a note to bugs that switch to the new Promise.jsm.

We got this wrong, see the example here: http://dom.spec.whatwg.org/#i-promise-you-an-introduction The spec is not crystal clear, when it mentions "values" and "arguments" it means the expanded function parameters, not arrays. We can't implement that at present, because we never invoke resolution callbacks with multiple arguments (and I'm not sure we should in the future, the Promise.jsm API is already quite different from DOM Promises). To solve this, we may just rename "every" back to "all" (to clarify that we are using a different interface) and fix the issue of resolving to "undefined" by resolving to an empty array if the given array is empty in the first place. (It made sense to say "undefined" in the DOM Promises specification because it actually meant an empty argument list.)

Comment on attachment 783738 [details] [diff] [review] Rename Promise.every() to .all() to reduce confusion about our differing implementation Thanks!