Thank you for the review.
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);
On 2009/07/16 22:52:21, ihab.awad wrote:
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);
}
}));
Done.
http://codereview.appspot.com/95051/diff/1001/1007#newcode2631
Line 2631: markFuncFreeze(prepareModule);
On 2009/07/16 22:52:21, ihab.awad wrote:
So far, in Cajita, we don't bother to freeze functions that we put on
___.
Perhaps just remain consistent with that?
Done.
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: //
On 2009/07/16 22:52:21, ihab.awad wrote:
2009
Done.
http://codereview.appspot.com/95051/diff/1001/1005#newcode30
Line 30:
On 2009/07/16 22:52:21, ihab.awad wrote:
Code style: Please remove blank line.
Done.
http://codereview.appspot.com/95051/diff/1001/1005#newcode86
Line 86: */
On 2009/07/16 22:52:21, ihab.awad wrote:
Just a reminder to remove commented-out stuff and unused variables
before
checking in.
Done.
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;
On 2009/07/16 22:52:21, ihab.awad wrote:
All the "topLevel" changes here should be unnecessary -- see remarks
in
CajitaRewriter.
Done.
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;
On 2009/07/16 22:52:21, ihab.awad wrote:
If you make this final, you avoid the confusion of assigning
(accidentally?) to
"this.moduleManager" I noted later on.
Done.
http://codereview.appspot.com/95051/diff/1001/1004#newcode183
Line 183: public ParseTreeNode fetchStaticModule(
On 2009/07/16 22:52:21, ihab.awad wrote:
Can you delete this now?
Done.
http://codereview.appspot.com/95051/diff/1001/1004#newcode537
Line 537: new Rule() {
On 2009/07/16 22:52:21, ihab.awad wrote:
Please move this rule down under the "Module envelope" comment.
Done.
http://codereview.appspot.com/95051/diff/1001/1004#newcode566
Line 566: && ((UncajoledModule)node).isTopLevel()) {
On 2009/07/16 22:52:21, ihab.awad wrote:
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.
Done.
http://codereview.appspot.com/95051/diff/1001/1004#newcode567
Line 567: moduleManager = new ModuleManager(buildInfo, pluginEnv, mq);
On 2009/07/16 22:52:21, ihab.awad wrote:
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.
Done.
http://codereview.appspot.com/95051/diff/1001/1004#newcode572
Line 572: Map<String, CajoledModule> moduleMap =
moduleManager.getModuleMap();
On 2009/07/16 22:52:21, ihab.awad wrote:
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.
Done.
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.
On 2009/07/16 22:52:21, ihab.awad wrote:
2009
Done.
http://codereview.appspot.com/95051/diff/1001/1003#newcode50
Line 50: private MessageQueue mq;
On 2009/07/16 22:52:21, ihab.awad wrote:
Prefer final member variables where possible; it clarifies the
implementation of
a class and catches contract violations in the class's code.
Done.
http://codereview.appspot.com/95051/diff/1001/1003#newcode63
Line 63: this.moduleContentMap = new HashMap<String, CajoledModule>();
On 2009/07/16 22:52:21, ihab.awad wrote:
These initializations can go in the member declarations, and the
member
declarations can be final.
Done.
http://codereview.appspot.com/95051/diff/1001/1003#newcode64
Line 64: this.moduleCounter = 0;
On 2009/07/16 22:52:21, ihab.awad wrote:
And this can go into the member declaration (though cannot be final).
Done.
http://codereview.appspot.com/95051/diff/1001/1003#newcode71
Line 71: public void RemoveTaint(ParseTreeNode node) {
On 2009/07/16 22:52:21, ihab.awad wrote:
Method names should always start with lowercase. But maybe this method
is not
even needed -- see below.
Done.
http://codereview.appspot.com/95051/diff/1001/1003#newcode79
Line 79: String curName = "module_" + moduleCounter;
On 2009/07/16 22:52:21, ihab.awad wrote:
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.
Done.
http://codereview.appspot.com/95051
