WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
160410
[ES2016] Implement Object.values
https://bugs.webkit.org/show_bug.cgi?id=160410
Summary
[ES2016] Implement Object.values
GSkachkov
Reported
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
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
saam
: review+
Details
Formatted Diff
Diff
Show Obsolete
(6)
View All
Add attachment
proposed patch, testcase, etc.
GSkachkov
Comment 1
2016-08-01 12:54:56 PDT
I will take this one
GSkachkov
Comment 2
2016-08-01 13:08:49 PDT
Spec for this feature
http://tc39.github.io/proposal-object-values-entries/
GSkachkov
Comment 3
2016-08-02 14:40:28 PDT
Created
attachment 285140
[details]
Patch Patch for review
GSkachkov
Comment 4
2016-08-02 14:51:58 PDT
Created
attachment 285142
[details]
Patch Patch for review after rebase
Build Bot
Comment 5
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
Build Bot
Comment 6
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
Build Bot
Comment 7
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
Build Bot
Comment 8
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
Saam Barati
Comment 9
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.
GSkachkov
Comment 10
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.
GSkachkov
Comment 11
2016-08-06 12:49:59 PDT
Created
attachment 285494
[details]
Patch
GSkachkov
Comment 12
2016-08-06 12:51:30 PDT
Created
attachment 285495
[details]
Patch Fix review comments & update changelogs
GSkachkov
Comment 13
2016-08-06 13:53:42 PDT
Created
attachment 285496
[details]
Patch Fix review comments & update changelogs & rebase
Yusuke Suzuki
Comment 14
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).
GSkachkov
Comment 15
2016-08-08 14:40:55 PDT
Created
attachment 285590
[details]
Patch Check build after fixing comments
Yusuke Suzuki
Comment 16
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`.
GSkachkov
Comment 17
2016-08-09 14:35:39 PDT
Created
attachment 285676
[details]
Patch Fix build
GSkachkov
Comment 18
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
Saam Barati
Comment 19
2016-08-09 14:55:12 PDT
Comment on
attachment 285676
[details]
Patch You should add a test for Object.values.length === 1
Saam Barati
Comment 20
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.
GSkachkov
Comment 21
2016-08-10 15:37:27 PDT
Committed
r204358
: <
http://trac.webkit.org/changeset/204358
> All reviewed patches have been landed. Closing bug.
JF Bastien
Comment 22
2017-03-13 09:10:51 PDT
***
Bug 150131
has been marked as a duplicate of this bug. ***
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug