[webkit-reviews] review denied: [Bug 7831] .apply/call does not set this for eval : [Attachment 7133] Makes eval.apply/call pay attention to the this argument.

[bugzilla-request-daemon at opendarwin.org]

Maciej Stachowiak <mjs at apple.com> has denied Maciej Stachowiak
<mjs 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 7133: Makes eval.apply/call pay attention to the this argument.
http://bugzilla.opendarwin.org/attachment.cgi?id=7133&action=edit

------- Additional Comments from Maciej Stachowiak <mjs at apple.com>
The substance of the change looks good. I have some style concerns:

1) I don't think you need comments explaining what the code used to do or why
it was wrong. That can go in the ChangeLog.

2) This if statement shouldn't be all on one line.

+		if(!thisObj||thisObj->isUndefinedOrNull()) thisObj=
static_cast<JSObject *>(exec->context().thisValue());

Can thisObj ever actually truly be a null pointer as opposed to JS null?

3) Needs a ChangeLog entry.

4) Needs a test case - you can look in LayoutTests/fast/js/resources/*.js, make
a JS file like that using shouldBe and then use make-js-test-wrappers to make
the HTML wrapper.

r- for ChangeLog and test case, but the substance of the change looks correct.