Initial check in for securable modules

Implementation for the simplest case: static module inlining

[ihab.awad, 2009-07-14 01:22:06]

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.

[maoziqing, 2009-07-14 17:50:19]

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.

[ihab.awad, 2009-07-14 20:33:44]

LGTM++

Ok to check in after dealing with existing comments.

http://codereview.appspot.com/91107/diff/1028/18
File src/com/google/caja/parser/quasiliteral/CajitaRewriter.java (right):

http://codereview.appspot.com/91107/diff/1028/18#newcode501
Line 501: substitutes="___.markFuncFreeze(function() {\n"
Usually, we don't bother to terminate quasi expressions with "\n" ... it's
probably good to remain consistent.

http://codereview.appspot.com/91107/diff/1028/18#newcode504
Line 504: + "      @body\n"
Missing semicolon.

http://codereview.appspot.com/91107/diff/1028/18#newcode514
Line 514: + "  })();")
Indent this line one step out.

http://codereview.appspot.com/91107/diff/1028/18#newcode537
Line 537: // error messages were logged in the function fetctStaticModule
Trailing whitespace on this line. Also, fetchStaticModule mis-spelled.

http://codereview.appspot.com/91107/diff/1028/16
File tests/com/google/caja/parser/quasiliteral/CajitaRewriterTest.java (right):

http://codereview.appspot.com/91107/diff/1028/16#newcode2304
Line 2304: MessageLevel.FATAL_ERROR);
Maybe add a test for the "cannot load dynamic" failure condition?

[maoziqing, 2009-07-14 20:51:02]

Committed revision 3570.

http://codereview.appspot.com/91107/diff/1028/18
File src/com/google/caja/parser/quasiliteral/CajitaRewriter.java (right):

http://codereview.appspot.com/91107/diff/1028/18#newcode501
Line 501: substitutes="___.markFuncFreeze(function() {\n"
On 2009/07/14 20:33:45, ihab.awad wrote:
> Usually, we don't bother to terminate quasi expressions with "\n" ... it's
> probably good to remain consistent.

Done.

http://codereview.appspot.com/91107/diff/1028/18#newcode504
Line 504: + "      @body\n"
On 2009/07/14 20:33:45, ihab.awad wrote:
> Missing semicolon.

Done.

http://codereview.appspot.com/91107/diff/1028/18#newcode514
Line 514: + "  })();")
On 2009/07/14 20:33:45, ihab.awad wrote:
> Indent this line one step out.

Done.

http://codereview.appspot.com/91107/diff/1028/18#newcode537
Line 537: // error messages were logged in the function fetctStaticModule
On 2009/07/14 20:33:45, ihab.awad wrote:
> Trailing whitespace on this line. Also, fetchStaticModule mis-spelled.

Done.

http://codereview.appspot.com/91107/diff/1028/16
File tests/com/google/caja/parser/quasiliteral/CajitaRewriterTest.java (right):

http://codereview.appspot.com/91107/diff/1028/16#newcode2304
Line 2304: MessageLevel.FATAL_ERROR);
On 2009/07/14 20:33:45, ihab.awad wrote:
> Maybe add a test for the "cannot load dynamic" failure condition?

Done.