Bug 154759 – [ES6] Implement Proxy.[[DefineOwnProperty]]

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Saam Barati]

...

[Saam Barati]

Created attachment 272462 [details]
WIP

[Saam Barati]

Created attachment 272466 [details]
patch

[Geoffrey Garen]

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.

[Mark Lam]

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".

[Saam Barati]

landed in:
http://trac.webkit.org/changeset/197533

[Saam Barati]

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.