http://codereview.appspot.com/95051/diff/1001/1007
File src/com/google/caja/cajita.js (right):
http://codereview.appspot.com/95051/diff/1001/1007#newcode2628
Line 2628: setStatic(theModule, 'cajoledDate', module.cajoledDate);
It should be possible to enumerate over the keys of 'module' and assign
them to 'theModule', rather than hard-coding keys like 'cajolerName',
etc. Perhaps:
forOwnKeys(module, markFuncFreeze(function(k, v) {
if (k !== 'instantiate') {
setStatic(theModule, k, v);
}
}));
http://codereview.appspot.com/95051/diff/1001/1007#newcode2631
Line 2631: markFuncFreeze(prepareModule);
So far, in Cajita, we don't bother to freeze functions that we put on
___. Perhaps just remain consistent with that?
http://codereview.appspot.com/95051/diff/1001/1005
File src/com/google/caja/parser/js/CajoledModuleExpression.java (right):
http://codereview.appspot.com/95051/diff/1001/1005#newcode2
Line 2: //
2009
http://codereview.appspot.com/95051/diff/1001/1005#newcode29
Line 29: public class CajoledModuleExpression extends AbstractExpression
{
Awesome!
http://codereview.appspot.com/95051/diff/1001/1005#newcode30
Line 30:
Code style: Please remove blank line.
http://codereview.appspot.com/95051/diff/1001/1005#newcode86
Line 86: */
Just a reminder to remove commented-out stuff and unused variables
before checking in.
http://codereview.appspot.com/95051/diff/1001/1006
File src/com/google/caja/parser/js/UncajoledModule.java (right):
http://codereview.appspot.com/95051/diff/1001/1006#newcode38
Line 38: private boolean isTopLevel = true;
All the "topLevel" changes here should be unnecessary -- see remarks in
CajitaRewriter.
http://codereview.appspot.com/95051/diff/1001/1004
File src/com/google/caja/parser/quasiliteral/CajitaRewriter.java
(right):
http://codereview.appspot.com/95051/diff/1001/1004#newcode102
Line 102: private ModuleManager moduleManager;
If you make this final, you avoid the confusion of assigning
(accidentally?) to "this.moduleManager" I noted later on.
http://codereview.appspot.com/95051/diff/1001/1004#newcode183
Line 183: public ParseTreeNode fetchStaticModule(
Can you delete this now?
http://codereview.appspot.com/95051/diff/1001/1004#newcode537
Line 537: new Rule() {
Please move this rule down under the "Module envelope" comment.
http://codereview.appspot.com/95051/diff/1001/1004#newcode566
Line 566: && ((UncajoledModule)node).isTopLevel()) {
You are top level iff (node instanceof UncajoledModule && moduleManager
== null). There is no need for a topLevel property on UncajoledModule.
In fact, one could argue that the UncajoledModule should *not* know, or
need to know, whether it's top level; top level-ness is a property of
the *process* of cajoling it.
http://codereview.appspot.com/95051/diff/1001/1004#newcode567
Line 567: moduleManager = new ModuleManager(buildInfo, pluginEnv, mq);
You probably don't want to assign to *this.moduleManager* -- you are
creating a new ModuleManager for *child* CajitaRewriters to use. See
note above regarding making instance variable 'moduleManager' private.
http://codereview.appspot.com/95051/diff/1001/1004#newcode572
Line 572: Map<String, CajoledModule> moduleMap =
moduleManager.getModuleMap();
At this point, if the module manager has cajoled only *one* module, you
should return it immediately without creating an envelope. Thus a
singleton module gets cajoled exactly as before, and so all the existing
tests should pass (they don't at the moment!), and the top-level module
envelope only gets created when necessary.
http://codereview.appspot.com/95051/diff/1001/1003
File src/com/google/caja/parser/quasiliteral/ModuleManager.java (right):
http://codereview.appspot.com/95051/diff/1001/1003#newcode1
Line 1: // Copyright (C) 2007 Google Inc.
2009
http://codereview.appspot.com/95051/diff/1001/1003#newcode50
Line 50: private MessageQueue mq;
Prefer final member variables where possible; it clarifies the
implementation of a class and catches contract violations in the class's
code.
http://codereview.appspot.com/95051/diff/1001/1003#newcode63
Line 63: this.moduleContentMap = new HashMap<String, CajoledModule>();
These initializations can go in the member declarations, and the member
declarations can be final.
http://codereview.appspot.com/95051/diff/1001/1003#newcode64
Line 64: this.moduleCounter = 0;
And this can go into the member declaration (though cannot be final).
http://codereview.appspot.com/95051/diff/1001/1003#newcode71
Line 71: public void RemoveTaint(ParseTreeNode node) {
Method names should always start with lowercase. But maybe this method
is not even needed -- see below.
http://codereview.appspot.com/95051/diff/1001/1003#newcode79
Line 79: String curName = "module_" + moduleCounter;
Why do you need to use names? Can you just use integer indices? This
might simplify things in this class and elsewhere, and reduce the size
of the cajoled code too.
http://codereview.appspot.com/95051
