On Jun 5, 2011, at 23:22, [email protected] wrote: > By no means comprehensive. Just some things I spotted.
As a general remark, I can see that I should have noted that es5-guards.js is, ah, not exactly polished and thoughtfully designed. It is merely the repository of everything I needed to support Domado which was a preexisting API. > http://codereview.appspot.com/4535118/diff/2001/src/com/google/caja/es5-guards.js > File src/com/google/caja/es5-guards.js (right): > > http://codereview.appspot.com/4535118/diff/2001/src/com/google/caja/es5-guards.js#newcode40 > src/com/google/caja/es5-guards.js:40: toString: Object.freeze(function > tokenToString() { > fotgot to freeze tokenToString.prototype. This comes up below too. > Should probably just use SES's cajaVM.def. I had not previously noticed that SES provided a cajaVM object, and assumed it was strictly an ES5/3 feature. I will examine it for relevant facilities. > http://codereview.appspot.com/4535118/diff/2001/src/com/google/caja/es5-guards.js#newcode100 > src/com/google/caja/es5-guards.js:100: var failFunc = opt_failFunc || > function (x) { return x; }; > This will evaluate the function expression to a new identity function on > every call to callWithEjector. Better to factor out. > > http://codereview.appspot.com/4535118/diff/2001/src/com/google/caja/es5-guards.js#newcode103 > src/com/google/caja/es5-guards.js:103: // token.UNCATCHABLE___ = true; > // TODO(kpreid): can't do in ES5? > Correct. The whole uncatchable notion cannot be supported on a > lite-verification-only strategy. Right; the question is really whether it matters. For example: given that we cannot have uncatchables for our ejector implementation, should we perhaps define the thrown object so that it makes more sense to outside code which observes it? > http://codereview.appspot.com/4535118/diff/2001/src/com/google/caja/es5-guards.js#newcode152 > src/com/google/caja/es5-guards.js:152: // newTable as well, but perhaps > we should. > Yes, I think we should be using WeakMap-based permissions amplification > rather than MarcS's pattern of authority amplification. Agreed; I will change this code. Most of es53-guards was hacked up before I moved to actually using WeakMaps anywhere. > http://codereview.appspot.com/4535118/diff/2001/src/com/google/caja/es5-guards.js#newcode181 > src/com/google/caja/es5-guards.js:181: this.seal = seal; > Using "this" in defensive code is generally a really bad idea. This > should be creating and returning a new object instead. Why does it matter? This constructor is using 'this' just as a convenient bundle for the three functions; the 'this' object is not used further by the code. That said, I can see that it ought to freeze the prototype and constructor for defensive consistency. > http://codereview.appspot.com/4535118/diff/2001/src/com/google/caja/es5-guards.js#newcode208 > src/com/google/caja/es5-guards.js:208: _es5g_mark: > markSealing.seal(Object.freeze(function(obj) { // TODO(kpreid): make > this nonenumerable > please stay within 80 columns I have bothered to ensure 80 columns only on those lines which I expect to make it into the final commit. > http://codereview.appspot.com/4535118/diff/2001/src/com/google/caja/es53.js > File src/com/google/caja/es53.js (right): > > http://codereview.appspot.com/4535118/diff/2001/src/com/google/caja/es53.js#newcode2033 > src/com/google/caja/es53.js:2033: // Tokens > Are you planning to remove the parts of es53.js that are redundant with > es5-guards.js? (I hope so) I had not considered modifying es53.js, but I agree it would be good to have only one implementation of these facilities. Supposing that this were to happen, what would be a good name for the file? It is not all about guards, after all. > http://codereview.appspot.com/4535118/diff/2001/src/com/google/caja/ses/whitelist.js > File src/com/google/caja/ses/whitelist.js (right): > > http://codereview.appspot.com/4535118/diff/2001/src/com/google/caja/ses/whitelist.js#newcode115 > src/com/google/caja/ses/whitelist.js:115: WeakMap: t, > "WeakMap: {..." appears below, so this seems redundant. My error; I was poking around trying to get the WeakMap constructor to show up in the actual runtime environment. Removed. > http://codereview.appspot.com/4535118/ -- Kevin Reid <http://switchb.org/kpreid/>
