Addressed David-Sarah's comments on CL 110096 which has been submitted

Submitted @3739, 3742

[DavidSarah, 2009-09-24 00:14:32]

http://codereview.appspot.com/116114/diff/1/4
File src/com/google/caja/parser/quasiliteral/AlphaRenamingRewriter.java (right):

http://codereview.appspot.com/116114/diff/1/4#newcode129
Line 129: if (!isSynthetic(p.getIdentifier())) {
It is OK to rename a formal called "arguments", but I had to scratch my head
about that. Maybe a comment if you think it's needed.

http://codereview.appspot.com/116114/diff/1/4#newcode152
Line 152: // scope, the function name should always refer to itself.
I think part of the reason I was confused by this is that you have to have
understood the second comment before the first makes sense. Also the example
doesn't make clear why the "&& !newScope.isDeclaredFunction(name.getName())"
is needed. Consider combining them and adding another example, e.g.

// For a declaration, a name is normally introduced in both the scope
// containing the declaration, and the function body scope.
// We produce a declaration with the outer name, but in the inner
// scope the function name should refer to the function itself.
// The only exception is that if there is a local declaration
// inside the local scope that masks the function name, then we
// should not clobber it.
// Examples:
//   (function f() { var f = 0; return f; })() === 0
// and
//   (function f() { function f() { return 0; } return f(); })() === 0
//
// Because the var f or inner function f masks the outer function f,
// the name "f" should not be considered to refer to the function
// within its body. The condition
//    "newScope.isFunction(name.getName())
//  && !newScope.isDeclaredFunction(name.getName())"
// checks that the name still refers to the outer function, not a
// variable or a different function that is declared within the body.

(This is quite long, but I think the issue is subtle enough to justify it.
It turns out the Jacaranda prototype implementation got this wrong, so thanks.)

[MikeSamuel, 2009-09-24 01:16:44]

http://codereview.appspot.com/116114/diff/1/4
File src/com/google/caja/parser/quasiliteral/AlphaRenamingRewriter.java (right):

http://codereview.appspot.com/116114/diff/1/4#newcode129
Line 129: if (!isSynthetic(p.getIdentifier())) {
On 2009/09/24 00:14:32, DavidSarah wrote:
> It is OK to rename a formal called "arguments", but I had to scratch my head
> about that. Maybe a comment if you think it's needed.

I added a testcase for that.

http://codereview.appspot.com/116114/diff/1/4#newcode152
Line 152: // scope, the function name should always refer to itself.
I used your comment text and added a bit at the end to explain the Scope
idiosyncrasy that requires the !isDeclaredFunction clause.

[MikeSamuel, 2009-09-24 02:13:40]

I reworked the handling of this and argument a bit.  Not rewriting this and
arguments defeats the point of the alpha renaming since two uses with the same
name could be binding in different scopes.

Now
  (function () { return this; })
will rename to
  (function () { var a = this; return a; })

[DavidSarah, 2009-09-24 04:45:26]

http://codereview.appspot.com/116114/diff/1007/2019
File src/com/google/caja/parser/quasiliteral/AlphaRenamingRewriter.java (right):

http://codereview.appspot.com/116114/diff/1007/2019#newcode202
Line 202: // isDeclaredFunction but we need to distinguish the two cases.
Do you mean "isDeclaredFunction implies isFunction"?

Otherwise LGTM.

[MikeSamuel, 2009-09-24 17:45:14]

http://codereview.appspot.com/116114/diff/1007/2019
File src/com/google/caja/parser/quasiliteral/AlphaRenamingRewriter.java (right):

http://codereview.appspot.com/116114/diff/1007/2019#newcode202
Line 202: // isDeclaredFunction but we need to distinguish the two cases.
On 2009/09/24 04:45:27, DavidSarah wrote:
> Do you mean "isDeclaredFunction implies isFunction"?
> 
> Otherwise LGTM.

Yes

[DavidSarah, 2009-09-24 18:08:55]

LGTM.