Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(72)

Issue 2695753002: [ESnext] Implement Promise.prototype.finally (Closed)

Created:
9 months, 1 week ago by gsathya
Modified:
9 months ago
Reviewers:
neis, adamk
CC:
v8-reviews_googlegroups.com
Target Ref:
refs/pending/heads/master
Project:
v8
Visibility:
Public.

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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+976 lines, -2 lines) Patch
M src/bootstrapper.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 2 chunks +62 lines, -0 lines 0 comments Download
M src/builtins/builtins.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +5 lines, -0 lines 0 comments Download
M src/builtins/builtins-promise.h View 1 2 3 4 5 6 7 8 2 chunks +21 lines, -0 lines 0 comments Download
M src/builtins/builtins-promise.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 2 chunks +216 lines, -1 line 0 comments Download
M src/contexts.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +8 lines, -0 lines 0 comments Download
M src/flag-definitions.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +2 lines, -1 line 0 comments Download
M src/objects-printer.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +1 line, -0 lines 0 comments Download
A test/mjsunit/harmony/promise-prototype-finally.js View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +661 lines, -0 lines 0 comments Download

Messages

Total messages: 58 (43 generated)
gsathya
PTAL. Will add more tests tomorrow.
9 months, 1 week ago (2017-02-14 17:10:07 UTC) #14
neis
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#newcode3647 src/bootstrapper.cc:3647: native_context()->set_promise_then_finally_shared_fun(*info); Nit: put this block ...
9 months, 1 week ago (2017-02-15 12:40:40 UTC) #21
gsathya
patchset10 provides a nice diff with the requested changes, patchset11 is just rebasing on ToT ...
9 months ago (2017-02-16 15:05:30 UTC) #24
neis
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#newcode3647 src/bootstrapper.cc:3647: native_context()->set_promise_then_finally_shared_fun(*info); On 2017/02/16 15:05:29, gsathya wrote: ...
9 months ago (2017-02-17 10:22:55 UTC) #27
gsathya
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#newcode3647 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 ...
9 months ago (2017-02-17 11:26:16 UTC) #30
neis
> > https://codereview.chromium.org/2695753002/diff/150001/test/mjsunit/harmony/promise-prototype-finally.js#newcode164 > test/mjsunit/harmony/promise-prototype-finally.js:164: assertAsyncDone(); > On 2017/02/17 10:22:54, neis wrote: > > I'm ...
9 months ago (2017-02-17 12:26:05 UTC) #33
neis
> Can you comment on the latest patch set? I can't find this. There's one ...
9 months ago (2017-02-17 12:30:16 UTC) #34
neis
https://codereview.chromium.org/2695753002/diff/210001/test/mjsunit/harmony/promise-prototype-finally.js File test/mjsunit/harmony/promise-prototype-finally.js (right): https://codereview.chromium.org/2695753002/diff/210001/test/mjsunit/harmony/promise-prototype-finally.js#newcode233 test/mjsunit/harmony/promise-prototype-finally.js:233: }) On 2017/02/17 12:30:16, neis wrote: > here keep ...
9 months ago (2017-02-17 12:31:51 UTC) #35
gsathya
https://codereview.chromium.org/2695753002/diff/210001/test/mjsunit/harmony/promise-prototype-finally.js File test/mjsunit/harmony/promise-prototype-finally.js (right): https://codereview.chromium.org/2695753002/diff/210001/test/mjsunit/harmony/promise-prototype-finally.js#newcode119 test/mjsunit/harmony/promise-prototype-finally.js:119: }) On 2017/02/17 12:30:16, neis wrote: > here i'm ...
9 months ago (2017-02-17 12:54:24 UTC) #36
neis
lgtm
9 months ago (2017-02-17 13:57:52 UTC) #39
adamk
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#oldcode477 src/objects-debug.cc:477: Stray whitespace change? ...
9 months ago (2017-02-17 19:28:17 UTC) #42
gsathya
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#oldcode477 src/objects-debug.cc:477: On 2017/02/17 19:28:17, adamk wrote: > Stray whitespace change? ...
9 months ago (2017-02-17 21:26:16 UTC) #43
adamk
lgtm https://codereview.chromium.org/2695753002/diff/270001/test/mjsunit/harmony/promise-prototype-finally.js File test/mjsunit/harmony/promise-prototype-finally.js (right): https://codereview.chromium.org/2695753002/diff/270001/test/mjsunit/harmony/promise-prototype-finally.js#newcode657 test/mjsunit/harmony/promise-prototype-finally.js:657: assertFalse(descriptor.enumerable); On 2017/02/17 21:26:16, gsathya wrote: > On ...
9 months ago (2017-02-17 21:47:49 UTC) #50
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2695753002/310001
9 months ago (2017-02-17 22:08:47 UTC) #55
commit-bot: I haz the power
9 months ago (2017-02-17 22:10:37 UTC) #58
Message was sent while issue was closed.
Committed patchset #17 (id:310001) as
https://chromium.googlesource.com/v8/v8/+/18ad0f13afeaabff4e035fddd9edc3d3191...

Powered by Google App Engine
This is Rietveld efc10ee0f