Bug 154425

Summary: [ES6] Implement Proxy.[[Call]]
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   
Bug Depends on: 154853    
Bug Blocks:    
Attachments:
Description Flags
patch
none
patch
mark.lam: review+
patch for landing mark.lam: review+

Description Saam Barati 2016-02-18 16:46:24 PST
...
Comment 1 Saam Barati 2016-02-18 18:22:30 PST
Created attachment 271721 [details]
patch
Comment 2 Saam Barati 2016-02-18 23:02:38 PST
Created attachment 271736 [details]
patch

forgot to update some es6.yaml tests
Comment 3 Mark Lam 2016-02-19 12:22:42 PST
Comment on attachment 271736 [details]
patch

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

r=me with fixes.

> Source/JavaScriptCore/runtime/ProxyObject.cpp:71
> +    CallData ignored;
> +    m_isCallable = jsCast<JSObject*>(target)->methodTable(vm)->getCallData(jsCast<JSObject*>(target), ignored) != CallTypeNone;
> +
>      m_target.set(vm, this, jsCast<JSObject*>(target));

nit: you could factor out the jsCast of target and make this code a little less verbose and easier to read:

JSObject* targetObj =  jsCast<JSObject*>(target);
m_isCallable = targetObj->methodTable(vm)->getCallData(targetObj, ignored) != CallTypeNone;
m_target.set(vm, this, targetObj);

> Source/JavaScriptCore/runtime/ProxyObject.cpp:304
> +    if (handlerValue.isNull())
> +        return throwVMTypeError(exec, ASCIILiteral("Proxy 'handler' is null. It should be an Object."));

Do we need this check?  ProxyObject::finishCreation() already guarantees that handlerValue isObject().

> Source/JavaScriptCore/tests/stress/proxy-call.js:103
> +        proxy.call(thisValue, 20, 45, "foo");

Shouldn't you assert(called) followed by set called = false here?

> Source/JavaScriptCore/tests/stress/proxy-call.js:135
> +        assert(proxy.call(thisValue, 20, 45, "foo") === thisValue);

Ditto: assert called and reset it after here?

> Source/JavaScriptCore/tests/stress/proxy-call.js:225
> +        called = false;

Don't you want to assert(called) before this?
Comment 4 Saam Barati 2016-02-19 14:18:56 PST
Comment on attachment 271736 [details]
patch

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

>> Source/JavaScriptCore/runtime/ProxyObject.cpp:71
>>      m_target.set(vm, this, jsCast<JSObject*>(target));
> 
> nit: you could factor out the jsCast of target and make this code a little less verbose and easier to read:
> 
> JSObject* targetObj =  jsCast<JSObject*>(target);
> m_isCallable = targetObj->methodTable(vm)->getCallData(targetObj, ignored) != CallTypeNone;
> m_target.set(vm, this, targetObj);

will do

>> Source/JavaScriptCore/runtime/ProxyObject.cpp:304
>> +        return throwVMTypeError(exec, ASCIILiteral("Proxy 'handler' is null. It should be an Object."));
> 
> Do we need this check?  ProxyObject::finishCreation() already guarantees that handlerValue isObject().

We really do need this.
It's in the spec. Right now we don't implement revocable Proxys, but when we do,
we'll have a test that covers this.

>> Source/JavaScriptCore/tests/stress/proxy-call.js:103
>> +        proxy.call(thisValue, 20, 45, "foo");
> 
> Shouldn't you assert(called) followed by set called = false here?

Yeah. I'll add

>> Source/JavaScriptCore/tests/stress/proxy-call.js:225
>> +        called = false;
> 
> Don't you want to assert(called) before this?

yeah
Comment 5 Saam Barati 2016-02-19 14:36:42 PST
Created attachment 271803 [details]
patch for landing
Comment 6 Saam Barati 2016-02-19 14:37:14 PST
Comment on attachment 271803 [details]
patch for landing

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

> Source/JavaScriptCore/runtime/ProxyObject.h:38
> +    const static unsigned StructureFlags = Base::StructureFlags | OverridesGetOwnPropertySlot | TypeOfShouldCallGetCallData;

this is new

> Source/JavaScriptCore/tests/stress/proxy-call.js:394
> +{
> +    let target = (x) => x;
> +    let handler = {
> +        apply: function(theTarget, thisArg, argArray) {
> +            return theTarget.apply(thisArg, argArray);
> +        }
> +    };
> +    let proxy = new Proxy(target, handler);
> +    for (let i = 0; i < 500; i++) {
> +        assert(typeof proxy === "function");
> +    }
> +}
> +
> +{
> +    let target = function() { }
> +    let handler = {
> +        apply: function(theTarget, thisArg, argArray) {
> +            return theTarget.apply(thisArg, argArray);
> +        }
> +    };
> +    let proxy = new Proxy(target, handler);
> +    for (let i = 0; i < 500; i++) {
> +        assert(typeof proxy === "function");
> +    }
> +}
> +
> +{
> +    let target = Error;
> +    let handler = {
> +        apply: function(theTarget, thisArg, argArray) {
> +            return theTarget.apply(thisArg, argArray);
> +        }
> +    };
> +    let proxy = new Proxy(target, handler);
> +    for (let i = 0; i < 500; i++) {
> +        assert(typeof proxy === "function");
> +    }
> +}
> +
> +{
> +    let target = (function foo() { }).bind({});
> +    let handler = {
> +        apply: function(theTarget, thisArg, argArray) {
> +            return theTarget.apply(thisArg, argArray);
> +        }
> +    };
> +    let proxy = new Proxy(target, handler);
> +    for (let i = 0; i < 500; i++) {
> +        assert(typeof proxy === "function");
> +    }
> +}
> +
> +{
> +    let target = function() { };
> +    let handler = {};
> +    let proxy = new Proxy(target, handler);
> +    for (let i = 0; i < 500; i++) {
> +        assert(typeof proxy === "function");
> +    }
> +}
> +
> +{
> +    let target = {};
> +    let handler = {};
> +    let proxy = new Proxy(target, handler);
> +    for (let i = 0; i < 500; i++) {
> +        assert(typeof proxy === "object");
> +    }
> +}
> +
> +{
> +    let target = [];
> +    let handler = {};
> +    let proxy = new Proxy(target, handler);
> +    for (let i = 0; i < 500; i++) {
> +        assert(typeof proxy === "object");
> +    }
> +}
> +
> +{
> +    let target = new String("foo");
> +    let handler = {};
> +    let proxy = new Proxy(target, handler);
> +    for (let i = 0; i < 500; i++) {
> +        assert(typeof proxy === "object");
> +    }
> +}

these are new
Comment 7 Mark Lam 2016-02-19 14:53:49 PST
Comment on attachment 271803 [details]
patch for landing

LGTM.
Comment 8 Saam Barati 2016-02-19 14:56:57 PST
landed in:
http://trac.webkit.org/changeset/196836