Very clever! :) lgtm++
http://codereview.appspot.com/6060046/diff/15001/src/com/google/caja/plugin/caja.js File src/com/google/caja/plugin/caja.js (right): http://codereview.appspot.com/6060046/diff/15001/src/com/google/caja/plugin/caja.js#newcode46 src/com/google/caja/plugin/caja.js:46: window[rndName] = function (result) { Is it not possible to install the named callback function *only* in the 'window' of the loader document, rather than installing it in the top-level window as well? The way we have it right here, it'll pollute the host frame.... http://codereview.appspot.com/6060046/diff/15001/src/com/google/caja/plugin/caja.js#newcode243 src/com/google/caja/plugin/caja.js:243: server = full['server'] = String( This should be done in 'initialize' or something. resolveConfig didn't side effect anything.... http://codereview.appspot.com/6060046/diff/15001/src/com/google/caja/plugin/caja.js#newcode456 src/com/google/caja/plugin/caja.js:456: var loaderDocument; Per our conversation, perhaps add tests where the container-supplied fetcher uses XHR and <script> tag appendChild, to make sure we're not depending on some feature of document.write-ing <script> HTML. http://codereview.appspot.com/6060046/diff/15001/src/com/google/caja/plugin/caja.js#newcode463 src/com/google/caja/plugin/caja.js:463: .replace(/[$]name/g, name)) This assignment pins the parent's handler function and, iiuc, this reference is never cleared out. http://codereview.appspot.com/6060046/diff/15001/src/com/google/caja/plugin/caja.js#newcode579 src/com/google/caja/plugin/caja.js:579: return caja; Why was it necessary to move this down? Felix's recent refactoring put the 'return' where it was because he wanted each module to look like "bunch of imperative code" followed by "bunch of function declarations". I'm fine with either style, personally -- just wondering why the change. http://codereview.appspot.com/6060046/diff/15001/src/com/google/caja/plugin/domado.js File src/com/google/caja/plugin/domado.js (right): http://codereview.appspot.com/6060046/diff/15001/src/com/google/caja/plugin/domado.js#newcode91 src/com/google/caja/plugin/domado.js:91: naiveUriPolicy.fetch(undefined, mime, callback); What does fetching 'undefined' do exactly? What is this case doing? http://codereview.appspot.com/6060046/diff/15001/src/com/google/caja/plugin/domado.js#newcode4919 src/com/google/caja/plugin/domado.js:4919: return domicile.writeHook.apply(undefined, arguments); I don't think document.write is *supposed* to return anything.... http://codereview.appspot.com/6060046/diff/15001/src/com/google/caja/plugin/guest-manager.js File src/com/google/caja/plugin/guest-manager.js (right): http://codereview.appspot.com/6060046/diff/15001/src/com/google/caja/plugin/guest-manager.js#newcode147 src/com/google/caja/plugin/guest-manager.js:147: setTimeout(opt_runDone, 1000); !! http://codereview.appspot.com/6060046/diff/15001/src/com/google/caja/plugin/html-emitter.js File src/com/google/caja/plugin/html-emitter.js (right): http://codereview.appspot.com/6060046/diff/15001/src/com/google/caja/plugin/html-emitter.js#newcode391 src/com/google/caja/plugin/html-emitter.js:391: function defineUntrustedExternalStylesheet(url, marker, continuation) { This function and the next have a lot of commonality that can be abstracted away. http://codereview.appspot.com/6060046/diff/15001/src/com/google/caja/plugin/html-emitter.js#newcode431 src/com/google/caja/plugin/html-emitter.js:431: var pendingExternal = undefined; I'm interested in the assumption that seems to be made here that documentWriter methods are never going to be called reentrantly. http://codereview.appspot.com/6060046/diff/15001/src/com/google/caja/plugin/html-emitter.js#newcode435 src/com/google/caja/plugin/html-emitter.js:435: startDoc: function() { documentLoaded = Q.defer(); }, Ok, and just to clarify to myself as I read this, the "Q" you are using here is in the taming frame. Which means its asynchrony ultimately bottoms out in the setTimeout() function of the taming frame. Given that any cross-iframe invocations are happening within the same domain, and therefore in the same event loop, things should compose without surprises. http://codereview.appspot.com/6060046/diff/15001/src/com/google/caja/plugin/html-sanitizer.js File src/com/google/caja/plugin/html-sanitizer.js (right): http://codereview.appspot.com/6060046/diff/15001/src/com/google/caja/plugin/html-sanitizer.js#newcode325 src/com/google/caja/plugin/html-sanitizer.js:325: if (h.startDoc && initial == 0) { h.startDoc(param); } Instead of this conditional every time -- if h.doSomething { ... } -- why not just wrap 'h' with something that contains the conditionals, then from that point on just use it without checking? http://codereview.appspot.com/6060046/diff/15001/src/com/google/caja/plugin/ses-frame-group.js File src/com/google/caja/plugin/ses-frame-group.js (right): http://codereview.appspot.com/6060046/diff/15001/src/com/google/caja/plugin/ses-frame-group.js#newcode378 src/com/google/caja/plugin/ses-frame-group.js:378: }); indentation http://codereview.appspot.com/6060046/diff/15001/src/com/google/caja/service/AbstractCajolingHandler.java File src/com/google/caja/service/AbstractCajolingHandler.java (right): http://codereview.appspot.com/6060046/diff/15001/src/com/google/caja/service/AbstractCajolingHandler.java#newcode150 src/com/google/caja/service/AbstractCajolingHandler.java:150: stray whitespace http://codereview.appspot.com/6060046/diff/15001/src/com/google/caja/service/AbstractCajolingHandler.java#newcode165 src/com/google/caja/service/AbstractCajolingHandler.java:165: protected static void renderAsJSON( un-indent 2 sp http://codereview.appspot.com/6060046/diff/15001/src/com/google/caja/service/ProxyHandler.java File src/com/google/caja/service/ProxyHandler.java (right): http://codereview.appspot.com/6060046/diff/15001/src/com/google/caja/service/ProxyHandler.java#newcode1 src/com/google/caja/service/ProxyHandler.java:1: // Copyright 2009 Google Inc. All Rights Reserved. Copyright year http://codereview.appspot.com/6060046/diff/15001/tests/com/google/caja/plugin/GeneralBrowserTest.java File tests/com/google/caja/plugin/GeneralBrowserTest.java (right): http://codereview.appspot.com/6060046/diff/15001/tests/com/google/caja/plugin/GeneralBrowserTest.java#newcode205 tests/com/google/caja/plugin/GeneralBrowserTest.java:205: runTestCase("es53-test-external-script.html"); For consistency, please name this es53-test-external-script-guest.html. (Yes I know es53-test-inline-script.html is not consistent -- we can change that now or later.) http://codereview.appspot.com/6060046/
