Created attachment 94531 [details] Patch

Comment on attachment 94531 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=94531&action=review In general, we'd like to improve the readability & usability of a test when we're converting it. While this patch does convert the test, I'd like to see more improvements in test descriptions and code. > LayoutTests/ChangeLog:7 > + Missing description. > LayoutTests/editing/deleting/5206311-2-expected.txt:2 > +This empties the last row, it should be removed. 'world!' should also be brought into the second cell of the second row.: It's odd to have a period before a colon. > LayoutTests/editing/deleting/5206311-2-expected.txt:33 > +This empties a the last row of the first table and the first of the second, they should both be removed.: the first row of the second table? I know you're just converting the existing tests but please revise these descriptions. > LayoutTests/editing/deleting/5206311-2.html:1 > +<script src="../../resources/dump-as-markup.js"></script> Missing DOCTYPE, html, & body. > LayoutTests/editing/deleting/5206311-2.html:3 > function runTest(num) I would have modified this function to take an element instead of a number like this.

Comment on attachment 94531 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=94531&action=review >> LayoutTests/ChangeLog:7 >> + > > Missing description. Done. >> LayoutTests/editing/deleting/5206311-2-expected.txt:2 >> +This empties the last row, it should be removed. 'world!' should also be brought into the second cell of the second row.: > > It's odd to have a period before a colon. Oops, I didn't notice that Markup.dump() added the colon. I removed the periods from the ends of the descriptions. >> LayoutTests/editing/deleting/5206311-2-expected.txt:33 >> +This empties a the last row of the first table and the first of the second, they should both be removed.: > > the first row of the second table? I know you're just converting the existing tests but please revise these descriptions. Done. >> LayoutTests/editing/deleting/5206311-2.html:1 >> +<script src="../../resources/dump-as-markup.js"></script> > > Missing DOCTYPE, html, & body. Done. >> LayoutTests/editing/deleting/5206311-2.html:3 >> function runTest(num) > > I would have modified this function to take an element instead of a number like this. Sorry I didn't address this yet. I wasn't sure of a cleaner approach to the problem of grouping all the elements needed for a single test run together (description, root, selection start, selection end). Should I wrap the entire test in a container element, hang classes off the special elements, and query for the correct ones with getElementsByClassName? It would make the code more verbose, but there would be less string concatenation. Let me know what you think.

Created attachment 94668 [details] Patch

Comment on attachment 94531 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=94531&action=review >>> LayoutTests/editing/deleting/5206311-2.html:3 >>> function runTest(num) >> >> I would have modified this function to take an element instead of a number like this. > > Sorry I didn't address this yet. I wasn't sure of a cleaner approach to the problem of grouping all the elements needed for a single test run together (description, root, selection start, selection end). Should I wrap the entire test in a container element, hang classes off the special elements, and query for the correct ones with getElementsByClassName? It would make the code more verbose, but there would be less string concatenation. Let me know what you think. That would work too.

Comment on attachment 94668 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=94668&action=review > LayoutTests/editing/deleting/5206311-2.html:18 > +<p id="description1">This empties the last row, it should be removed. 'world!' should also be brought into the second cell of the second row</p> This isn't really an accurate description, is it? This test removes the last row but it also removes the 2 rows from the second row. I would have stated that this test removes cells 5 through 9 so that it's clear what we should get by just looking at the expected result. When I just see last row should be removed on expected.txt, I can't tell whether it has been removed properly but that what I'm seeing there is still the last row. > LayoutTests/editing/deleting/5206311-2.html:32 > +<p id="description2">This empties a the last row of the first table and the first row of the second table. They should both be removed</p> Ditto.

Comment on attachment 94668 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=94668&action=review > LayoutTests/editing/deleting/5206311-2.html:5 > function runTest(num) I changed this to take an element and use getElementsByClassName() to find the root, description, start and end. Hopefully it's a little cleaner now. >> LayoutTests/editing/deleting/5206311-2.html:18 >> +<p id="description1">This empties the last row, it should be removed. 'world!' should also be brought into the second cell of the second row</p> > > This isn't really an accurate description, is it? This test removes the last row but it also removes the 2 rows from the second row. I would have stated that this test removes cells 5 through 9 so that it's clear what we should get by just looking at the expected result. When I just see last row should be removed on expected.txt, I can't tell whether it has been removed properly but that what I'm seeing there is still the last row. I couldn't come up with a short sentence that describes this accurately, so I wrote up descriptions that are a bit more verbose. Let me know what you think. >> LayoutTests/editing/deleting/5206311-2.html:32 >> +<p id="description2">This empties a the last row of the first table and the first row of the second table. They should both be removed</p> > > Ditto. Here too.

Created attachment 94691 [details] Patch

Comment on attachment 94691 [details] Patch Clearing flags on attachment: 94691 Committed r87262: <http://trac.webkit.org/changeset/87262>

All reviewed patches have been landed. Closing bug.