Bug 160410 – [ES2016] Implement Object.values

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[GSkachkov]

Short example from ES2016 compatibility table:

/////////
var obj = Object.create({ a: "qux", d: "qux" });
obj.a = "foo"; obj.b = "bar"; obj.c = "baz";
var v = Object.values(obj);
return Array.isArray(v) && String(v) === "foo,bar,bad";
///////
See description by following url https://github.com/tc39/proposal-object-values-entries

[GSkachkov]

I will take this one

[GSkachkov]

Spec for this feature http://tc39.github.io/proposal-object-values-entries/

[GSkachkov]

Created attachment 285140 [details]
Patch

Patch for review

[GSkachkov]

Created attachment 285142 [details]
Patch

Patch for review after rebase

[Build Bot]

Comment on attachment 285142 [details]
Patch

Attachment 285142 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1800734

New failing tests:
js/Object-getOwnPropertyNames.html

[Build Bot]

Created attachment 285149 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 285142 [details]
Patch

Attachment 285142 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1800736

New failing tests:
js/Object-getOwnPropertyNames.html

[Build Bot]

Created attachment 285150 [details]
Archive of layout-test-results from ews104 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Saam Barati]

Comment on attachment 285142 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=285142&action=review

> Source/JavaScriptCore/runtime/ObjectConstructor.cpp:781
> +// http://tc39.github.io/proposal-object-values-entries/

Lets provide links to the spec instead of proposals.

> Source/JavaScriptCore/runtime/ObjectConstructor.cpp:783
> +JSArray* ownEnumerableProperties(ExecState* exec, JSObject* object, ResultKind resultKind, DontEnumPropertiesMode dontEnumPropertiesMode)

I'd really like to see this written in JavaScript as a builtin instead.
It'd probably require making certain Reflect APIs and maybe Reflect itself available as a private symbol.
I think that would remove a certain class of bugs and might even be faster if it gets inlined into the caller.

> Source/JavaScriptCore/runtime/ObjectConstructor.cpp:802
> +            keys->push(exec, jsOwnedString(exec, identifier.string()));

can push() throw an exception?

> Source/JavaScriptCore/runtime/ObjectConstructor.cpp:813
> +            JSValue result = baseValue.get(exec, identifier, slot);

This needs an exception check afterwards if you keep it in C++ code

> Source/JavaScriptCore/runtime/ObjectConstructor.h:36
> +EncodedJSValue JSC_HOST_CALL ownEnumerablePropertyValues(ExecState*);

Not used.

> Source/JavaScriptCore/runtime/ObjectConstructor.h:43
> +enum class ResultKind {
> +    Key = 1 << 0,
> +    Value = 1 << 1,
> +    KeyAndValue = Key | Value,
> +};

Don't we already have an enum that represents this?

> JSTests/stress/object-values.js:1
> +var obj = Object.create({ a: "qux", d: "qux" });

Lets add some Proxy tests and ensure proper methods are called.

[GSkachkov]

(In reply to comment #9)
> Comment on attachment 285142 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=285142&action=review
> 
> > Source/JavaScriptCore/runtime/ObjectConstructor.cpp:781
> > +// http://tc39.github.io/proposal-object-values-entries/
> 
> Lets provide links to the spec instead of proposals.
> 
> > Source/JavaScriptCore/runtime/ObjectConstructor.cpp:783
> > +JSArray* ownEnumerableProperties(ExecState* exec, JSObject* object, ResultKind resultKind, DontEnumPropertiesMode dontEnumPropertiesMode)
> 
> I'd really like to see this written in JavaScript as a builtin instead.
> It'd probably require making certain Reflect APIs and maybe Reflect itself
> available as a private symbol.
> I think that would remove a certain class of bugs and might even be faster
> if it gets inlined into the caller.
> 
Ok. I'll back with updated patch

> > Source/JavaScriptCore/runtime/ObjectConstructor.cpp:802
> > +            keys->push(exec, jsOwnedString(exec, identifier.string()));
> 
> can push() throw an exception?
> 
> > Source/JavaScriptCore/runtime/ObjectConstructor.cpp:813
> > +            JSValue result = baseValue.get(exec, identifier, slot);
> 
> This needs an exception check afterwards if you keep it in C++ code
> 
> > Source/JavaScriptCore/runtime/ObjectConstructor.h:36
> > +EncodedJSValue JSC_HOST_CALL ownEnumerablePropertyValues(ExecState*);
> 
> Not used.
> 
> > Source/JavaScriptCore/runtime/ObjectConstructor.h:43
> > +enum class ResultKind {
> > +    Key = 1 << 0,
> > +    Value = 1 << 1,
> > +    KeyAndValue = Key | Value,
> > +};
> 
> Don't we already have an enum that represents this?
> 
> > JSTests/stress/object-values.js:1
> > +var obj = Object.create({ a: "qux", d: "qux" });
> 
> Lets add some Proxy tests and ensure proper methods are called.

[GSkachkov]

Created attachment 285494 [details]
Patch

[GSkachkov]

Created attachment 285495 [details]
Patch

Fix review comments & update changelogs

[GSkachkov]

Created attachment 285496 [details]
Patch

Fix review comments & update changelogs & rebase

[Yusuke Suzuki]

Comment on attachment 285496 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=285496&action=review

Super nice!

> Source/JavaScriptCore/builtins/ObjectConstructor.js:34
> +        let nextKey =  ownKeys[i];

Let's check `If Type(key) is String, then` in the spec 4-a since we can get Symbol here.
(And adding a test for that is very nice thing)

> Source/JavaScriptCore/builtins/ObjectConstructor.js:38
> +                properties.push(object[nextKey]);

Array.prototype.push can be replaced by users.
So, let's use `properties.@push(object[nextKey])`. (And adding the test for replacing Array.prototype.push is good).

> Source/JavaScriptCore/builtins/ObjectConstructor.js:46
> +function values(o)

Right, the spec says this argument as O. But `object` is more readable I think :)

