I'm still reviewing repair-framework and test-repair-framework. The rest
LGTM
https://codereview.appspot.com/54450044/diff/1/src/com/google/caja/ses/WeakMap.js
File src/com/google/caja/ses/WeakMap.js (right):
https://codereview.appspot.com/54450044/diff/1/src/com/google/caja/ses/WeakMap.js#newcode194
src/com/google/caja/ses/WeakMap.js:194: // } catch (e) {}
On 2014/01/29 20:30:21, kpreid2 wrote:
On 2014/01/29 19:32:49, MarkM wrote:
> Now that this code is within a test function, the commented out code
should
also
> always return something appropriate.
Done.
no it isn't. What happens if the try block doesn't throw but the test
within the try block is false?
https://codereview.appspot.com/54450044/diff/1/src/com/google/caja/ses/WeakMap.js#newcode199
src/com/google/caja/ses/WeakMap.js:199: preSeverity:
severities.SAFE_SPEC_VIOLATION,
On 2014/01/29 20:30:21, kpreid2 wrote:
On 2014/01/29 19:32:49, MarkM wrote:
> This preSeverity does not seem severe enough. Why would this be
safe?
SAFE_SPEC_VIOLATION is described as "safe (in an integrity sense) even
if
unrepaired. May still lead to inappropriate failures". Throwing is an
inappropriate failure. No?
Good point. Done.
https://codereview.appspot.com/54450044/diff/1/src/com/google/caja/ses/repair-framework.js
File src/com/google/caja/ses/repair-framework.js (right):
https://codereview.appspot.com/54450044/diff/1/src/com/google/caja/ses/repair-framework.js#newcode39
src/com/google/caja/ses/repair-framework.js:39: *
<dt>MAGICAL_UNICORN</dt><dd>Unachievable magical mode used for testing.
Missing </dd>
https://codereview.appspot.com/54450044/diff/1/src/com/google/caja/ses/repair-framework.js#newcode40
src/com/google/caja/ses/repair-framework.js:40: * <dt>SAFE</dt><dd>no
problem.
Missing </dd>
https://codereview.appspot.com/54450044/diff/1/src/com/google/caja/ses/repair-framework.js#newcode183
src/com/google/caja/ses/repair-framework.js:183: callback(objAsMap[key],
key, self);
Shouldn't the "key" argument instead be the substring of key without the
final '$'?
https://codereview.appspot.com/54450044/diff/1/src/com/google/caja/ses/repairES5.js
File src/com/google/caja/ses/repairES5.js (right):
https://codereview.appspot.com/54450044/diff/1/src/com/google/caja/ses/repairES5.js#newcode140
src/com/google/caja/ses/repairES5.js:140: // TODO(kpreid): Replace uses
of this with new repair framework
On 2014/01/29 20:30:21, kpreid2 wrote:
On 2014/01/29 19:32:49, MarkM wrote:
> Unless I'm missing something, doing this now isn't much harder than
adding
this
> TODO, so better to just do it.
updateMaxSeverity is used in such things as startSES's
property-traversal. I
don't want to add reworking that to this change.
ok
https://codereview.appspot.com/54450044/diff/20001/src/com/google/caja/ses/WeakMap.js
File src/com/google/caja/ses/WeakMap.js (right):
https://codereview.appspot.com/54450044/diff/20001/src/com/google/caja/ses/WeakMap.js#newcode243
src/com/google/caja/ses/WeakMap.js:243: tests: []
Isn't think another empty test section that should eventually have
tests, and therefore should have todos?
https://codereview.appspot.com/54450044/
--
---
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.
