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? http://codereview.appspot.com/96177
