admit MAGICAL_UNICORN for firefox 22 (beta)

some automode tests use an impossibly strict severity level
named MAGICAL_UNICORN to force a switch to es53.

repairES5 validates severity levels and rejects MAGICAL_UNICORN,
replacing it with SAFE_SPEC_VIOLATION.

firefox 22 meets the standard of SAFE_SPEC_VIOLATION, so the
automode tests fail to switch to es53, causing the tests to fail.

this fix admits MAGICAL_UNICORN exists

[MarkM, 2013-06-17 17:42:30]

Please include me as a secondary reviewer on all CLs with any ses/* files.

LGTM.

https://codereview.appspot.com/10341043/diff/1/src/com/google/caja/ses/repair...
File src/com/google/caja/ses/repairES5.js (right):

https://codereview.appspot.com/10341043/diff/1/src/com/google/caja/ses/repair...
src/com/google/caja/ses/repairES5.js:4039: if (postSeverity !== severities.SAFE
&& disposition(kludge).permit) {
This "!==" should be ">"

[kpreid2, 2013-06-17 17:50:42]

LGTM provided the below are fixed

https://codereview.appspot.com/10341043/diff/1/src/com/google/caja/ses/repair...
File src/com/google/caja/ses/repairES5.js (right):

https://codereview.appspot.com/10341043/diff/1/src/com/google/caja/ses/repair...
src/com/google/caja/ses/repairES5.js:222: sev.level >=
ses.severities.MAGICAL_UNICORN.level &&
I propose introducing the following constants next to the definition of
ses.severities.
var LOWEST_SEVERITY = ses.severities.MAGICAL_UNICORN;
var HIGHEST_SEVERITY = ses.severities.NOT_SUPPORTED;
These mean that future revisions will be more likely to preserve the correctness
of this code without needing an equivalent of this CL.

https://codereview.appspot.com/10341043/diff/1/src/com/google/caja/ses/repair...
src/com/google/caja/ses/repairES5.js:4039: if (postSeverity !== severities.SAFE
&& disposition(kludge).permit) {
On 2013/06/17 17:42:30, MarkM wrote:
> This "!==" should be ">"

Only with .level, that is, postSeverity.level > severities.SAFE.level.

(The test works as is because it is really comparing against the initialization
of "postSeverity = severities.SAFE;", not an arbitrary severity — but I agree
this is appropriate in principle.)

[felix8a, 2013-06-17 19:52:52]

some automode tests use an impossibly strict severity level
named MAGICAL_UNICORN to force a switch to es53.

repairES5 validates severity levels and rejects MAGICAL_UNICORN,
replacing it with SAFE_SPEC_VIOLATION.

firefox 22 meets the standard of SAFE_SPEC_VIOLATION, so the
automode tests fail to switch to es53, causing the tests to fail.

this fix admits MAGICAL_UNICORN exists

[felix8a, 2013-06-17 19:53:07]

https://codereview.appspot.com/10341043/diff/1/src/com/google/caja/ses/repair...
File src/com/google/caja/ses/repairES5.js (right):

https://codereview.appspot.com/10341043/diff/1/src/com/google/caja/ses/repair...
src/com/google/caja/ses/repairES5.js:222: sev.level >=
ses.severities.MAGICAL_UNICORN.level &&
On 2013/06/17 17:50:42, kpreid2 wrote:
> I propose introducing the following constants next to the definition of
> ses.severities.
> var LOWEST_SEVERITY = ses.severities.MAGICAL_UNICORN;
> var HIGHEST_SEVERITY = ses.severities.NOT_SUPPORTED;
> These mean that future revisions will be more likely to preserve the
correctness
> of this code without needing an equivalent of this CL.

I think that's a bad idea because it obscures why the comparison is >= LOWEST
but < HIGHEST. It's deceptively similar to a half-open interval, which it isn't.

https://codereview.appspot.com/10341043/diff/1/src/com/google/caja/ses/repair...
src/com/google/caja/ses/repairES5.js:4039: if (postSeverity !== severities.SAFE
&& disposition(kludge).permit) {
On 2013/06/17 17:50:42, kpreid2 wrote:
> On 2013/06/17 17:42:30, MarkM wrote:
> > This "!==" should be ">"
> 
> Only with .level, that is, postSeverity.level > severities.SAFE.level.
> 
> (The test works as is because it is really comparing against the
initialization
> of "postSeverity = severities.SAFE;", not an arbitrary severity — but I agree
> this is appropriate in principle.)

Done.

[kpreid2, 2013-06-17 20:10:48]

LGTM

https://codereview.appspot.com/10341043/diff/1/src/com/google/caja/ses/repair...
File src/com/google/caja/ses/repairES5.js (right):

https://codereview.appspot.com/10341043/diff/1/src/com/google/caja/ses/repair...
src/com/google/caja/ses/repairES5.js:222: sev.level >=
ses.severities.MAGICAL_UNICORN.level &&
On 2013/06/17 19:53:07, felix8a wrote:
> On 2013/06/17 17:50:42, kpreid2 wrote:
> > I propose introducing the following constants next to the definition of
> > ses.severities.
> > var LOWEST_SEVERITY = ses.severities.MAGICAL_UNICORN;
> > var HIGHEST_SEVERITY = ses.severities.NOT_SUPPORTED;
> > These mean that future revisions will be more likely to preserve the
> correctness
> > of this code without needing an equivalent of this CL.
> 
> I think that's a bad idea because it obscures why the comparison is >= LOWEST
> but < HIGHEST. It's deceptively similar to a half-open interval, which it
isn't.

Ah, I misread the code. I still think that we should use a LOWEST_SEVERITY for
the >=, but I agree that "< NOT_SUPPORTED" should be left as is.

[felix8a, 2013-06-17 20:23:05]

@r5451