Bug 153799

Summary: [JSC] Implement Object.getOwnPropertyDescriptors() proposal
Product: WebKit Reporter: Caitlin Potter (:caitp) <caitp>
Component: JavaScriptCoreAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, caitp, cdumez, commit-queue, darin, keith_miller, kling, ljharb, mark.lam, msaboff, rniwa, sbarati
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
[JSC] Implement Object.getOwnPropertyDescriptors() proposal
none
Patch
none
Archive of layout-test-results from ews103 for mac-yosemite
none
Archive of layout-test-results from ews104 for mac-yosemite-wk2
none
Archive of layout-test-results from ews117 for mac-yosemite
none
Patch
none
Patch none

Description Caitlin Potter (:caitp) 2016-02-02 13:34:55 PST
[JSC] Implement Object.getOwnPropertyDescriptors() proposal
Comment 1 Caitlin Potter (:caitp) 2016-02-02 13:36:22 PST
Created attachment 270511 [details]
[JSC] Implement Object.getOwnPropertyDescriptors() proposal
Comment 2 Caitlin Potter (:caitp) 2016-02-02 13:37:55 PST
This proposal (https://github.com/tc39/proposal-object-getownpropertydescriptors) was bumped to Stage 3 at the last meeting, thought I'd send a patch for implementing it.

Tests are ported from my V8 implementation, which is itself influenced by the new Test262 entries
Comment 3 Caitlin Potter (:caitp) 2016-02-02 13:52:37 PST
Created attachment 270513 [details]
Patch
Comment 4 Geoffrey Garen 2016-02-02 14:15:29 PST
Comment on attachment 270513 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=270513&action=review

r=me

> Source/JavaScriptCore/tests/es6/Object_static_methods_Object.getOwnPropertyDescriptors.js:85
> +// FIXME: enable test once Proxy has been implemented.

See below.

> Source/JavaScriptCore/tests/es6/Object_static_methods_Object.getOwnPropertyDescriptors.js:125
> +// FIXME: enable test once Proxy has been implemented.

Let's make this a separate test, expected to fail. That way, updating it once we have Proxies will be automatic.
Comment 5 Build Bot 2016-02-02 14:39:46 PST
Comment on attachment 270513 [details]
Patch

Attachment 270513 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/774088

New failing tests:
js/Object-getOwnPropertyNames.html
Comment 6 Build Bot 2016-02-02 14:39:51 PST
Created attachment 270518 [details]
Archive of layout-test-results from ews103 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 7 Build Bot 2016-02-02 14:44:11 PST
Comment on attachment 270513 [details]
Patch

Attachment 270513 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/774091

New failing tests:
js/Object-getOwnPropertyNames.html
Comment 8 Build Bot 2016-02-02 14:44:16 PST
Created attachment 270519 [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 Build Bot 2016-02-02 14:45:17 PST
Comment on attachment 270513 [details]
Patch

Attachment 270513 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/774080

New failing tests:
js/Object-getOwnPropertyNames.html
Comment 10 Build Bot 2016-02-02 14:45:22 PST
Created attachment 270521 [details]
Archive of layout-test-results from ews117 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 11 Caitlin Potter (:caitp) 2016-02-02 14:51:25 PST
Created attachment 270524 [details]
Patch
Comment 12 Caitlin Potter (:caitp) 2016-02-02 14:52:35 PST
Comment on attachment 270513 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=270513&action=review

>> Source/JavaScriptCore/tests/es6/Object_static_methods_Object.getOwnPropertyDescriptors.js:125
>> +// FIXME: enable test once Proxy has been implemented.
> 
> Let's make this a separate test, expected to fail. That way, updating it once we have Proxies will be automatic.

Done, and fixed the failing layout test
Comment 13 Caitlin Potter (:caitp) 2016-02-02 14:52:39 PST
Comment on attachment 270513 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=270513&action=review

>> Source/JavaScriptCore/tests/es6/Object_static_methods_Object.getOwnPropertyDescriptors.js:125
>> +// FIXME: enable test once Proxy has been implemented.
> 
> Let's make this a separate test, expected to fail. That way, updating it once we have Proxies will be automatic.

Done, and fixed the failing layout test
Comment 14 Darin Adler 2016-02-02 16:47:31 PST
Comment on attachment 270524 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=270524&action=review

r=ggaren

> Source/JavaScriptCore/ChangeLog:21
> +        * runtime/ObjectConstructor.cpp:
> +        (JSC::objectConstructorGetOwnPropertyDescriptors):
> +        * runtime/ObjectConstructor.h:
> +        * tests/es6.yaml:
> +        * tests/es6/Object_static_methods_Object.getOwnPropertyDescriptors-proxy.js: Added.
> +        (shouldBe):
> +        (shouldThrow):
> +        (shouldBeDataProperty):
> +        * tests/es6/Object_static_methods_Object.getOwnPropertyDescriptors.js: Added.
> +        (shouldBe):
> +        (shouldThrow):
> +        (shouldBeDataProperty):
> +        (testMeta):
> +        (testPrototypeProperties.F):

Not a great change log. Would be nice to write comments here and remove the lists of function names where they aren’t helpful.

> Source/JavaScriptCore/runtime/ObjectConstructor.cpp:261
> +    for (auto it = properties.begin(); it != properties.end(); ++it) {
> +        const Identifier& propertyName = *it;

Should use a modern for loop:

    for (auto& propertyName : properties) {
Comment 15 Caitlin Potter (:caitp) 2016-02-02 18:12:59 PST
Created attachment 270538 [details]
Patch
Comment 16 Caitlin Potter (:caitp) 2016-02-02 18:13:54 PST
Comment on attachment 270524 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=270524&action=review

>> Source/JavaScriptCore/ChangeLog:21
>> +        (testPrototypeProperties.F):
> 
> Not a great change log. Would be nice to write comments here and remove the lists of function names where they aren’t helpful.

Done

>> Source/JavaScriptCore/runtime/ObjectConstructor.cpp:261
>> +        const Identifier& propertyName = *it;
> 
> Should use a modern for loop:
> 
>     for (auto& propertyName : properties) {

Done
Comment 17 WebKit Commit Bot 2016-02-02 19:17:59 PST
Comment on attachment 270538 [details]
Patch

Clearing flags on attachment: 270538

Committed r196040: <http://trac.webkit.org/changeset/196040>
Comment 18 WebKit Commit Bot 2016-02-02 19:18:07 PST
All reviewed patches have been landed.  Closing bug.