The one thing that really stands out is that this has an optimization-dependent failure mode where a runAsync call could get optimized out of a method.
http://gwt-code-reviews.appspot.com/33848/diff/1/4 File dev/core/src/com/google/gwt/core/ext/soyc/impl/SplitPointRecorder.java (right): http://gwt-code-reviews.appspot.com/33848/diff/1/4#newcode107 Line 107: Map<Integer, String> names = new HashMap<Integer, String>(); The use of an ordered map would make the logging statement above easier to read if you're digging through the trace messages. http://gwt-code-reviews.appspot.com/33848/diff/1/5 File dev/core/src/com/google/gwt/core/linker/SoycReportLinker.java (right): http://gwt-code-reviews.appspot.com/33848/diff/1/5#newcode43 Line 43: if (soycFiles.getDepFile() != null) { Is this a defensive check or a bugfix? http://gwt-code-reviews.appspot.com/33848/diff/1/10 File dev/core/src/com/google/gwt/dev/jjs/JavaToJavaScriptCompiler.java (right): http://gwt-code-reviews.appspot.com/33848/diff/1/10#newcode327 Line 327: Could you extract the SOYC report setup code into its own method? This one is starting to get a bit long. http://gwt-code-reviews.appspot.com/33848/diff/1/10#newcode363 Line 363: System.out.println("Completed SOYC phase in " System.out -> logger? http://gwt-code-reviews.appspot.com/33848/diff/1/11 File dev/core/src/com/google/gwt/dev/jjs/ast/JProgram.java (right): http://gwt-code-reviews.appspot.com/33848/diff/1/11#newcode231 Line 231: private Map<Integer, RunAsyncReplacement> runAsyncReplacements = new HashMap<Integer, RunAsyncReplacement>(); Can this be null or dev.util.collect.Maps.create()? The setter below simply reassigns the field. http://gwt-code-reviews.appspot.com/33848/diff/1/11#newcode1051 Line 1051: splitPointInitialSequence.addAll(list); Should this method be called "set" or "append"? http://gwt-code-reviews.appspot.com/33848/diff/1/6 File dev/core/src/com/google/gwt/dev/jjs/impl/CodeSplitter.java (right): http://gwt-code-reviews.appspot.com/33848/diff/1/6#newcode340 Line 340: * TODO(spoon) accept a labeled runAsync call, once runAsyncs can be labeled The label would be some kind of name-bearing annotation on the callback type? http://gwt-code-reviews.appspot.com/33848/diff/1/6#newcode349 Line 349: + PROP_INITIAL_SEQUENCE + ": " + refString); Do you want to throw UnableToCompleteException here? http://gwt-code-reviews.appspot.com/33848/diff/1/6#newcode380 Line 380: "Method includes multiple runAsync calls, so it's ambiguous which one is meant: " Split the string literal to improve formatting. This happens after pruning, right? So it's possible that a developer could write a method containing multiple runAsync calls that would sometimes pass/fail depending on optimization. It's probably not worthwhile to add logic to check for this before pruning, but changing the message to "After optimization, this method includes..." http://gwt-code-reviews.appspot.com/33848/diff/1/6#newcode451 Line 451: Map<JMethod, List<Integer>> revmap = new java.util.HashMap<JMethod, List<Integer>>(); Why the fully-qualified name here? http://gwt-code-reviews.appspot.com/33848/diff/1/8 File dev/core/src/com/google/gwt/dev/jjs/impl/ReplaceRunAsyncs.java (right): http://gwt-code-reviews.appspot.com/33848/diff/1/8#newcode36 Line 36: * " by calls to a fragment loader. This double-quote seems to have been out of place in the initial code. http://gwt-code-reviews.appspot.com/33848 --~--~---------~--~----~------------~-------~--~----~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~----------~----~----~----~------~----~------~--~---
