Bug 160410 - [ES2016] Implement Object.values
Summary: [ES2016] Implement Object.values
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
: 150131 (view as bug list)
Depends on:
Blocks: 160392 160844
  Show dependency treegraph
 
Reported: 2016-08-01 10:45 PDT by GSkachkov
Modified: 2017-03-13 09:10 PDT (History)
13 users (show)

See Also:


Attachments
Patch (8.29 KB, patch)
2016-08-02 14:40 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (8.28 KB, patch)
2016-08-02 14:51 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-yosemite (959.33 KB, application/zip)
2016-08-02 15:38 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews104 for mac-yosemite-wk2 (818.54 KB, application/zip)
2016-08-02 15:42 PDT, Build Bot
no flags Details
Patch (11.71 KB, patch)
2016-08-06 12:49 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (11.71 KB, patch)
2016-08-06 12:51 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (11.02 KB, patch)
2016-08-06 13:53 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (32.57 KB, patch)
2016-08-08 14:40 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (35.72 KB, patch)
2016-08-09 14:35 PDT, GSkachkov
sbarati: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description GSkachkov 2016-08-01 10:45:56 PDT
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
Comment 1 GSkachkov 2016-08-01 12:54:56 PDT
I will take this one
Comment 2 GSkachkov 2016-08-01 13:08:49 PDT
Spec for this feature http://tc39.github.io/proposal-object-values-entries/
Comment 3 GSkachkov 2016-08-02 14:40:28 PDT
Created attachment 285140 [details]
Patch

Patch for review
Comment 4 GSkachkov 2016-08-02 14:51:58 PDT
Created attachment 285142 [details]
Patch

Patch for review after rebase
Comment 5 Build Bot 2016-08-02 15:38:47 PDT
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
Comment 6 Build Bot 2016-08-02 15:38:51 PDT
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
Comment 7 Build Bot 2016-08-02 15:42:20 PDT
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
Comment 8 Build Bot 2016-08-02 15:42:24 PDT
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
Comment 9 Saam Barati 2016-08-02 15:46:30 PDT
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.
Comment 10 GSkachkov 2016-08-03 13:49:29 PDT
(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.
Comment 11 GSkachkov 2016-08-06 12:49:59 PDT
Created attachment 285494 [details]
Patch
Comment 12 GSkachkov 2016-08-06 12:51:30 PDT
Created attachment 285495 [details]
Patch

Fix review comments & update changelogs
Comment 13 GSkachkov 2016-08-06 13:53:42 PDT
Created attachment 285496 [details]
Patch

Fix review comments & update changelogs & rebase
Comment 14 Yusuke Suzuki 2016-08-06 19:51:18 PDT
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).
Comment 15 GSkachkov 2016-08-08 14:40:55 PDT
Created attachment 285590 [details]
Patch

Check build after fixing comments
Comment 16 Yusuke Suzuki 2016-08-09 07:01:31 PDT
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`.
Comment 17 GSkachkov 2016-08-09 14:35:39 PDT
Created attachment 285676 [details]
Patch

Fix build
Comment 18 GSkachkov 2016-08-09 14:37:41 PDT
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
Comment 19 Saam Barati 2016-08-09 14:55:12 PDT
Comment on attachment 285676 [details]
Patch

You should add a test for Object.values.length === 1
Comment 20 Saam Barati 2016-08-09 15:16:48 PDT
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.
Comment 21 GSkachkov 2016-08-10 15:37:27 PDT
Committed r204358: <http://trac.webkit.org/changeset/204358>

All reviewed patches have been landed.  Closing bug.
Comment 22 JF Bastien 2017-03-13 09:10:51 PDT
*** Bug 150131 has been marked as a duplicate of this bug. ***