http://gwt-code-reviews.appspot.com/725801/diff/12001/13004 File dev/core/src/com/google/gwt/dev/shell/GWTBridgeImpl.java (right):
http://gwt-code-reviews.appspot.com/725801/diff/12001/13004#newcode26 dev/core/src/com/google/gwt/dev/shell/GWTBridgeImpl.java:26: protected static ThreadLocal<String> uniqueID = I don't think these changes to GWT* are necessary. See comment in DevModeLoggingFixes. http://gwt-code-reviews.appspot.com/725801/diff/12001/13006 File dev/core/src/com/google/gwt/dev/shell/rewrite/UseMirroredClasses.java (right): http://gwt-code-reviews.appspot.com/725801/diff/12001/13006#newcode39 dev/core/src/com/google/gwt/dev/shell/rewrite/UseMirroredClasses.java:39: private static class MyMethodVisitor extends MethodAdapter { Better name for this? How about MethodInterceptor? http://gwt-code-reviews.appspot.com/725801/diff/12001/13006#newcode45 dev/core/src/com/google/gwt/dev/shell/rewrite/UseMirroredClasses.java:45: mirroredMethods = new HashMap<String, String>(); This is fairly high maintenance, and I can think of a lot of ways to improve it (mostly using annotations). The improvements are also pretty incremental, so they don't have to be big bang. It's not necessary that you do that now, but maybe you should add a TODO to that effect so we don't allow this code to grow unwieldy? http://gwt-code-reviews.appspot.com/725801/diff/12001/13006#newcode67 dev/core/src/com/google/gwt/dev/shell/rewrite/UseMirroredClasses.java:67: String mirrorClassMethod = mirroredMethods.get(owner + "." + name); If you're not going to change this code to handle the above, then you might consider optimizing it into a two-level lookup to avoid generating garbage. It will run for every single instruction for every single class in the client program, right? That could be millions of times. http://gwt-code-reviews.appspot.com/725801/diff/12001/13006#newcode67 dev/core/src/com/google/gwt/dev/shell/rewrite/UseMirroredClasses.java:67: String mirrorClassMethod = mirroredMethods.get(owner + "." + name); This won't catch dispatches via subtypes on instance or static methods. Here's some example code: public class TestIntercept { static class A { public void foo() { } public static void bar() { } } static class B extends A { } public static void main(String[] args) { new A().foo(); new B().foo(); A.bar(); B.bar(); } } public static void main(java.lang.String[]); Code: 0: new #2; //class TestIntercept$A 3: dup 4: invokespecial #3; //Method TestIntercept$A."<init>":()V 7: invokevirtual #4; //Method TestIntercept$A.foo:()V 10: new #5; //class TestIntercept$B 13: dup 14: invokespecial #6; //Method TestIntercept$B."<init>":()V 17: invokevirtual #7; //Method TestIntercept$B.foo:()V 20: invokestatic #8; //Method TestIntercept$A.bar:()V 23: invokestatic #9; //Method TestIntercept$B.bar:()V 26: return } If you had an intercept rule for A.foo, or A.bar, then your intercept rule would not have caught the calls to them via the subclass B. Writing that code is tricky, because you need to look at the class hierarchy of the owner of the invoke you're examining. I don't know how important this is to you for your specific case, (How many people subclass Logger or LogManager?), but you should at least document that this code doesn't work under those general circumstances. http://gwt-code-reviews.appspot.com/725801/diff/12001/13006#newcode68 dev/core/src/com/google/gwt/dev/shell/rewrite/UseMirroredClasses.java:68: if (mirrorClassMethod != null) { As a code style, I personally prefer early returns. It prevents having lots of levels of indentation, like you have here. It's also easier(personally) to see all the code paths. http://gwt-code-reviews.appspot.com/725801/diff/12001/13006#newcode73 dev/core/src/com/google/gwt/dev/shell/rewrite/UseMirroredClasses.java:73: if (temp.length > 1) { Shouldn't this be an assert? http://gwt-code-reviews.appspot.com/725801/diff/12001/13006#newcode83 dev/core/src/com/google/gwt/dev/shell/rewrite/UseMirroredClasses.java:83: // using the method descriptor string You should only change the argument types if you're changing from a virtual dispatch to a static dispatch. You'll still need to change the owner though. http://gwt-code-reviews.appspot.com/725801/diff/12001/13013 File user/src/com/google/gwt/logging/impl/DevModeLoggingFixes.java (right): http://gwt-code-reviews.appspot.com/725801/diff/12001/13013#newcode69 user/src/com/google/gwt/logging/impl/DevModeLoggingFixes.java:69: return GWT.getUniqueThreadId() + "."; For simplicity's sake, why not just use the actual thread id? http://gwt-code-reviews.appspot.com/725801/diff/12001/13013#newcode72 user/src/com/google/gwt/logging/impl/DevModeLoggingFixes.java:72: private static String removeLoggerPrefix(String name) { Dead code? http://gwt-code-reviews.appspot.com/725801/diff/12001/13014 File user/src/com/google/gwt/logging/impl/LoggerImplRegular.java (right): http://gwt-code-reviews.appspot.com/725801/diff/12001/13014#newcode32 user/src/com/google/gwt/logging/impl/LoggerImplRegular.java:32: public static Logger staticGetLoggerHelper(String name) { Is this being called from somewhere else? I don't understand why it got out-lined and made public. http://gwt-code-reviews.appspot.com/725801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
