Not separated into separate CL yet. Snapshotting first so you can see
the changes made in response to your comments inline. However, don't
respond again until you see these broken into separate CLs.
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
On 2013/06/10 20:44:35, kpreid2 wrote:
"an ... platforms" — Original was consistent, this isn't.
Done.
https://codereview.appspot.com/10075043/diff/38001/src/com/google/caja/ses/repairES5.js#newcode649
src/com/google/caja/ses/repairES5.js:649: /**
On 2013/06/10 20:44:35, kpreid2 wrote:
I think it'd be good to have a leading comment about why this
verification stuff
is living in repairES5.
Done. Moved ses.verifyStrictProgram first and put the documentation
there.
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) {
On 2013/06/10 20:44:35, kpreid2 wrote:
Please comment out or omit the entire try-catch instead. Rethrowing is
obnoxious
to debug through.
Done.
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") {
On 2013/06/10 20:44:35, kpreid2 wrote:
===, single quotes
Done.
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
On 2013/06/10 20:44:35, kpreid2 wrote:
"This horrible way" is overly obscure to the skimming reader. Please
be more
explicit.
Done.
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;
On 2013/06/10 20:44:35, kpreid2 wrote:
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.
Done.
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
On 2013/06/10 20:44:35, kpreid2 wrote:
Don't pin this one on me.
Done. Since you didn't suggest anyone else, I pinned it on me and
decided. verifyStrictProgramByParsing is gone. We always repair this
with verifyStrictProgramByEvalThrowing.
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,
On 2013/06/10 20:44:35, kpreid2 wrote:
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.
Done.
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
On 2013/06/10 20:44:35, kpreid2 wrote:
Not new: It'd be nice if this explained what good the two tests do.
Done.
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,
On 2013/06/10 20:44:35, kpreid2 wrote:
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.
Done. I folded opt_sourcePosition into opt_mitigateOpts in an upwards
compatible way.
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.
