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/config/file/site/ref_send.js
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.
http://codereview.appspot.com/96177
