Securable modules implementation II

Now it includes the module only once when the module is referenced multiple times.

[ihab.awad, 2009-07-16 22:52:21]

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.

[maoziqing, 2009-07-17 21:10:48]

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.

[ihab.awad, 2009-07-17 22:01:44]

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 ihab.awad@gmail.com (Ihab Awad)
maoziqing@gmail.com :)

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.

[maoziqing, 2009-07-18 00:55:39]

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 ihab.awad@gmail.com (Ihab Awad)
On 2009/07/17 22:01:44, ihab.awad wrote:
> mailto:maoziqing@gmail.com :)

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.