The layout test fast/parser/comment-in-textarea.html can be written using dumpAsText so that the results can be independent of minor platform differences.
Created attachment 43352 [details] Rewrite the test fast/parser/comment-in-textarea to use dumpAsText
Comment on attachment 43352 [details] Rewrite the test fast/parser/comment-in-textarea to use dumpAsText Yeah! More 'dumpAsText' tests! Few nits: > <html> > -<body> > -<textarea> > +<head> <head> doesn't really add to the test, could be removed. > + <script> > + function log(msg) the <script> tag with content is usually not indented. > + } > + function test() Could be nice to have an empty line between these. r- because if you fix those, we'll just run it through commit-bot.
Created attachment 43410 [details] Rewrite the test fast/parser/comment-in-textarea to use dumpAsText (Rev.2)
Comment on attachment 43410 [details] Rewrite the test fast/parser/comment-in-textarea to use dumpAsText (Rev.2) > +<div id="console"></div> > +<script> > +function log(msg) > +{ > + document.getElementById('console').appendChild(document.createTextNode(msg + '\n')); > +} I'm not sure why you're including a newline in a text node you're putting inside a <div>. The newline will just be treated the same way as any other whitespace. It won't cause the next item to move to a new line.
Comment on attachment 43410 [details] Rewrite the test fast/parser/comment-in-textarea to use dumpAsText (Rev.2) I agree with Darin, your log() function doens't really do what it intends to do. You might as well just use the log/shouldBe stuff provided by fast/js/resources/js-test-pre.js See all the other TEMPLATE.html files and http://trac.webkit.org/wiki/Writing%20Layout%20Tests%20for%20DumpRenderTree You don't actually have to make a TEMPLATE.html and use real script-tests to include the script test headers. You can just modify this test to include the necessary <div id='console'> and the two script tags (like every template does) and you're set. Or you could convert the whole thing to a script-test (with or w/o a custom template). If you don't use a custom template, you'd need to use innerHTML in order to test the parser.
Comment on attachment 43410 [details] Rewrite the test fast/parser/comment-in-textarea to use dumpAsText (Rev.2) r- because it's kinda silly to add a log() function which doesn't work as intended. You don't need to do any of the conversion I suggested above, just fix log() and I'm happy to r+. You should definitely familiarize yourself with js-test-pre.js and script-tests as you'll undoubtably use them in the future.
Created attachment 43481 [details] Rewrite the test fast/parser/comment-in-textarea to use dumpAsText (Rev.2)
Thanks for reviewing. The code was a bit careless and wasn't using standard js test functions (even though I've used them before), sorry for that. Updated the patch with TEMPLATES/js-test-pre.js.
Comment on attachment 43481 [details] Rewrite the test fast/parser/comment-in-textarea to use dumpAsText (Rev.2) This patch looks good but does not include script-tests/comment-in-textarea.js. Very close!
Created attachment 43485 [details] Rewrite the test fast/parser/comment-in-textarea to use dumpAsText (added comment-in-textarea.js)
Created attachment 43486 [details] Rewrite the test fast/parser/comment-in-textarea to use dumpAsText (added comment-in-textarea.js and updated ChangeLog)
Comment on attachment 43486 [details] Rewrite the test fast/parser/comment-in-textarea to use dumpAsText (added comment-in-textarea.js and updated ChangeLog) This no longer tests the original bug. That bug had to do with the HTML parser when in textarea mode, but I don't think setting the innerHTML of a textarea is sufficient. Instead I think you need to set innerHTML of some other element and include a <textarea> element in the markup. That's the danger of rewriting a test. One of the things about the original test is that it was run before the bug was fixed, and so we know it fails if the bug exists. But to do that now you'd have to find a way to roll out the original fix and reintroduce the bug to double check that it still tests the bug. I think you'd be OK if you created some other kind of element and used innerHTML with markup that includes the "<textarea>" and "</textarea>", not just the comment.
(In reply to comment #12) > (From update of attachment 43486 [details]) > This no longer tests the original bug. That bug had to do with the HTML parser > when in textarea mode, but I don't think setting the innerHTML of a textarea is > sufficient. Instead I think you need to set innerHTML of some other element and > include a <textarea> element in the markup. > > That's the danger of rewriting a test. One of the things about the original > test is that it was run before the bug was fixed, and so we know it fails if > the bug exists. But to do that now you'd have to find a way to roll out the > original fix and reintroduce the bug to double check that it still tests the > bug. > > I think you'd be OK if you created some other kind of element and used > innerHTML with markup that includes the "<textarea>" and "</textarea>", not > just the comment. Thanks for your careful review. For later changes I'll try reading the original bug report (probably bug 19597 in this case?) and try understand what needs to be tested. I'm updating the patch based on your comment-
Created attachment 43834 [details] Rewrite the test fast/parser/comment-in-textarea to use dumpAsText (revised comment-in-textarea.js)
Comment on attachment 43834 [details] Rewrite the test fast/parser/comment-in-textarea to use dumpAsText (revised comment-in-textarea.js) Looks right to me.
Comment on attachment 43834 [details] Rewrite the test fast/parser/comment-in-textarea to use dumpAsText (revised comment-in-textarea.js) Clearing flags on attachment: 43834 Committed r51427: <http://trac.webkit.org/changeset/51427>
All reviewed patches have been landed. Closing bug.