RESOLVED FIXED147307
[ES6] Implement Reflect.ownKeys 
https://bugs.webkit.org/show_bug.cgi?id=147307
Summary [ES6] Implement Reflect.ownKeys
Yusuke Suzuki
Reported 2015-07-25 23:28:12 PDT
[ES6] Implement Reflect.ownKeys
Attachments
Patch (11.95 KB, patch)
2015-07-25 23:31 PDT, Yusuke Suzuki 		no flags
DetailsFormatted DiffDiff
Patch (11.91 KB, patch)
2015-07-25 23:35 PDT, Yusuke Suzuki 		sam: review+
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Yusuke Suzuki
Comment 1 2015-07-25 23:31:26 PDT
Created attachment 257533 [details] Patch
Yusuke Suzuki
Comment 2 2015-07-25 23:35:33 PDT
Created attachment 257534 [details] Patch
Sam Weinig
Comment 3 2015-07-26 12:26:28 PDT
Comment on attachment 257534 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=257534&action=review > Source/JavaScriptCore/runtime/ObjectConstructor.cpp:280 > + return JSValue::encode(ownPropertyKeys(exec, object, PropertyNameMode::Both, DontEnumPropertiesMode::Exclude)); It's not new in this patch, but I find the name PropertyNameMode::Both confusing since when I am reading it, I don't know what Both applies to. Perhaps an explicit StringsAndSymbols would be better. But again, has nothing to do with this patch.
Yusuke Suzuki
Comment 4 2015-07-26 13:56:00 PDT
Comment on attachment 257534 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=257534&action=review Thanks for your review! >> Source/JavaScriptCore/runtime/ObjectConstructor.cpp:280 >> + return JSValue::encode(ownPropertyKeys(exec, object, PropertyNameMode::Both, DontEnumPropertiesMode::Exclude)); > > It's not new in this patch, but I find the name PropertyNameMode::Both confusing since when I am reading it, I don't know what Both applies to. Perhaps an explicit StringsAndSymbols would be better. But again, has nothing to do with this patch. Make sense. I'll open the issue to rename PropertyNameMode::Both to PropertyNameMode::StringsAndSymbols!
Yusuke Suzuki
Comment 5 2015-07-26 14:00:56 PDT
Comment on attachment 257534 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=257534&action=review >>> Source/JavaScriptCore/runtime/ObjectConstructor.cpp:280 >>> + return JSValue::encode(ownPropertyKeys(exec, object, PropertyNameMode::Both, DontEnumPropertiesMode::Exclude)); >> >> It's not new in this patch, but I find the name PropertyNameMode::Both confusing since when I am reading it, I don't know what Both applies to. Perhaps an explicit StringsAndSymbols would be better. But again, has nothing to do with this patch. > > Make sense. I'll open the issue to rename PropertyNameMode::Both to PropertyNameMode::StringsAndSymbols! Opened the issue to fix it. https://bugs.webkit.org/show_bug.cgi?id=147311
Yusuke Suzuki
Comment 6 2015-07-26 14:03:47 PDT
Committed r187408: <http://trac.webkit.org/changeset/187408>
mitz
Comment 7 2015-07-26 14:07:35 PDT
Comment on attachment 257534 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=257534&action=review > Source/JavaScriptCore/runtime/ObjectConstructor.cpp:629 > + ASSERT(!identifier.isSymbol()); This statement here broke the Debug build.
Yusuke Suzuki
Comment 8 2015-07-26 14:12:13 PDT
Committed r187409: <http://trac.webkit.org/changeset/187409>
Yusuke Suzuki
Comment 9 2015-07-26 14:12:54 PDT
(In reply to comment #7) > Comment on attachment 257534 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=257534&action=review > > > Source/JavaScriptCore/runtime/ObjectConstructor.cpp:629 > > + ASSERT(!identifier.isSymbol()); > > This statement here broke the Debug build. Thanks, I've fixed it!
Note You need to log in before you can comment on or make changes to this bug.