Bug 154759 - [ES6] Implement Proxy.[[DefineOwnProperty]]
Summary: [ES6] Implement Proxy.[[DefineOwnProperty]]
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Saam Barati
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-02-26 15:59 PST by Saam Barati
Modified: 2016-03-03 17:08 PST (History)
10 users (show)

See Also:


Attachments
WIP (9.09 KB, patch)
2016-02-28 13:10 PST, Saam Barati
no flags Details | Formatted Diff | Diff
patch (28.20 KB, patch)
2016-02-28 14:58 PST, Saam Barati
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Saam Barati 2016-02-26 15:59:52 PST
...
Comment 1 Saam Barati 2016-02-28 13:10:17 PST
Created attachment 272462 [details]
WIP
Comment 2 Saam Barati 2016-02-28 14:58:05 PST
Created attachment 272466 [details]
patch
Comment 3 Geoffrey Garen 2016-03-02 14:42:26 PST
Comment on attachment 272466 [details]
patch

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

r=me

> Source/JavaScriptCore/runtime/ObjectConstructor.h:96
> +inline JSObject* fromPropertyDescriptor(ExecState* exec, const PropertyDescriptor& descriptor)

Let's call this constructObject or constructObjectFromPropertyDescriptor.

> Source/JavaScriptCore/runtime/ProxyObject.cpp:620
> +    } else {

This should be an early return instead of an else.
Comment 4 Mark Lam 2016-03-02 14:55:53 PST
Comment on attachment 272466 [details]
patch

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

I see lots of opportunities to use vm.hadException() instead of exec->hadException().  Please change where applicable.

> Source/JavaScriptCore/runtime/ObjectConstructor.cpp:236
>      if (exec->hadException())
>          return jsUndefined();

You can check for (!result) here instead of (exec->hadException()) and save on the exec->vm() lookup.

> Source/JavaScriptCore/runtime/ObjectConstructor.h:99
> +    if (exec->hadException())

You should cache the vm from exec->vm() in a local first especially since you need the vm below.

> Source/JavaScriptCore/runtime/ProxyObject.cpp:581
> +    if (exec->hadException())

Replace with a null check of descriptorObject, or vm.hasException() instead.

> Source/JavaScriptCore/runtime/ProxyObject.cpp:586
> +    arguments.append(identifierToSafePublicJSValue(vm, Identifier::fromUid(&vm, propertyName.uid())));

Should we be trapping private symbols?

> Source/JavaScriptCore/runtime/ProxyObject.cpp:605
> +    bool isConfigurableFalse;

The spec calls this "settingConfigFalse".  Maybe we should use that name here too.  I do thing "settingConfigFalse" is clearer than "isConfigurableFalse".
Comment 5 Saam Barati 2016-03-03 17:07:44 PST
landed in:
http://trac.webkit.org/changeset/197533
Comment 6 Saam Barati 2016-03-03 17:08:55 PST
Comment on attachment 272466 [details]
patch

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

>> Source/JavaScriptCore/runtime/ObjectConstructor.cpp:236
>>          return jsUndefined();
> 
> You can check for (!result) here instead of (exec->hadException()) and save on the exec->vm() lookup.

did.

>> Source/JavaScriptCore/runtime/ObjectConstructor.h:96
>> +inline JSObject* fromPropertyDescriptor(ExecState* exec, const PropertyDescriptor& descriptor)
> 
> Let's call this constructObject or constructObjectFromPropertyDescriptor.

I went with constructObjectFromPropertyDescriptor

>> Source/JavaScriptCore/runtime/ObjectConstructor.h:99
>> +    if (exec->hadException())
> 
> You should cache the vm from exec->vm() in a local first especially since you need the vm below.

Done.

>> Source/JavaScriptCore/runtime/ProxyObject.cpp:581
>> +    if (exec->hadException())
> 
> Replace with a null check of descriptorObject, or vm.hasException() instead.

went with vm.exception()

>> Source/JavaScriptCore/runtime/ProxyObject.cpp:586
>> +    arguments.append(identifierToSafePublicJSValue(vm, Identifier::fromUid(&vm, propertyName.uid())));
> 
> Should we be trapping private symbols?

No we shouldn't. The landed patch fixes it.

>> Source/JavaScriptCore/runtime/ProxyObject.cpp:605
>> +    bool isConfigurableFalse;
> 
> The spec calls this "settingConfigFalse".  Maybe we should use that name here too.  I do thing "settingConfigFalse" is clearer than "isConfigurableFalse".

I went with "settingConfigurableToFalse"

>> Source/JavaScriptCore/runtime/ProxyObject.cpp:620
>> +    } else {
> 
> This should be an early return instead of an else.

done.