[webkit-reviews] review denied: [Bug 4166] Function object does not support caller property : [Attachment 7359] proposed patch + test

[bugzilla-request-daemon at opendarwin.org]

Geoffrey Garen <ggaren at apple.com> has denied Geoffrey Garen
<ggaren at apple.com>'s request for review:
Bug 4166: Function object does not support caller property
http://bugzilla.opendarwin.org/show_bug.cgi?id=4166

Attachment 7359: proposed patch + test
http://bugzilla.opendarwin.org/attachment.cgi?id=7359&action=edit

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
Arthur,

This is the first patch I've seen you author for the WebKit OpenSource project.
Welcome!

Allow me to further welcome you with an r-. :)

Issues that require an r-:

1. ChangeLog. Each patch requires one. The prepare-ChangeLog script will do
some of the work for you. Then you need to fill in an explanation of why you
changed what you changed.

2. Copyright. To keep things legal, you need to add your name to the copyright
notice of any file you changed. You can find examples of this scattered
throughout our source code.

3. Modifying the caller property. The test doesn't verify that the property
cannot be deleted. Also, should the property be read-only? Since Mozilla
invented this, what does Firefox do?

4. The code adds the caller property as a permanent fixture to the function
object's property map, but the Mozilla doc says, "The caller property is
available only within the body of a function. If used outside a function
declaration, the caller property is null." So I think the code is wrong in a
case like this:

foo();
alert(foo.caller) // should be "null"

At the very least you would have to delete the property after executing the
function. However,

5. Adding and deleting an extra property during every function call may have a
negative effect on performance. 

Since caller is a specially funny property, a better approach might be to
provide a special funny accessor in FunctionImp::getOwnPropertySlot. That would
avoid the twp property map hits. The argumentsGetter and lengthGetter methods
are examples. In callerGetter, you could get at the calling context through
exec->context()->callingContext(). You could recurse up through calling
contexts to (1) find the caller function and (2) determine if the current
function were executing (since you would need to return jsNull if it weren't.)

Other issues:

1. NULL. Our preferred style is to use 0 for NULL in c/c++ code, and nil for
NULL in objc code.

2. if (curImp). Can a Context ever have a NULL imp? I don't think so. No other
code checks for such a thing.