Promises: some adaptations to spec

Promises: some adaptations to spec

- Rename Promise.{resolved,rejected,deferred} to Promise.{resolve,reject,defer}
- Rename Promise.one to Promise.race
- Make all failures asynchronous, EXCEPT type errors for resolver
- Disallow non-construct call to Promise constructor
- Don't make combinators go through public this.defer

Also, don't bother using IsCallable.

R=dslomov@chromium.org, yhirano@chromium.org
BUG=

Committed: https://code.google.com/p/v8/source/detail?r=18515

[Dmitry Lomov (no reviews), 2013-12-04 09:24:42]

Mostly stylistic comments + objection to shipping deref/relying on defer in
tests that might be addressed in a separate CL

https://codereview.chromium.org/99573002/diff/1/src/promise.js
File src/promise.js (right):

https://codereview.chromium.org/99573002/diff/1/src/promise.js#newcode230
src/promise.js:230: typeof (then = x.then) === 'function') {
Can we not have assignments in conditions? Pretty please? Replace with
  if (IS_NULL_OR_UNDEFINED(x)) return x;
  var then = x.then;
  if (typeof(then) !== 'function') return x;

https://codereview.chromium.org/99573002/diff/1/src/promise.js#newcode257
src/promise.js:257: var deferred = %_CallFunction(this, PromiseDeferred);
Instead of using %_CallFunction here and above, let's have utility method
PromiseDeferredImpl, replace these calls with PromiseDeferredImpl(this), and
   Promise.defer = function(this) { PromiseDeferredImpl(this); }
Even better, I suggest not shipping defer (see below) :)

https://codereview.chromium.org/99573002/diff/1/src/promise.js#newcode270
src/promise.js:270: function(r) { deferred.reject(r) }
Why 'count > 0' condition disappeared here? What if more than one of values
fail?

https://codereview.chromium.org/99573002/diff/1/src/promise.js#newcode281
src/promise.js:281: var deferred = %_CallFunction(this, PromiseDeferred);
Ditto re: %_CallFunction

https://codereview.chromium.org/99573002/diff/1/src/promise.js#newcode301
src/promise.js:301: "defer", PromiseDeferred,
So, my understanding is that 'defer' is non-standard as of now? Let's not ship
non-standard API maybe? (maybe a separate CL)

https://codereview.chromium.org/99573002/diff/1/test/mjsunit/harmony/promises.js
File test/mjsunit/harmony/promises.js (right):

https://codereview.chromium.org/99573002/diff/1/test/mjsunit/harmony/promises...
test/mjsunit/harmony/promises.js:343: var deferred = Promise.defer()
Non necessary to fix in this CL, but I think it would be useful to make this
whole test more portable by not relying on non-standard Promise.defer() API. Use
a "user-land" deferred implementation for the test.

[rossberg, 2013-12-06 11:43:29]

https://codereview.chromium.org/99573002/diff/1/src/promise.js
File src/promise.js (right):

https://codereview.chromium.org/99573002/diff/1/src/promise.js#newcode230
src/promise.js:230: typeof (then = x.then) === 'function') {
On 2013/12/04 09:24:43, Dmitry Lomov (chromium) wrote:
> Can we not have assignments in conditions? Pretty please? Replace with
>   if (IS_NULL_OR_UNDEFINED(x)) return x;
>   var then = x.then;
>   if (typeof(then) !== 'function') return x;

Done.

https://codereview.chromium.org/99573002/diff/1/src/promise.js#newcode257
src/promise.js:257: var deferred = %_CallFunction(this, PromiseDeferred);
On 2013/12/04 09:24:43, Dmitry Lomov (chromium) wrote:
> Instead of using %_CallFunction here and above, let's have utility method
> PromiseDeferredImpl, replace these calls with PromiseDeferredImpl(this), and
>    Promise.defer = function(this) { PromiseDeferredImpl(this); }
> Even better, I suggest not shipping defer (see below) :)

Why? Avoiding extra hoops like that is exactly why we have %_CallFunction (which
really just is a safe and efficient version of f.call).

https://codereview.chromium.org/99573002/diff/1/src/promise.js#newcode270
src/promise.js:270: function(r) { deferred.reject(r) }
On 2013/12/04 09:24:43, Dmitry Lomov (chromium) wrote:
> Why 'count > 0' condition disappeared here? What if more than one of values
> fail?

Reject used to throw on non-pending promises in an earlier version. For legacy
compatibility, I already changed that to just ignore the attempt, so the extra
check here became redundant (as in PromiseOne below).

https://codereview.chromium.org/99573002/diff/1/src/promise.js#newcode301
src/promise.js:301: "defer", PromiseDeferred,
On 2013/12/04 09:24:43, Dmitry Lomov (chromium) wrote:
> So, my understanding is that 'defer' is non-standard as of now? Let's not ship
> non-standard API maybe? (maybe a separate CL)

As per off-line discussion, I leave it in for the time being.

[Dmitry Lomov (no reviews), 2013-12-06 11:49:10]

lgtm

[yhirano, 2013-12-09 02:31:35]

https://codereview.chromium.org/99573002/diff/20001/src/promise.js
File src/promise.js (right):

