This looks great! It looks good to me, but there are a couple of things I would think about:
1) Right now, the dashboard gets created only if users do NOT use the detailed stories, but otherwise by default. Is this what we want, and if yes, we need to make sure users know what their options are. 2) When the SOYC dashboard gets created during the compile, you call three separate methods in the dashboard, while at the same time maintaining the option to run the dashboard from the command line. This definitely works, but it seems a bit harder to maintain these three separate calls than if there were just one. http://gwt-code-reviews.appspot.com/69801/diff/1001/2022 File dev/core/src/com/google/gwt/core/ext/linker/CompilationAnalysis.java (right): http://gwt-code-reviews.appspot.com/69801/diff/1001/2022#newcode48 Line 48: public abstract List<SoycArtifact> getReportFiles(); Are there potentially other classes that extend CompilationAnalysis that this will break? http://gwt-code-reviews.appspot.com/69801/diff/1001/2024 File dev/core/src/com/google/gwt/core/linker/SoycReportLinker.java (right): http://gwt-code-reviews.appspot.com/69801/diff/1001/2024#newcode76 Line 76: "Error while generating a Story of Your Compile", e); Compile report? http://gwt-code-reviews.appspot.com/69801/diff/1001/2036 File dev/core/src/com/google/gwt/dev/jjs/JavaToJavaScriptCompiler.java (right): http://gwt-code-reviews.appspot.com/69801/diff/1001/2036#newcode940 Line 940: PerfLogger.start("Recording SOYC output"); Call this compile report instead? http://gwt-code-reviews.appspot.com/69801/diff/1001/2036#newcode973 Line 973: if (sizeBreakdowns != null) { This seems to be conditioned on whether we use SOYLite. Is this really what we want? If yes, we have to make sure this is obvious to the user (i.e., if they're using the detailed stories, the dashboard will not be created). http://gwt-code-reviews.appspot.com/69801/diff/1001/2036#newcode974 Line 974: PerfLogger.start("Generating SOYC dashboard"); Call this compile report instead? http://gwt-code-reviews.appspot.com/69801/diff/1001/2036#newcode996 Line 996: } What's the reasoning behind calling three separate methods here? I'm wondering because the dashboard can still also be invoked with just one (command-line) call. http://gwt-code-reviews.appspot.com/69801/diff/1001/2036#newcode1071 Line 1071: } This doesn't have to be specific to SOYC - take out the references to SOYC in the variable names and the comments? http://gwt-code-reviews.appspot.com/69801/diff/1001/2028 File dev/core/src/com/google/gwt/soyc/GlobalInformation.java (right): http://gwt-code-reviews.appspot.com/69801/diff/1001/2028#newcode42 Line 42: private final String permutationId; That's fine, but now the GlobalInformation is no longer global, but specific to a permutation - should we rename this class? http://gwt-code-reviews.appspot.com/69801/diff/1001/2032 File dev/core/src/com/google/gwt/soyc/MakeTopLevelHtmlForPerm.java (right): http://gwt-code-reviews.appspot.com/69801/diff/1001/2032#newcode755 Line 755: // here Not sure about this - is it done? http://gwt-code-reviews.appspot.com/69801/diff/1001/2025 File dev/core/src/com/google/gwt/soyc/Settings.java (right): http://gwt-code-reviews.appspot.com/69801/diff/1001/2025#newcode185 Line 185: " present only for backwards compatibility; add any resources directory to the classpath")); any -> a? Also, I kind of liked that the older version had an (albeit short) explanation of what the resources directory was supposed to contain. http://gwt-code-reviews.appspot.com/69801/diff/1001/2027 File dev/core/src/com/google/gwt/soyc/SoycDashboard.java (right): http://gwt-code-reviews.appspot.com/69801/diff/1001/2027#newcode77 Line 77: + "java com.google.gwt.soyc.SoycDashboard options stories0.xml[.gz] [dependencies0.xml[.gz]] [splitpoints0.xml[.gz]])"); + resources directory? http://gwt-code-reviews.appspot.com/69801/diff/1001/2027#newcode84 Line 84: System.out.println("Generating the Story of Your Compile..."); Call this compile report now? http://gwt-code-reviews.appspot.com/69801/diff/1001/2020 File tools/soyc-vis/build.xml (right): http://gwt-code-reviews.appspot.com/69801/diff/1001/2020#newcode18 Line 18: <fileset dir="${gwt.root}/dev/core/src/com/google/gwt/soyc/resources"/> Should this be here or rather in the dev's build file? http://gwt-code-reviews.appspot.com/69801 --~--~---------~--~----~------------~-------~--~----~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~----------~----~----~----~------~----~------~--~---
