testcase <script> function runTest() { window.getSelection().setBaseAndExtent(start, 0, null, 0); document.execCommand("Indent"); } </script> <meta content="2"/><body onLoad="runTest();"> ><defs contenteditable="true" id="start"> <rt>AAAAAAA0A0AAAA00
Created attachment 106728 [details] Patch
Comment on attachment 106728 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=106728&action=review This looks OK. It's a little strange to just do nothing if the node is not an element and the patch would be better if it explained why that is the correct thing to do, not just that it won’t crash. > LayoutTests/editing/execCommand/ident-crashes-topnode-is-text.html:9 > + document.writeln("This test ensures WebKit does not crash.<br><br>PASS"); The wording here is wrong. The test does not ensure that WebKit does not crash. The test has no effect on WebKit. It's good to say that the test has passed if it doesn’t crash, and also probably good to say as clearly as possible why there was a time when the test would crash. > LayoutTests/editing/execCommand/ident-crashes-topnode-is-text.html:12 > +<meta content="2"/><body onLoad="runTest();"> What is the significance of this "meta" element?
Created attachment 106847 [details] Patch
(In reply to comment #2) > (From update of attachment 106728 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=106728&action=review > > This looks OK. It's a little strange to just do nothing if the node is not an element and the patch would be better if it explained why that is the correct thing to do, not just that it won’t crash. I added some explanation in a test case. > > > LayoutTests/editing/execCommand/ident-crashes-topnode-is-text.html:9 > > + document.writeln("This test ensures WebKit does not crash.<br><br>PASS"); > > The wording here is wrong. The test does not ensure that WebKit does not crash. The test has no effect on WebKit. Fixed. > > It's good to say that the test has passed if it doesn’t crash, and also probably good to say as clearly as possible why there was a time when the test would crash. > > > LayoutTests/editing/execCommand/ident-crashes-topnode-is-text.html:12 > > +<meta content="2"/><body onLoad="runTest();"> > > What is the significance of this "meta" element? It did have no meaning. I removed it.
(In reply to comment #3) > Created an attachment (id=106847) [details] > Patch Your patch already had r+ by Darin. So you may land the updated patch without review if you'd like to do so. 1. Update "Reviewed by" lines in ChangeLog entries for the reviewer. Reviewed by Darin Adler. 2. Post the path to this bug. 3. Don't set "review?", set "commit-queue?" instead.
Created attachment 106849 [details] Patch
(In reply to comment #5) > (In reply to comment #3) > > Created an attachment (id=106847) [details] [details] > > Patch > > Your patch already had r+ by Darin. > So you may land the updated patch without review if you'd like to do so. > 1. Update "Reviewed by" lines in ChangeLog entries for the reviewer. > Reviewed by Darin Adler. > 2. Post the path to this bug. > 3. Don't set "review?", set "commit-queue?" instead. Thanks for telling me the process! I updated the 'Reviewed by' lines in ChangeLog, and set commit-queue? mark.
Comment on attachment 106849 [details] Patch Clearing flags on attachment: 106849 Committed r94841: <http://trac.webkit.org/changeset/94841>
All reviewed patches have been landed. Closing bug.