[JSC] Implement Object.getOwnPropertyDescriptors() proposal

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Caitlin Potter (:caitp)]

[JSC] Implement Object.getOwnPropertyDescriptors() proposal

[Caitlin Potter (:caitp)]

Created attachment 270511 [details]
[JSC] Implement Object.getOwnPropertyDescriptors() proposal

[Caitlin Potter (:caitp)]

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

[Caitlin Potter (:caitp)]

Created attachment 270513 [details]
Patch

[Geoffrey Garen]

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.

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Caitlin Potter (:caitp)]

Created attachment 270524 [details]
Patch

[Caitlin Potter (:caitp)]

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

[Caitlin Potter (:caitp)]

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

[Darin Adler]

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) {

[Caitlin Potter (:caitp)]

Created attachment 270538 [details]
Patch

[Caitlin Potter (:caitp)]

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

[WebKit Commit Bot]

Comment on attachment 270538 [details]
Patch

Clearing flags on attachment: 270538

Committed r196040: <http://trac.webkit.org/changeset/196040>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.