General comment: I am concerned about the number of unrelated changes
here. I think this would be best split into several changesets, mainly
so that the parsing/Function change does not have any distractions in
it:
1. removing whitelist 'skip' support
2. PROTO_SETTER_UNGETTABLE
3. CANT_SAFELY_VERIFY_SYNTAX/parsing/Function
The grammar/comment fixups and removing uri.js from explicit.html can go
wherever or separately.
https://codereview.appspot.com/10075043/diff/38001/src/com/google/caja/ses/repairES5.js
File src/com/google/caja/ses/repairES5.js (right):
https://codereview.appspot.com/10075043/diff/38001/src/com/google/caja/ses/repairES5.js#newcode16
src/com/google/caja/ses/repairES5.js:16: * @fileoverview Monkey patch an
almost ES5 platforms into a closer
"an ... platforms" — Original was consistent, this isn't.
https://codereview.appspot.com/10075043/diff/38001/src/com/google/caja/ses/repairES5.js#newcode649
src/com/google/caja/ses/repairES5.js:649: /**
I think it'd be good to have a leading comment about why this
verification stuff is living in repairES5.
https://codereview.appspot.com/10075043/diff/38001/src/com/google/caja/ses/repairES5.js#newcode672
src/com/google/caja/ses/repairES5.js:672: } catch (err) {
Please comment out or omit the entire try-catch instead. Rethrowing is
obnoxious to debug through.
https://codereview.appspot.com/10075043/diff/38001/src/com/google/caja/ses/repairES5.js#newcode720
src/com/google/caja/ses/repairES5.js:720: if (err == "not a
SyntaxError") {
===, single quotes
https://codereview.appspot.com/10075043/diff/38001/src/com/google/caja/ses/repairES5.js#newcode2372
src/com/google/caja/ses/repairES5.js:2372: // We test this horrible way
first, since the test we'd like to use
"This horrible way" is overly obscure to the skimming reader. Please be
more explicit.
https://codereview.appspot.com/10075043/diff/38001/src/com/google/caja/ses/repairES5.js#newcode2380
src/com/google/caja/ses/repairES5.js:2380: ses.__bogus__ = false;
I don't like the use of a __foo__ name, and also that this is otherwise
unnamespaced. How about "ses.CANT_SAFELY_VERIFY_SYNTAX_canary"?
Also, delete the property afterward for cleanup of the public interface.
https://codereview.appspot.com/10075043/diff/38001/src/com/google/caja/ses/repairES5.js#newcode3290
src/com/google/caja/ses/repairES5.js:3290: * <p>TODO(kpreid): Is it
worth using ses.mitigateSrcGotchas for
Don't pin this one on me.
https://codereview.appspot.com/10075043/diff/38001/src/com/google/caja/ses/repairES5.js#newcode4108
src/com/google/caja/ses/repairES5.js:4108: repair:
repair_CANT_SAFELY_VERIFY_SYNTAX,
Please add something like "This does not repair Function but only
verifyStrictProgram (see above)" here, so that the kludge table can be
accurately read as a summary.
https://codereview.appspot.com/10075043/diff/38001/src/com/google/caja/ses/startSES.js
File src/com/google/caja/ses/startSES.js (right):
https://codereview.appspot.com/10075043/diff/38001/src/com/google/caja/ses/startSES.js#newcode518
src/com/google/caja/ses/startSES.js:518: * double parens it still parses
as a strict Program. We place a
Not new: It'd be nice if this explained what good the two tests do.
https://codereview.appspot.com/10075043/diff/38001/src/com/google/caja/ses/startSES.js#newcode832
src/com/google/caja/ses/startSES.js:832: opt_mitigateOpts,
Adding this parameter is an API change for clients who use
opt_sourcePosition. Either place the parameter last or document it
([APICHANGE] tag, description of impact / what to fix) in the change
description.
Also, this is an awful lot of positional optional parameters. Perhaps it
should be a record.
https://codereview.appspot.com/10075043/
--
---
You received this message because you are subscribed to the Google Groups "Google Caja Discuss" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/groups/opt_out.
