Bug 154759

Summary: [ES6] Implement Proxy.[[DefineOwnProperty]]
Product: WebKit Reporter: Saam Barati <saam>
Component: JavaScriptCoreAssignee: Saam Barati <saam>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, fpizlo, ggaren, gskachkov, keith_miller, mark.lam, msaboff, oliver, sukolsak, ysuzuki
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
WIP
none
patch ggaren: review+

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.