https://codereview.appspot.com/258110043/diff/1/src/com/google/caja/ses/startSES.js
File src/com/google/caja/ses/startSES.js (right):
https://codereview.appspot.com/258110043/diff/1/src/com/google/caja/ses/startSES.js#newcode263
src/com/google/caja/ses/startSES.js:263: * the time of this writing,
with TRY_GLOBAL_SIMPLE_FREEZE_FIRST
On 2015/08/05 17:46:53, kpreid_google wrote:
Say "as of 2015-08-05" instead of "at the time of this writing". (The
information is of course available through history, but giving a
timestamp makes
staleness instantly understandable.)
Done.
https://codereview.appspot.com/258110043/diff/1/src/com/google/caja/ses/startSES.js#newcode1523
src/com/google/caja/ses/startSES.js:1523: * Although, in the official
spec language, only objects are
On 2015/08/05 17:46:53, kpreid_google wrote:
I suggest swapping these two sentences and putting the new second one
in
parentheses. This way, the comment begins with what it's actually
about.
Done.
https://codereview.appspot.com/258110043/diff/1/src/com/google/caja/ses/startSES.js#newcode1533
src/com/google/caja/ses/startSES.js:1533: * the EcmaScript notion of
"global object" is split into two
On 2015/08/05 17:46:53, kpreid_google wrote:
capitalization: "ECMAScript"
Do you mean to say it is split _in browsers_ into ...?
Done.
https://codereview.appspot.com/258110043/diff/1/src/com/google/caja/ses/startSES.js#newcode1535
src/com/google/caja/ses/startSES.js:1535: * from one url to another, a
fresh realm (set of primordials) is
On 2015/08/05 17:46:53, kpreid_google wrote:
"URL"
Done.
https://codereview.appspot.com/258110043/diff/1/src/com/google/caja/ses/startSES.js#newcode1595
src/com/google/caja/ses/startSES.js:1595: * <i>name</i> property in all
three cases. For the legacy or simple
On 2015/08/04 18:23:35, MarkM wrote:
delete "three"
Done.
https://codereview.appspot.com/258110043/diff/1/src/com/google/caja/ses/startSES.js#newcode1597
src/com/google/caja/ses/startSES.js:1597: *
<tt>Object.defineProperty</tt>.
On 2015/08/05 17:46:53, kpreid_google wrote:
use <code> not <tt> (here and below).
Done. But why? "<tt>" is much less noisy in the source.
(Makes we wonder whether we should switch to markdown, but of course not
in this CL.)
https://codereview.appspot.com/258110043/diff/1/src/com/google/caja/ses/startSES.js#newcode1620
src/com/google/caja/ses/startSES.js:1620: * case above, so we'll go
ahead and first try to delete wp.f
On 2015/08/04 18:23:34, MarkM wrote:
delete "so"
Done.
https://codereview.appspot.com/258110043/diff/1/src/com/google/caja/ses/startSES.js#newcode1658
src/com/google/caja/ses/startSES.js:1658: // Might be specced, simple,
legacy, or mized
On 2015/08/04 18:23:34, MarkM wrote:
"mized" --> "mixed"
Done.
https://codereview.appspot.com/258110043/diff/1/src/com/google/caja/ses/startSES.js#newcode1658
src/com/google/caja/ses/startSES.js:1658: // Might be specced, simple,
legacy, or mized
On 2015/08/05 17:46:53, kpreid_google wrote:
Can be one sentence per line. Rewrap so it is.
Done.
https://codereview.appspot.com/258110043/diff/1/src/com/google/caja/ses/startSES.js#newcode1662
src/com/google/caja/ses/startSES.js:1662: // In case it is simple or
legacy, then we try to freeze it
On 2015/08/04 18:23:34, MarkM wrote:
delete "then"
Done.
https://codereview.appspot.com/258110043/diff/1/src/com/google/caja/ses/startSES.js#newcode1667
src/com/google/caja/ses/startSES.js:1667: if ('document' in global) {
On 2015/08/05 17:46:53, kpreid_google wrote:
Extract this test into a function returning boolean, so it's less
likely to be
reinvented or copied, and the comments on how it's a bad test can go
in there
and not distract from what this is doing.
Done. We were testing whether we're in a browser in two places in
repairES5.js as well, which were using slightly different tests. I
consolidated all these to use the new ses.isInBrowser() test.
In adding ses.isInBrowser to the "// * provides" and "// * requires"
lists at the top of repairES5.js and startSES.js, I noticed various
anomalies there which I also cleaned up.
https://codereview.appspot.com/258110043/diff/1/src/com/google/caja/ses/startSES.js#newcode1717
src/com/google/caja/ses/startSES.js:1717: return;
On 2015/08/04 18:23:34, MarkM wrote:
Delete "return;". Not needed with this control flow.
Done.
https://codereview.appspot.com/258110043/diff/1/src/com/google/caja/ses/startSES.js#newcode1729
src/com/google/caja/ses/startSES.js:1729: * <p>Since we can't use
getOwnPropertyDescriptor to see if we
On 2015/08/04 18:23:34, MarkM wrote:
Delete "Since"
Done.
https://codereview.appspot.com/258110043/diff/1/src/com/google/caja/ses/startSES.js#newcode1729
src/com/google/caja/ses/startSES.js:1729: * <p>Since we can't use
getOwnPropertyDescriptor to see if we
On 2015/08/05 17:46:53, kpreid_google wrote:
Explain why we can't, at least as "See comments on freezeGlobalProp".
Done.
https://codereview.appspot.com/258110043/diff/1/src/com/google/caja/ses/startSES.js#newcode1736
src/com/google/caja/ses/startSES.js:1736: if (!(ses.is(oldValue,
desc.value))) {
On 2015/08/05 17:46:53, kpreid_google wrote:
This will give a false negative if the property's value is undefined.
This will throw an unhelpful error if the property does not exist
(because desc
is not an object).
Parentheses are unnecessary.
Done.
https://codereview.appspot.com/258110043/diff/1/src/com/google/caja/ses/startSES.js#newcode1761
src/com/google/caja/ses/startSES.js:1761: } catch (err) {}
On 2015/08/04 18:23:34, MarkM wrote:
Always insert a comment in an empty catch clause.
Done.
https://codereview.appspot.com/258110043/diff/1/src/com/google/caja/ses/startSES.js#newcode1764
src/com/google/caja/ses/startSES.js:1764: } catch (err) {}
On 2015/08/04 18:23:34, MarkM wrote:
Always insert a comment in an empty catch clause.
Done.
https://codereview.appspot.com/258110043/diff/1/src/com/google/caja/ses/startSES.js#newcode1806
src/com/google/caja/ses/startSES.js:1806: var newDesc = {
On 2015/08/05 17:46:53, kpreid_google wrote:
Inline this literal into the defProp statement below, so that it can
be read in
context of it being the descriptor for sharedImports.
Done.
https://codereview.appspot.com/258110043/
--
---
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.
