Minor uncontributed improvements

Several little things
* changed many double quoted strings to single quotes
* StringMap can now be used without ses
* optional source position for evalators have been moved into the options object
* removed "skip" from whitelist and whitelist mechanism since we don't currently need it

[MarkM, 2013-06-06 06:36:35]

Array.prototype.length no longer needs to be skipped as the relevant
bug is long dead.
Test for Opera specific bug
Test for Function constructor bug -- but description record commented
out for now.

[kpreid2, 2013-06-06 16:41:43]

https://codereview.appspot.com/10075043/diff/2001/src/com/google/caja/ses/rep...
File src/com/google/caja/ses/repairES5.js (right):

https://codereview.appspot.com/10075043/diff/2001/src/com/google/caja/ses/rep...
src/com/google/caja/ses/repairES5.js:3901: * reactions to tests and repairs. Off
for now because this is
Unfortunately said client-controlled reactions are unsuitable for this case
because we have no notion of SES 'containing' repairES5 (e.g. there's only one
maxSeverity state and correspondingly only one 'client' interface).

I think it would be a worthwhile idea to introduce that, but until then I
recommend that we consider this a SAFE_SPEC_VIOLATION (with comments explaining
why). Then we can turn it on right now, including having resolveOptions use it
to decide whether to force parseProgram on.

https://codereview.appspot.com/10075043/diff/2001/src/com/google/caja/ses/whi...
File src/com/google/caja/ses/whitelist.js (right):

https://codereview.appspot.com/10075043/diff/2001/src/com/google/caja/ses/whi...
src/com/google/caja/ses/whitelist.js:75: * can retire all "skip" entries by the
time SES is ready for secure
There are no longer any 'skip' entries. Should we retire the skip mechanism
itself, now? If not, should these comments be lightly updated?

[MarkM, 2013-06-07 02:57:23]

https://codereview.appspot.com/10075043/diff/2001/src/com/google/caja/ses/rep...
File src/com/google/caja/ses/repairES5.js (right):

https://codereview.appspot.com/10075043/diff/2001/src/com/google/caja/ses/rep...
src/com/google/caja/ses/repairES5.js:3901: * reactions to tests and repairs. Off
for now because this is
On 2013/06/06 16:41:43, kpreid2 wrote:
> Unfortunately said client-controlled reactions are unsuitable for this case
> because we have no notion of SES 'containing' repairES5 (e.g. there's only one
> maxSeverity state and correspondingly only one 'client' interface).
> 
> I think it would be a worthwhile idea to introduce that, but until then I
> recommend that we consider this a SAFE_SPEC_VIOLATION (with comments
explaining
> why). Then we can turn it on right now, including having resolveOptions use it
> to decide whether to force parseProgram on.

If SES were always run with source mitigations available, that would be ok. But
when SES is run standalone without source mitigations, that change of setting
would fail unsafe.

I don't think I yet understand why I wouldn't be able to use your current CL to
express this. I'll hold off until you submit that one and then see what happens
when I try.

[kpreid2, 2013-06-08 00:23:16]

https://codereview.appspot.com/10075043/diff/2001/src/com/google/caja/ses/rep...
File src/com/google/caja/ses/repairES5.js (right):

https://codereview.appspot.com/10075043/diff/2001/src/com/google/caja/ses/rep...
src/com/google/caja/ses/repairES5.js:3901: * reactions to tests and repairs. Off
for now because this is
On 2013/06/07 02:57:23, MarkM wrote:
> If SES were always run with source mitigations available, that would be ok.
> But when SES is run standalone without source mitigations, that change of
> setting would fail unsafe.

Agreed.

> I don't think I yet understand why I wouldn't be able to use your current CL
> to express this. I'll hold off until you submit that one and then see what
> happens when I try.

r5442 is in.

[kpreid2, 2013-06-08 00:23:16]

https://codereview.appspot.com/10075043/diff/2001/src/com/google/caja/ses/rep...
File src/com/google/caja/ses/repairES5.js (right):

https://codereview.appspot.com/10075043/diff/2001/src/com/google/caja/ses/rep...
src/com/google/caja/ses/repairES5.js:3901: * reactions to tests and repairs. Off
for now because this is
On 2013/06/07 02:57:23, MarkM wrote:
> If SES were always run with source mitigations available, that would be ok.
> But when SES is run standalone without source mitigations, that change of
> setting would fail unsafe.

