LGTM++
http://codereview.appspot.com/95051/diff/35/1018 File src/com/google/caja/cajita.js (right): http://codereview.appspot.com/95051/diff/35/1018#newcode2623 Line 2623: return module.instantiate(___, imports); Code style: Too much indentation (indent 2 spaces out) http://codereview.appspot.com/95051/diff/35/1018#newcode2629 Line 2629: setStatic(theModule, k, v); Code style: Too much indentation (indent 4 spaces out) http://codereview.appspot.com/95051/diff/35/1016 File src/com/google/caja/parser/js/CajoledModuleExpression.java (right): http://codereview.appspot.com/95051/diff/35/1016#newcode67 Line 67: new ExpressionStmt(FilePosition.UNKNOWN, e).render(r); Don't have to wrap in ExpressionStmt - just render 'e' http://codereview.appspot.com/95051/diff/35/1015 File src/com/google/caja/parser/quasiliteral/CajitaModuleRewriter.java (right): http://codereview.appspot.com/95051/diff/35/1015#newcode1 Line 1: // Copyright (C) 2007 Google Inc. 2009, this is a new file http://codereview.appspot.com/95051/diff/35/1015#newcode36 Line 36: * Add a top-level module envelop envelope http://codereview.appspot.com/95051/diff/35/1015#newcode37 Line 37: * Call the CajoleRewriter to do the actual rewriting CajitaRewriter http://codereview.appspot.com/95051/diff/35/1015#newcode39 Line 39: * @author [email protected] (Ihab Awad) [email protected] :) http://codereview.appspot.com/95051/diff/35/1015#newcode74 Line 74: /* TODO(ihab.awad): manifest */ Actually, for an "envelope" module, these 4 fields don't really make sense ... I think we can just delete them for good and not worry about them. http://codereview.appspot.com/95051/diff/35/1015#newcode120 Line 120: * Creates a Cajita rewriter CajitaModuleRewriter http://codereview.appspot.com/95051/diff/35/1015#newcode132 Line 132: this(buildInfo, PluginEnvironment.CLOSED_PLUGIN_ENVIRONMENT, logging); Very good! http://codereview.appspot.com/95051/diff/35/1014 File src/com/google/caja/parser/quasiliteral/CajitaRewriter.java (right): http://codereview.appspot.com/95051/diff/35/1014#newcode446 Line 446: throw new RuntimeException("Failed to get the module manager"); Can turn this into an assertion in the constructor -- assert(moduleManager != null) -- thus catching the problem earlier *and* simplifying this code. http://codereview.appspot.com/95051/diff/35/1014#newcode449 Line 449: int index = moduleManager.getModule((StringLiteral)arg); Code style: space after type cast: "(StringLiteral) arg" http://codereview.appspot.com/95051/diff/35/1013 File src/com/google/caja/parser/quasiliteral/ModuleManager.java (right): http://codereview.appspot.com/95051/diff/35/1013#newcode70 Line 70: (UncajoledModule)uncajoledModule.clone(); Don't bother to clone :) http://codereview.appspot.com/95051/diff/35/1013#newcode73 Line 73: (CajoledModule)dcr.expand(clonedUncajoledModule, mq); Code style: Continuation indent is 4 spaces, and add space between typecast operator and its value "(CajoledModule) dcr...." http://codereview.appspot.com/95051/diff/35/1013#newcode105 Line 105: this.pluginEnv.loadExternalResource(er, "text/javascript"); Code style: Continuation indent = 4 spaces. And a bunch of other places in this file. http://codereview.appspot.com/95051/diff/35/1012 File tests/com/google/caja/parser/quasiliteral/CajitaRewriterTest.java (right): http://codereview.appspot.com/95051/diff/35/1012#newcode2297 Line 2297: */ Remove commented material http://codereview.appspot.com/95051/diff/35/1012#newcode2298 Line 2298: CajitaModuleRewriter moduleRewriter = new CajitaModuleRewriter( Please add a TODO to refactor the test classes somehow so that this local change -- setRewriter(moduleRewriter) and changing the kind of node passed to checkAddsMessage -- is not necessary. http://codereview.appspot.com/95051
