PTAL
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
On 2015/02/13 21:30:37, kpreid_google wrote:
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).
Done.
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
On 2015/02/13 21:30:37, kpreid_google wrote:
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).
Done.
I put the qualifier in because, at the time, I only wrote it so that the
brandTester was defensive. I hadn't worried about whether
makeBrandTester was defensive when it ran. However, reading it now, I
don't see any problem.
Nevertheless, even if the taming membrane uses it, it should probably do
all the needed calls to makeBrandTester at setup time anyway, before
untrusted code runs.
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) {
On 2015/02/13 21:30:37, kpreid_google wrote:
For consistency with the rest of the codebase and (I think) common
usage
elsewhere, I suggest using “ctor” instead of “constr”.
Done.
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; }
On 2015/02/13 21:30:37, kpreid_google wrote:
Suggest giving this function a descriptive name for the sake of
debugging, like:
function nonexistentTypeBrandTester(specimen) { ...
Done.
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) {
On 2015/02/13 21:30:37, kpreid_google wrote:
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; }}
Extremely good idea! The proxy case fails on FF 35.0.1 which we must
still support, so I added extra logic to survive when that self-test
fails, and added comments explaining why this is currently safe. All
other browsers I tested so far -- Chrome, FF Nightly, Safari, and Opera
pass fine. I won't be able to test IE until tomorrow, but I will test it
before submitting.
So, with these caveats, Done. PTAL.
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]]
On 2015/02/13 21:30:37, kpreid_google wrote:
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.
Correct. Done.
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);
On 2015/02/13 21:30:37, kpreid_google wrote:
Horizontal alignment like this is undesirable, because it breaks under
find-and-replace renaming.
Put "makeBrandTester(" on the previous line instead.
Done.
https://codereview.appspot.com/198470043/diff/20001/src/com/google/caja/ses/repairES5.js#newcode1500
src/com/google/caja/ses/repairES5.js:1500: /**
On 2015/02/13 21:30:37, kpreid_google wrote:
Either don't pretend this is a doc comment, or put one on each of the
four
functions.
Done.
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
On 2015/02/13 21:30:37, kpreid_google wrote:
While we're touching this, I'd like to s/constr/ctor/ as discussed
above.
Done.
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',
On 2015/02/13 21:30:37, kpreid_google wrote:
What specific browsers have these bugs, or is this purely a
conformance check
for the sake of accurate brands?
As of this writing, at least Chrome, FF, Safari, and Opera violate
these. I haven't yet tested IE.
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.