> Source/JavaScriptCore/builtins/ObjectConstructor.js:54
> +    if (o === null)
> +        throw new @TypeError("null is not an object (evaluating 'Object.values(value)')");
> +
> +    if (typeof o === "undefined")
> +        throw new @TypeError("undefined is not an object (evaluating 'Object.values(value)')");

Use `if (object == null)` to unify the above 2 errors. See the other builtin JS for the error message cases. (e.g. ArrayPrototype.js).

> Source/JavaScriptCore/builtins/ObjectConstructor.js:56
> +    return @enumerableOwnProperties(o, 'value');

How about using builtin intrinsic constant (for example, @IterationKindValue) instead of the string "value"?
We can easily expose C++ enums to builtin JS world by using builtin intrinsic constant. For example, in GeneratorPrototype.js, we use @GeneratorResumeModeNormal etc.
And unifying XXXIterationKind to one enum IterationKind is good cleaning up (For example, currently, there are ArrayIterationKind, MapIterationKind, and SetIterationKind in C++ world).

[GSkachkov]

Created attachment 285590 [details]
Patch

Check build after fixing comments

[Yusuke Suzuki]

Comment on attachment 285590 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=285590&action=review

Great! Maybe SerializedScriptValue.cpp needs some cares, but other part seems almost complete!

> Source/JavaScriptCore/builtins/ObjectConstructor.js:29
> +{

Let's set "use strict" ;)

> Source/JavaScriptCore/runtime/IterationKind.h:27
> +#define IterationKind_h

Use #pragma once for newly added headers.

> Source/JavaScriptCore/runtime/ObjectConstructor.cpp:87
> +  values                    JSBuiltin                                   DontEnum|Function 2

I think it should be `1`.

[GSkachkov]

Created attachment 285676 [details]
Patch

Fix build

[GSkachkov]

Comment on attachment 285590 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=285590&action=review

>> Source/JavaScriptCore/builtins/ObjectConstructor.js:29
>> +{
> 
> Let's set "use strict" ;)

Done

>> Source/JavaScriptCore/runtime/IterationKind.h:27
>> +#define IterationKind_h
> 
> Use #pragma once for newly added headers.

Done

>> Source/JavaScriptCore/runtime/ObjectConstructor.cpp:87
>> +  values                    JSBuiltin                                   DontEnum|Function 2
> 
> I think it should be `1`.

Done

[Saam Barati]

Comment on attachment 285676 [details]
Patch

You should add a test for Object.values.length === 1

[Saam Barati]

Comment on attachment 285676 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=285676&action=review

r=me

> Source/JavaScriptCore/builtins/ObjectConstructor.js:36
> +        let nextKey =  ownKeys[i];

style: one too many spaces

> Source/JavaScriptCore/builtins/ObjectConstructor.js:42
> +            // Will be more soon, at least 'key+value' and possible 'key'

This should either be a FIXME with a bug or you can remove the comment.

[GSkachkov]

Committed r204358: <http://trac.webkit.org/changeset/204358>

All reviewed patches have been landed.  Closing bug.

[JF Bastien]

*** Bug 150131 has been marked as a duplicate of this bug. ***