Add %TypedArray% to proto chain

Add %TypedArray% to proto chain

According to the ES6 spec, the main methods and getters shouldn't
be properties of the individual TypedArray objects and prototypes
but instead on %TypedArray% and %TypedArray%.prototype. This
difference is observable through introspection. This patch moves
some methods and getters to the proper place, with the exception
of %TypedArray%.prototype.subarray and harmony methods. These will
be moved in follow-on patches.

BUG=v8:4085
LOG=Y
R=adamk

Committed: https://crrev.com/a10590158260737b256fac3254b4939f48f90095
Cr-Commit-Position: refs/heads/master@{#29057}

[adamk, 2015-06-13 00:49:39]

https://codereview.chromium.org/1186733002/diff/1/src/runtime/runtime-functio...
File src/runtime/runtime-function.cc (right):

https://codereview.chromium.org/1186733002/diff/1/src/runtime/runtime-functio...
src/runtime/runtime-function.cc:174: // Set constructor.prototype
I'd expand this to something like:

// Set the "prototype" property of a constructor Function.

https://codereview.chromium.org/1186733002/diff/1/src/runtime/runtime-object.cc
File src/runtime/runtime-object.cc (right):

https://codereview.chromium.org/1186733002/diff/1/src/runtime/runtime-object....
src/runtime/runtime-object.cc:159: // Set object.__proto__
I'd say "Set the [[Prototype]] internal slot" instead, since
Runtime_SetPrototype below is what we have to use for actually setting __proto__
from script.

https://codereview.chromium.org/1186733002/diff/1/src/typedarray.js
File src/typedarray.js (right):

https://codereview.chromium.org/1186733002/diff/1/src/typedarray.js#newcode59
src/typedarray.js:59: throw MakeTypeError(kIncompatibleMethodReceiver,
"TypedArray.buffer", this);
s/buffer/byteLength/

https://codereview.chromium.org/1186733002/diff/1/src/typedarray.js#newcode66
src/typedarray.js:66: throw MakeTypeError(kIncompatibleMethodReceiver,
"TypedArray.buffer", this);
byteOffset

https://codereview.chromium.org/1186733002/diff/1/src/typedarray.js#newcode72
src/typedarray.js:72: if (!%IsTypedArray(this)) {
I don't know if we can afford a runtime call for these trivial accessors. Would
blocking this change on fixing
https://code.google.com/p/v8/issues/detail?id=4182 be problematic?

Alternatively, we could add inlineable versions of this runtime function (they'd
be pretty trivial to create).

https://codereview.chromium.org/1186733002/diff/1/src/typedarray.js#newcode73
src/typedarray.js:73: throw MakeTypeError(kIncompatibleMethodReceiver,
"TypedArray.buffer", this);
length

https://codereview.chromium.org/1186733002/diff/1/src/typedarray.js#newcode326
src/typedarray.js:326: utils.InstallGetter(TypedArray.prototype, "byteOffset",
TypedArray_GetByteOffset,
Nit: wrap at 80 chars (I don't know why we don't have a linter to catch this).
Same problem on some lines below.

https://codereview.chromium.org/1186733002/diff/1/src/typedarray.js#newcode351
src/typedarray.js:351: "subarray", NAMESubArray
I'm guessing this is also blocked on issue 4182?

https://codereview.chromium.org/1186733002/diff/1/test/mjsunit/es6/built-in-a...
File test/mjsunit/es6/built-in-accessor-names.js (right):

https://codereview.chromium.org/1186733002/diff/1/test/mjsunit/es6/built-in-a...
test/mjsunit/es6/built-in-accessor-names.js:50: assertGetterName('get buffer',
f.prototype, 'buffer', true);
Wouldn't this work with the old code just changed to:

assertGetterName('get buffer', f.prototype.__proto__, 'buffer')

The imprecision of GetPropertyDescriptor's proto chain walk seems bad to me.

https://codereview.chromium.org/1186733002/diff/1/test/mjsunit/harmony/shared...
File test/mjsunit/harmony/sharedarraybuffer.js (right):

https://codereview.chromium.org/1186733002/diff/1/test/mjsunit/harmony/shared...
test/mjsunit/harmony/sharedarraybuffer.js:90: function
GetPropertyDescriptor(object, field) {
Same worry here, I'd prefer not to do a prototype walk if it can be avoided.

https://codereview.chromium.org/1186733002/diff/1/test/mjsunit/harmony/typeda...
File test/mjsunit/harmony/typedarrays.js (right):

https://codereview.chromium.org/1186733002/diff/1/test/mjsunit/harmony/typeda...
test/mjsunit/harmony/typedarrays.js:124: function GetPropertyDescriptor(object,
field) {
And here.

Maybe I'm missing something as to why the callers weren't just changed to
.__proto__?

[Dan Ehrenberg, 2015-06-13 05:54:09]

The new version isn't ready to submit until %IsTypedArray becomes faster.

https://codereview.chromium.org/1186733002/diff/1/src/runtime/runtime-functio...
File src/runtime/runtime-function.cc (right):

https://codereview.chromium.org/1186733002/diff/1/src/runtime/runtime-functio...
src/runtime/runtime-function.cc:174: // Set constructor.prototype
On 2015/06/13 00:49:38, adamk wrote:
> I'd expand this to something like:
> 
> // Set the "prototype" property of a constructor Function.

Done.

https://codereview.chromium.org/1186733002/diff/1/src/runtime/runtime-object.cc
File src/runtime/runtime-object.cc (right):

https://codereview.chromium.org/1186733002/diff/1/src/runtime/runtime-object....
src/runtime/runtime-object.cc:159: // Set object.__proto__
On 2015/06/13 00:49:38, adamk wrote:
> I'd say "Set the [[Prototype]] internal slot" instead, since
> Runtime_SetPrototype below is what we have to use for actually setting
__proto__
> from script.

Done.

https://codereview.chromium.org/1186733002/diff/1/src/typedarray.js
File src/typedarray.js (right):

https://codereview.chromium.org/1186733002/diff/1/src/typedarray.js#newcode59
src/typedarray.js:59: throw MakeTypeError(kIncompatibleMethodReceiver,
"TypedArray.buffer", this);
On 2015/06/13 00:49:38, adamk wrote:
> s/buffer/byteLength/

Done.

https://codereview.chromium.org/1186733002/diff/1/src/typedarray.js#newcode66
src/typedarray.js:66: throw MakeTypeError(kIncompatibleMethodReceiver,
"TypedArray.buffer", this);
On 2015/06/13 00:49:38, adamk wrote:
> byteOffset

Done.

https://codereview.chromium.org/1186733002/diff/1/src/typedarray.js#newcode72
src/typedarray.js:72: if (!%IsTypedArray(this)) {
On 2015/06/13 00:49:38, adamk wrote:
> I don't know if we can afford a runtime call for these trivial accessors.
Would
> blocking this change on fixing
> https://code.google.com/p/v8/issues/detail?id=4182 be problematic?
> 
> Alternatively, we could add inlineable versions of this runtime function
(they'd
> be pretty trivial to create).

This is sort of unrelated to v8:4182, except in that that bug probably also
needs an inlineable way to get the TypedArray tag to be done efficiently. I
think we should have both functions, and not rely on the latter to form a funny
way of checking the former.

Until then, I uploaded a new version of this patch. I won't submit this one
until that one is in, but code-wise there's no dependency, so I'll just leave
this out for review separately.

https://codereview.chromium.org/1186733002/diff/1/src/typedarray.js#newcode73
src/typedarray.js:73: throw MakeTypeError(kIncompatibleMethodReceiver,
"TypedArray.buffer", this);
On 2015/06/13 00:49:38, adamk wrote:
> length

Done.

https://codereview.chromium.org/1186733002/diff/1/src/typedarray.js#newcode326
src/typedarray.js:326: utils.InstallGetter(TypedArray.prototype, "byteOffset",
TypedArray_GetByteOffset,
On 2015/06/13 00:49:38, adamk wrote:
> Nit: wrap at 80 chars (I don't know why we don't have a linter to catch this).
> Same problem on some lines below.

Done.

https://codereview.chromium.org/1186733002/diff/1/src/typedarray.js#newcode351
src/typedarray.js:351: "subarray", NAMESubArray
On 2015/06/13 00:49:38, adamk wrote:
> I'm guessing this is also blocked on issue 4182?

Yep.

https://codereview.chromium.org/1186733002/diff/1/test/mjsunit/es6/built-in-a...
File test/mjsunit/es6/built-in-accessor-names.js (right):

https://codereview.chromium.org/1186733002/diff/1/test/mjsunit/es6/built-in-a...
test/mjsunit/es6/built-in-accessor-names.js:50: assertGetterName('get buffer',
f.prototype, 'buffer', true);
On 2015/06/13 00:49:38, adamk wrote:
> Wouldn't this work with the old code just changed to:
> 
> assertGetterName('get buffer', f.prototype.__proto__, 'buffer')
> 
> The imprecision of GetPropertyDescriptor's proto chain walk seems bad to me.

I thought it was nicer to do the proto walk because if something came along
later and shadowed that property, then that'd show up instead. I'll change this
to a numerical parameter for which level in the chain to check for, and assert
that it doesn't exist lower.

[arv (Not doing code reviews), 2015-06-15 16:23:11]

arv@chromium.org changed reviewers:
+ arv@chromium.org

[arv (Not doing code reviews), 2015-06-15 16:23:12]

Can you ensure that the following works as expected?

- [[Call]]ing %TypedArray% should always fail.
- [[Construct]]ing should fail if the right internal slots are not present. It
should work otherwise. You probably need to use Reflect.construct to test this

I do not think the [[Construct]] behavior works and it seems complicated to get
that right too.

What is %TypedArray%.name? I think it should not have a name property at all but
it is not clear to me.

https://codereview.chromium.org/1186733002/diff/40001/src/typedarray.js
File src/typedarray.js (right):

https://codereview.chromium.org/1186733002/diff/40001/src/typedarray.js#newco...
src/typedarray.js:59: throw MakeTypeError(kIncompatibleMethodReceiver,
"TypedArray.byteLength", this);
long line

https://codereview.chromium.org/1186733002/diff/40001/src/typedarray.js#newco...
src/typedarray.js:327: TypedArray_GetByteOffset, DONT_ENUM | DONT_DELETE);
why DONT_DELETE?

https://codereview.chromium.org/1186733002/diff/40001/src/typedarray.js#newco...
src/typedarray.js:329: TypedArray_GetByteLength, DONT_ENUM | DONT_DELETE);
same here and other places. We should also have tests that these accessors have
the right flags.

https://codereview.chromium.org/1186733002/diff/40001/src/typedarray.js#newco...
src/typedarray.js:333: TypedArrayGetToStringTag);
DONT_ENUM?

https://codereview.chromium.org/1186733002/diff/40001/test/mjsunit/es6/built-...
File test/mjsunit/es6/built-in-accessor-names.js (right):

https://codereview.chromium.org/1186733002/diff/40001/test/mjsunit/es6/built-...
test/mjsunit/es6/built-in-accessor-names.js:9: function
GetPropertyDescriptor(object, field, expectedDepth) {
getP...

https://codereview.chromium.org/1186733002/diff/40001/test/mjsunit/es6/built-...
test/mjsunit/es6/built-in-accessor-names.js:47: assertGetterName('get buffer',
f.prototype, 'buffer', 1);
I would probably just change this to `f.prototype.__proto__` or remove the loop
and just test Uint8Array.prototype.__proto__. There are already other tests
covering that these all have the correct prototype chain.

https://codereview.chromium.org/1186733002/diff/40001/test/mjsunit/regress/re...
File test/mjsunit/regress/regress-typedarray-length.js (right):

https://codereview.chromium.org/1186733002/diff/40001/test/mjsunit/regress/re...
test/mjsunit/regress/regress-typedarray-length.js:74: // Ensure we cannot delete
length, byteOffset, byteLength.
I think this is invalid. Maybe justs file a bug and we can get back to this.

[adamk, 2015-06-15 16:50:35]

Per the spec, %TypedArray%.name is "TypedArray":
https://people.mozilla.org/~jorendorff/es6-draft.html#sec-properties-of-the-%...

[Dan Ehrenberg, 2015-06-15 23:28:35]

With an upcoming patch that I am working on, it'll be possible for me to
implement the proper behavior for [[Construct]] on TypedArray. As a "bonus",
this can also give proper behavior for normal constructors, since they are
spec'd to just call the super constructor and get their individual behavior from
Table 49. But let's save that for a little later than this patch.

https://codereview.chromium.org/1186733002/diff/40001/src/typedarray.js
File src/typedarray.js (right):

https://codereview.chromium.org/1186733002/diff/40001/src/typedarray.js#newco...
src/typedarray.js:59: throw MakeTypeError(kIncompatibleMethodReceiver,
"TypedArray.byteLength", this);
On 2015/06/15 16:23:12, arv wrote:
> long line

Done.

https://codereview.chromium.org/1186733002/diff/40001/src/typedarray.js#newco...
src/typedarray.js:327: TypedArray_GetByteOffset, DONT_ENUM | DONT_DELETE);
On 2015/06/15 16:23:12, arv wrote:
> why DONT_DELETE?

Oops, fixed it.

https://codereview.chromium.org/1186733002/diff/40001/src/typedarray.js#newco...
src/typedarray.js:329: TypedArray_GetByteLength, DONT_ENUM | DONT_DELETE);
On 2015/06/15 16:23:12, arv wrote:
> same here and other places. We should also have tests that these accessors
have
> the right flags.

Done.

https://codereview.chromium.org/1186733002/diff/40001/src/typedarray.js#newco...
src/typedarray.js:333: TypedArrayGetToStringTag);
On 2015/06/15 16:23:12, arv wrote:
> DONT_ENUM?

Done.

https://codereview.chromium.org/1186733002/diff/40001/test/mjsunit/es6/built-...
File test/mjsunit/es6/built-in-accessor-names.js (right):

https://codereview.chromium.org/1186733002/diff/40001/test/mjsunit/es6/built-...
test/mjsunit/es6/built-in-accessor-names.js:47: assertGetterName('get buffer',
f.prototype, 'buffer', 1);
On 2015/06/15 16:23:12, arv wrote:
> I would probably just change this to `f.prototype.__proto__` or remove the
loop
> and just test Uint8Array.prototype.__proto__. There are already other tests
> covering that these all have the correct prototype chain.

Where are those tests?

https://codereview.chromium.org/1186733002/diff/40001/test/mjsunit/regress/re...
File test/mjsunit/regress/regress-typedarray-length.js (right):

https://codereview.chromium.org/1186733002/diff/40001/test/mjsunit/regress/re...
test/mjsunit/regress/regress-typedarray-length.js:74: // Ensure we cannot delete
length, byteOffset, byteLength.
On 2015/06/15 16:23:12, arv wrote:
> I think this is invalid. Maybe justs file a bug and we can get back to this.

Fixed it.

[arv (Not doing code reviews), 2015-06-16 16:24:49]

LGTM

https://codereview.chromium.org/1186733002/diff/40001/test/mjsunit/es6/built-...
File test/mjsunit/es6/built-in-accessor-names.js (right):

https://codereview.chromium.org/1186733002/diff/40001/test/mjsunit/es6/built-...
test/mjsunit/es6/built-in-accessor-names.js:47: assertGetterName('get buffer',
f.prototype, 'buffer', 1);
On 2015/06/15 23:28:34, littledan wrote:
> On 2015/06/15 16:23:12, arv wrote:
> > I would probably just change this to `f.prototype.__proto__` or remove the
> loop
> > and just test Uint8Array.prototype.__proto__. There are already other tests
> > covering that these all have the correct prototype chain.
> 
> Where are those tests?

I assume you have added tests that asserts that f.prototype.__proto__ ===
%TypedArray%.prototype somewhere?

https://codereview.chromium.org/1186733002/diff/80001/src/harmony-typedarray.js
File src/harmony-typedarray.js (right):

https://codereview.chromium.org/1186733002/diff/80001/src/harmony-typedarray....
src/harmony-typedarray.js:51: var TypedArrayPrototype =
GlobalUint8Array.prototype.__proto__;
This is the same as TypedArray.prototype which reads better:

var TypedArrayPrototype = TypedArray.prototype;

https://codereview.chromium.org/1186733002/diff/80001/test/mjsunit/regress/re...
File test/mjsunit/regress/regress-typedarray-length.js (right):

https://codereview.chromium.org/1186733002/diff/80001/test/mjsunit/regress/re...
test/mjsunit/regress/regress-typedarray-length.js:107:
assertTrue(Int32Array.prototype.__proto__.hasOwnProperty("length"));
Maybe add tests that check the descriptor instead?

[Dan Ehrenberg, 2015-06-16 22:38:23]

https://codereview.chromium.org/1186733002/diff/40001/test/mjsunit/es6/built-...
File test/mjsunit/es6/built-in-accessor-names.js (right):

https://codereview.chromium.org/1186733002/diff/40001/test/mjsunit/es6/built-...
test/mjsunit/es6/built-in-accessor-names.js:47: assertGetterName('get buffer',
f.prototype, 'buffer', 1);
On 2015/06/16 16:24:49, arv wrote:
> On 2015/06/15 23:28:34, littledan wrote:
> > On 2015/06/15 16:23:12, arv wrote:
> > > I would probably just change this to `f.prototype.__proto__` or remove the
> > loop
> > > and just test Uint8Array.prototype.__proto__. There are already other
tests
> > > covering that these all have the correct prototype chain.
> > 
> > Where are those tests?
> 
> I assume you have added tests that asserts that f.prototype.__proto__ ===
> %TypedArray%.prototype somewhere?

I'm not sure how I could add that test, as there's no direct way to get at
%TypedArray%.prototype besides through f.prototype.__proto__.

https://codereview.chromium.org/1186733002/diff/80001/src/harmony-typedarray.js
File src/harmony-typedarray.js (right):

https://codereview.chromium.org/1186733002/diff/80001/src/harmony-typedarray....
src/harmony-typedarray.js:51: var TypedArrayPrototype =
GlobalUint8Array.prototype.__proto__;
On 2015/06/16 16:24:49, arv wrote:
> This is the same as TypedArray.prototype which reads better:
> 
> var TypedArrayPrototype = TypedArray.prototype;

TypedArray is not exposed.

[Dan Ehrenberg, 2015-06-16 22:38:29]

The CQ bit was checked by littledan@chromium.org

[Dan Ehrenberg, 2015-06-16 22:38:29]

The patchset sent to the CQ was uploaded after l-g-t-m from arv@chromium.org
Link to the patchset: https://codereview.chromium.org/1186733002/#ps100001
(title: "changes from arv's review")

[commit-bot: I haz the power, 2015-06-16 22:38:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1186733002/100001

[arv (Not doing code reviews), 2015-06-16 22:48:42]

https://codereview.chromium.org/1186733002/diff/40001/test/mjsunit/es6/built-...
File test/mjsunit/es6/built-in-accessor-names.js (right):

https://codereview.chromium.org/1186733002/diff/40001/test/mjsunit/es6/built-...
test/mjsunit/es6/built-in-accessor-names.js:47: assertGetterName('get buffer',
f.prototype, 'buffer', 1);
On 2015/06/16 22:38:22, littledan wrote:
> On 2015/06/16 16:24:49, arv wrote:
> > On 2015/06/15 23:28:34, littledan wrote:
> > > On 2015/06/15 16:23:12, arv wrote:
> > > > I would probably just change this to `f.prototype.__proto__` or remove
the
> > > loop
> > > > and just test Uint8Array.prototype.__proto__. There are already other
> tests
> > > > covering that these all have the correct prototype chain.
> > > 
> > > Where are those tests?
> > 
> > I assume you have added tests that asserts that f.prototype.__proto__ ===
> > %TypedArray%.prototype somewhere?
> 
> I'm not sure how I could add that test, as there's no direct way to get at
> %TypedArray%.prototype besides through f.prototype.__proto__.

The way to do it is something like this:

var TypedArray = Uint8Array.__proto__;
var TypedArrayPrototype = TypedArray.prototype;

for (let f of typedArrays) {
  assertEquals(TypedArray, f.__proto__);
  assertEquals(TypedArrayPrototype, f.prototype.__proto__);
}

And then test the names by themselves.

assertGetterName('get byteOffset', TypedArrayPrototype, 'byteOffset')

https://codereview.chromium.org/1186733002/diff/80001/src/harmony-typedarray.js
File src/harmony-typedarray.js (right):

https://codereview.chromium.org/1186733002/diff/80001/src/harmony-typedarray....
src/harmony-typedarray.js:51: var TypedArrayPrototype =
GlobalUint8Array.prototype.__proto__;
On 2015/06/16 22:38:23, littledan wrote:
> On 2015/06/16 16:24:49, arv wrote:
> > This is the same as TypedArray.prototype which reads better:
> > 
> > var TypedArrayPrototype = TypedArray.prototype;
> 
> TypedArray is not exposed.

You defined it on line 50.

[commit-bot: I haz the power, 2015-06-16 23:39:06]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-06-16 23:39:23]

Patchset 6 (id:??) landed as
https://crrev.com/a10590158260737b256fac3254b4939f48f90095
Cr-Commit-Position: refs/heads/master@{#29057}

[Michael Achenbach, 2015-06-17 09:05:03]

A revert of this CL (patchset #6 id:100001) has been created in
https://codereview.chromium.org/1192433003/ by machenbach@chromium.org.

The reason for reverting is: [Sheriff] Changes layout tests:
http://build.chromium.org/p/client.v8.fyi/builders/V8-Blink%20Linux%2032/buil...

See e.g.:
https://storage.googleapis.com/chromium-layout-test-archives/V8-Blink_Linux_3...

Please upload a blink side needsmanualrebaseline change first for these tests if
the change is intended. Please also add a blink trybot on a reland of this CL..