Bug 147307 - [ES6] Implement Reflect.ownKeys
Summary: [ES6] Implement Reflect.ownKeys
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-07-25 23:28 PDT by Yusuke Suzuki
Modified: 2015-07-26 14:12 PDT (History)
7 users (show)

See Also:

Attachments
Patch (11.95 KB, patch)
2015-07-25 23:31 PDT, Yusuke Suzuki 		no flags Details | Formatted Diff | Diff
Patch (11.91 KB, patch)
2015-07-25 23:35 PDT, Yusuke Suzuki 		sam: review+
Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2015-07-25 23:28:12 PDT 
[ES6] Implement Reflect.ownKeys
Comment 1 Yusuke Suzuki 2015-07-25 23:31:26 PDT 
Created attachment 257533 [details]
Patch
Comment 2 Yusuke Suzuki 2015-07-25 23:35:33 PDT 
Created attachment 257534 [details]
Patch
Comment 3 Sam Weinig 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.
Comment 4 Yusuke Suzuki 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!
Comment 5 Yusuke Suzuki 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
Comment 6 Yusuke Suzuki 2015-07-26 14:03:47 PDT 
Committed r187408: <http://trac.webkit.org/changeset/187408>
Comment 7 mitz 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.
Comment 8 Yusuke Suzuki 2015-07-26 14:12:13 PDT 
Committed r187409: <http://trac.webkit.org/changeset/187409>
Comment 9 Yusuke Suzuki 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!