Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(367)

Issue 1574903004: TypedArray and ArrayBuffer support for @@species (Closed)

Created:
4 years, 11 months ago by Dan Ehrenberg
Modified:
4 years, 11 months ago
Reviewers:
adamk
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

TypedArray and ArrayBuffer support for @@species This patch improves ArrayBuffer and TypedArray subclassing by adding support for @@species and constructing outputs to certain methods by creating an instance of the constructor determined by the SpeciesConstructor algorithm, rather than fixed to a superclass or naively the constructor. The new behavior is enabled by the --harmony-species flag. Care is taken to not significantly change the observable behavior when the flag is off. Previously, TypedArrays already supported subclassing by reading the constructor of the receiver, but ArrayBuffers did not, and this old behavior is preserved and tested for, to avoid a multi-stage upgrade path and keep things simple for users. R=adamk BUG=v8:4093 LOG=Y Committed: https://crrev.com/2bd9bdbe62f27af9416bd74670ef96569c8728fe Cr-Commit-Position: refs/heads/master@{#33223}

Patch Set 1 #

Patch Set 2 : Preserve old behavior #

Total comments: 16

Patch Set 3 : Upload tests and make some code more clear #

Unified diffs Side-by-side diffs Delta from patch set Stats (+322 lines, -78 lines) Patch
M src/js/arraybuffer.js View 1 2 2 chunks +16 lines, -3 lines 0 comments Download
M src/js/runtime.js View 1 2 3 chunks +43 lines, -2 lines 0 comments Download
M src/js/typedarray.js View 1 2 11 chunks +53 lines, -38 lines 0 comments Download
M src/messages.h View 1 2 4 chunks +8 lines, -1 line 0 comments Download
M src/runtime/runtime.h View 1 chunk +1 line, -1 line 0 comments Download
M src/runtime/runtime-typedarray.cc View 1 chunk +5 lines, -3 lines 0 comments Download
A test/mjsunit/es6/legacy-subclassing.js View 1 2 1 chunk +35 lines, -0 lines 0 comments Download
M test/mjsunit/es6/typedarray-of.js View 3 chunks +24 lines, -20 lines 0 comments Download
A test/mjsunit/harmony/arraybuffer-species.js View 1 2 1 chunk +36 lines, -0 lines 0 comments Download
A test/mjsunit/harmony/typedarray-species.js View 1 2 1 chunk +86 lines, -0 lines 0 comments Download
M test/mjsunit/mjsunit.status View 2 chunks +1 line, -4 lines 0 comments Download
M test/mjsunit/regress/regress-544991.js View 1 chunk +14 lines, -6 lines 0 comments Download

Messages

Total messages: 21 (7 generated)
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1574903004/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1574903004/1
4 years, 11 months ago (2016-01-11 22:56:47 UTC) #2
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
4 years, 11 months ago (2016-01-11 23:24:39 UTC) #4
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1574903004/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1574903004/20001
4 years, 11 months ago (2016-01-11 23:40:50 UTC) #7
Dan Ehrenberg
4 years, 11 months ago (2016-01-11 23:41:40 UTC) #8
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
4 years, 11 months ago (2016-01-12 00:06:10 UTC) #10
adamk
https://codereview.chromium.org/1574903004/diff/20001/src/js/arraybuffer.js File src/js/arraybuffer.js (right): https://codereview.chromium.org/1574903004/diff/20001/src/js/arraybuffer.js#newcode69 src/js/arraybuffer.js:69: if (!IS_ARRAYBUFFER(result)) { Is this exercised in some test? ...
4 years, 11 months ago (2016-01-12 00:11:00 UTC) #11
Dan Ehrenberg
I forgot to git add a couple files of tests (mjsunit/harmony/{typedarray,arraybuffer}-species.js), and this version also ...
4 years, 11 months ago (2016-01-12 01:38:33 UTC) #12
adamk
Ah, looks better with tests. Just one point of confusion I hope you can clear ...
4 years, 11 months ago (2016-01-12 01:47:44 UTC) #13
Dan Ehrenberg
On 2016/01/12 at 01:47:44, adamk wrote: > Ah, looks better with tests. Just one point ...
4 years, 11 months ago (2016-01-12 01:53:19 UTC) #14
Dan Ehrenberg
4 years, 11 months ago (2016-01-12 01:53:23 UTC) #15
adamk
On 2016/01/12 01:53:19, Dan Ehrenberg wrote: > On 2016/01/12 at 01:47:44, adamk wrote: > > ...
4 years, 11 months ago (2016-01-12 01:59:02 UTC) #16
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1574903004/20002 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1574903004/20002
4 years, 11 months ago (2016-01-12 05:20:18 UTC) #18
commit-bot: I haz the power
Committed patchset #3 (id:20002)
4 years, 11 months ago (2016-01-12 06:07:52 UTC) #19
commit-bot: I haz the power
4 years, 11 months ago (2016-01-12 06:08:07 UTC) #21
Message was sent while issue was closed.
Patchset 3 (id:??) landed as
https://crrev.com/2bd9bdbe62f27af9416bd74670ef96569c8728fe
Cr-Commit-Position: refs/heads/master@{#33223}

Powered by Google App Engine
This is Rietveld 408576698