Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2695753002:
[ESnext] Implement Promise.prototype.finally (Closed)
Description[ESnext] Implement Promise.prototype.finally
Adds five new TF builtins for the spec defined functions/closures. This follows
mechanism similar to promise resolving functions approach where we store the
closure variables in a custom context.
Adds a new --harmony-promise-finally flag.
BUG=v8:5967
Review-Url: https://codereview.chromium.org/2695753002
Cr-Commit-Position: refs/heads/master@{#43294}
Committed: https://chromium.googlesource.com/v8/v8/+/18ad0f13afeaabff4e035fddd9edc3d319152160
Patch Set 1 #Patch Set 2 : rebase #Patch Set 3 : add flag #Patch Set 4 : add tests #Patch Set 5 : naive implementation #Patch Set 6 : fixes #Patch Set 7 : remove unused var #Patch Set 8 : rebase #Patch Set 9 : add comments #
Total comments: 40
Patch Set 10 : add tests #
Total comments: 2
Patch Set 11 : rebase #Patch Set 12 : fixes #
Total comments: 5
Patch Set 13 : custom then #Patch Set 14 : customthen2 #Patch Set 15 : ffs #
Total comments: 5
Patch Set 16 : Fixes #Patch Set 17 : Rename GotoUnless #
Messages
Total messages: 58 (43 generated)
The CQ bit was checked by gsathya@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [wip] implement Promise.prototype.finally BUG= ========== to ========== [ESnext] Implement Promise.prototype.finally Adds five new TF builtins for the spec defined functions/closures. This follows mechanism similar to promise resolving functions approach where we store the closure variables in a custom context. Adds a new --harmony-promise-finally flag. BUG=v8:5967 ==========
gsathya@chromium.org changed reviewers: + adamk@chromium.org, neis@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
Description was changed from ========== [ESnext] Implement Promise.prototype.finally Adds five new TF builtins for the spec defined functions/closures. This follows mechanism similar to promise resolving functions approach where we store the closure variables in a custom context. Adds a new --harmony-promise-finally flag. BUG=v8:5967 ========== to ========== [ESnext] Implement Promise.prototype.finally Adds five new TF builtins for the spec defined functions/closures. This follows mechanism similar to promise resolving functions approach where we store the closure variables in a custom context. Adds a new --harmony-promise-finally flag. BUG=v8:5967 ==========
The CQ bit was checked by gsathya@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by gsathya@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by gsathya@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL. Will add more tests tomorrow.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/20914) v8_linux_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng_triggered/b...)
The CQ bit was checked by gsathya@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/20942) v8_linux_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng_triggered/b...)
Looks pretty good overall! https://codereview.chromium.org/2695753002/diff/150001/src/bootstrapper.cc File src/bootstrapper.cc (right): https://codereview.chromium.org/2695753002/diff/150001/src/bootstrapper.cc#ne... src/bootstrapper.cc:3647: native_context()->set_promise_then_finally_shared_fun(*info); Nit: put this block in { }. Same below. Should we mark (some of) these SFIs as native? https://codereview.chromium.org/2695753002/diff/150001/src/builtins/builtins-... File src/builtins/builtins-promise.cc (right): https://codereview.chromium.org/2695753002/diff/150001/src/builtins/builtins-... src/builtins/builtins-promise.cc:1606: Node* PromiseBuiltinsAssembler::CreateValueThunkFunction(Node* value, nit: s/Function// https://codereview.chromium.org/2695753002/diff/150001/src/builtins/builtins-... src/builtins/builtins-promise.cc:1620: Node* const parent = Parameter(0); This will typically be undefined. https://codereview.chromium.org/2695753002/diff/150001/src/builtins/builtins-... src/builtins/builtins-promise.cc:1622: Node* const context = Parameter(4); Maybe use CSA_ASSERT_JS_ARGC_EQ here and elsewhere? https://codereview.chromium.org/2695753002/diff/150001/src/builtins/builtins-... src/builtins/builtins-promise.cc:1667: Node* native_context) { nit: s/Function// https://codereview.chromium.org/2695753002/diff/150001/src/builtins/builtins-... src/builtins/builtins-promise.cc:1681: Node* const value = Parameter(1); nit: s/value/reason/ https://codereview.chromium.org/2695753002/diff/150001/src/builtins/builtins-... src/builtins/builtins-promise.cc:1707: Return(result); Ouch! Return(promise_capability) This suggests a lack of test coverage. https://codereview.chromium.org/2695753002/diff/150001/src/builtins/builtins-... src/builtins/builtins-promise.cc:1729: // 3. Let thenFinally be ! CreateThenFinally(onFinally). The few lines above are already part of steps 3 and 4. https://codereview.chromium.org/2695753002/diff/150001/src/builtins/builtins-... src/builtins/builtins-promise.cc:1744: // 1. If IsCallable(onFinally) is not true, return onFinally. I find these comments here confusing, since they are from a different function in the spec. (in particular we do not return from PromiseFinally) https://codereview.chromium.org/2695753002/diff/150001/src/builtins/builtins-... src/builtins/builtins-promise.cc:1751: GotoIf(TaggedIsSmi(promise), &if_custompromise); We already know that promise is a JS_PROMISE object, so it can't be a Smi. https://codereview.chromium.org/2695753002/diff/150001/src/builtins/builtins-... src/builtins/builtins-promise.cc:1753: The step 5 comment should appear here. https://codereview.chromium.org/2695753002/diff/150001/src/flag-definitions.h File src/flag-definitions.h (right): https://codereview.chromium.org/2695753002/diff/150001/src/flag-definitions.h... src/flag-definitions.h:208: V(harmony_promise_finally, "harmony promise finally") "harmony Promise.prototype.finally" is more useful https://codereview.chromium.org/2695753002/diff/150001/test/mjsunit/harmony/p... File test/mjsunit/harmony/promise-prototype-finally.js (right): https://codereview.chromium.org/2695753002/diff/150001/test/mjsunit/harmony/p... test/mjsunit/harmony/promise-prototype-finally.js:50: }) Is this .then call useful at all? https://codereview.chromium.org/2695753002/diff/150001/test/mjsunit/harmony/p... test/mjsunit/harmony/promise-prototype-finally.js:56: assertUnreachable assertUnreachable here is wrong since it would just reject the promise (even if perhaps here it doesn't matter). https://codereview.chromium.org/2695753002/diff/150001/test/mjsunit/harmony/p... test/mjsunit/harmony/promise-prototype-finally.js:83: assertAsync(arguments.length === 0); Equals? https://codereview.chromium.org/2695753002/diff/150001/test/mjsunit/harmony/p... test/mjsunit/harmony/promise-prototype-finally.js:164: assertAsyncDone(); Please add some tests that check that .finally just calls .then (using a custom .then). One of these should pass an argument to .finally that is neither undefined nor a function and make sure that it's being passed on to .then unmodified. Also add a test that checks where .finally is called on a rejected promise and its handler returns a resolved promise. Add some tests involving multiple .finally calls.
The CQ bit was checked by gsathya@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
patchset10 provides a nice diff with the requested changes, patchset11 is just rebasing on ToT https://codereview.chromium.org/2695753002/diff/150001/src/bootstrapper.cc File src/bootstrapper.cc (right): https://codereview.chromium.org/2695753002/diff/150001/src/bootstrapper.cc#ne... src/bootstrapper.cc:3647: native_context()->set_promise_then_finally_shared_fun(*info); On 2017/02/15 12:40:39, neis wrote: > Nit: put this block in { }. Same below. Why? > Should we mark (some of) these SFIs as native? PromiseThenFinally and PromiseCatchFinally call userland js, so marking them as native. https://codereview.chromium.org/2695753002/diff/150001/src/builtins/builtins-... File src/builtins/builtins-promise.cc (right): https://codereview.chromium.org/2695753002/diff/150001/src/builtins/builtins-... src/builtins/builtins-promise.cc:1606: Node* PromiseBuiltinsAssembler::CreateValueThunkFunction(Node* value, On 2017/02/15 12:40:39, neis wrote: > nit: s/Function// Leaving as such to be consistent with CreatePromiseResolvingFunctions and CreatePromiseFinallyFunctions. Or do you prefer to have this changed? https://codereview.chromium.org/2695753002/diff/150001/src/builtins/builtins-... src/builtins/builtins-promise.cc:1620: Node* const parent = Parameter(0); On 2017/02/15 12:40:39, neis wrote: > This will typically be undefined. Done. https://codereview.chromium.org/2695753002/diff/150001/src/builtins/builtins-... src/builtins/builtins-promise.cc:1622: Node* const context = Parameter(4); On 2017/02/15 12:40:39, neis wrote: > Maybe use CSA_ASSERT_JS_ARGC_EQ here and elsewhere? Done. https://codereview.chromium.org/2695753002/diff/150001/src/builtins/builtins-... src/builtins/builtins-promise.cc:1667: Node* native_context) { On 2017/02/15 12:40:39, neis wrote: > nit: s/Function// Same as CreateValueThunkFunction https://codereview.chromium.org/2695753002/diff/150001/src/builtins/builtins-... src/builtins/builtins-promise.cc:1681: Node* const value = Parameter(1); On 2017/02/15 12:40:39, neis wrote: > nit: s/value/reason/ Done. https://codereview.chromium.org/2695753002/diff/150001/src/builtins/builtins-... src/builtins/builtins-promise.cc:1707: Return(result); On 2017/02/15 12:40:39, neis wrote: > Ouch! Return(promise_capability) > > This suggests a lack of test coverage. Done. https://codereview.chromium.org/2695753002/diff/150001/src/builtins/builtins-... src/builtins/builtins-promise.cc:1729: // 3. Let thenFinally be ! CreateThenFinally(onFinally). On 2017/02/15 12:40:39, neis wrote: > The few lines above are already part of steps 3 and 4. Done. https://codereview.chromium.org/2695753002/diff/150001/src/builtins/builtins-... src/builtins/builtins-promise.cc:1744: // 1. If IsCallable(onFinally) is not true, return onFinally. On 2017/02/15 12:40:39, neis wrote: > I find these comments here confusing, since they are from a different function > in the spec. (in particular we do not return from PromiseFinally) Yeah, that's fair. Removed https://codereview.chromium.org/2695753002/diff/150001/src/builtins/builtins-... src/builtins/builtins-promise.cc:1751: GotoIf(TaggedIsSmi(promise), &if_custompromise); On 2017/02/15 12:40:39, neis wrote: > We already know that promise is a JS_PROMISE object, so it can't be a Smi. Done. https://codereview.chromium.org/2695753002/diff/150001/src/builtins/builtins-... src/builtins/builtins-promise.cc:1753: On 2017/02/15 12:40:39, neis wrote: > The step 5 comment should appear here. Done. https://codereview.chromium.org/2695753002/diff/150001/src/flag-definitions.h File src/flag-definitions.h (right): https://codereview.chromium.org/2695753002/diff/150001/src/flag-definitions.h... src/flag-definitions.h:208: V(harmony_promise_finally, "harmony promise finally") On 2017/02/15 12:40:39, neis wrote: > "harmony Promise.prototype.finally" is more useful Done. https://codereview.chromium.org/2695753002/diff/150001/test/mjsunit/harmony/p... File test/mjsunit/harmony/promise-prototype-finally.js (right): https://codereview.chromium.org/2695753002/diff/150001/test/mjsunit/harmony/p... test/mjsunit/harmony/promise-prototype-finally.js:50: }) On 2017/02/15 12:40:40, neis wrote: > Is this .then call useful at all? Removed https://codereview.chromium.org/2695753002/diff/150001/test/mjsunit/harmony/p... test/mjsunit/harmony/promise-prototype-finally.js:56: assertUnreachable On 2017/02/15 12:40:40, neis wrote: > assertUnreachable here is wrong since it would just reject the promise (even if > perhaps here it doesn't matter). assertUnreachable now aborts the test https://codereview.chromium.org/2695753002/diff/150001/test/mjsunit/harmony/p... test/mjsunit/harmony/promise-prototype-finally.js:83: assertAsync(arguments.length === 0); On 2017/02/15 12:40:39, neis wrote: > Equals? assertAsync is fine here since we check if true? Or do you want me to change all these to be assertEqualAsync(0, arguments.length)? https://codereview.chromium.org/2695753002/diff/150001/test/mjsunit/harmony/p... test/mjsunit/harmony/promise-prototype-finally.js:164: assertAsyncDone(); On 2017/02/15 12:40:39, neis wrote: > Please add some tests that check that .finally just calls .then (using a custom > .then). One of these should pass an argument to .finally that is neither > undefined nor a function and make sure that it's being passed on to .then > unmodified. > > Also add a test that checks where .finally is called on a rejected promise and > its handler returns a resolved promise. > > Add some tests involving multiple .finally calls. Added a whole bunch of tests. PTAL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm with comments https://codereview.chromium.org/2695753002/diff/150001/src/bootstrapper.cc File src/bootstrapper.cc (right): https://codereview.chromium.org/2695753002/diff/150001/src/bootstrapper.cc#ne... src/bootstrapper.cc:3647: native_context()->set_promise_then_finally_shared_fun(*info); On 2017/02/16 15:05:29, gsathya wrote: > On 2017/02/15 12:40:39, neis wrote: > > Nit: put this block in { }. Same below. > > Why? I find it easier to read if it's immediately clear that code and info are local to each block. https://codereview.chromium.org/2695753002/diff/150001/test/mjsunit/harmony/p... File test/mjsunit/harmony/promise-prototype-finally.js (right): https://codereview.chromium.org/2695753002/diff/150001/test/mjsunit/harmony/p... test/mjsunit/harmony/promise-prototype-finally.js:50: }) On 2017/02/16 15:05:30, gsathya wrote: > On 2017/02/15 12:40:40, neis wrote: > > Is this .then call useful at all? > > Removed Still there. https://codereview.chromium.org/2695753002/diff/150001/test/mjsunit/harmony/p... test/mjsunit/harmony/promise-prototype-finally.js:83: assertAsync(arguments.length === 0); On 2017/02/16 15:05:30, gsathya wrote: > On 2017/02/15 12:40:39, neis wrote: > > Equals? > > assertAsync is fine here since we check if true? Or do you want me to change all > these to be assertEqualAsync(0, arguments.length)? Yeah I don't see why you don't use assertEqualsAsync here (which you use almost everywhere else). https://codereview.chromium.org/2695753002/diff/150001/test/mjsunit/harmony/p... test/mjsunit/harmony/promise-prototype-finally.js:164: assertAsyncDone(); I'm still missing a test passing an argument to .finally that is neither undefined nor a function and makes sure that it's being passed on to .then unmodified. https://codereview.chromium.org/2695753002/diff/170001/src/builtins/builtins-... File src/builtins/builtins-promise.cc (right): https://codereview.chromium.org/2695753002/diff/170001/src/builtins/builtins-... src/builtins/builtins-promise.cc:1756: // promiseCapability). Sorry, actually should appear above the BranchIfFastPath.
The CQ bit was checked by gsathya@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2695753002/diff/150001/src/bootstrapper.cc File src/bootstrapper.cc (right): https://codereview.chromium.org/2695753002/diff/150001/src/bootstrapper.cc#ne... src/bootstrapper.cc:3647: native_context()->set_promise_then_finally_shared_fun(*info); On 2017/02/17 10:22:54, neis wrote: > On 2017/02/16 15:05:29, gsathya wrote: > > On 2017/02/15 12:40:39, neis wrote: > > > Nit: put this block in { }. Same below. > > > > Why? > > I find it easier to read if it's immediately clear that code and info are local > to each block. Ok. I made each SFI creation its own block https://codereview.chromium.org/2695753002/diff/150001/test/mjsunit/harmony/p... File test/mjsunit/harmony/promise-prototype-finally.js (right): https://codereview.chromium.org/2695753002/diff/150001/test/mjsunit/harmony/p... test/mjsunit/harmony/promise-prototype-finally.js:50: }) On 2017/02/17 10:22:54, neis wrote: > On 2017/02/16 15:05:30, gsathya wrote: > > On 2017/02/15 12:40:40, neis wrote: > > > Is this .then call useful at all? > > > > Removed > > Still there. Can you comment on the latest patch set? I can't find this. There's one test which does resolve/then/finally/then which I think is useful. https://codereview.chromium.org/2695753002/diff/150001/test/mjsunit/harmony/p... test/mjsunit/harmony/promise-prototype-finally.js:83: assertAsync(arguments.length === 0); On 2017/02/17 10:22:54, neis wrote: > On 2017/02/16 15:05:30, gsathya wrote: > > On 2017/02/15 12:40:39, neis wrote: > > > Equals? > > > > assertAsync is fine here since we check if true? Or do you want me to change > all > > these to be assertEqualAsync(0, arguments.length)? > > Yeah I don't see why you don't use assertEqualsAsync here (which you use almost > everywhere else). Done. https://codereview.chromium.org/2695753002/diff/150001/test/mjsunit/harmony/p... test/mjsunit/harmony/promise-prototype-finally.js:164: assertAsyncDone(); On 2017/02/17 10:22:54, neis wrote: > I'm still missing a test passing an argument to .finally that is neither > undefined nor a function and makes sure that it's being passed on to .then > unmodified. You mean, makes sure that it's not being passed on to .then, right? Done. https://codereview.chromium.org/2695753002/diff/170001/src/builtins/builtins-... File src/builtins/builtins-promise.cc (right): https://codereview.chromium.org/2695753002/diff/170001/src/builtins/builtins-... src/builtins/builtins-promise.cc:1756: // promiseCapability). On 2017/02/17 10:22:54, neis wrote: > Sorry, actually should appear above the BranchIfFastPath. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
> > https://codereview.chromium.org/2695753002/diff/150001/test/mjsunit/harmony/p... > test/mjsunit/harmony/promise-prototype-finally.js:164: assertAsyncDone(); > On 2017/02/17 10:22:54, neis wrote: > > I'm still missing a test passing an argument to .finally that is neither > > undefined nor a function and makes sure that it's being passed on to .then > > unmodified. > > You mean, makes sure that it's not being passed on to .then, right? Done. No, I meant what I wrote. CreateThenFInally etc just return it immediately if it's not callable.
> Can you comment on the latest patch set? I can't find this. There's one test which does resolve/then/finally/then which I think is useful. See below. https://codereview.chromium.org/2695753002/diff/210001/test/mjsunit/harmony/p... File test/mjsunit/harmony/promise-prototype-finally.js (right): https://codereview.chromium.org/2695753002/diff/210001/test/mjsunit/harmony/p... test/mjsunit/harmony/promise-prototype-finally.js:119: }) here https://codereview.chromium.org/2695753002/diff/210001/test/mjsunit/harmony/p... test/mjsunit/harmony/promise-prototype-finally.js:233: }) here
https://codereview.chromium.org/2695753002/diff/210001/test/mjsunit/harmony/p... File test/mjsunit/harmony/promise-prototype-finally.js (right): https://codereview.chromium.org/2695753002/diff/210001/test/mjsunit/harmony/p... test/mjsunit/harmony/promise-prototype-finally.js:233: }) On 2017/02/17 12:30:16, neis wrote: > here keep this one if you like
https://codereview.chromium.org/2695753002/diff/210001/test/mjsunit/harmony/p... File test/mjsunit/harmony/promise-prototype-finally.js (right): https://codereview.chromium.org/2695753002/diff/210001/test/mjsunit/harmony/p... test/mjsunit/harmony/promise-prototype-finally.js:119: }) On 2017/02/17 12:30:16, neis wrote: > here i'm keeping this. there's no other test for resolve/then/finally/then as mentioned in my previous comment (it's not strictly necessary but i'd like to keep it) https://codereview.chromium.org/2695753002/diff/210001/test/mjsunit/harmony/p... test/mjsunit/harmony/promise-prototype-finally.js:233: }) On 2017/02/17 12:31:51, neis wrote: > On 2017/02/17 12:30:16, neis wrote: > > here > > keep this one if you like keeping this because the then throws
The CQ bit was checked by gsathya@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm with a few trivial tests https://codereview.chromium.org/2695753002/diff/270001/src/objects-debug.cc File src/objects-debug.cc (left): https://codereview.chromium.org/2695753002/diff/270001/src/objects-debug.cc#o... src/objects-debug.cc:477: Stray whitespace change? You could leave this file out of the CL looks like. https://codereview.chromium.org/2695753002/diff/270001/test/mjsunit/harmony/p... File test/mjsunit/harmony/promise-prototype-finally.js (right): https://codereview.chromium.org/2695753002/diff/270001/test/mjsunit/harmony/p... test/mjsunit/harmony/promise-prototype-finally.js:657: assertFalse(descriptor.enumerable); Please also check that Promise.prototype.finally.name is "finally" and that its .name is 1
https://codereview.chromium.org/2695753002/diff/270001/src/objects-debug.cc File src/objects-debug.cc (left): https://codereview.chromium.org/2695753002/diff/270001/src/objects-debug.cc#o... src/objects-debug.cc:477: On 2017/02/17 19:28:17, adamk wrote: > Stray whitespace change? You could leave this file out of the CL looks like. Done. https://codereview.chromium.org/2695753002/diff/270001/test/mjsunit/harmony/p... File test/mjsunit/harmony/promise-prototype-finally.js (right): https://codereview.chromium.org/2695753002/diff/270001/test/mjsunit/harmony/p... test/mjsunit/harmony/promise-prototype-finally.js:657: assertFalse(descriptor.enumerable); On 2017/02/17 19:28:17, adamk wrote: > Please also check that Promise.prototype.finally.name is "finally" and that its > .name is 1 I think you mean .length. Done.
The CQ bit was checked by gsathya@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...) v8_linux_verify_csa_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_verify_csa_rel_ng/...) v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/17691)
The CQ bit was checked by gsathya@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm https://codereview.chromium.org/2695753002/diff/270001/test/mjsunit/harmony/p... File test/mjsunit/harmony/promise-prototype-finally.js (right): https://codereview.chromium.org/2695753002/diff/270001/test/mjsunit/harmony/p... test/mjsunit/harmony/promise-prototype-finally.js:657: assertFalse(descriptor.enumerable); On 2017/02/17 21:26:16, gsathya wrote: > On 2017/02/17 19:28:17, adamk wrote: > > Please also check that Promise.prototype.finally.name is "finally" and that > its > > .name is 1 > > I think you mean .length. Done. Indeed I did, thanks.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by gsathya@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from neis@chromium.org Link to the patchset: https://codereview.chromium.org/2695753002/#ps310001 (title: "Rename GotoUnless")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 310001, "attempt_start_ts": 1487369320050570,
"parent_rev": "00a379a03e31bf6eb87cd53212fde8aa1d3c1f67", "commit_rev":
"18ad0f13afeaabff4e035fddd9edc3d319152160"}
Message was sent while issue was closed.
Description was changed from ========== [ESnext] Implement Promise.prototype.finally Adds five new TF builtins for the spec defined functions/closures. This follows mechanism similar to promise resolving functions approach where we store the closure variables in a custom context. Adds a new --harmony-promise-finally flag. BUG=v8:5967 ========== to ========== [ESnext] Implement Promise.prototype.finally Adds five new TF builtins for the spec defined functions/closures. This follows mechanism similar to promise resolving functions approach where we store the closure variables in a custom context. Adds a new --harmony-promise-finally flag. BUG=v8:5967 Review-Url: https://codereview.chromium.org/2695753002 Cr-Commit-Position: refs/heads/master@{#43294} Committed: https://chromium.googlesource.com/v8/v8/+/18ad0f13afeaabff4e035fddd9edc3d3191... ==========
Message was sent while issue was closed.
Committed patchset #17 (id:310001) as https://chromium.googlesource.com/v8/v8/+/18ad0f13afeaabff4e035fddd9edc3d3191... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
