I still have yet to review repair-framework and test-repair-framework.
But these are my comments so far.
https://codereview.appspot.com/54450044/diff/1/src/com/google/caja/ses/WeakMap.js
File src/com/google/caja/ses/WeakMap.js (left):
https://codereview.appspot.com/54450044/diff/1/src/com/google/caja/ses/WeakMap.js#oldcode170
src/com/google/caja/ses/WeakMap.js:170: var hop =
Object.prototype.hasOwnProperty;
For understandable reasons, codereview diffing hit a jackpot, reporting
large blocks are replaced with new large blocks. In these large blocks,
I'm generally assuming that the parts that seem internally unmodified
are. If that means I'm missing something, please bring it to my
attention.
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#newcode166
src/com/google/caja/ses/WeakMap.js:166: sections: [], // TODO(kpreid):
cite ES6 when published
If we were using urls rather than section numbers,
https://people.mozilla.org/~jorendorff/es6-draft.html#sec-weakmap-objects
is a fine URL to use for now.
Just FYI, nothing need to be done here for now.
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) {}
Now that this code is within a test function, the commented out code
should also always return something appropriate.
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,
This preSeverity does not seem severe enough. Why would this be safe?
https://codereview.appspot.com/54450044/diff/1/src/com/google/caja/ses/WeakMap.js#newcode201
src/com/google/caja/ses/WeakMap.js:201: urls: [],
I think we've (you've?) already filed FF issues on this, so you should
cite those URLs here.
https://codereview.appspot.com/54450044/diff/1/src/com/google/caja/ses/WeakMap.js#newcode203
src/com/google/caja/ses/WeakMap.js:203: tests: []
Please add a todo on empty test section for things that test262 should
test.
https://codereview.appspot.com/54450044/diff/1/src/com/google/caja/ses/WeakMap.js#newcode249
src/com/google/caja/ses/WeakMap.js:249: Proxy = undefined;
You also do this for the same reason at line 160 above. Can we safely
remove one or kill the redundancy somehow?
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
Unless I'm missing something, doing this now isn't much harder than
adding this TODO, so better to just do it.
https://codereview.appspot.com/54450044/diff/1/src/com/google/caja/ses/repairES5.js#newcode3436
src/com/google/caja/ses/repairES5.js:3436: ////////////////////// Kludge
Records /////////////////////
Stale terminology. Please search for other possible occurrences.
https://codereview.appspot.com/54450044/diff/1/src/com/google/caja/ses/repairES5.js#newcode3445
src/com/google/caja/ses/repairES5.js:3445: var baseKludges = [
Stale terminology
https://codereview.appspot.com/54450044/diff/1/src/com/google/caja/ses/repairES5.js#newcode3473
src/com/google/caja/ses/repairES5.js:3473: var supportedKludges = [
Stale terminology
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.
