Committed revision 3595.
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); On 2009/07/17 22:01:44, ihab.awad wrote:
Code style: Too much indentation (indent 2 spaces out)
Done. http://codereview.appspot.com/95051/diff/35/1018#newcode2629 Line 2629: setStatic(theModule, k, v); On 2009/07/17 22:01:44, ihab.awad wrote:
Code style: Too much indentation (indent 4 spaces out)
Done. 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); On 2009/07/17 22:01:44, ihab.awad wrote:
Don't have to wrap in ExpressionStmt - just render 'e'
Done. 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. On 2009/07/17 22:01:44, ihab.awad wrote:
2009, this is a new file
Done. http://codereview.appspot.com/95051/diff/35/1015#newcode36 Line 36: * Add a top-level module envelop On 2009/07/17 22:01:44, ihab.awad wrote:
envelope
Done. http://codereview.appspot.com/95051/diff/35/1015#newcode37 Line 37: * Call the CajoleRewriter to do the actual rewriting On 2009/07/17 22:01:44, ihab.awad wrote:
CajitaRewriter
Done. http://codereview.appspot.com/95051/diff/35/1015#newcode39 Line 39: * @author [email protected] (Ihab Awad) On 2009/07/17 22:01:44, ihab.awad wrote:
mailto:[email protected] :)
Done. http://codereview.appspot.com/95051/diff/35/1015#newcode74 Line 74: /* TODO(ihab.awad): manifest */ On 2009/07/17 22:01:44, ihab.awad wrote:
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.
Done. http://codereview.appspot.com/95051/diff/35/1015#newcode120 Line 120: * Creates a Cajita rewriter On 2009/07/17 22:01:44, ihab.awad wrote:
CajitaModuleRewriter
Done. 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"); On 2009/07/17 22:01:44, ihab.awad wrote:
Can turn this into an assertion in the constructor --
assert(moduleManager !=
null) -- thus catching the problem earlier *and* simplifying this
code. Done. http://codereview.appspot.com/95051/diff/35/1014#newcode449 Line 449: int index = moduleManager.getModule((StringLiteral)arg); On 2009/07/17 22:01:44, ihab.awad wrote:
Code style: space after type cast: "(StringLiteral) arg"
Done. 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(); On 2009/07/17 22:01:44, ihab.awad wrote:
Don't bother to clone :)
Done. http://codereview.appspot.com/95051/diff/35/1013#newcode73 Line 73: (CajoledModule)dcr.expand(clonedUncajoledModule, mq); On 2009/07/17 22:01:44, ihab.awad wrote:
Code style: Continuation indent is 4 spaces, and add space between
typecast
operator and its value "(CajoledModule) dcr...."
Done. http://codereview.appspot.com/95051/diff/35/1013#newcode105 Line 105: this.pluginEnv.loadExternalResource(er, "text/javascript"); On 2009/07/17 22:01:44, ihab.awad wrote:
Code style: Continuation indent = 4 spaces. And a bunch of other
places in this
file.
Done. 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: */ On 2009/07/17 22:01:44, ihab.awad wrote:
Remove commented material
Done. http://codereview.appspot.com/95051/diff/35/1012#newcode2298 Line 2298: CajitaModuleRewriter moduleRewriter = new CajitaModuleRewriter( On 2009/07/17 22:01:44, ihab.awad wrote:
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.
Done. http://codereview.appspot.com/95051
