Bug 153799 - [JSC] Implement Object.getOwnPropertyDescriptors() proposal
Summary: [JSC] Implement Object.getOwnPropertyDescriptors() proposal
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-02-02 13:34 PST by Caitlin Potter (:caitp)
Modified: 2016-02-03 08:49 PST (History)
12 users (show)

See Also:


Attachments
[JSC] Implement Object.getOwnPropertyDescriptors() proposal (13.84 KB, patch)
2016-02-02 13:36 PST, Caitlin Potter (:caitp)
no flags Details | Formatted Diff | Diff
Patch (13.85 KB, patch)
2016-02-02 13:52 PST, Caitlin Potter (:caitp)
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-yosemite (902.75 KB, application/zip)
2016-02-02 14:39 PST, Build Bot
no flags Details
Archive of layout-test-results from ews104 for mac-yosemite-wk2 (748.08 KB, application/zip)
2016-02-02 14:44 PST, Build Bot
no flags Details
Archive of layout-test-results from ews117 for mac-yosemite (817.07 KB, application/zip)
2016-02-02 14:45 PST, Build Bot
no flags Details
Patch (17.09 KB, patch)
2016-02-02 14:51 PST, Caitlin Potter (:caitp)
no flags Details | Formatted Diff | Diff
Patch (16.90 KB, patch)
2016-02-02 18:12 PST, Caitlin Potter (:caitp)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.