Issue 6818060: Fixes error in SES caused by trailing line comment (Closed)
|
Created:
13 years, 5 months ago by Jasvir Modified:
13 years, 3 months ago CC:
google-caja-discuss_googlegroups.com Base URL:
http://google-caja.googlecode.com/svn/trunk/ Visibility:
Public. |
DescriptionFixes http://code.google.com/p/google-caja/issues/detail?id=1578&sort=-id
Modifies an existing end-to-end test to test for this case.
@5195
Patch Set 1 #
Total comments: 7
Patch Set 2 : Fixes error in SES caused by trailing line comment #
Total comments: 6
Patch Set 3 : Fixes error in SES caused by trailing line comment #
Total comments: 9
Patch Set 4 : Fixes error in SES caused by trailing line comment #
MessagesTotal messages: 12
https://codereview.appspot.com/6818060/diff/1/src/com/google/caja/ses/startSE... File src/com/google/caja/ses/startSES.js (right): https://codereview.appspot.com/6818060/diff/1/src/com/google/caja/ses/startSE... src/com/google/caja/ses/startSES.js:703: var exprSrc = '(function() {' + modSrc + '\n}).call(this)'; Are these regressions? I thought I'd done this in the first place.
Sign in to reply to this message.
https://codereview.appspot.com/6818060/diff/1/src/com/google/caja/plugin/ses-... File src/com/google/caja/plugin/ses-frame-group.js (right): https://codereview.appspot.com/6818060/diff/1/src/com/google/caja/plugin/ses-... src/com/google/caja/plugin/ses-frame-group.js:390: '(function () {' + theContent + '\n})()')); Add a comment about why the \n is necessary. https://codereview.appspot.com/6818060/diff/1/src/com/google/caja/ses/startSE... File src/com/google/caja/ses/startSES.js (right): https://codereview.appspot.com/6818060/diff/1/src/com/google/caja/ses/startSE... src/com/google/caja/ses/startSES.js:703: var exprSrc = '(function() {' + modSrc + '\n}).call(this)'; Add a comment about why the \n is necessary. https://codereview.appspot.com/6818060/diff/1/tests/com/google/caja/plugin/es... File tests/com/google/caja/plugin/es53-test-inline-script.html (right): https://codereview.appspot.com/6818060/diff/1/tests/com/google/caja/plugin/es... tests/com/google/caja/plugin/es53-test-inline-script.html:18: ><script>document.write("<b><script>");//</script Please add a separate test case with an appropriate name. This is too likely to get removed. Since this is a guest-side test, adding another test is not expensive.
Sign in to reply to this message.
snapshot https://codereview.appspot.com/6818060/diff/1/src/com/google/caja/plugin/ses-... File src/com/google/caja/plugin/ses-frame-group.js (right): https://codereview.appspot.com/6818060/diff/1/src/com/google/caja/plugin/ses-... src/com/google/caja/plugin/ses-frame-group.js:390: '(function () {' + theContent + '\n})()')); On 2012/10/30 21:55:50, kpreid2 wrote: > Add a comment about why the \n is necessary. Done. https://codereview.appspot.com/6818060/diff/1/src/com/google/caja/ses/startSE... File src/com/google/caja/ses/startSES.js (right): https://codereview.appspot.com/6818060/diff/1/src/com/google/caja/ses/startSE... src/com/google/caja/ses/startSES.js:703: var exprSrc = '(function() {' + modSrc + '\n}).call(this)'; Not a regression - this was not done everywhere. https://codereview.appspot.com/6818060/diff/1/tests/com/google/caja/plugin/es... File tests/com/google/caja/plugin/es53-test-inline-script.html (right): https://codereview.appspot.com/6818060/diff/1/tests/com/google/caja/plugin/es... tests/com/google/caja/plugin/es53-test-inline-script.html:18: ><script>document.write("<b><script>");//</script On 2012/10/30 21:55:50, kpreid2 wrote: > Please add a separate test case with an appropriate name. This is too likely to > get removed. > > Since this is a guest-side test, adding another test is not expensive. Done.
Sign in to reply to this message.
https://codereview.appspot.com/6818060/diff/7001/src/com/google/caja/plugin/s... File src/com/google/caja/plugin/ses-frame-group.js (right): https://codereview.appspot.com/6818060/diff/7001/src/com/google/caja/plugin/s... src/com/google/caja/plugin/ses-frame-group.js:390: '(function () {' + theContent + '\n})()')); Still no comment. https://codereview.appspot.com/6818060/diff/7001/tests/com/google/caja/plugin... File tests/com/google/caja/plugin/es53-test-inline-script.html (right): https://codereview.appspot.com/6818060/diff/7001/tests/com/google/caja/plugin... tests/com/google/caja/plugin/es53-test-inline-script.html:26: ></div> The <> linebreak gimmicks are not necessary for this test; they are in testDocumentWriteRecursive because it is matching innerText in order to exercise document.write. Please format normally and use a variable instead of document.write to communicate. This testcontainer should be immediately above the corresponding jsunitRegister. https://codereview.appspot.com/6818060/diff/7001/tests/com/google/caja/plugin... tests/com/google/caja/plugin/es53-test-inline-script.html:46: jsunitRegisterIf(inES5Mode, Does not need to be an ES5-mode-only test. Should be placed in a separate <script> block immediately following its testcontainer, not grouped with other scripts.
Sign in to reply to this message.
Jasvir, ping
Sign in to reply to this message.
Fixes http://code.google.com/p/google-caja/issues/detail?id=1578&sort=-id Modifies an existing end-to-end test to test for this case.
Sign in to reply to this message.
https://codereview.appspot.com/6818060/diff/7001/src/com/google/caja/plugin/s... File src/com/google/caja/plugin/ses-frame-group.js (right): https://codereview.appspot.com/6818060/diff/7001/src/com/google/caja/plugin/s... src/com/google/caja/plugin/ses-frame-group.js:390: '(function () {' + theContent + '\n})()')); On 2012/10/31 16:35:46, kpreid2 wrote: > Still no comment. Done. https://codereview.appspot.com/6818060/diff/7001/tests/com/google/caja/plugin... File tests/com/google/caja/plugin/es53-test-inline-script.html (right): https://codereview.appspot.com/6818060/diff/7001/tests/com/google/caja/plugin... tests/com/google/caja/plugin/es53-test-inline-script.html:26: ></div> On 2012/10/31 16:35:46, kpreid2 wrote: > The <> linebreak gimmicks are not necessary for this test; they are in > testDocumentWriteRecursive because it is matching innerText in order to exercise > document.write. Please format normally and use a variable instead of > document.write to communicate. > > This testcontainer should be immediately above the corresponding jsunitRegister. Done. https://codereview.appspot.com/6818060/diff/7001/tests/com/google/caja/plugin... tests/com/google/caja/plugin/es53-test-inline-script.html:46: jsunitRegisterIf(inES5Mode, On 2012/10/31 16:35:46, kpreid2 wrote: > Does not need to be an ES5-mode-only test. > > Should be placed in a separate <script> block immediately following its > testcontainer, not grouped with other scripts. Done.
Sign in to reply to this message.
LGTM modulo formatting https://codereview.appspot.com/6818060/diff/12001/tests/com/google/caja/plugi... File tests/com/google/caja/plugin/es53-test-inline-script.html (right): https://codereview.appspot.com/6818060/diff/12001/tests/com/google/caja/plugi... tests/com/google/caja/plugin/es53-test-inline-script.html:21: stray added blank line https://codereview.appspot.com/6818060/diff/12001/tests/com/google/caja/plugi... tests/com/google/caja/plugin/es53-test-inline-script.html:44: window.result = "trailing line comment";// delibrate comment</script> single quotes indent 4 spaces https://codereview.appspot.com/6818060/diff/12001/tests/com/google/caja/plugi... tests/com/google/caja/plugin/es53-test-inline-script.html:48: function testTrailingLineComment () { Stray space "t (". Fix testDocumentWriteRecursive too if you want. https://codereview.appspot.com/6818060/diff/12001/tests/com/google/caja/plugi... tests/com/google/caja/plugin/es53-test-inline-script.html:49: assertEquals("Full text", "trailing line comment", window.result); single quotes
Sign in to reply to this message.
Fixes http://code.google.com/p/google-caja/issues/detail?id=1578&sort=-id Modifies an existing end-to-end test to test for this case.
Sign in to reply to this message.
https://codereview.appspot.com/6818060/diff/12001/tests/com/google/caja/plugi... File tests/com/google/caja/plugin/es53-test-inline-script.html (right): https://codereview.appspot.com/6818060/diff/12001/tests/com/google/caja/plugi... tests/com/google/caja/plugin/es53-test-inline-script.html:21: On 2012/12/13 17:36:29, kpreid2 wrote: > stray added blank line Done. https://codereview.appspot.com/6818060/diff/12001/tests/com/google/caja/plugi... tests/com/google/caja/plugin/es53-test-inline-script.html:44: window.result = "trailing line comment";// delibrate comment</script> On 2012/12/13 17:36:29, kpreid2 wrote: > single quotes > > indent 4 spaces Done. https://codereview.appspot.com/6818060/diff/12001/tests/com/google/caja/plugi... tests/com/google/caja/plugin/es53-test-inline-script.html:48: function testTrailingLineComment () { On 2012/12/13 17:36:29, kpreid2 wrote: > Stray space "t (". Fix testDocumentWriteRecursive too if you want. Done. https://codereview.appspot.com/6818060/diff/12001/tests/com/google/caja/plugi... tests/com/google/caja/plugin/es53-test-inline-script.html:49: assertEquals("Full text", "trailing line comment", window.result); On 2012/12/13 17:36:29, kpreid2 wrote: > single quotes Done.
Sign in to reply to this message.
Message was sent while issue was closed.
https://codereview.appspot.com/6818060/diff/12001/tests/com/google/caja/plugi... File tests/com/google/caja/plugin/es53-test-inline-script.html (right): https://codereview.appspot.com/6818060/diff/12001/tests/com/google/caja/plugi... tests/com/google/caja/plugin/es53-test-inline-script.html:44: window.result = "trailing line comment";// delibrate comment</script> "Deliberate"
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