Agreed.

> I don't think I yet understand why I wouldn't be able to use your current CL
> to express this. I'll hold off until you submit that one and then see what
> happens when I try.

r5442 is in.

[MarkM, 2013-06-09 01:34:21]

Array.prototype.length no longer needs to be skipped as the relevant
bug is long dead.
Test for Opera specific bug
Test for Function constructor bug -- but description record commented
out for now.

[MarkM, 2013-06-09 05:41:57]

Array.prototype.length no longer needs to be skipped as the relevant
bug is long dead.
Test for Opera specific bug
Test for Function constructor bug -- but description record commented
out for now.

[MarkM, 2013-06-09 05:50:22]

But "ant runtests" fails differently, and I don't know how to diagnose it.

https://codereview.appspot.com/10075043/diff/2001/src/com/google/caja/ses/rep...
File src/com/google/caja/ses/repairES5.js (right):

https://codereview.appspot.com/10075043/diff/2001/src/com/google/caja/ses/rep...
src/com/google/caja/ses/repairES5.js:3901: * reactions to tests and repairs. Off
for now because this is
On 2013/06/08 00:23:16, kpreid2 wrote:
> On 2013/06/07 02:57:23, MarkM wrote:
> > If SES were always run with source mitigations available, that would be ok.
> > But when SES is run standalone without source mitigations, that change of
> > setting would fail unsafe.
> 
> Agreed.
> 
> > I don't think I yet understand why I wouldn't be able to use your current CL
> > to express this. I'll hold off until you submit that one and then see what
> > happens when I try.
> 
> r5442 is in.

Done. But differently than expected. Please take a careful look.

https://codereview.appspot.com/10075043/diff/2001/src/com/google/caja/ses/whi...
File src/com/google/caja/ses/whitelist.js (right):

https://codereview.appspot.com/10075043/diff/2001/src/com/google/caja/ses/whi...
src/com/google/caja/ses/whitelist.js:75: * can retire all "skip" entries by the
time SES is ready for secure
On 2013/06/06 16:41:43, kpreid2 wrote:
> There are no longer any 'skip' entries. Should we retire the skip mechanism
> itself, now? If not, should these comments be lightly updated?

Done. Retired.

[Mark S. Miller, 2013-06-09 05:54:22]

