Bug 154425

Created attachment 271721 [details] patch

Created attachment 271736 [details] patch forgot to update some es6.yaml tests

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

Created attachment 271803 [details] patch for landing

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 on attachment 271803 [details] patch for landing LGTM.

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