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?
On 2009/07/14 01:22:06, ihab.awad wrote:
Use "text/javascript"
Done.
http://codereview.appspot.com/91107/diff/1/7#newcode210
Line 210: MessagePart.Factory.valueOf("module not found"));
On 2009/07/14 01:22:06, ihab.awad wrote:
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?
Done.
http://codereview.appspot.com/91107/diff/1/7#newcode228
Line 228: MessagePart.Factory.valueOf("parsing module failed"));
On 2009/07/14 01:22:06, ihab.awad wrote:
As above, how about a separate PARSING_MODULE_FAILED error?
Done.
http://codereview.appspot.com/91107/diff/1/7#newcode230
Line 230: } catch (IllegalArgumentException e) {
On 2009/07/14 01:22:06, ihab.awad wrote:
What is it that may throw the IllegalArgumentException? Is it
something that can
be caught more specifically?
I guess this is not necessary. I removed that.
http://codereview.appspot.com/91107/diff/1/7#newcode235
Line 235: MessagePart.Factory.valueOf("parsing module failed"));
On 2009/07/14 01:22:06, ihab.awad wrote:
"parsing module failed" for an IllegalArgumentException?
Done.
http://codereview.appspot.com/91107/diff/1/7#newcode512
Line 512: + " function instantiate$_1(IMPORTS___) {\n"
On 2009/07/14 01:22:06, ihab.awad wrote:
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___?
Done.
http://codereview.appspot.com/91107/diff/1/7#newcode516
Line 516: + " ___.markFuncOnly(instantiate$_2, 'instantiate$_2');\n"
On 2009/07/14 01:22:06, ihab.awad wrote:
I would defensively freeze instantiate$_2 as well.
Done.
http://codereview.appspot.com/91107/diff/1/7#newcode519
Line 519: + " return ___.markFuncFreeze(instantiate$_1,
'instantiate$_1');"
On 2009/07/14 01:22:06, ihab.awad wrote:
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...?
Done.
http://codereview.appspot.com/91107/diff/1/7#newcode528
Line 528: if (scope.isOuter("loader")) {
On 2009/07/14 01:22:06, ihab.awad wrote:
Can do (bindings != null && scope.isOuter("loader")) together
Done.
http://codereview.appspot.com/91107/diff/1/7#newcode547
Line 547: return node;
On 2009/07/14 01:22:06, ihab.awad wrote:
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. :)
Done.
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),
On 2009/07/14 01:22:06, ihab.awad wrote:
Un-indent above 2 lines.
Done.
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:
On 2009/07/14 01:22:06, ihab.awad wrote:
This file has only whitespace changes. Please revert.
Done.
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}); "
On 2009/07/14 01:22:06, ihab.awad wrote:
Code style: Spacing: "x: 6, y: 3"
Done.
http://codereview.appspot.com/91107/diff/1/4#newcode2296
Line 2296: );
On 2009/07/14 01:22:06, ihab.awad wrote:
Please move this line up.
Done.
http://codereview.appspot.com/91107/diff/1/4#newcode2304
Line 2304: js(fromString("var m=loader.load('foo/c');")),
On 2009/07/14 01:22:06, ihab.awad wrote:
Code style: Spaces around "=" operator.
Done.
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:
On 2009/07/14 01:22:06, ihab.awad wrote:
Blank line.
Done.
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});
On 2009/07/14 01:22:06, ihab.awad wrote:
Code style: Spacing: "x: x + 1"
Done.
http://codereview.appspot.com/91107/diff/1/2#newcode2
Line 2:
On 2009/07/14 01:22:06, ihab.awad wrote:
Blank line.
Done.
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])")
On 2009/07/14 01:22:07, ihab.awad wrote:
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.
Done.
http://codereview.appspot.com/91107
