Dynamic module loading for Cajita

Implemented two methods of loading a dynamic module, using script tag and XHR

[ihab.awad, 2009-08-03 18:24:09]

Preliminary comments attached.

http://codereview.appspot.com/96177/diff/1028/26
File src/com/google/caja/cajita.js (right):

http://codereview.appspot.com/96177/diff/1028/26#newcode3172
Line 3172: /*
The promise implementation and the definition of Q can be moved to a different
file, right? You can have the file contain something like:

  var Q;
  (function() {
    /* encapsulated definitions ... */
    Q = /* something */;
  })();

Then cajita.js can check if Q is available, and do:

  if (Q) { /* freeze Q */ }
  cajita = { Q: Q, /* ... */ };

People who want to provide Q to their Cajita would then be sure to include the
necessary file before "cajita.js". And we would modify our build scripts to
always bundle that file in by default.

The advantage of this approach is it makes it possible to diff directly with
Tyler's implementation and move towards a more common Q.

The file should contain a comment based on Tyler's original, giving credit etc.

http://codereview.appspot.com/96177/diff/1028/26#newcode3176
Line 3176: * Mostly taken from the web_send implementation by Tyler Close
I think you meant ref_send.js? (ref_send is a component of web_send.)

http://codereview.appspot.com/96177/diff/1028/26#newcode3576
Line 3576: 
Extra leading spaces added to this line.

http://codereview.appspot.com/96177/diff/1028/26#newcode3691
Line 3691: secureModuleOf: secureModuleOf,
TODO(ihab.awad): Check whether this is actually used anywhere. Since
secureModuleOf just passes thru to eval() at the moment (and hence isn't
secure), it is risky unless necessary.

http://codereview.appspot.com/96177/diff/1028/26#newcode3750
Line 3750: /**
I think the module loader makers can be in a separate file as well. The platform
can choose to include these and use them if it wishes. They need not be part of
cajita.js proper.

Again, our build scripts may or may not choose to bundle these into the default
minified chunks.

In a comment in the resulting file, please summarize the API of a "loader
object" as returned by either of the *ModuleLoaderMaker() functions.

Also ... I am coming to the conclusion that maybe your module loaders are
actually best seen as a JS API to the cajoling service; see:

http://docs.google.com/Doc?docid=0AVWyIFBvIVXGZGZneGI3Z2tfNjVkbjV0OXdmaw&hl=en

If you agree, then maybe in refactoring them, we should just optimize their API
to work with the cajoling service and, for other stuff that's different, just
let programmers build whatever they want as long as it satisfies the (dead
simple!) loader interface?

http://codereview.appspot.com/96177/diff/1028/26#newcode3756
Line 3756: xhrModuleLoaderMaker = function(urlRewriter) {
You know what? I was thinking and maybe it's easiest to just make the
xhrModuleLoader a *singleton* in the page in which it lives. If I want to
provide an attenuated loader, I just do:

  var theLoader = ___.primFreeze({
    load: ___.markFuncFreeze(function(name) {
      return xhrModuleLoader.load(
          'http://cajoler.org/' + name + '.js');
    })
  });

or whatever. Would that be easier and simpler and more easily explainable to the
universe than having to deal with the "URL rewriter chain" in the loaders?

http://codereview.appspot.com/96177/diff/1028/26#newcode3763
Line 3763: if (xhr.status  === 200) {
Extra space before "===".

http://codereview.appspot.com/96177/diff/1028/26#newcode3767
Line 3767: r.resolve(theModule);
Can eliminate local variable 'theModule' -> simpler?

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

http://codereview.appspot.com/96177/diff/1028/25#newcode437
Line 437: matches="sload(@arg)",
Interesting. What was the reason for choosing to rename the loader syntax versus
just recognizing "loader.load()" with a string literal?

http://codereview.appspot.com/96177/diff/1028/18
File tests/com/google/caja/a.js (right):

http://codereview.appspot.com/96177/diff/1028/18#newcode1
Line 1: ({
Why do you need to create explicit cajoled modules in the test source directory?
You can instead use the build file to cajole some things into the ant-lib
directory, similarly to how (say) domita_test_untrusted.html is cajoled at build
time.

http://codereview.appspot.com/96177/diff/1028/22
File tests/com/google/caja/plugin/domita_test.html (right):

http://codereview.appspot.com/96177/diff/1028/22#newcode408
Line 408: function (src) {return '/tests/com/google/caja/' + src;});
How does that work without the "http://localhost:8000/..." stuff added to the
front? :)

http://codereview.appspot.com/96177/diff/1028/23
File tests/com/google/caja/plugin/domita_test_untrusted.html (right):

http://codereview.appspot.com/96177/diff/1028/23#newcode2508
Line 2508: function testXhrModuleLoader() {
Indentation.

http://codereview.appspot.com/96177/diff/1028/23#newcode2519
Line 2519: var r = Q.when(m, f1, f2);
Variable 'r' unused.

http://codereview.appspot.com/96177/diff/1028/23#newcode2555
Line 2555: function testScriptModuleLoader() {
Indentation.

[maoziqing, 2009-08-16 21:31:41]

http://codereview.appspot.com/96177/diff/1028/26
File src/com/google/caja/cajita.js (right):

http://codereview.appspot.com/96177/diff/1028/26#newcode3172
Line 3172: /*
On 2009/08/03 18:24:09, ihab.awad wrote:
> The promise implementation and the definition of Q can be moved to a different
> file, right? You can have the file contain something like:
> 
>   var Q;
>   (function() {
>     /* encapsulated definitions ... */
>     Q = /* something */;
>   })();
> 
> Then cajita.js can check if Q is available, and do:
> 
>   if (Q) { /* freeze Q */ }
>   cajita = { Q: Q, /* ... */ };
> 
> People who want to provide Q to their Cajita would then be sure to include the
> necessary file before "cajita.js". And we would modify our build scripts to
> always bundle that file in by default.
> 
> The advantage of this approach is it makes it possible to diff directly with
> Tyler's implementation and move towards a more common Q.
> 
> The file should contain a comment based on Tyler's original, giving credit
etc.

