https://codereview.appspot.com/54750043/diff/130003/src/com/google/caja/ses/WeakMap.js
File src/com/google/caja/ses/WeakMap.js (right):
https://codereview.appspot.com/54750043/diff/130003/src/com/google/caja/ses/WeakMap.js#newcode445
src/com/google/caja/ses/WeakMap.js:445: keys.push(key);
On 2014/01/24 23:00:30, kpreid2 wrote:
If I were feeling especially paranoid (considering such things as
slow-script-stopping), I might check here the invariant that keys and
values are
the same length, since a violation would result in mis-associating all
future
keys and values, a particularly horrible outcome.
Thanks for raising it. The possibility had not occurred to me. Rather
than checking, I'm rewriting to make the algorithm more crash safe.
Please have a look.
https://codereview.appspot.com/54750043/diff/130003/src/com/google/caja/ses/WeakMap.js#newcode461
src/com/google/caja/ses/WeakMap.js:461: values.splice(index, 1);
On 2014/01/24 23:00:30, kpreid2 wrote:
Splicing is probably O(n); instead, move the last entry into this
index. Since
WeakMaps are non-enumerable the ordering makes no observable
difference.
(Should you choose to adopt the abovementioned paranoia, erase either
the key or
value of the destination, copy the other, and copy the blanked value.)
Done.
https://codereview.appspot.com/54750043/diff/130003/tests/com/google/caja/ses/test-ses-parts.js
File tests/com/google/caja/ses/test-ses-parts.js (right):
https://codereview.appspot.com/54750043/diff/130003/tests/com/google/caja/ses/test-ses-parts.js#newcode15
tests/com/google/caja/ses/test-ses-parts.js:15: var preFrozen =
Object.freeze({});
On 2014/01/24 23:00:30, kpreid2 wrote:
+1 for thoroughness
Thanks.
https://codereview.appspot.com/54750043/diff/130003/tests/com/google/caja/ses/test-ses-parts.js#newcode38
tests/com/google/caja/ses/test-ses-parts.js:38: var wm1 = new WeakMap();
On 2014/01/24 23:00:30, kpreid2 wrote:
Either give these more descriptive names or add comments explaining
which roles
they play in the test, so that one doesn't have to figure out the
logic of the
tests by reading all the asserts.
Done.
https://codereview.appspot.com/54750043/diff/130003/tests/com/google/caja/ses/test-ses-parts.js#newcode43
tests/com/google/caja/ses/test-ses-parts.js:43:
assertEquals(wm1.set(postFrozen, 10), wm1);
On 2014/01/24 23:00:30, kpreid2 wrote:
Expected values go first, actual values go second. Jsunit error
messages are
written to assume that.
I didn't know that. Done.
https://codereview.appspot.com/54750043/
--
---
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.
