http://codereview.appspot.com/91107/diff/1/7
File src/com/google/caja/parser/quasiliteral/CajitaRewriter.java
(right):
http://codereview.appspot.com/91107/diff/1/7#newcode203
Line 203: // what should we put as the expected MIME type?
Use "text/javascript"
http://codereview.appspot.com/91107/diff/1/7#newcode210
Line 210: MessagePart.Factory.valueOf("module not found"));
It's nice to have *all* the text of an error message either in the
message itself, or from the parse tree. (E.g., it helps with i18n, and
also to make sure the errors are at a fine enough granularity.) So
instead of passing in the "module not found" reason, why not just create
a separate MODULE_NOT_FOUND (or something) error?
http://codereview.appspot.com/91107/diff/1/7#newcode228
Line 228: MessagePart.Factory.valueOf("parsing module failed"));
As above, how about a separate PARSING_MODULE_FAILED error?
http://codereview.appspot.com/91107/diff/1/7#newcode230
Line 230: } catch (IllegalArgumentException e) {
What is it that may throw the IllegalArgumentException? Is it something
that can be caught more specifically?
http://codereview.appspot.com/91107/diff/1/7#newcode235
Line 235: MessagePart.Factory.valueOf("parsing module failed"));
"parsing module failed" for an IllegalArgumentException?
http://codereview.appspot.com/91107/diff/1/7#newcode512
Line 512: + " function instantiate$_1(IMPORTS___) {\n"
I know instantiate$_1 and instantiate$_2 are inaccessible to the input
code, but as a general convention, since they are "system" variables, it
would be nice to make them unmentionable. And also to follow the
existing Caja convention of using triple underscores (aka wonderbars :).
How about instantiate_1___ and instantiate_2___?
http://codereview.appspot.com/91107/diff/1/7#newcode516
Line 516: + " ___.markFuncOnly(instantiate$_2, 'instantiate$_2');\n"
I would defensively freeze instantiate$_2 as well.
http://codereview.appspot.com/91107/diff/1/7#newcode519
Line 519: + " return ___.markFuncFreeze(instantiate$_1,
'instantiate$_1');"
For the "human-readable" name of instantiate$_1, perhaps just call it
'instantiate', since that is the name that the developer's code will
see, and I suggest we should simplify away the $_1 and $_2 stuff...?
http://codereview.appspot.com/91107/diff/1/7#newcode528
Line 528: if (scope.isOuter("loader")) {
Can do (bindings != null && scope.isOuter("loader")) together
http://codereview.appspot.com/91107/diff/1/7#newcode547
Line 547: return node;
Please put a comment above this line saying that this case (i.e., where
cajoledModule == null) is an error condition, and that you are relying
on ERRORs having been logged elsewhere. That would reassure a worried
reader why it's ok to just "return node" here. :)
http://codereview.appspot.com/91107/diff/1/7#newcode553
Line 553: node.getFilePosition());
Nice. :)
http://codereview.appspot.com/91107/diff/1/7#newcode2550
Line 2550: this(buildInfo, PluginEnvironment.CLOSED_PLUGIN_ENVIRONMENT,
logging);
Great!
http://codereview.appspot.com/91107/diff/1/6
File src/com/google/caja/parser/quasiliteral/RewriterMessageType.java
(right):
http://codereview.appspot.com/91107/diff/1/6#newcode165
Line 165: MessageLevel.FATAL_ERROR),
Un-indent above 2 lines.
http://codereview.appspot.com/91107/diff/1/5
File src/com/google/caja/parser/quasiliteral/Scope.java (right):
http://codereview.appspot.com/91107/diff/1/5#newcode192
Line 192:
This file has only whitespace changes. Please revert.
http://codereview.appspot.com/91107/diff/1/4
File tests/com/google/caja/parser/quasiliteral/CajitaRewriterTest.java
(right):
http://codereview.appspot.com/91107/diff/1/4#newcode2294
Line 2294: + "var r = m.instantiate({x:6, y:3}); "
Code style: Spacing: "x: 6, y: 3"
http://codereview.appspot.com/91107/diff/1/4#newcode2296
Line 2296: );
Please move this line up.
http://codereview.appspot.com/91107/diff/1/4#newcode2304
Line 2304: js(fromString("var m=loader.load('foo/c');")),
Code style: Spaces around "=" operator.
http://codereview.appspot.com/91107/diff/1/3
File tests/com/google/caja/parser/quasiliteral/c.js (right):
http://codereview.appspot.com/91107/diff/1/3#newcode2
Line 2:
Blank line.
http://codereview.appspot.com/91107/diff/1/2
File tests/com/google/caja/parser/quasiliteral/foo/b.js (right):
http://codereview.appspot.com/91107/diff/1/2#newcode1
Line 1: loader.load('../c').instantiate({x:x + 1, y: y + 1});
Code style: Spacing: "x: x + 1"
http://codereview.appspot.com/91107/diff/1/2#newcode2
Line 2:
Blank line.
http://codereview.appspot.com/91107/diff/9/1014
File src/com/google/caja/parser/quasiliteral/CajitaRewriter.java
(right):
http://codereview.appspot.com/91107/diff/9/1014#newcode523
Line 523: + "'cajoledDate', @cajoledDate])")
I have to say it would be nice for the result of loader.load(...) to be
function-like, so one could do:
loader.load('x')({ a: 2, b: 3});
but it's also definitely nice to be able to do:
loader.load('x').instantiate({ a: 2, b: 3});
... loader.load('x').cajolerName ...
Let's chat in person and/or on google-caja-discuss@ about the best way
to achieve this effect.
http://codereview.appspot.com/91107