Done.

http://codereview.appspot.com/96177/diff/1028/26#newcode3176
Line 3176: * Mostly taken from the web_send implementation by Tyler Close
On 2009/08/03 18:24:09, ihab.awad wrote:
> I think you meant ref_send.js? (ref_send is a component of web_send.)

Done.

http://codereview.appspot.com/96177/diff/1028/26#newcode3576
Line 3576: 
On 2009/08/03 18:24:09, ihab.awad wrote:
> Extra leading spaces added to this line.

Done.

http://codereview.appspot.com/96177/diff/1028/26#newcode3756
Line 3756: xhrModuleLoaderMaker = function(urlRewriter) {
On 2009/08/03 18:24:09, ihab.awad wrote:
> You know what? I was thinking and maybe it's easiest to just make the
> xhrModuleLoader a *singleton* in the page in which it lives. If I want to
> provide an attenuated loader, I just do:
> 
>   var theLoader = ___.primFreeze({
>     load: ___.markFuncFreeze(function(name) {
>       return xhrModuleLoader.load(
>           'http://cajoler.org/' + name + '.js');
>     })
>   });
> 
> or whatever. Would that be easier and simpler and more easily explainable to
the
> universe than having to deal with the "URL rewriter chain" in the loaders?

Done.

http://codereview.appspot.com/96177/diff/1028/26#newcode3763
Line 3763: if (xhr.status  === 200) {
On 2009/08/03 18:24:09, ihab.awad wrote:
> Extra space before "===".

Done.

http://codereview.appspot.com/96177/diff/1028/26#newcode3767
Line 3767: r.resolve(theModule);
On 2009/08/03 18:24:09, ihab.awad wrote:
> Can eliminate local variable 'theModule' -> simpler?

Done.

http://codereview.appspot.com/96177/diff/1028/18
File tests/com/google/caja/a.js (right):

http://codereview.appspot.com/96177/diff/1028/18#newcode1
Line 1: ({
On 2009/08/03 18:24:09, ihab.awad wrote:
> Why do you need to create explicit cajoled modules in the test source
directory?
> You can instead use the build file to cajole some things into the ant-lib
> directory, similarly to how (say) domita_test_untrusted.html is cajoled at
build
> time.

Done.

http://codereview.appspot.com/96177/diff/1028/22
File tests/com/google/caja/plugin/domita_test.html (right):

http://codereview.appspot.com/96177/diff/1028/22#newcode408
Line 408: function (src) {return '/tests/com/google/caja/' + src;});
On 2009/08/03 18:24:09, ihab.awad wrote:
> How does that work without the "http://localhost:8000/..." stuff added to the
> front? :)

"http://localhost:8000" is implicit because the page is on localhost:8000. I did
not include that because using xhr loader implicitly requires the module to be
retrieved from the same domain.

http://codereview.appspot.com/96177/diff/1028/23
File tests/com/google/caja/plugin/domita_test_untrusted.html (right):

http://codereview.appspot.com/96177/diff/1028/23#newcode2508
Line 2508: function testXhrModuleLoader() {
On 2009/08/03 18:24:09, ihab.awad wrote:
> Indentation.

Done.

http://codereview.appspot.com/96177/diff/1028/23#newcode2519
Line 2519: var r = Q.when(m, f1, f2);
On 2009/08/03 18:24:09, ihab.awad wrote:
> Variable 'r' unused.

Done.

http://codereview.appspot.com/96177/diff/1028/23#newcode2555
Line 2555: function testScriptModuleLoader() {
On 2009/08/03 18:24:09, ihab.awad wrote:
> Indentation.

Done.

[ihab.awad, 2009-08-18 17:33:46]

Comments below, to discuss as well.

http://codereview.appspot.com/96177/diff/1028/22
File tests/com/google/caja/plugin/domita_test.html (right):

http://codereview.appspot.com/96177/diff/1028/22#newcode408
Line 408: function (src) {return '/tests/com/google/caja/' + src;});
On 2009/08/16 21:31:41, maoziqing wrote:
> "http://localhost:8000" is implicit because the page
> is on localhost:8000.

Ah, right. Ok.

http://codereview.appspot.com/96177/diff/4026/5034
File src/com/google/caja/cajita-module.js (right):

http://codereview.appspot.com/96177/diff/4026/5034#newcode8
Line 8: var xhrModuleLoaderMaker = function() {
Since this is stateless, does it need to be a maker? Can it just be a singleton?

http://codereview.appspot.com/96177/diff/4026/5034#newcode12
Line 12: if (window.moduleCache === undefined) {
Maybe "moduleCache___" to try to reduce the risk of conflicts with other apps?

http://codereview.appspot.com/96177/diff/4026/5034#newcode19
Line 19: else {
Code style: move "else" to previous line.

http://codereview.appspot.com/96177/diff/4026/5034#newcode26
Line 26: xhr.responseText.replace("___.loadModule", "")));
While this might work, it would be nice to instead do a
"___.setNewModuleHandler" and *use* the "___.loadModule" rather than trying to
textually strip it out. For example, strictly speaking, the thing you end up
with from your "replace" contains a trailing semicolon and is hence and
statement, not an expression....

http://codereview.appspot.com/96177/diff/4026/5034#newcode31
Line 31: else {
Code style as before.

http://codereview.appspot.com/96177/diff/4026/5034#newcode42
Line 42: xhr.overrideMimeType("application/json");
Why not "text/javascript" or "application/javascript"? The return value of a
cajoling service contains behavior so, strictly speaking, it is never truly
JSON.

http://codereview.appspot.com/96177/diff/4026/5034#newcode56
Line 56: var scriptModuleLoaderMaker = function() {
Again, since this is stateless (or, more correctly, since all the state is
global), can this just be a function, not a maker?

http://codereview.appspot.com/96177/diff/4026/5034#newcode100
Line 100: script.setAttribute("onerror", timeoutCall);
I believe you should be able to do script.onerror = <aFunction> directly.

http://codereview.appspot.com/96177/diff/4026/5034#newcode116
Line 116: if (window.moduleCache === undefined) {
Again, maybe add "___" to the end of all of these global variables?

http://codereview.appspot.com/96177/diff/4026/5034#newcode116
Line 116: if (window.moduleCache === undefined) {
It seems that, if you define a singleton <script> loader inside a closure, then
maybe only *one* of these variables needs to be a *true* global; the rest can be
local to the closure.

http://codereview.appspot.com/96177/diff/4026/5033
File src/com/google/caja/cajita-promise.js (right):

http://codereview.appspot.com/96177/diff/4026/5033#newcode25
Line 25: function reject(reason) {
Please enclose everything in a function closure so that the only thing put into
the global scope is "Q".

http://codereview.appspot.com/96177/diff/4026/5041
File src/com/google/caja/cajita.js (right):

http://codereview.appspot.com/96177/diff/4026/5041#newcode2653
Line 2653: var module = eval(str);
As long as our cajoler output always starts with "___.loadModule", we should
probably do module handler magic *inside* here ....

http://codereview.appspot.com/96177/diff/4026/5037
File src/com/google/caja/parser/quasiliteral/CajitaRewriter.java (right):

http://codereview.appspot.com/96177/diff/4026/5037#newcode251
Line 251: RewriterMessageType.CANNOT_LOAD_A_DYNAMIC_MODULE,
We should probably change this message to something like "dynamically computed
names should use load.async()" or something.

http://codereview.appspot.com/96177/diff/4026/5036
File src/com/google/caja/parser/quasiliteral/DefaultValijaRewriter.java (right):

http://codereview.appspot.com/96177/diff/4026/5036#newcode202
Line 202: RewriterMessageType.CANNOT_LOAD_A_DYNAMIC_MODULE,
See similar comment re Cajita ... "includeScript.async()".

http://codereview.appspot.com/96177/diff/4026/5035
File src/com/google/caja/parser/quasiliteral/ModuleManager.java (right):

http://codereview.appspot.com/96177/diff/4026/5035#newcode135
Line 135: else {
Code style: move else to previous line.

http://codereview.appspot.com/96177/diff/4026/5039
File src/com/google/caja/plugin/BuildServiceImplementation.java (right):

http://codereview.appspot.com/96177/diff/4026/5039#newcode239
Line 239: String callback = (String) options.get("callback");
If we're not doing JSONP, do we need these changes here?

[maoziqing, 2009-08-18 21:22:26]

http://codereview.appspot.com/96177/diff/4026/5034
File src/com/google/caja/cajita-module.js (right):

http://codereview.appspot.com/96177/diff/4026/5034#newcode8
Line 8: var xhrModuleLoaderMaker = function() {
On 2009/08/18 17:33:47, ihab.awad wrote:
> Since this is stateless, does it need to be a maker? Can it just be a
singleton?

Done.

http://codereview.appspot.com/96177/diff/4026/5034#newcode12
Line 12: if (window.moduleCache === undefined) {
On 2009/08/18 17:33:47, ihab.awad wrote:
> Maybe "moduleCache___" to try to reduce the risk of conflicts with other apps?

Done.

http://codereview.appspot.com/96177/diff/4026/5034#newcode19
Line 19: else {
On 2009/08/18 17:33:47, ihab.awad wrote:
> Code style: move "else" to previous line.

Done.

http://codereview.appspot.com/96177/diff/4026/5034#newcode26
Line 26: xhr.responseText.replace("___.loadModule", "")));
On 2009/08/18 17:33:47, ihab.awad wrote:
> While this might work, it would be nice to instead do a
> "___.setNewModuleHandler" and *use* the "___.loadModule" rather than trying to
> textually strip it out. For example, strictly speaking, the thing you end up
> with from your "replace" contains a trailing semicolon and is hence and
> statement, not an expression....

If we use ___.setNewModuleHandler, the xhr requests should be maintained in the
same queue as the script requests.

If we directly remove the ___.loadModule, the xhr requests can be sent in
parallel.

http://codereview.appspot.com/96177/diff/4026/5034#newcode31
Line 31: else {
On 2009/08/18 17:33:47, ihab.awad wrote:
> Code style as before.

Done.

http://codereview.appspot.com/96177/diff/4026/5034#newcode42
Line 42: xhr.overrideMimeType("application/json");
On 2009/08/18 17:33:47, ihab.awad wrote:
> Why not "text/javascript" or "application/javascript"? The return value of a
> cajoling service contains behavior so, strictly speaking, it is never truly
> JSON.

Done.

http://codereview.appspot.com/96177/diff/4026/5034#newcode56
Line 56: var scriptModuleLoaderMaker = function() {
On 2009/08/18 17:33:47, ihab.awad wrote:
> Again, since this is stateless (or, more correctly, since all the state is
> global), can this just be a function, not a maker?

Done.

http://codereview.appspot.com/96177/diff/4026/5034#newcode100
Line 100: script.setAttribute("onerror", timeoutCall);
On 2009/08/18 17:33:47, ihab.awad wrote:
> I believe you should be able to do script.onerror = <aFunction> directly.

Done.

http://codereview.appspot.com/96177/diff/4026/5034#newcode116
Line 116: if (window.moduleCache === undefined) {
On 2009/08/18 17:33:47, ihab.awad wrote:
> Again, maybe add "___" to the end of all of these global variables?

Done.

http://codereview.appspot.com/96177/diff/4026/5034#newcode116
Line 116: if (window.moduleCache === undefined) {
On 2009/08/18 17:33:47, ihab.awad wrote:
> It seems that, if you define a singleton <script> loader inside a closure,
then
> maybe only *one* of these variables needs to be a *true* global; the rest can
be
> local to the closure.

Done.

http://codereview.appspot.com/96177/diff/4026/5033
File src/com/google/caja/cajita-promise.js (right):

http://codereview.appspot.com/96177/diff/4026/5033#newcode25
Line 25: function reject(reason) {
On 2009/08/18 17:33:47, ihab.awad wrote:
> Please enclose everything in a function closure so that the only thing put
into
> the global scope is "Q".

Done.

http://codereview.appspot.com/96177/diff/4026/5041
File src/com/google/caja/cajita.js (right):

http://codereview.appspot.com/96177/diff/4026/5041#newcode2653
Line 2653: var module = eval(str);
On 2009/08/18 17:33:47, ihab.awad wrote:
> As long as our cajoler output always starts with "___.loadModule", we should
> probably do module handler magic *inside* here ....

Done.

http://codereview.appspot.com/96177/diff/4026/5037
File src/com/google/caja/parser/quasiliteral/CajitaRewriter.java (right):

http://codereview.appspot.com/96177/diff/4026/5037#newcode251
Line 251: RewriterMessageType.CANNOT_LOAD_A_DYNAMIC_MODULE,
On 2009/08/18 17:33:47, ihab.awad wrote:
> We should probably change this message to something like "dynamically computed
> names should use load.async()" or something.

Done.

http://codereview.appspot.com/96177/diff/4026/5036
File src/com/google/caja/parser/quasiliteral/DefaultValijaRewriter.java (right):

http://codereview.appspot.com/96177/diff/4026/5036#newcode202
Line 202: RewriterMessageType.CANNOT_LOAD_A_DYNAMIC_MODULE,
On 2009/08/18 17:33:47, ihab.awad wrote:
> See similar comment re Cajita ... "includeScript.async()".

Done.

http://codereview.appspot.com/96177/diff/4026/5035
File src/com/google/caja/parser/quasiliteral/ModuleManager.java (right):

http://codereview.appspot.com/96177/diff/4026/5035#newcode135
Line 135: else {
On 2009/08/18 17:33:47, ihab.awad wrote:
> Code style: move else to previous line.

Done.

http://codereview.appspot.com/96177/diff/4026/5039
File src/com/google/caja/plugin/BuildServiceImplementation.java (right):

http://codereview.appspot.com/96177/diff/4026/5039#newcode239
Line 239: String callback = (String) options.get("callback");
On 2009/08/18 17:33:47, ihab.awad wrote:
> If we're not doing JSONP, do we need these changes here?

Done.

[ihab.awad, 2009-08-18 21:27:47]

http://codereview.appspot.com/96177/diff/4026/5034
File src/com/google/caja/cajita-module.js (right):

http://codereview.appspot.com/96177/diff/4026/5034#newcode26
Line 26: xhr.responseText.replace("___.loadModule", "")));
On 2009/08/18 21:22:26, maoziqing wrote:
> If we directly remove the ___.loadModule, the xhr requests
> can be sent in parallel.

Echoing our verbal discussion for the folks in TV land. :) If we save the
previous module handler and restore it afterwards, and *assuming* there is no
browser quirk that executes the JavaScript "multi-threaded", we should be ok.

[maoziqing, 2009-08-18 21:36:44]

Done. Thanks.

On 2009/08/18 21:27:47, ihab.awad wrote:
> http://codereview.appspot.com/96177/diff/4026/5034
> File src/com/google/caja/cajita-module.js (right):
> 
> http://codereview.appspot.com/96177/diff/4026/5034#newcode26
> Line 26: xhr.responseText.replace("___.loadModule", "")));
> On 2009/08/18 21:22:26, maoziqing wrote:
> > If we directly remove the ___.loadModule, the xhr requests
> > can be sent in parallel.
> 
> Echoing our verbal discussion for the folks in TV land. :) If we save the
> previous module handler and restore it afterwards, and *assuming* there is no
> browser quirk that executes the JavaScript "multi-threaded", we should be ok.

[ihab.awad, 2009-08-19 23:34:38]

Looking pretty good!

Some more comments.

http://codereview.appspot.com/96177/diff/6048/7049
File src/com/google/caja/cajita-module.js (right):

http://codereview.appspot.com/96177/diff/6048/7049#newcode1
Line 1: /**
Missing copyright header.

http://codereview.appspot.com/96177/diff/6048/7049#newcode5
Line 5: * function given the source URL.
Also add your own name as the author in the top-of-file comment, as in
http://google-caja.googlecode.com/svn/trunk/src/com/google/caja/cajita.js.

http://codereview.appspot.com/96177/diff/6048/7049#newcode11
Line 11: cache = new Object();
In general, "cache = {}" is equivalent, shorter and more JavaScriptey.

http://codereview.appspot.com/96177/diff/6048/7049#newcode30
Line 30: cache[queue[head].src] = securedModule;
Just "cache[src] = ..."?

http://codereview.appspot.com/96177/diff/6048/7049#newcode48
Line 48: }
Please move the above 3 lines to a common function - to simplify refactoring in
case things end up needing to become more complicated.

http://codereview.appspot.com/96177/diff/6048/7049#newcode60
Line 60: var queue = new Object();
If you said, "var queue = []", thus declaring it as an array, you could do away
with the "counter" variable and just use "queue.length" instead.

http://codereview.appspot.com/96177/diff/6048/7049#newcode71
Line 71: if (head < counter) {
If you declared "queue" as an array, you could use "queue.length" instead of
"counter" here.

http://codereview.appspot.com/96177/diff/6048/7049#newcode92
Line 92: r.resolve(Q.reject("Retrieving module failed."));
Indentation

http://codereview.appspot.com/96177/diff/6048/7049#newcode96
Line 96: }
If this were an "} else { ... }" block, what would be in it? Under what
circumstances is "p !== head" (my reading is that this would be a serious
violation of your assumed invariants). At least, please add an "else" block with
a comment as to what that alternative situation would be.

http://codereview.appspot.com/96177/diff/6048/7049#newcode108
Line 108: timeout(savedHead);
Are you sure you want to call timeout() here? This is the *success* case, right?

http://codereview.appspot.com/96177/diff/6048/7049#newcode127
Line 127: busy = true;
It seems to me you don't need this line since dequeue() already sets the "busy"
state, right?

http://codereview.appspot.com/96177/diff/6048/7048
File src/com/google/caja/cajita-promise.js (right):

http://codereview.appspot.com/96177/diff/6048/7048#newcode20
Line 20: * Add the isPromise___ flag to support function promise
I thought about this some more. Assuming this work is derived from some or other
of the following:

http://waterken.svn.sourceforge.net/viewvc/waterken/server/trunk/waterken/con...

The MIT X license requires that we maintain the original copyright notice. I
think we should put at the top of the file:

"Copyright 2007-2009 Tyler Close under the terms of the MIT X license found at
http://www.opensource.org/licenses/mit-license.html"

and remove the Google copyright and Apache header. Then add yourself as a
contributor.

http://codereview.appspot.com/96177/diff/6048/7050
File src/com/google/caja/parser/quasiliteral/ModuleManager.java (right):

http://codereview.appspot.com/96177/diff/6048/7050#newcode51
Line 51: 
Trailing spaces at end of this line.

http://codereview.appspot.com/96177/diff/6048/7050#newcode60
Line 60: boolean isValija) {
Trailing spaces at end of this line.

http://codereview.appspot.com/96177/diff/6048/7051
File src/com/google/caja/parser/quasiliteral/PermitTemplate.java (right):

http://codereview.appspot.com/96177/diff/6048/7051#newcode66
Line 66: "am", CanCall,
This seems like a spurious diff caused by weird "svn update" order.

http://codereview.appspot.com/96177/diff/6048/7053
File src/com/google/caja/parser/quasiliteral/RewriterMessageType.java (right):

http://codereview.appspot.com/96177/diff/6048/7053#newcode184
Line 184: "%s: Dynamically computed names should use includeScript.async()",
Excellent!

http://codereview.appspot.com/96177/diff/6048/7039
File tests/com/google/caja/parser/quasiliteral/DefaultValijaRewriterTest.java
(right):

http://codereview.appspot.com/96177/diff/6048/7039#newcode19
Line 19: import com.google.caja.lexer.InputSource;
Please move these imports in with the rest of the "com.google.caja.*" ones,
sorted alphabetically.

http://codereview.appspot.com/96177/diff/6048/7039#newcode43
Line 43: import java.util.Collections;
Please rearrange imports for minimal delta from previous version.

http://codereview.appspot.com/96177/diff/6048/7045
File tests/com/google/caja/plugin/domita_test.html (right):

http://codereview.appspot.com/96177/diff/6048/7045#newcode393
Line 393: async: ___.markFuncFreeze(function(src) {
Please outdent 2 spaces (to be consistent with the other 3 structures below) or
indent the below 3 structures 2 spaces.

http://codereview.appspot.com/96177/diff/6048/7045#newcode395
Line 395: '/ant-lib/com/google/caja/' + src);
Nice demonstration of wrapping loaders with name remapping!

http://codereview.appspot.com/96177/diff/6048/7045#newcode411
Line 411: function(module) {return module({$v: testImports.$v}); });
Spaces after squiggly, as in "{ return ... }".

Also, this line should probably either be aligned with "m" above it, or indented
4 spaces.

(Same for "scriptIncludeScript" case below.)

(Yes these are nits.)

http://codereview.appspot.com/96177/diff/6048/7046
File tests/com/google/caja/plugin/domita_test_untrusted.html (right):

http://codereview.appspot.com/96177/diff/6048/7046#newcode396
Line 396: Recursive Module</p>
Can we move these to be in exactly the same order as the test in the JavaScript
below, to simplify keeping track of things in this file?

http://codereview.appspot.com/96177/diff/6048/7046#newcode628
Line 628: jsunitRegister('scriptModuleLoader',
testScriptModuleLoader

http://codereview.appspot.com/96177/diff/6048/7046#newcode634
Line 634: async: function(src) {return scriptModuleLoad.async('foo/' + src); }
Space before "return", as in "{ return ...". May be in other places in this file
as well. Also, continuation indent is only 4 spaces.

http://codereview.appspot.com/96177/diff/6048/7046#newcode635
Line 635: }, Q: Q});
Shouldn't you also pass 'fail'? 'b.js' uses it.

http://codereview.appspot.com/96177/diff/6048/7046#newcode649
Line 649: else {
Please move "else" one line up. Same for a few more cases somewhere (um...) else
in this file.

http://codereview.appspot.com/96177/diff/6048/7046#newcode661
Line 661: jsunitRegister('recursiveModule',
testRecursiveModule

http://codereview.appspot.com/96177/diff/6048/7046#newcode662
Line 662: function testRecursiveModule() {
This can be moved down -- since it has no timeout, it doesn't need to be at the
top of the file, right?

http://codereview.appspot.com/96177/diff/6048/7046#newcode686
Line 686: function testScriptModuleLoader() {
testScriptModuleLoaderFailTest?

http://codereview.appspot.com/96177/diff/6048/7046#newcode2724
Line 2724: jsunitRegister('xhrModuleLoader',
testXhrModuleLoader

http://codereview.appspot.com/96177/diff/6048/7046#newcode2725
Line 2725: function testXhrModuleLoader() {
Why are the XHR cases and the script cases totally different functions? I would
have figured that you would just call a common function like:

  function testAsyncModuleLoaders(load, scriptInclude)

then call it with the appropriate values of 'load' and 'scriptInclude'.

You could probably also get by with one failure test:

  function testAsyncModuleLoaderFailure(load, scriptInclude)

?

http://codereview.appspot.com/96177/diff/6048/7046#newcode2739
Line 2739: var m = xhrIncludeScript.async('x.vo');
Indentation

http://codereview.appspot.com/96177/diff/6048/7046#newcode2750
Line 2750: jsunitRegister('xhrModuleLoaderFailTest',
testXhrModuleLoaderFailTest (the test names need to be consistent with the HTML
IDs they correspond to)

http://codereview.appspot.com/96177/diff/6048/7046#newcode2751
Line 2751: function testScriptModuleLoader() {
testXhrModuleLoaderFailTest

[maoziqing, 2009-08-20 19:35:02]

http://codereview.appspot.com/96177/diff/6048/7049
File src/com/google/caja/cajita-module.js (right):

http://codereview.appspot.com/96177/diff/6048/7049#newcode1
Line 1: /**
On 2009/08/19 23:34:38, ihab.awad wrote:
> Missing copyright header.

Done.

http://codereview.appspot.com/96177/diff/6048/7049#newcode5
Line 5: * function given the source URL.
On 2009/08/19 23:34:38, ihab.awad wrote:
> Also add your own name as the author in the top-of-file comment, as in
> http://google-caja.googlecode.com/svn/trunk/src/com/google/caja/cajita.js.

Done.

http://codereview.appspot.com/96177/diff/6048/7049#newcode11
Line 11: cache = new Object();
On 2009/08/19 23:34:38, ihab.awad wrote:
> In general, "cache = {}" is equivalent, shorter and more JavaScriptey.

Done.

http://codereview.appspot.com/96177/diff/6048/7049#newcode30
Line 30: cache[queue[head].src] = securedModule;
On 2009/08/19 23:34:38, ihab.awad wrote:
> Just "cache[src] = ..."?

Done.

http://codereview.appspot.com/96177/diff/6048/7049#newcode48
Line 48: }
On 2009/08/19 23:34:38, ihab.awad wrote:
> Please move the above 3 lines to a common function - to simplify refactoring
in
> case things end up needing to become more complicated.

Done.

http://codereview.appspot.com/96177/diff/6048/7049#newcode60
Line 60: var queue = new Object();
On 2009/08/19 23:34:38, ihab.awad wrote:
> If you said, "var queue = []", thus declaring it as an array, you could do
away
> with the "counter" variable and just use "queue.length" instead.

Done.

http://codereview.appspot.com/96177/diff/6048/7049#newcode71
Line 71: if (head < counter) {
On 2009/08/19 23:34:38, ihab.awad wrote:
> If you declared "queue" as an array, you could use "queue.length" instead of
> "counter" here.

Done.

http://codereview.appspot.com/96177/diff/6048/7049#newcode92
Line 92: r.resolve(Q.reject("Retrieving module failed."));
On 2009/08/19 23:34:38, ihab.awad wrote:
> Indentation

Done.

http://codereview.appspot.com/96177/diff/6048/7049#newcode96
Line 96: }
On 2009/08/19 23:34:38, ihab.awad wrote:
> If this were an "} else { ... }" block, what would be in it? Under what
> circumstances is "p !== head" (my reading is that this would be a serious
> violation of your assumed invariants). At least, please add an "else" block
with
> a comment as to what that alternative situation would be.

See below.

http://codereview.appspot.com/96177/diff/6048/7049#newcode108
Line 108: timeout(savedHead);
On 2009/08/19 23:34:38, ihab.awad wrote:
> Are you sure you want to call timeout() here? This is the *success* case,
right?

this is both the failure and the success case.

In IE, this is the only event I can find for a failed loading. So we use "if (p
=== head)" in the timeout call the determine whether it is a failure or a
success.

Previously I was adding timeout for this call. Not sure whether it is necessary.

Maybe we can discuss this in more details in person.

http://codereview.appspot.com/96177/diff/6048/7049#newcode127
Line 127: busy = true;
On 2009/08/19 23:34:38, ihab.awad wrote:
> It seems to me you don't need this line since dequeue() already sets the
"busy"
> state, right?

Done.

http://codereview.appspot.com/96177/diff/6048/7048
File src/com/google/caja/cajita-promise.js (right):

http://codereview.appspot.com/96177/diff/6048/7048#newcode20
Line 20: * Add the isPromise___ flag to support function promise
On 2009/08/19 23:34:38, ihab.awad wrote:
> I thought about this some more. Assuming this work is derived from some or
other
> of the following:
> 
>
http://waterken.svn.sourceforge.net/viewvc/waterken/server/trunk/waterken/con...
> 
> The MIT X license requires that we maintain the original copyright notice. I
> think we should put at the top of the file:
> 
> "Copyright 2007-2009 Tyler Close under the terms of the MIT X license found at
> http://www.opensource.org/licenses/mit-license.html%22
> 
> and remove the Google copyright and Apache header. Then add yourself as a
> contributor.

Done.

http://codereview.appspot.com/96177/diff/6048/7050
File src/com/google/caja/parser/quasiliteral/ModuleManager.java (right):

http://codereview.appspot.com/96177/diff/6048/7050#newcode51
Line 51: 
On 2009/08/19 23:34:38, ihab.awad wrote:
> Trailing spaces at end of this line.

Done.

http://codereview.appspot.com/96177/diff/6048/7050#newcode60
Line 60: boolean isValija) {
On 2009/08/19 23:34:38, ihab.awad wrote:
> Trailing spaces at end of this line.

Done.

http://codereview.appspot.com/96177/diff/6048/7051
File src/com/google/caja/parser/quasiliteral/PermitTemplate.java (right):

http://codereview.appspot.com/96177/diff/6048/7051#newcode66
Line 66: "am", CanCall,
On 2009/08/19 23:34:38, ihab.awad wrote:
> This seems like a spurious diff caused by weird "svn update" order.

My fault. Done.

http://codereview.appspot.com/96177/diff/6048/7053
File src/com/google/caja/parser/quasiliteral/RewriterMessageType.java (right):

http://codereview.appspot.com/96177/diff/6048/7053#newcode184
Line 184: "%s: Dynamically computed names should use includeScript.async()",
On 2009/08/19 23:34:38, ihab.awad wrote:
> Excellent!

Done.

http://codereview.appspot.com/96177/diff/6048/7039
File tests/com/google/caja/parser/quasiliteral/DefaultValijaRewriterTest.java
(right):

http://codereview.appspot.com/96177/diff/6048/7039#newcode19
Line 19: import com.google.caja.lexer.InputSource;
On 2009/08/19 23:34:38, ihab.awad wrote:
> Please move these imports in with the rest of the "com.google.caja.*" ones,
> sorted alphabetically.

Done.

http://codereview.appspot.com/96177/diff/6048/7039#newcode43
Line 43: import java.util.Collections;
On 2009/08/19 23:34:38, ihab.awad wrote:
> Please rearrange imports for minimal delta from previous version.

Done.

http://codereview.appspot.com/96177/diff/6048/7045
File tests/com/google/caja/plugin/domita_test.html (right):

http://codereview.appspot.com/96177/diff/6048/7045#newcode393
Line 393: async: ___.markFuncFreeze(function(src) {
On 2009/08/19 23:34:38, ihab.awad wrote:
> Please outdent 2 spaces (to be consistent with the other 3 structures below)
or
> indent the below 3 structures 2 spaces.

Done.

http://codereview.appspot.com/96177/diff/6048/7045#newcode395
Line 395: '/ant-lib/com/google/caja/' + src);
On 2009/08/19 23:34:38, ihab.awad wrote:
> Nice demonstration of wrapping loaders with name remapping!

Done.

http://codereview.appspot.com/96177/diff/6048/7045#newcode411
Line 411: function(module) {return module({$v: testImports.$v}); });
On 2009/08/19 23:34:38, ihab.awad wrote:
> Spaces after squiggly, as in "{ return ... }".
> 
> Also, this line should probably either be aligned with "m" above it, or
indented
> 4 spaces.
> 
> (Same for "scriptIncludeScript" case below.)
> 
> (Yes these are nits.)

Done.

http://codereview.appspot.com/96177/diff/6048/7046
File tests/com/google/caja/plugin/domita_test_untrusted.html (right):

http://codereview.appspot.com/96177/diff/6048/7046#newcode628
Line 628: jsunitRegister('scriptModuleLoader',
On 2009/08/19 23:34:38, ihab.awad wrote:
> testScriptModuleLoader

Done.

http://codereview.appspot.com/96177/diff/6048/7046#newcode634
Line 634: async: function(src) {return scriptModuleLoad.async('foo/' + src); }
On 2009/08/19 23:34:38, ihab.awad wrote:
> Space before "return", as in "{ return ...". May be in other places in this
file
> as well. Also, continuation indent is only 4 spaces.

Done.

http://codereview.appspot.com/96177/diff/6048/7046#newcode635
Line 635: }, Q: Q});
On 2009/08/19 23:34:38, ihab.awad wrote:
> Shouldn't you also pass 'fail'? 'b.js' uses it.

Done.

http://codereview.appspot.com/96177/diff/6048/7046#newcode649
Line 649: else {
On 2009/08/19 23:34:38, ihab.awad wrote:
> Please move "else" one line up. Same for a few more cases somewhere (um...)
else
> in this file.

Done.

http://codereview.appspot.com/96177/diff/6048/7046#newcode661
Line 661: jsunitRegister('recursiveModule',
On 2009/08/19 23:34:38, ihab.awad wrote:
> testRecursiveModule

Done.

http://codereview.appspot.com/96177/diff/6048/7046#newcode662
Line 662: function testRecursiveModule() {
On 2009/08/19 23:34:38, ihab.awad wrote:
> This can be moved down -- since it has no timeout, it doesn't need to be at
the
> top of the file, right?

Done.

http://codereview.appspot.com/96177/diff/6048/7046#newcode686
Line 686: function testScriptModuleLoader() {
On 2009/08/19 23:34:38, ihab.awad wrote:
> testScriptModuleLoaderFailTest?

Done.

http://codereview.appspot.com/96177/diff/6048/7046#newcode2724
Line 2724: jsunitRegister('xhrModuleLoader',
On 2009/08/19 23:34:38, ihab.awad wrote:
> testXhrModuleLoader

Done.

http://codereview.appspot.com/96177/diff/6048/7046#newcode2725
Line 2725: function testXhrModuleLoader() {
On 2009/08/19 23:34:38, ihab.awad wrote:
> Why are the XHR cases and the script cases totally different functions? I
would
> have figured that you would just call a common function like:
> 
>   function testAsyncModuleLoaders(load, scriptInclude)
> 
> then call it with the appropriate values of 'load' and 'scriptInclude'.
> 
> You could probably also get by with one failure test:
> 
>   function testAsyncModuleLoaderFailure(load, scriptInclude)
> 
> ?

Done. Partially because of the caching.

http://codereview.appspot.com/96177/diff/6048/7046#newcode2739
Line 2739: var m = xhrIncludeScript.async('x.vo');
On 2009/08/19 23:34:38, ihab.awad wrote:
> Indentation

Done.

http://codereview.appspot.com/96177/diff/6048/7046#newcode2750
Line 2750: jsunitRegister('xhrModuleLoaderFailTest',
On 2009/08/19 23:34:38, ihab.awad wrote:
> testXhrModuleLoaderFailTest (the test names need to be consistent with the
HTML
> IDs they correspond to)

Done.

http://codereview.appspot.com/96177/diff/6048/7046#newcode2751
Line 2751: function testScriptModuleLoader() {
On 2009/08/19 23:34:38, ihab.awad wrote:
> testXhrModuleLoaderFailTest

Done.

[ihab.awad, 2009-08-20 21:07:16]

Plus verbal comments re error conditions in cajita-module.js

http://codereview.appspot.com/96177/diff/7064/6075
File src/com/google/caja/valija-cajita.js (right):

http://codereview.appspot.com/96177/diff/7064/6075#newcode500
Line 500: 
Again, this looks like a spurious diff.

http://codereview.appspot.com/96177/diff/7064/6075#newcode521
Line 521: 
Extra whitespace on this line.

http://codereview.appspot.com/96177/diff/7064/6066
File tests/com/google/caja/plugin/domita_test_untrusted.html (right):

http://codereview.appspot.com/96177/diff/7064/6066#newcode2683
Line 2683: function testScriptModuleLoader2() {
Indentation (and in a couple other places in this file).

[ihab.awad, 2009-08-20 22:10:58]

http://codereview.appspot.com/96177/diff/7064/6069
File src/com/google/caja/cajita-module.js (right):

http://codereview.appspot.com/96177/diff/7064/6069#newcode92
Line 92: handle: ___.markFuncFreeze(function theHandler(module) {
As we discussed, this function should exit without doing anything if it detects
"savedHead !== head".

http://codereview.appspot.com/96177/diff/7064/6069#newcode107
Line 107: function timeout(p) {
As we discussed, this does not need a param -- just use "savedHead". Also add a
comment clarifying the case where "savedHead !== head".

http://codereview.appspot.com/96177/diff/7064/6069#newcode118
Line 118: var savedHead = head;
As we discussed, move savedHead up ....

http://codereview.appspot.com/96177/diff/7064/6069#newcode122
Line 122: timeout(savedHead);
As we discussed, script.onerror and script.onreadystatechange should *both* do
"script.onerror = script.onreadystatechange = null" to clear things out.

[maoziqing, 2009-08-20 23:12:11]

http://codereview.appspot.com/96177/diff/7064/6069
File src/com/google/caja/cajita-module.js (right):

http://codereview.appspot.com/96177/diff/7064/6069#newcode92
Line 92: handle: ___.markFuncFreeze(function theHandler(module) {
On 2009/08/20 22:10:58, ihab.awad wrote:
> As we discussed, this function should exit without doing anything if it
detects
> "savedHead !== head".

Done.

http://codereview.appspot.com/96177/diff/7064/6069#newcode107
Line 107: function timeout(p) {
On 2009/08/20 22:10:58, ihab.awad wrote:
> As we discussed, this does not need a param -- just use "savedHead". Also add
a
> comment clarifying the case where "savedHead !== head".

Done.

http://codereview.appspot.com/96177/diff/7064/6069#newcode118
Line 118: var savedHead = head;
On 2009/08/20 22:10:58, ihab.awad wrote:
> As we discussed, move savedHead up ....

Done.

http://codereview.appspot.com/96177/diff/7064/6069#newcode122
Line 122: timeout(savedHead);
On 2009/08/20 22:10:58, ihab.awad wrote:
> As we discussed, script.onerror and script.onreadystatechange should *both* do
> "script.onerror = script.onreadystatechange = null" to clear things out.

Done.

http://codereview.appspot.com/96177/diff/7064/6075
File src/com/google/caja/valija-cajita.js (right):

http://codereview.appspot.com/96177/diff/7064/6075#newcode500
Line 500: 
On 2009/08/20 21:07:16, ihab.awad wrote:
> Again, this looks like a spurious diff.

Done.

http://codereview.appspot.com/96177/diff/7064/6075#newcode521
Line 521: 
On 2009/08/20 21:07:16, ihab.awad wrote:
> Extra whitespace on this line.

Done.

http://codereview.appspot.com/96177/diff/7064/6066
File tests/com/google/caja/plugin/domita_test_untrusted.html (right):

http://codereview.appspot.com/96177/diff/7064/6066#newcode2683
Line 2683: function testScriptModuleLoader2() {
On 2009/08/20 21:07:16, ihab.awad wrote:
> Indentation (and in a couple other places in this file).

Done.

[ihab.awad, 2009-08-21 00:46:12]

LGTM (2 remaining comments, but please check in after addressing these).

This is great progress!!

http://codereview.appspot.com/96177/diff/6080/7083
File src/com/google/caja/cajita-module.js (right):

http://codereview.appspot.com/96177/diff/6080/7083#newcode111
Line 111: }
Indentation is wrong here.

http://codereview.appspot.com/96177/diff/6080/7083#newcode167
Line 167: for (var m in cache) {
Use cajita.forOwnKeys()

[maoziqing, 2009-08-21 17:15:26]

committed revision 3654

http://codereview.appspot.com/96177/diff/6080/7083
File src/com/google/caja/cajita-module.js (right):

http://codereview.appspot.com/96177/diff/6080/7083#newcode111
Line 111: }
On 2009/08/21 00:46:12, ihab.awad wrote:
> Indentation is wrong here.

Done.

http://codereview.appspot.com/96177/diff/6080/7083#newcode167
Line 167: for (var m in cache) {
On 2009/08/21 00:46:12, ihab.awad wrote:
> Use cajita.forOwnKeys()

Done.