Functionally LGTM. Some style suggestions.
http://gwt-code-reviews.appspot.com/169801/diff/1/2 File dev/core/src/com/google/gwt/dev/js/JsDuplicateFunctionRemover.java (right): http://gwt-code-reviews.appspot.com/169801/diff/1/2#newcode43 Line 43: private class DuplicateFunctionBodyRecorder extends JsVisitor { Fields should all be "final". http://gwt-code-reviews.appspot.com/169801/diff/1/2#newcode45 Line 45: Stack<JsExpression> contextStack = new Stack<JsExpression>(); Suggest "Stack<JsNameRef> invocationQualifiers" and guard pushes w/ invocation.getQualifier instanceof JsNameRef? http://gwt-code-reviews.appspot.com/169801/diff/1/2#newcode104 Line 104: private void checkName(JsName jsName) { You know, I might just inline this into the call sites. Here's my rationale... In JsNameOf, you really just want to unconditionally blacklist x.getName() (and maybe assert that it's non-null, I forget the semantics). But in JsNameRef, what you really want to know is whether or not (x == contextStack.peek()), rather than ask the more complicated question of whether x.getName() == contextStack.peek().getName(). (In fact, you could pre-init contextStack with a dummy null to avoid having to check isEmpty()). http://gwt-code-reviews.appspot.com/169801/diff/1/2#newcode123 Line 123: private Set<JsName> blacklist; These should both be "final". http://gwt-code-reviews.appspot.com/169801/diff/1/3 File dev/core/test/com/google/gwt/dev/js/JsDuplicateFunctionRemoverTest.java (right): http://gwt-code-reviews.appspot.com/169801/diff/1/3#newcode21 Line 21: public class JsDuplicateFunctionRemoverTest extends OptimizerTestBase { LGTM, but I have a non-blocking style suggestion... I think the test ends up more easily readable using the optimize(...).into(...) pattern in DeadCodeEliminationTest. What do you think? http://gwt-code-reviews.appspot.com/169801 -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
