https://codereview.appspot.com/222570043/diff/80001/src/com/google/caja/ses/repairES5.js#newcode3693 src/com/google/caja/ses/repairES5.js:3693: ses.optForeignForIn =
inTestFrame(function(window) {
Without having any specific ideas in mind: note that inTestFrame wasn't
written to be defensive — I wrote it solely for doing cross-frame repair
tests, i.e. without more widely exposing its results. Might be good to
review inTestFrame with that in mind.
Also, please mark this as private in some way, such as a leading _
(precedent with ses._EarlyStringMap above). We need to work on defining
our public interface more precisely, so that future changes can be
evaluated for whether they can break clients, and this is a small first
step.
https://codereview.appspot.com/222570043/diff/80001/src/com/google/caja/ses/startSES.js#newcode266 src/com/google/caja/ses/startSES.js:266: // Note: Imperative update, but
should be ok.
The danger of an imperative update is not localized to the update.
Please document the fact that we're doing this at additional locations:
where whitelist is defined, and where it is next read — that is,
sufficient for a maintainer to tell that this code must not be
_reordered_.
On 2015/04/20 17:39:10, kpreid_google wrote:
> Without having any specific ideas in mind: note that inTestFrame
wasn't written
> to be defensive — I wrote it solely for doing cross-frame repair
tests, i.e.
> without more widely exposing its results. Might be good to review
inTestFrame
> with that in mind.
Enhanced comments on itTestFrame.
> Also, please mark this as private in some way, such as a leading _
(precedent
> with ses._EarlyStringMap above). We need to work on defining our
public
> interface more precisely, so that future changes can be evaluated
for whether
> they can break clients, and this is a small first step.
The first case is the most correct document structure.
The second case is less correct but preferable over the alternatives if
<body> is missing (e.g. if the document is still being parsed, or the
document is a frameset).
The third case is necessary if nothing but a documentElement (<html>)
exists.
The fourth case is necessary if a documentElement does not exist (a
highly unusual but valid state of the DOM).