Bug 21224 - Store the callee ScopeChain, not the caller ScopeChain, in the call frame header
Summary: Store the callee ScopeChain, not the caller ScopeChain, in the call frame header
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Geoffrey Garen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-09-29 16:03 PDT by Geoffrey Garen
Modified: 2008-09-29 17:46 PDT (History)
0 users

See Also:

Attachments
patch (89.48 KB, patch)
2008-09-29 16:04 PDT, Geoffrey Garen 		zwarich: review+
Details | Formatted Diff | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Geoffrey Garen 2008-09-29 16:03:41 PDT 
Patch coming.
Comment 1 Geoffrey Garen 2008-09-29 16:04:09 PDT 
Created attachment 23922 [details]
patch

Still running the rest of the performance tests, but this is ready for review.
Comment 2 Cameron Zwarich (cpst) 2008-09-29 16:36:36 PDT 
Comment on attachment 23922 [details]
patch

There is a typo in the ChangeLog: "Exec::m_scopeChain" should be "ExecState::m_scopeChain".

You should add a test for the big you say you fixed, but you told me you will do that.

 + // FIXME: callerCodeBlock can be NULL.

You should make a bug for this and add the bug number to the FIXME.

 +      for ( ; exec; exec = exec->m_prev)

Most of these in our code are of the form "for (; " with no space between the '(' and the ';'.

Other than that, r=me.
Comment 3 Geoffrey Garen 2008-09-29 17:45:34 PDT 
> There is a typo in the ChangeLog: "Exec::m_scopeChain" should be
> "ExecState::m_scopeChain".

Fixed.

> You should add a test for the big you say you fixed, but you told me you will
> do that.

Fixed.

>  + // FIXME: callerCodeBlock can be NULL.
> 
> You should make a bug for this and add the bug number to the FIXME.

Added a bug. Removed the FIXME. FIXMEs are not so good for tracking future work. My bad.

>  +      for ( ; exec; exec = exec->m_prev)
> 
> Most of these in our code are of the form "for (; " with no space between the
> '(' and the ';'.

Fixed.

Thanks!
Comment 4 Geoffrey Garen 2008-09-29 17:46:42 PDT 
Committed revision 37086.