1119217 - Implement %TypedArray%.prototype.{keys, values, entries}

Status: RESOLVED FIXED

(Core :: JavaScript: Standard Library, defect)

Description

[Tom S [:evilpie]]

We already have %TypedArray%.prototype.[@@iterator], but we need to adjust it after implementing values:
"The initial value of the @@iterator property is the same function object as the initial value of the %TypedArray%.prototype.values property." (22.2.3.31)

Comment 1

[Tom S [:evilpie]]

Attachment: v1 - Implement %TypedArray%.prototype.{keys, values, entries}

Comment 2

[Till Schneidereit [:till]]

Comment on attachment 8547084 [details] [diff] [review]
v1 - Implement %TypedArray%.prototype.{keys, values, entries}

Review of attachment 8547084 [details] [diff] [review]:
-----------------------------------------------------------------

Most excellent. r=me with nits addressed.

::: js/src/builtin/TypedArray.js
@@ +1,5 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +// ES6 draft rev29 (2014/12/06) 22.2.3.6 %TypedArray%.prototype.entries()

New draft is out (and even referenced one function down), so please update the reference here and below.

@@ +11,5 @@
> +    if (!IsObject(O) || !IsTypedArray(O)) {
> +        return callFunction(CallTypedArrayMethodIfWrapped, O, "TypedArrayEntries");
> +    }
> +
> +    // Step 4-6. ToDo: detachment

Please link to a bug covering detachment for all methods here and below.

::: js/src/vm/CommonPropertyNames.h
@@ +210,5 @@
>      macro(useGrouping, useGrouping, "useGrouping") \
>      macro(useAsm, useAsm, "use asm") \
>      macro(useStrict, useStrict, "use strict") \
>      macro(value, value, "value") \
> +    macro(values, values, "values") \

nice

::: js/src/vm/TypedArrayObject.cpp
@@ +696,5 @@
>  
> +static bool
> +FinishTypedArrayInit(JSContext *cx, HandleObject ctor, HandleObject proto)
> +{
> +    // Define values and @@iterator manually, because they are supposed to be the same object.

Use "`" or "|" around "values" and "@@iterator" to denote them as code.

@@ +706,5 @@
> +    RootedValue funValue(cx, ObjectValue(*fun));
> +    if (!JSObject::defineProperty(cx, proto, cx->names().values, funValue))
> +        return false;
> +
> +#ifdef JS_HAS_SYMBOLS

Do we even do symbol-less builds anymore? We should probably rip out the ifdefs soon-ish.

@@ +802,5 @@
>  }
>  
>  /* static */ const JSFunctionSpec
>  TypedArrayObject::protoFunctions[] = {
> +    JS_SELF_HOSTED_SYM_FN(iterator, "TypedArrayValues", 0, JSPROP_DEFINE_LATE),

I would move this to the bottom so it comes right after "values".

@@ +819,5 @@
>      JS_SELF_HOSTED_FN("reverse", "TypedArrayReverse", 0, 0),
>      JS_SELF_HOSTED_FN("some", "TypedArraySome", 2, 0),
> +    JS_SELF_HOSTED_FN("entries", "TypedArrayEntries", 0, 0),
> +    JS_SELF_HOSTED_FN("keys", "TypedArrayKeys", 0, 0),
> +    JS_SELF_HOSTED_FN("values", "TypedArrayValues", 0, JSPROP_DEFINE_LATE),

So this essentially has documentation value (no pun intended), right? That should at least be pointed out with a comment referring to the function to `FinishTypedArrayInit`.

::: js/xpconnect/tests/chrome/test_xrayToJS.xul
@@ +178,5 @@
>    }
>    gPrototypeProperties['TypedArray'] =
>      ["length", "buffer", "byteLength", "byteOffset", kIteratorSymbol, "subarray",
>       "set", "copyWithin", "find", "findIndex", "indexOf", "lastIndexOf", "reverse",
> +     "join", "every", "some", "reduce", "reduceRight", "entries", "keys", "values"];

An xpconnect peer needs to r+ this. (Though I wonder how useful that is, really. What's a scenario where we'd get an r- for implementing a spec'ed method?)

Comment 3

[Tom S [:evilpie]]

Can you give a comment if adding those functions to test_xrayToJS.xul is fine? I actually allowed some other patches before that just added functions here as well.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Yes, that looks fine.  The idea is that the things that should show up there are whatever would appear on vanilla TypedArray prototype objects that no one has messed with, so as we add new stuff to typed arrays they will need to be added to that test.

Comment 5

[Tom S [:evilpie]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=335b720ef94f
https://hg.mozilla.org/integration/mozilla-inbound/rev/93c6ac70dc60

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/93c6ac70dc60

Comment 7

[Florian Scholz (Open Web Docs)]

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/TypedArray/keys
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/TypedArray/values
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/TypedArray/entries
https://developer.mozilla.org/en-US/Firefox/Releases/37#JavaScript