[webkit-reviews] review denied: [Bug 7831] .apply/call does not set this for eval : [Attachment 7146] Modified Patch

[bugzilla-request-daemon at opendarwin.org]

Darin Adler <darin at apple.com> has denied Darin Adler <darin at apple.com>'s
request for review:
Bug 7831: .apply/call does not set this for eval
http://bugzilla.opendarwin.org/show_bug.cgi?id=7831

Attachment 7146: Modified Patch
http://bugzilla.opendarwin.org/attachment.cgi?id=7146&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
Thanks for taking this on!

There are many minor things wrong with this patch.

1) no tests -- patches that fix bugs require tests that demonstrate the bug is
fixed (although I suppose the fact that this fixes on of the Mozilla tests
might be an OK substitute)
2) no change log entry
3) commented-out code left in internal.cpp, should not be
4) patch that changes only whitespace in function.cpp
5) tabs in the patch (we use spaces for indenting)
6) no space between "if" and the "(" following it in the code in
function_object.cpp
7) no spaces around the "==" operator
8) "ContextImp *cpy= " should be "ContextImp* cpy = ".
9) We don't declare multiple variables with a single declaration as is done
with "cpy" and "ctx".
10) local variable named "cpy" -- "copy" would be better
11) local variable named "ctx" -- name that is either a word or just a single
letter would be better
12) no spaces after commas in some function calls

However, the main issue here is almost certainly performance. Adding an
inherits check to every function call is probably not a good idea. And as to
the reason that's needed, I don't see what information
FunctionProtoFunc::callAsFunction has that GlobalFuncImp::callAsFunction will
not have.

Won't the internal.cpp change fix the bug without any change to
function_object.cpp at all? If not, what's the high level description of what
the function_object.cpp change does?