https://codereview.appspot.com/198470043/diff/20001/src/com/google/caja/ses/repairES5.js
File src/com/google/caja/ses/repairES5.js (right):
https://codereview.appspot.com/198470043/diff/20001/src/com/google/caja/ses/repairES5.js#newcode488
src/com/google/caja/ses/repairES5.js:488: * Using Allen's trick from
This is an explaining-the-implementation comment more than a doc
comment.
I think having an actual doc comment specifying the semantics of the
parameters would be worthwhile (noting things like that constr may be
undefined).
https://codereview.appspot.com/198470043/diff/20001/src/com/google/caja/ses/repairES5.js#newcode491
src/com/google/caja/ses/repairES5.js:491: * makeBrandTester runs at SES
initialization time, but the
Clarify: s/runs/is only called/.
Also, this comment suggests that makeBrandTester is _not_ safe when
called after SES initialization is complete. That would be surprising
because it implies mutability. If it is not the case, why are you
calling it out specially here?
I'm interested because the taming membrane, for example, might like to
reuse this utility for different builtins (e.g. typed arrays).
https://codereview.appspot.com/198470043/diff/20001/src/com/google/caja/ses/repairES5.js#newcode495
src/com/google/caja/ses/repairES5.js:495: function
makeBrandTester(constr, methodName, args, opt_example) {
For consistency with the rest of the codebase and (I think) common usage
elsewhere, I suggest using “ctor” instead of “constr”.
https://codereview.appspot.com/198470043/diff/20001/src/com/google/caja/ses/repairES5.js#newcode499
src/com/google/caja/ses/repairES5.js:499: return function(specimen) {
return false; }
Suggest giving this function a descriptive name for the sake of
debugging, like:
function nonexistentTypeBrandTester(specimen) { ...
https://codereview.appspot.com/198470043/diff/20001/src/com/google/caja/ses/repairES5.js#newcode512
src/com/google/caja/ses/repairES5.js:512: strictForEachFn([null,
undefined, true, 1, 'x', {}], function(v) {
for style consistency, void 0 instead of undefined?
I'd like to see some hairier pretend-to-be-the-object test cases, like
1. a Proxy whose target would pass (but you may not want to depend on
Proxy yet).
2. {valueOf() { return anActualInstance; }}
https://codereview.appspot.com/198470043/diff/20001/src/com/google/caja/ses/repairES5.js#newcode565
src/com/google/caja/ses/repairES5.js:565: * "WeakMap", or, in ES6
terminology, whether it has a [[WeakMapData]]
This description is arguably nonsense, because (assuming I have my facts
straight) ES5 doesn't have WeakMap and ES6 doesn't have [[Class]], so
[[Class]] never took on the value WeakMap in any specification.
https://codereview.appspot.com/198470043/diff/20001/src/com/google/caja/ses/repairES5.js#newcode570
src/com/google/caja/ses/repairES5.js:570: global.WeakMap ? new WeakMap()
: void 0);
Horizontal alignment like this is undesirable, because it breaks under
find-and-replace renaming.
Put "makeBrandTester(" on the previous line instead.
https://codereview.appspot.com/198470043/diff/20001/src/com/google/caja/ses/repairES5.js#newcode1500
src/com/google/caja/ses/repairES5.js:1500: /**
Either don't pretend this is a doc comment, or put one on each of the
four functions.
https://codereview.appspot.com/198470043/diff/20001/src/com/google/caja/ses/repairES5.js#newcode3194
src/com/google/caja/ses/repairES5.js:3194: * <p>This only works when
{@code constr} is the constructor of
While we're touching this, I'd like to s/constr/ctor/ as discussed
above.
https://codereview.appspot.com/198470043/diff/20001/src/com/google/caja/ses/repairES5.js#newcode4178
src/com/google/caja/ses/repairES5.js:4178: id: 'NUMBER_PROTO_IS_NUMBER',
What specific browsers have these bugs, or is this purely a conformance
check for the sake of accurate brands?
https://codereview.appspot.com/198470043/
--
---
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/d/optout.
