http://codereview.appspot.com/4535118/diff/17001/src/com/google/caja/es53.js
File src/com/google/caja/es53.js (right):
http://codereview.appspot.com/4535118/diff/17001/src/com/google/caja/es53.js#newcode2339
src/com/google/caja/es53.js:2339: * {@code typename} is used for
toString and {@code errorMessage} is used
On 2011/06/24 18:21:09, MikeSamuel wrote:
Maybe @param typename
Done.
http://codereview.appspot.com/4535118/diff/17001/src/com/google/caja/plugin/bridal.js
File src/com/google/caja/plugin/bridal.js (right):
http://codereview.appspot.com/4535118/diff/17001/src/com/google/caja/plugin/bridal.js#newcode29
src/com/google/caja/plugin/bridal.js:29: // TODO(kpreid): These were
previously gotten from the taming frame, not the host frame; is this OK?
On 2011/06/24 18:21:09, MikeSamuel wrote:
Getting them from the host frame allows the host frame to easily
remove/wrap/monkey patch these which may be useful.
OK, I will leave this as is and remove this comment. (In theory, bridal
simply provides tools for the one document and doesn't know anything
about taming vs. host...)
http://codereview.appspot.com/4535118/diff/17001/src/com/google/caja/plugin/bridal.js#newcode744
src/com/google/caja/plugin/bridal.js:744: }
On 2011/06/24 18:21:09, MikeSamuel wrote:
;
Done.
On the subject of this file, do you have a suggestion for where to
provide getWindow, as putting it on something named "fooMaker" is
clunky?
http://codereview.appspot.com/4535118/diff/17001/src/com/google/caja/plugin/caja.js
File src/com/google/caja/plugin/caja.js (right):
http://codereview.appspot.com/4535118/diff/17001/src/com/google/caja/plugin/caja.js#newcode328
src/com/google/caja/plugin/caja.js:328: * TODO(kpreid): document this.
On 2011/06/24 18:21:09, MikeSamuel wrote:
Yes please.
Done.
http://codereview.appspot.com/4535118/diff/17001/src/com/google/caja/plugin/caja.js#newcode332
src/com/google/caja/plugin/caja.js:332: if (typeof o !== 'undefined' &&
o !== null) {
On 2011/06/24 18:21:09, MikeSamuel wrote:
Isn't the below only effective for objects and functions. Are we
expecting
functions to be passed to this function? Can the test become
('object' ===
typeof o)?
I've changed it to (typeof o === 'object' && o !== null && !('v___' in
o)). I don't expect functions or value primitives, but I *do* expect
undefined or null.
http://codereview.appspot.com/4535118/diff/17001/src/com/google/caja/plugin/caja.js#newcode339
src/com/google/caja/plugin/caja.js:339: if (!(typeof p === 'number' ||
('' + (+p)) === p) && !(/__$/).test(p)) {
On 2011/06/24 18:21:09, MikeSamuel wrote:
How can typeof p be number when you've just converted it to a string
above?
This was a inlining of isNumericName from es53 without any attempt at
optimization. Fixed.
http://codereview.appspot.com/4535118/diff/17001/src/com/google/caja/plugin/caja.js#newcode340
src/com/google/caja/plugin/caja.js:340: if (typeof o[p] === 'function')
{
On 2011/06/24 18:21:09, MikeSamuel wrote:
Should we read o[p] once instead of here and again below before
applying.
Done, but shouldn't the same change then be made to es53.js?
http://codereview.appspot.com/4535118/diff/17001/src/com/google/caja/plugin/caja.js#newcode347
src/com/google/caja/plugin/caja.js:347: o.HasProperty___ = function (P)
{ return P in this; };
On 2011/06/24 18:21:09, MikeSamuel wrote:
P -> p
Done. This is what was in es53.js, but consistency here seems a fine
alternative.
http://codereview.appspot.com/4535118/diff/17001/src/com/google/caja/plugin/domado.js
File src/com/google/caja/plugin/domado.js (right):
http://codereview.appspot.com/4535118/diff/17001/src/com/google/caja/plugin/domado.js#newcode1
src/com/google/caja/plugin/domado.js:1: // Copyright (C) 2008-2011
Google Inc.
On 2011/06/24 18:21:09, MikeSamuel wrote:
Is this based on domita.js via svn mv in your local client?
When you diff via svn diff --notice_ancestry, do you get smaller
diffs?
It was but isn't at the moment. I haven't tried playing with the
diffing; but I think a diff-against-copied-file already gets the best
result. IIRC Rietveld doesn't diff-against-copy-source even if it's in
svn that way.
http://codereview.appspot.com/4535118/diff/17001/src/com/google/caja/ses/es5shim.js
File src/com/google/caja/ses/es5shim.js (right):
http://codereview.appspot.com/4535118/diff/17001/src/com/google/caja/ses/es5shim.js#newcode677
src/com/google/caja/ses/es5shim.js:677:
!desc.ses_ignoreBug_propertyWillAppearAsOwn) {
On 2011/06/24 18:21:09, MikeSamuel wrote:
Could you comment on this?
Done.
http://codereview.appspot.com/4535118/
