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
I will take this one
Spec for this feature http://tc39.github.io/proposal-object-values-entries/
Created attachment 285140 [details] Patch Patch for review
Created attachment 285142 [details] Patch Patch for review after rebase
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
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 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
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 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.
(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.
Created attachment 285494 [details] Patch
Created attachment 285495 [details] Patch Fix review comments & update changelogs
Created attachment 285496 [details] Patch Fix review comments & update changelogs & rebase
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).
Created attachment 285590 [details] Patch Check build after fixing comments
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`.
Created attachment 285676 [details] Patch Fix build
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 on attachment 285676 [details] Patch You should add a test for Object.values.length === 1
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.
Committed r204358: <http://trac.webkit.org/changeset/204358> All reviewed patches have been landed. Closing bug.
*** Bug 150131 has been marked as a duplicate of this bug. ***