https://codereview.chromium.org/99573002/diff/20001/src/promise.js#newcode63
src/promise.js:63: function Promise(resolver) {
I'm still not sure.
The spec has not been changed at this point, has it?

[rossberg, 2013-12-10 10:11:14]

https://codereview.chromium.org/99573002/diff/20001/src/promise.js
File src/promise.js (right):

https://codereview.chromium.org/99573002/diff/20001/src/promise.js#newcode63
src/promise.js:63: function Promise(resolver) {
On 2013/12/09 02:31:36, yhirano wrote:
> I'm still not sure.
> The spec has not been changed at this point, has it?

No, but the spec requires that exceptions from the function itself are delayed,
so this CL makes it "more conforming" at least. An extra type check would be
needed to implement the synchronous type error.

[arv (Not doing code reviews), 2013-12-10 14:21:24]

I didn't see any discussion regarding the race/one function name.

https://codereview.chromium.org/99573002/diff/20001/src/promise.js
File src/promise.js (right):

https://codereview.chromium.org/99573002/diff/20001/src/promise.js#newcode303
src/promise.js:303: "one", PromiseOne,
This should be race

[yhirano, 2013-12-11 04:46:31]

https://codereview.chromium.org/99573002/diff/20001/src/promise.js
File src/promise.js (right):

https://codereview.chromium.org/99573002/diff/20001/src/promise.js#newcode63
src/promise.js:63: function Promise(resolver) {
On 2013/12/10 10:11:15, rossberg wrote:
> On 2013/12/09 02:31:36, yhirano wrote:
> > I'm still not sure.
> > The spec has not been changed at this point, has it?
> 
> No, but the spec requires that exceptions from the function itself are
delayed,
> so this CL makes it "more conforming" at least. An extra type check would be
> needed to implement the synchronous type error.
Then can you write a comment?

[rossberg, 2014-01-09 12:51:43]

https://codereview.chromium.org/99573002/diff/20001/src/promise.js
File src/promise.js (right):

https://codereview.chromium.org/99573002/diff/20001/src/promise.js#newcode63
src/promise.js:63: function Promise(resolver) {
On 2013/12/11 04:46:31, yhirano wrote:
> On 2013/12/10 10:11:15, rossberg wrote:
> > On 2013/12/09 02:31:36, yhirano wrote:
> > > I'm still not sure.
> > > The spec has not been changed at this point, has it?
> > 
> > No, but the spec requires that exceptions from the function itself are
> delayed,
> > so this CL makes it "more conforming" at least. An extra type check would be
> > needed to implement the synchronous type error.
> Then can you write a comment?

Changed to throw

https://codereview.chromium.org/99573002/diff/20001/src/promise.js#newcode303
src/promise.js:303: "one", PromiseOne,
On 2013/12/10 14:21:24, arv wrote:
> This should be race

Done.

[rossberg, 2014-01-09 13:01:07]

Committed patchset #4 manually as r18515 (presubmit successful).

[arv (Not doing code reviews), 2014-01-09 15:47:15]

https://codereview.chromium.org/99573002/diff/90001/src/promise.js
File src/promise.js (right):

https://codereview.chromium.org/99573002/diff/90001/src/promise.js#newcode65
src/promise.js:65: if (!%_IsConstructCall()) throw
MakeTypeError('not_a_promise', [this]);
Doesn't this break sub classing?

function SubPromise(resolver) {
  Promise.call(this, resolver);
}
SubPromise.prototype = {
  __proto__: Promise.prototype,
  constructor: SubPromise
};

https://codereview.chromium.org/99573002/diff/90001/src/promise.js#newcode280
src/promise.js:280: function PromiseOne(values) {
Rename this to PromiseRace to match?

Why isn't the done check needed any more?

https://codereview.chromium.org/99573002/diff/90001/src/promise.js#newcode309
src/promise.js:309: "chain", PromiseChain,
Any plan to upstream chain into the spec?

[rossberg, 2014-01-09 16:09:56]

https://codereview.chromium.org/99573002/diff/90001/src/promise.js
File src/promise.js (right):

https://codereview.chromium.org/99573002/diff/90001/src/promise.js#newcode65
src/promise.js:65: if (!%_IsConstructCall()) throw
MakeTypeError('not_a_promise', [this]);
On 2014/01/09 15:47:15, arv wrote:
> Doesn't this break sub classing?
> 
> function SubPromise(resolver) {
>   Promise.call(this, resolver);
> }
> SubPromise.prototype = {
>   __proto__: Promise.prototype,
>   constructor: SubPromise
> };

I agree, but it's what the spec says.

https://codereview.chromium.org/99573002/diff/90001/src/promise.js#newcode280
src/promise.js:280: function PromiseOne(values) {
On 2014/01/09 15:47:15, arv wrote:
> Rename this to PromiseRace to match?
> 
> Why isn't the done check needed any more?

Because the resolve/reject functions at some point changed to sloppily ignore
that case, so it became redundant.

https://codereview.chromium.org/99573002/diff/90001/src/promise.js#newcode309
src/promise.js:309: "chain", PromiseChain,
On 2014/01/09 15:47:15, arv wrote:
> Any plan to upstream chain into the spec?

Yes, but current stakeholders refuse to consider it, and it fell off the agenda
of the last meeting. :(

[arv (Not doing code reviews), 2014-01-09 19:13:27]

https://codereview.chromium.org/99573002/diff/90001/src/promise.js
File src/promise.js (right):

https://codereview.chromium.org/99573002/diff/90001/src/promise.js#newcode65
src/promise.js:65: if (!%_IsConstructCall()) throw
MakeTypeError('not_a_promise', [this]);
On 2014/01/09 16:09:56, rossberg wrote:
> On 2014/01/09 15:47:15, arv wrote:
> > Doesn't this break sub classing?
> > 
> > function SubPromise(resolver) {
> >   Promise.call(this, resolver);
> > }
> > SubPromise.prototype = {
> >   __proto__: Promise.prototype,
> >   constructor: SubPromise
> > };
> 
> I agree, but it's what the spec says.

I think this is the right call for now. Once we support @@create we should be
able to fix this part to check the internal slots as required by the spec.