http://codereview.appspot.com/126062/diff/2028/3038
File src/com/google/caja/parser/js/CajoledModule.java (right):
http://codereview.appspot.com/126062/diff/2028/3038#newcode48
Line 48: (Expression) QuasiBuilder.substV("___.loadModule");
= should start the line. Please don't add a dependency from parser.js
to QuasiBuilder. Just do
Operator.create(Operator.MEMBER_ACCESS, ...);
http://codereview.appspot.com/126062/diff/2028/3038#newcode109
Line 109: Expression expr = callbackFunction == DEFAULT_MODULE_CALLBACK
Is == the right thing to do here?
http://codereview.appspot.com/126062/diff/2028/3038#newcode128
Line 128: Appendable out, Callback<IOException> exHandler) {
Please comment this method. Maybe a pointer to where things like
originalSources and callbackFunction are described.
http://codereview.appspot.com/126062/diff/2028/3038#newcode182
Line 182: Appendable out,
What is the reason for all the non-standard rendering code in this
class?
http://codereview.appspot.com/126062/diff/2028/3038#newcode191
Line 191: if (callbackFunction != DEFAULT_MODULE_CALLBACK) {
Again, is != right here?
http://codereview.appspot.com/126062/diff/2028/3038#newcode196
Line 196: hout.append("\n");
What is hout? Does it go through the normal printer?
If so, why is ___.prepareModule one token?
If not, why are ___.prepareModule and ( different chunks?
http://codereview.appspot.com/126062/diff/2028/3038#newcode233
Line 233: if (callbackFunction != DEFAULT_MODULE_CALLBACK) {
ditto
http://codereview.appspot.com/126062/diff/2028/3039
File src/com/google/caja/plugin/PluginMeta.java (right):
http://codereview.appspot.com/126062/diff/2028/3039#newcode28
Line 28: private boolean emitHtmlAsJs;
Do we want to emit html as js or only emit js?
http://codereview.appspot.com/126062/diff/2028/3039#newcode65
Line 65: public boolean isEmitHtmlAsJs() { return emitHtmlAsJs; }
isEmit -> shouldEmit or emit
http://codereview.appspot.com/126062/diff/2028/3040
File src/com/google/caja/plugin/html-emitter.js (right):
http://codereview.appspot.com/126062/diff/2028/3040#newcode35
Line 35: }
Please put this function with the other public entry points.
emit* functions should not clobber existing content.
Maybe check whether there are children, and if so, create a DIV, inject
the HTML, and then extract the children into base.
http://codereview.appspot.com/126062/diff/2028/3040#newcode253
Line 253: this.emitStatic = emitStatic;
Plese put in alphabetic order below discard.
http://codereview.appspot.com/126062/diff/2028/3032
File tests/com/google/caja/parser/quasiliteral/ModuleFormatTest.java
(right):
http://codereview.appspot.com/126062/diff/2028/3032#newcode133
Line 133: (Expression) QuasiBuilder.substV("foo.bar.baz"));
indentation
or maybe just use the render method on CajaTestCase.
http://codereview.appspot.com/126062/diff/2028/3032#newcode135
Line 135: ParseTreeNode reparsedModule =
operators should appear at the beginning of a line when the line is
split.
http://codereview.appspot.com/126062/diff/2028/3032#newcode138
Line 138: .children().get(0).children().get(0).children().get(0);
It's not clear what this is testing.
http://codereview.appspot.com/126062/diff/2028/3032#newcode152
Line 152: (Expression) QuasiBuilder.substV("foo.bar.baz"),
Maybe jsExpr(fromString(...)) instead of QuasiBuilder.substV
http://codereview.appspot.com/126062/diff/2028/3032#newcode158
Line 158: .children().get(0).children().get(0).children().get(0);
ditto
http://codereview.appspot.com/126062/diff/2028/3033
File tests/com/google/caja/plugin/html-emitter-test.html (right):
http://codereview.appspot.com/126062/diff/2028/3033#newcode33
Line 33: <blockquote id="base1"></blockquote>
Indentation
http://codereview.appspot.com/126062/diff/2028/3033#newcode83
Line 83: emitter___.emitStatic('<p>Hello world</p>');
</p> => <\/p>
http://codereview.appspot.com/126062/diff/2028/3033#newcode86
Line 86: '<p>Hello world</p>',
ditto.
http://codereview.appspot.com/126062/diff/2028/3034
File tests/com/google/caja/plugin/templates/TemplateCompilerTest.java
(right):
http://codereview.appspot.com/126062/diff/2028/3034#newcode342
Line 342: htmlFragment(fromString("<span/>")),
Why is this span here?
http://codereview.appspot.com/126062/diff/2028/3034#newcode346
Line 346: + " var el___; var emitter___ = IMPORTS___.htmlEmitter___;"
Please put each statement on its own line.
http://codereview.appspot.com/126062
