http://gwt-code-reviews.appspot.com/1448808/diff/1/dev/core/src/com/google/gwt/dev/CompileModule.java File dev/core/src/com/google/gwt/dev/CompileModule.java (right):
http://gwt-code-reviews.appspot.com/1448808/diff/1/dev/core/src/com/google/gwt/dev/CompileModule.java#newcode147 dev/core/src/com/google/gwt/dev/CompileModule.java:147: static UnableToCompleteException logAndTranslateException(TreeLogger logger, Throwable e) { This code seems to be duplicated several places, in JavaToJavaScriptCompiler and GwtAstBuilder. Can it be consolidated? Maybe as part of LoggerUtils class, etc.? http://gwt-code-reviews.appspot.com/1448808/diff/1/dev/core/src/com/google/gwt/dev/CompileModule.java#newcode194 dev/core/src/com/google/gwt/dev/CompileModule.java:194: Add an overview comment for this method? http://gwt-code-reviews.appspot.com/1448808/diff/1/dev/core/src/com/google/gwt/dev/CompileModule.java#newcode195 dev/core/src/com/google/gwt/dev/CompileModule.java:195: public boolean run(final TreeLogger logger) { It seems like the order that modules are processed is important here. Should it be sub-modules first, then parent modules, etc. Otherwise a parent module will end up including the units of a sub-module, and then if a sub-module is processed, it will have to again write a compiled sub-module file with the same units again. I guess, I'm not sure why units from a sub-module can be included in a parent module, shouldn't each sub-module contain only it's units (with possible updating to these if needed?). It seems like it is only writing a final top-level compiled module file, which can contain all units that haven't been found in any precompiled modules so far. For instance, we wouldn't want the units for a widget library to be separately included in each application module, instead we want to keep all the library's units in it's own pre-compiled cache. http://gwt-code-reviews.appspot.com/1448808/diff/1/dev/core/src/com/google/gwt/dev/CompileModule.java#newcode204 dev/core/src/com/google/gwt/dev/CompileModule.java:204: Might be useful to add module name to the speed tracer event. http://gwt-code-reviews.appspot.com/1448808/diff/1/dev/core/src/com/google/gwt/dev/CompileModule.java#newcode206 dev/core/src/com/google/gwt/dev/CompileModule.java:206: SpeedTracerLogger.start(CompilerEventType.LOAD_PRECOMPILED); compiledModuleURLs? http://gwt-code-reviews.appspot.com/1448808/diff/1/dev/core/src/com/google/gwt/dev/CompileModule.java#newcode219 dev/core/src/com/google/gwt/dev/CompileModule.java:219: if (compiledModule.getName().equals(moduleToCompile)) { Don't we risk throwing away already compiled units here? It seems like below, for 'moduleToCompile', a fresh CompiledGwtModule is created, and everything compiled fresh? http://gwt-code-reviews.appspot.com/1448808/diff/1/dev/core/src/com/google/gwt/dev/CompileModule.java#newcode233 dev/core/src/com/google/gwt/dev/CompileModule.java:233: } put in a finally block, in case an unchecked exception happens. http://gwt-code-reviews.appspot.com/1448808/diff/1/dev/core/src/com/google/gwt/dev/CompileModule.java#newcode255 dev/core/src/com/google/gwt/dev/CompileModule.java:255: if (!outputDir.isDirectory() && !outputDir.mkdirs()) { don't we also need to bail altogether, in this case? http://gwt-code-reviews.appspot.com/1448808/diff/1/dev/core/src/com/google/gwt/dev/CompiledGwtModule.java File dev/core/src/com/google/gwt/dev/CompiledGwtModule.java (right): http://gwt-code-reviews.appspot.com/1448808/diff/1/dev/core/src/com/google/gwt/dev/CompiledGwtModule.java#newcode36 dev/core/src/com/google/gwt/dev/CompiledGwtModule.java:36: Since this might not contain a complete set of units for a module, and in fact can contain units of nested modules, perhaps this should be called something like: ModuleCompiledUnitCache (or some such) http://gwt-code-reviews.appspot.com/1448808/diff/1/dev/core/src/com/google/gwt/dev/Precompile.java File dev/core/src/com/google/gwt/dev/Precompile.java (right): http://gwt-code-reviews.appspot.com/1448808/diff/1/dev/core/src/com/google/gwt/dev/Precompile.java#newcode349 dev/core/src/com/google/gwt/dev/Precompile.java:349: SpeedTracerLogger.start(CompilerEventType.LOAD_PRECOMPILED); compiledModuleURLs http://gwt-code-reviews.appspot.com/1448808/diff/1/dev/core/src/com/google/gwt/dev/Precompile.java#newcode368 dev/core/src/com/google/gwt/dev/Precompile.java:368: } put in finally block, in case of unchecked exception http://gwt-code-reviews.appspot.com/1448808/diff/1/dev/core/src/com/google/gwt/dev/cfg/ModuleDef.java File dev/core/src/com/google/gwt/dev/cfg/ModuleDef.java (right): http://gwt-code-reviews.appspot.com/1448808/diff/1/dev/core/src/com/google/gwt/dev/cfg/ModuleDef.java#newcode91 dev/core/src/com/google/gwt/dev/cfg/ModuleDef.java:91: This is essentially a list of URL's for nested modules which are compiled. Consider changing name to something like: compiledNestedGwtModuleURLs (well I guess that's ugly). Point is, I wasn't sure at first what this (and the associated methods below) were referring to. So maybe something slightly clearer? These are URL's, representing module files, for compiled nested modules....:) http://gwt-code-reviews.appspot.com/1448808/diff/1/dev/core/src/com/google/gwt/dev/cfg/ModuleDef.java#newcode138 dev/core/src/com/google/gwt/dev/cfg/ModuleDef.java:138: Change param name to 'compiledGwtModule'? http://gwt-code-reviews.appspot.com/1448808/diff/1/dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java File dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java (right): http://gwt-code-reviews.appspot.com/1448808/diff/1/dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java#newcode381 dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java:381: DELETE HIM http://gwt-code-reviews.appspot.com/1448808/diff/1/dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java#newcode484 dev/core/src/com/google/gwt/dev/javac/CompilationStateBuilder.java:484: public static void addCompiledGwtModule(TreeLogger logger, CompiledGwtModule module) { instance.unitCache? http://gwt-code-reviews.appspot.com/1448808/diff/1/dev/core/test/com/google/gwt/dev/CompiledGwtModuleTest.java File dev/core/test/com/google/gwt/dev/CompiledGwtModuleTest.java (right): http://gwt-code-reviews.appspot.com/1448808/diff/1/dev/core/test/com/google/gwt/dev/CompiledGwtModuleTest.java#newcode52 dev/core/test/com/google/gwt/dev/CompiledGwtModuleTest.java:52: CompiledGwtModule m2 = CompiledGwtModule.createFromFile(tmp); s/m2/m/ http://gwt-code-reviews.appspot.com/1448808/diff/1/dev/core/test/com/google/gwt/dev/CompiledGwtModuleTest.java#newcode56 dev/core/test/com/google/gwt/dev/CompiledGwtModuleTest.java:56: assertEquals(m2.findUnit(unit3.getResourcePath()).getTypeName(), MOCK_TYPE_3); Maybe also check that you read back what you put, so compare m equals m2 in each aspect. http://gwt-code-reviews.appspot.com/1448808/diff/1/dev/core/test/com/google/gwt/dev/CompiledGwtModuleTest.java#newcode59 dev/core/test/com/google/gwt/dev/CompiledGwtModuleTest.java:59: /** s/are insist/insist http://gwt-code-reviews.appspot.com/1448808/diff/1/dev/core/test/com/google/gwt/dev/CompiledGwtModuleTest.java#newcode64 dev/core/test/com/google/gwt/dev/CompiledGwtModuleTest.java:64: int numMockTypes = 100; don't really need an array to store mockTypes, can just use an iteration variable each time. http://gwt-code-reviews.appspot.com/1448808/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
