Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1538853002:
[promise] use PromiseCapabilities directly for Promise.race resolve/reject (Closed)
|
Created:
5 years ago by caitp (gmail) Modified:
4 years, 11 months ago CC:
v8-reviews_googlegroups.com Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[promise] use PromiseCapabilities directly for Promise.race resolve/reject
Does not remove the extra private state added, as doing so seems to break the
debugger.
Fixes new Test262 tests:
- built-ins/Promise/race/same-resolve-function
- built-ins/Promise/race/same-reject-function
BUG=v8:4632
LOG=N
R=littledan@chromium.org, cbruni@chromium.org
Committed: https://crrev.com/ee1671b9afdb9eb23179d08cd3e573e520f45904
Cr-Commit-Position: refs/heads/master@{#33214}
Patch Set 1 #Patch Set 2 : Remove test expectations #Patch Set 3 : rebase #Patch Set 4 : Rebased again #Messages
Total messages: 27 (10 generated)
Quick fix, passes test262 and works
Description was changed from ========== [promise] use PromiseCapabilities directly for Promise.race resolve/reject Does not remove the extra private state added, as doing so seems to break the debugger. Fixes new Test262 tests: - built-ins/Promise/race/same-resolve-function - built-ins/Promise/race/same-reject-function BUG=v8:4632 LOG=N R=littledan@chromium.org ========== to ========== [promise] use PromiseCapabilities directly for Promise.race resolve/reject Does not remove the extra private state added, as doing so seems to break the debugger. Fixes new Test262 tests: - built-ins/Promise/race/same-resolve-function - built-ins/Promise/race/same-reject-function BUG=v8:4632 LOG=N R=littledan@chromium.org, cbruni@chromium.org ==========
caitpotter88@gmail.com changed reviewers: + cbruni@chromium.org
The CQ bit was checked by littledan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1538853002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1538853002/1
On 2015/12/18 at 13:00:44, caitpotter88 wrote: > Quick fix, passes test262 and works Looks like a good change to meet the spec. Thanks, and sorry for the delayed review! Could you rebase and fix the test262 expectations?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux_nodcheck_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel/build...)
On 2016/01/04 at 02:56:35, commit-bot wrote: > Dry run: Try jobs failed on following builders: > v8_linux_nodcheck_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel/build...) Looks like this is failing because the two test262 tests just need to be marked PASS
On 2016/01/06 17:22:45, Dan Ehrenberg wrote: > On 2016/01/04 at 02:56:35, commit-bot wrote: > > Dry run: Try jobs failed on following builders: > > v8_linux_nodcheck_rel on tryserver.v8 (JOB_FAILED, > http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel/build...) > > Looks like this is failing because the two test262 tests just need to be marked > PASS they will probably need to be marked as skip or fail for Ignition too --- I'm not sure how to verify that locally though
On 2016/01/06 at 17:31:13, caitpotter88 wrote: > On 2016/01/06 17:22:45, Dan Ehrenberg wrote: > > On 2016/01/04 at 02:56:35, commit-bot wrote: > > > Dry run: Try jobs failed on following builders: > > > v8_linux_nodcheck_rel on tryserver.v8 (JOB_FAILED, > > http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel/build...) > > > > Looks like this is failing because the two test262 tests just need to be marked > > PASS > > they will probably need to be marked as skip or fail for Ignition too --- I'm not sure how to verify that locally though I usually let the trybot figure out the ignition failures for me.
caitpotter88@gmail.com changed reviewers: + adamk@chromium.org
On 2016/01/06 17:36:27, Dan Ehrenberg wrote: > On 2016/01/06 at 17:31:13, caitpotter88 wrote: > > On 2016/01/06 17:22:45, Dan Ehrenberg wrote: > > > On 2016/01/04 at 02:56:35, commit-bot wrote: > > > > Dry run: Try jobs failed on following builders: > > > > v8_linux_nodcheck_rel on tryserver.v8 (JOB_FAILED, > > > > http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel/build...) > > > > > > Looks like this is failing because the two test262 tests just need to be > marked > > > PASS > > > > they will probably need to be marked as skip or fail for Ignition too --- I'm > not sure how to verify that locally though > > I usually let the trybot figure out the ignition failures for me. It looks like ignition fixes aren't needed =) +adam for js/src/OWNERS, if that's alright!
PTAL --- this was the last of the pre-christmas Promise fixups I had put together, and is a very small one.
lgtm
The CQ bit was checked by caitpotter88@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1538853002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1538853002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: v8_presubmit on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/9607)
On 2016/01/11 14:02:35, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > v8_presubmit on tryserver.v8 (JOB_FAILED, > http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/9607) needs sign-off from top-level OWNER u_u
lgtm
The CQ bit was checked by caitpotter88@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1538853002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1538853002/60001
Message was sent while issue was closed.
Description was changed from ========== [promise] use PromiseCapabilities directly for Promise.race resolve/reject Does not remove the extra private state added, as doing so seems to break the debugger. Fixes new Test262 tests: - built-ins/Promise/race/same-resolve-function - built-ins/Promise/race/same-reject-function BUG=v8:4632 LOG=N R=littledan@chromium.org, cbruni@chromium.org ========== to ========== [promise] use PromiseCapabilities directly for Promise.race resolve/reject Does not remove the extra private state added, as doing so seems to break the debugger. Fixes new Test262 tests: - built-ins/Promise/race/same-resolve-function - built-ins/Promise/race/same-reject-function BUG=v8:4632 LOG=N R=littledan@chromium.org, cbruni@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== [promise] use PromiseCapabilities directly for Promise.race resolve/reject Does not remove the extra private state added, as doing so seems to break the debugger. Fixes new Test262 tests: - built-ins/Promise/race/same-resolve-function - built-ins/Promise/race/same-reject-function BUG=v8:4632 LOG=N R=littledan@chromium.org, cbruni@chromium.org ========== to ========== [promise] use PromiseCapabilities directly for Promise.race resolve/reject Does not remove the extra private state added, as doing so seems to break the debugger. Fixes new Test262 tests: - built-ins/Promise/race/same-resolve-function - built-ins/Promise/race/same-reject-function BUG=v8:4632 LOG=N R=littledan@chromium.org, cbruni@chromium.org Committed: https://crrev.com/ee1671b9afdb9eb23179d08cd3e573e520f45904 Cr-Commit-Position: refs/heads/master@{#33214} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/ee1671b9afdb9eb23179d08cd3e573e520f45904 Cr-Commit-Position: refs/heads/master@{#33214} |
