Bug 154314

Created attachment 271582 [details] it begins this patch still has the contents of the [[Get]] patch until it lands

*** Bug 154353 has been marked as a duplicate of this bug. ***

Created attachment 271616 [details] patch

Attachment 271616 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/JSObject.h:720: The parameter name "exec" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/runtime/JSObject.h:720: The parameter name "callData" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/runtime/JSObject.h:720: The parameter name "callType" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/runtime/JSObject.h:720: The parameter name "ident" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/runtime/JSObject.h:1477: The parameter name "descriptor" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 5 in 9 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 271616 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=271616&action=review > Source/JavaScriptCore/runtime/FunctionPrototype.cpp:167 > + if (targetObject->hasOwnProperty(exec, exec->propertyNames().length)) { > + if (exec->hadException()) > + return JSValue::encode(jsUndefined()); > + > // a. Let L be the length property of Target minus the length of A. > // b. Set the length own property of F to either 0 or L, whichever is larger. > - unsigned targetLength = (unsigned)target.get(exec, exec->propertyNames().length).asNumber(); > - if (targetLength > numBoundArgs) > - length = targetLength - numBoundArgs; > + JSValue lengthValue = target.get(exec, exec->propertyNames().length); > + if (lengthValue.isNumber()) { > + unsigned targetLength = (unsigned)lengthValue.asNumber(); > + if (targetLength > numBoundArgs) > + length = targetLength - numBoundArgs; > + } I added this here because it's wrong w.r.t to the spec. There is a kangax test that tests this in combination with Proxy.[[GetOwnProperty]]. To make us pass that test, I need to also implement Proxy.[[Call]]. I can wait or open another bug for this if anybody objects about having it in this patch.

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

Created attachment 271689 [details] follow up fix

follow up landed in: http://trac.webkit.org/changeset/196775

Comment on attachment 271616 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=271616&action=review > Source/JavaScriptCore/runtime/JSObject.cpp:2942 > + if (method.isUndefined() || method.isNull()) > + return jsUndefined(); > + > + if (!method.isCell()) { > + throwVMTypeError(exec, errorMessage); > + return jsUndefined(); > + } I would have written: if (!method.isCell()) { if (method.isUndefinedOrNull()) return jsUndefined(); return throwVMTypeError(exec, errorMessage); } Having one less branch seems slightly more elegant to me.

Comment on attachment 271616 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=271616&action=review >> Source/JavaScriptCore/runtime/FunctionPrototype.cpp:167 >> + if (targetObject->hasOwnProperty(exec, exec->propertyNames().length)) { >> + if (exec->hadException()) >> + return JSValue::encode(jsUndefined()); >> + >> // a. Let L be the length property of Target minus the length of A. >> // b. Set the length own property of F to either 0 or L, whichever is larger. >> - unsigned targetLength = (unsigned)target.get(exec, exec->propertyNames().length).asNumber(); >> - if (targetLength > numBoundArgs) >> - length = targetLength - numBoundArgs; >> + JSValue lengthValue = target.get(exec, exec->propertyNames().length); >> + if (lengthValue.isNumber()) { >> + unsigned targetLength = (unsigned)lengthValue.asNumber(); >> + if (targetLength > numBoundArgs) >> + length = targetLength - numBoundArgs; >> + } > > I added this here because it's wrong w.r.t to the spec. > There is a kangax test that tests this in combination with Proxy.[[GetOwnProperty]]. > To make us pass that test, I need to also implement Proxy.[[Call]]. I can wait > or open another bug for this if anybody objects about having it in this patch. I added this here because it's wrong w.r.t to the spec. There is a kangax test that tests this in combination with Proxy.[[GetOwnProperty]]. To make us pass that test, I need to also implement Proxy.[[Call]]. I can wait or open another bug for this if anybody objects about having it in this patch. >> Source/JavaScriptCore/runtime/JSObject.cpp:2942 >> + } > > I would have written: > > if (!method.isCell()) { > if (method.isUndefinedOrNull()) > return jsUndefined(); > return throwVMTypeError(exec, errorMessage); > } > > Having one less branch seems slightly more elegant to me. I agree. I'll upload a fix.

(In reply to comment #10) > Comment on attachment 271616 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=271616&action=review > > >> Source/JavaScriptCore/runtime/FunctionPrototype.cpp:167 > >> + if (targetObject->hasOwnProperty(exec, exec->propertyNames().length)) { > >> + if (exec->hadException()) > >> + return JSValue::encode(jsUndefined()); > >> + > >> // a. Let L be the length property of Target minus the length of A. > >> // b. Set the length own property of F to either 0 or L, whichever is larger. > >> - unsigned targetLength = (unsigned)target.get(exec, exec->propertyNames().length).asNumber(); > >> - if (targetLength > numBoundArgs) > >> - length = targetLength - numBoundArgs; > >> + JSValue lengthValue = target.get(exec, exec->propertyNames().length); > >> + if (lengthValue.isNumber()) { > >> + unsigned targetLength = (unsigned)lengthValue.asNumber(); > >> + if (targetLength > numBoundArgs) > >> + length = targetLength - numBoundArgs; > >> + } > > > > I added this here because it's wrong w.r.t to the spec. > > There is a kangax test that tests this in combination with Proxy.[[GetOwnProperty]]. > > To make us pass that test, I need to also implement Proxy.[[Call]]. I can wait > > or open another bug for this if anybody objects about having it in this patch. > > I added this here because it's wrong w.r.t to the spec. > There is a kangax test that tests this in combination with > Proxy.[[GetOwnProperty]]. > To make us pass that test, I need to also implement Proxy.[[Call]]. I can > wait > or open another bug for this if anybody objects about having it in this > patch. I definitely didn't mean to post this comment again. I think bugzilla cached my comment from before.

(In reply to comment #10) > Comment on attachment 271616 [details] > patch > >> Source/JavaScriptCore/runtime/JSObject.cpp:2942 > >> + } > > > > I would have written: > > > > if (!method.isCell()) { > > if (method.isUndefinedOrNull()) > > return jsUndefined(); > > return throwVMTypeError(exec, errorMessage); > > } > > > > Having one less branch seems slightly more elegant to me. > > I agree. > I'll upload a fix. the bug for the fix: https://bugs.webkit.org/show_bug.cgi?id=154603