[transform] WARNING : repairES5.js:789+7 - 15: Operation has no effect
[transform] repairES5.js:789:       desc.set; // yes, just reading it
[transform]                         ^^^^^^^^
[transform] LINT : repairES5.js:1845+7 - 22: a originally defined at
repairES5.js:1844+10 - 1848+6
[transform] repairES5.js:1845:       function a() {}
[transform]                          ^^^^^^^^^^^^^^^
[transform] repairES5.js:1844:     x = (function a() {
[transform]                             ^^^^^^^^^^^^^^^
[transform]   1 NO_SIDE_EFFECT
[transform]   1 SYMBOL_REDEFINED
[transform] Failed to build
/Users/erights/svn-changes/timings/google-caja/ant-lib/initSES.jslint.stamp
[transform] at
com.google.caja.tools.AbstractCajaAntTask$Output.build(AbstractCajaAntTask.java:212)
[transform] at
com.google.caja.tools.AbstractCajaAntTask.execute(AbstractCajaAntTask.java:91)
[transform] at
org.apache.tools.ant.UnknownElement.execute(UnknownElement.java:291)
[transform] at sun.reflect.GeneratedMethodAccessor4.invoke(Unknown Source)
[transform] at
sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
[transform] at java.lang.reflect.Method.invoke(Method.java:597)
[transform] at
org.apache.tools.ant.dispatch.DispatchUtils.execute(DispatchUtils.java:106)
[transform] at org.apache.tools.ant.Task.perform(Task.java:348)
[transform] at org.apache.tools.ant.Target.execute(Target.java:390)
[transform] at org.apache.tools.ant.Target.performTasks(Target.java:411)
[transform] at
org.apache.tools.ant.Project.executeSortedTargets(Project.java:1399)
[transform] at org.apache.tools.ant.Project.executeTarget(Project.java:1368)
[transform] at
org.apache.tools.ant.helper.DefaultExecutor.executeTargets(DefaultExecutor.java:41)
[transform] at
org.apache.tools.ant.Project.executeTargets(Project.java:1251)
[transform] at org.apache.tools.ant.Main.runBuild(Main.java:809)
[transform] at org.apache.tools.ant.Main.startAnt(Main.java:217)
[transform] at org.apache.tools.ant.launch.Launcher.run(Launcher.java:280)
[transform] at org.apache.tools.ant.launch.Launcher.main(Launcher.java:109)

BUILD FAILED
/Users/erights/svn-changes/timings/google-caja/build.xml:890: Failed to
build
/Users/erights/svn-changes/timings/google-caja/ant-lib/initSES.jslint.stamp

[MarkM, 2013-06-09 05:59:20]

Array.prototype.length no longer needs to be skipped as the relevant
bug is long dead.
Test for Opera specific bug
Test for Function constructor bug -- but description record commented
out for now.

[kpreid2, 2013-06-09 13:16:54]

On 2013/06/09 05:54:22, Mark S. Miller wrote:
> [transform] WARNING : repairES5.js:789+7 - 15: Operation has no effect
> [transform] repairES5.js:789:       desc.set; // yes, just reading it
> [transform]                         ^^^^^^^^

Warnings are fatal. I think the linter can be satisfied by "void desc.set;"
here.

(Have not done any review, just advising on the problem.)

[Mark S. Miller, 2013-06-09 15:39:43]

I had no idea. That was it. Tests pass now. Thanks.


On Sun, Jun 9, 2013 at 6:16 AM, <kpreid.switchb.org@gmail.com> wrote:

> On 2013/06/09 05:54:22, Mark S. Miller wrote:
>
>> [transform] WARNING : repairES5.js:789+7 - 15: Operation has no effect
>> [transform] repairES5.js:789:       desc.set; // yes, just reading it
>> [transform]                         ^^^^^^^^
>>
>
> Warnings are fatal. I think the linter can be satisfied by "void
> desc.set;" here.
>
> (Have not done any review, just advising on the problem.)
>
>
https://codereview.appspot.**com/10075043/<https://codereview.appspot.com/100...
>



-- 
    Cheers,
    --MarkM

[MarkM, 2013-06-09 15:40:44]

Array.prototype.length no longer needs to be skipped as the relevant
bug is long dead.
Test for Opera specific bug
Test for Function constructor bug -- but description record commented
out for now.

[MarkM, 2013-06-09 15:59:17]

Array.prototype.length no longer needs to be skipped as the relevant
bug is long dead.
Test for Opera specific bug
Test for Function constructor bug -- but description record commented
out for now.

[MarkM, 2013-06-09 16:26:29]

Mousing over the issue number in codereview indicates that I have two draft
comments, but I don't see them anywhere. Probably a mistake. Sending this in
hopes of either revealing them or resetting stale state.

[MarkM, 2013-06-09 16:27:25]

On 2013/06/09 16:26:29, MarkM wrote:
> Mousing over the issue number in codereview indicates that I have two draft
> comments, but I don't see them anywhere. Probably a mistake. Sending this in
> hopes of either revealing them or resetting stale state.

Neither happened

[felix8a, 2013-06-09 19:06:10]

On 2013/06/09 16:26:29, MarkM wrote:
> Mousing over the issue number in codereview indicates that I have two draft
> comments, but I don't see them anywhere. Probably a mistake. Sending this in
> hopes of either revealing them or resetting stale state.

my guess is draft comments in previous patch sets. expand those, it should show
you.

[Mark S. Miller, 2013-06-09 19:08:53]

Just tried it. No dice.


On Sun, Jun 9, 2013 at 12:06 PM, <felix8a@gmail.com> wrote:

> On 2013/06/09 16:26:29, MarkM wrote:
>
>> Mousing over the issue number in codereview indicates that I have two
>>
> draft
>
>> comments, but I don't see them anywhere. Probably a mistake. Sending
>>
> this in
>
>> hopes of either revealing them or resetting stale state.
>>
>
> my guess is draft comments in previous patch sets. expand those, it
> should show you.
>
>
https://codereview.appspot.**com/10075043/<https://codereview.appspot.com/100...
>



-- 
    Cheers,
    --MarkM

[kpreid2, 2013-06-10 20:44:35]

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/re...
File src/com/google/caja/ses/repairES5.js (right):

https://codereview.appspot.com/10075043/diff/38001/src/com/google/caja/ses/re...
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/re...
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/re...
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/re...
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/re...
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/re...
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/re...
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/re...
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/st...
File src/com/google/caja/ses/startSES.js (right):

https://codereview.appspot.com/10075043/diff/38001/src/com/google/caja/ses/st...
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/st...
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.

[MarkM, 2013-06-11 03:30:26]

Array.prototype.length no longer needs to be skipped as the relevant
bug is long dead.
Test for Opera specific bug
Test for Function constructor bug -- but description record commented
out for now.

[MarkM, 2013-06-11 03:35:47]

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/re...
File src/com/google/caja/ses/repairES5.js (right):

https://codereview.appspot.com/10075043/diff/38001/src/com/google/caja/ses/re...
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/re...
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/re...
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/re...
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/re...
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/re...
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/re...
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/re...
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/st...
File src/com/google/caja/ses/startSES.js (right):

https://codereview.appspot.com/10075043/diff/38001/src/com/google/caja/ses/st...
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/st...
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.

[MarkM, 2013-06-13 16:03:56]

Array.prototype.length no longer needs to be skipped as the relevant
bug is long dead.
Test for Opera specific bug
Test for Function constructor bug -- but description record commented
out for now.

[MarkM, 2013-06-13 16:07:27]

Removed changes which have moved to https://codereview.appspot.com/10181043/

Still haven't separated new Opera test case into separate CL.

[MarkM, 2013-07-06 22:57:10]

Array.prototype.length no longer needs to be skipped as the relevant
bug is long dead.
Test for Opera specific bug
Test for Function constructor bug -- but description record commented
out for now.

[MarkM, 2013-07-06 23:03:09]

On 2013/06/13 16:07:27, MarkM wrote:
> Removed changes which have moved to https://codereview.appspot.com/10181043/
> 
> Still haven't separated new Opera test case into separate CL.

Done -- Opera change saved off to the side. Will show up as a CL at some point.

[MarkM, 2013-07-07 02:49:38]

Array.prototype.length no longer needs to be skipped as the relevant
bug is long dead.
Test for Opera specific bug
Test for Function constructor bug -- but description record commented
out for now.

[kpreid2, 2013-07-09 16:34:59]

https://codereview.appspot.com/10075043/diff/86001/src/com/google/caja/ses/St...
File src/com/google/caja/ses/StringMap.js (right):

https://codereview.appspot.com/10075043/diff/86001/src/com/google/caja/ses/St...
src/com/google/caja/ses/StringMap.js:20: * @overrides ses
Instead of @overrides (I assume this is needed because of the var declaration?),
how about replacing the || {} chain with

if (typeof ses !== 'undefined' &&
ses.es5ProblemReports.FREEZING_BREAKS_PROTOTYPES.beforeFailure)

This will fail if the intermediate objects are missing, but that is an
appropriate inconsistency-failure to catch.

https://codereview.appspot.com/10075043/diff/86001/src/com/google/caja/ses/St...
src/com/google/caja/ses/StringMap.js:28: "use strict";
While we're fixing minor things: single quotes.

https://codereview.appspot.com/10075043/diff/86001/src/com/google/caja/ses/co...
File src/com/google/caja/ses/compileExprLater.js (right):

https://codereview.appspot.com/10075043/diff/86001/src/com/google/caja/ses/co...
src/com/google/caja/ses/compileExprLater.js:111: if (options.sourceUrl) {
This condition is incorrect because the empty string is falsy and is a valid
(relative) URL. Even if relative URLs are useless here we shouldn't treat them
as absent.

https://codereview.appspot.com/10075043/diff/86001/src/com/google/caja/ses/st...
File src/com/google/caja/ses/startSES.js (right):

https://codereview.appspot.com/10075043/diff/86001/src/com/google/caja/ses/st...
src/com/google/caja/ses/startSES.js:1438: if (result === '*') {
Note that my current typed-array-support draft requires 'skip'. I plan to look
into whether there's something we can do which is stricter than 'skip', but I
haven't done so yet.

You can still remove 'skip' now; I'm just noting that I might end up making
semi-reverting changes here.

(The problem — I forget whether I explained this already — is that the typed
array prototypes have accessor properties, and the whitelist mechanism
essentially assumes that every built-in property is a data property (var sub =
value[name]; in the register function), which is true for ES5, but not for typed
arrays as implemented now and as in ES6-draft.)

[kpreid2, 2013-07-09 16:35:48]

Oh, and please update the change description for the current contents, and to be
less elliptical.

[MarkM, 2013-07-12 02:25:00]

Array.prototype.length no longer needs to be skipped as the relevant
bug is long dead.
Test for Opera specific bug
Test for Function constructor bug -- but description record commented
out for now.

[MarkM, 2013-07-12 02:34:35]

Array.prototype.length no longer needs to be skipped as the relevant
bug is long dead.
Test for Opera specific bug
Test for Function constructor bug -- but description record commented
out for now.

[MarkM, 2013-07-12 02:34:57]

https://codereview.appspot.com/10075043/diff/86001/src/com/google/caja/ses/St...
File src/com/google/caja/ses/StringMap.js (right):

https://codereview.appspot.com/10075043/diff/86001/src/com/google/caja/ses/St...
src/com/google/caja/ses/StringMap.js:20: * @overrides ses
On 2013/07/09 16:34:59, kpreid2 wrote:
> Instead of @overrides (I assume this is needed because of the var
declaration?),
> how about replacing the || {} chain with
> 
> if (typeof ses !== 'undefined' &&
> ses.es5ProblemReports.FREEZING_BREAKS_PROTOTYPES.beforeFailure)
> 
> This will fail if the intermediate objects are missing, but that is an
> appropriate inconsistency-failure to catch.

Done.

https://codereview.appspot.com/10075043/diff/86001/src/com/google/caja/ses/St...
src/com/google/caja/ses/StringMap.js:28: "use strict";
On 2013/07/09 16:34:59, kpreid2 wrote:
> While we're fixing minor things: single quotes.

Done.

https://codereview.appspot.com/10075043/diff/86001/src/com/google/caja/ses/co...
File src/com/google/caja/ses/compileExprLater.js (right):

https://codereview.appspot.com/10075043/diff/86001/src/com/google/caja/ses/co...
src/com/google/caja/ses/compileExprLater.js:111: if (options.sourceUrl) {
On 2013/07/09 16:34:59, kpreid2 wrote:
> This condition is incorrect because the empty string is falsy and is a valid
> (relative) URL. Even if relative URLs are useless here we shouldn't treat them
> as absent.

Done. Good catch.

https://codereview.appspot.com/10075043/diff/86001/src/com/google/caja/ses/st...
File src/com/google/caja/ses/startSES.js (right):

https://codereview.appspot.com/10075043/diff/86001/src/com/google/caja/ses/st...
src/com/google/caja/ses/startSES.js:1438: if (result === '*') {
On 2013/07/09 16:34:59, kpreid2 wrote:
> Note that my current typed-array-support draft requires 'skip'. I plan to look
> into whether there's something we can do which is stricter than 'skip', but I
> haven't done so yet.
> 
> You can still remove 'skip' now; I'm just noting that I might end up making
> semi-reverting changes here.
> 
> (The problem — I forget whether I explained this already — is that the typed
> array prototypes have accessor properties, and the whitelist mechanism
> essentially assumes that every built-in property is a data property (var sub =
> value[name]; in the register function), which is true for ES5, but not for
typed
> arrays as implemented now and as in ES6-draft.)

Acknowledged. I am proceeding to remove it now. Add back in what you find useful
when you need it.

[kpreid2, 2013-07-12 20:50:13]

LGTM

https://codereview.appspot.com/10075043/diff/97001/src/com/google/caja/ses/st...
File src/com/google/caja/ses/startSES.js (right):

https://codereview.appspot.com/10075043/diff/97001/src/com/google/caja/ses/st...
src/com/google/caja/ses/startSES.js:292: // transient deprecated adaptor only,
since there used to be an
make this a TODO, and give time context to help decide when we can remove it

[kpreid2, 2013-07-12 20:50:51]

On 2013/07/12 20:50:13, kpreid2 wrote:
> LGTM

Correction: Please update the change description to reflect its new contents and
to be less elliptical before submitting.

[MarkM, 2013-07-12 21:53:49]

Several little things
* changed many double quoted strings to single quotes
* StringMap can now be used without ses
* optional source position for evalators have been moved into the options
object
* removed "skip" from whitelist and whitelist mechanism since we don't currently
need it

[MarkM, 2013-07-12 21:54:15]

https://codereview.appspot.com/10075043/diff/97001/src/com/google/caja/ses/st...
File src/com/google/caja/ses/startSES.js (right):

https://codereview.appspot.com/10075043/diff/97001/src/com/google/caja/ses/st...
src/com/google/caja/ses/startSES.js:292: // transient deprecated adaptor only,
since there used to be an
On 2013/07/12 20:50:14, kpreid2 wrote:
> make this a TODO, and give time context to help decide when we can remove it

Done.

[MarkM, 2013-07-13 04:05:11]

Several little things
* changed many double quoted strings to single quotes
* StringMap can now be used without ses
* optional source position for evalators have been moved into the options
object
* removed "skip" from whitelist and whitelist mechanism since we don't currently
need it

[MarkM, 2013-07-13 04:18:46]

Also added some minor fixes to WeakMap.js and atLeastFreeVariableNames.js to
address complaints from the linker. Please have a look. I won't submit until we
understand the StringMap problem. Any suspicions about what it might be?

https://codereview.appspot.com/10075043/diff/86001/src/com/google/caja/ses/St...
File src/com/google/caja/ses/StringMap.js (right):

https://codereview.appspot.com/10075043/diff/86001/src/com/google/caja/ses/St...
src/com/google/caja/ses/StringMap.js:20: * @overrides ses
On 2013/07/09 16:34:59, kpreid2 wrote:
> Instead of @overrides (I assume this is needed because of the var
declaration?),
> how about replacing the || {} chain with
> 
> if (typeof ses !== 'undefined' &&
> ses.es5ProblemReports.FREEZING_BREAKS_PROTOTYPES.beforeFailure)
> 
> This will fail if the intermediate objects are missing, but that is an
> appropriate inconsistency-failure to catch.

Doing as suggested (patch 14) broke runtests, apparently detecting the kind of
inconsistency you raised. With patch 15, which chains tests analogously to the
original, runtests completes successfully. Not sure why yet.

[kpreid2, 2013-07-13 04:24:01]

https://codereview.appspot.com/10075043/diff/86001/src/com/google/caja/ses/St...
File src/com/google/caja/ses/StringMap.js (right):

https://codereview.appspot.com/10075043/diff/86001/src/com/google/caja/ses/St...
src/com/google/caja/ses/StringMap.js:20: * @overrides ses
On 2013/07/13 04:18:47, MarkM wrote:
> Doing as suggested (patch 14) broke runtests, apparently detecting the kind of
> inconsistency you raised. With patch 15, which chains tests analogously to the
> original, runtests completes successfully. Not sure why yet.

That's a bit vague. Which of the objects in the chain is missing? (The error
should have a relevant property name.) Or more generally, which tests fail? What
does the Chrome console show for those which do?

[MarkM, 2013-07-13 15:27:28]

Several little things
* changed many double quoted strings to single quotes
* StringMap can now be used without ses
* optional source position for evalators have been moved into the options
object
* removed "skip" from whitelist and whitelist mechanism since we don't currently
need it

[MarkM, 2013-07-13 15:34:11]

On 2013/07/13 04:24:01, kpreid2 wrote:
>
https://codereview.appspot.com/10075043/diff/86001/src/com/google/caja/ses/St...
> File src/com/google/caja/ses/StringMap.js (right):
> 
>
https://codereview.appspot.com/10075043/diff/86001/src/com/google/caja/ses/St...
> src/com/google/caja/ses/StringMap.js:20: * @overrides ses
> On 2013/07/13 04:18:47, MarkM wrote:
> > Doing as suggested (patch 14) broke runtests, apparently detecting the kind
of
> > inconsistency you raised. With patch 15, which chains tests analogously to
the
> > original, runtests completes successfully. Not sure why yet.
> 
> That's a bit vague. Which of the objects in the chain is missing? (The error
> should have a relevant property name.) Or more generally, which tests fail?
What
> does the Chrome console show for those which do?

The problem is that some tests test that things fail clearly when repairES5.js
fails in various ways (test-ses.html). When the try/catch at the bottom of
repairES5 takes the catch branch, it will not initialize es5ProblemReports to
not get initialized, causing the revised StringMap to fail uncleanly. Patch 16
fixes StringMap to only check the problemReports if ses.ok(). Additionally, if
no ses, or if !ses.ok(), or if the specific problem is indicated, in all these
cases it assumes Object.create might be broken and takes the more conservative
route. This is a minor improvement over the pre-CL state, though I expect it
will never matter.