http://codereview.appspot.com/4535118/diff/61001/src/com/google/caja/plugin/domado.js
File src/com/google/caja/plugin/domado.js (right):
http://codereview.appspot.com/4535118/diff/61001/src/com/google/caja/plugin/domado.js#newcode66
src/com/google/caja/plugin/domado.js:66: // autoswitches as needed,
remapping to the (name[, value], proxy) order.
On 2011/07/29 00:26:38, MikeSamuel wrote:
Can we not change to be consistent?
If so, maybe add a TODO for whomever is likely to fix that.
My understanding is that Firefox is the one expected to change; else we
would not have done it differently. Stay or MarkM can comment further.
http://codereview.appspot.com/4535118/diff/61001/src/com/google/caja/plugin/domado.js#newcode88
src/com/google/caja/plugin/domado.js:88: var result = "can't see this";
On 2011/07/29 00:26:38, MikeSamuel wrote:
Why is this initial value here?
Because the linter complained about a possibly uninitialized value. I've
added a comment.
http://codereview.appspot.com/4535118/diff/61001/src/com/google/caja/plugin/domado.js#newcode104
src/com/google/caja/plugin/domado.js:104: return result;
On 2011/07/29 00:26:38, MikeSamuel wrote:
Maybe structure it like this
var needToSwap = false;
if (domitaModules.proxiesAvailable) {
// feature test to determine needToSwap
}
if (needToSwap) {
return <inline value of swapper here>
} else {
return <inline value of straight here>
}
instead of creating twice as many objects as needed even in the case
where
!domitaModules.proxiesAvailable.
Done.
http://codereview.appspot.com/4535118/diff/61001/src/com/google/caja/plugin/domado.js#newcode2281
src/com/google/caja/plugin/domado.js:2281: traceStartup("DT: about to
set toString for TameNode");
On 2011/07/29 00:26:38, MikeSamuel wrote:
Is this meant to be submitted?
traceStartup is statically a no-op above. I have found these debugging
lines sufficiently repeatedly useful for testing on Firefox (where
Firebug gives misleading stack traces for errors during Domado startup)
that I would rather not have to re-add them in the future, but if you
think they are too unsightly I would go ahead and do so.
http://codereview.appspot.com/4535118/diff/61001/src/com/google/caja/plugin/domado.js#newcode4610
src/com/google/caja/plugin/domado.js:4610: throw new
Error('document.write not provided for this document');
On 2011/07/29 00:26:38, MikeSamuel wrote:
write -> writeln
Done.
http://codereview.appspot.com/4535118/diff/61001/src/com/google/caja/plugin/domado.js#newcode4613
src/com/google/caja/plugin/domado.js:4613:
domicile.writeHook.call(undefined, '');
On 2011/07/29 00:26:38, MikeSamuel wrote:
What is this extra call supposed to do?
Can't this just become
var args = Array.slice.call(arguments, 0);
args.push("\n");
domicile.writeHook.apply(undefined, args);
?
The extra call was supposed to have a '\n'. The intent was to avoid
copying the arguments array.
http://codereview.appspot.com/4535118/
