Bug 61332 - Convert LayoutTests/editing/deleting/5206311-2.html to dump-as-markup
Summary: Convert LayoutTests/editing/deleting/5206311-2.html to dump-as-markup
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 57148
  Show dependency treegraph
 
Reported: 2011-05-23 18:11 PDT by Annie Sullivan
Modified: 2011-05-24 20:51 PDT (History)
2 users (show)

See Also:


Attachments
Patch (110.62 KB, patch)
2011-05-23 18:13 PDT, Annie Sullivan
no flags Details | Formatted Diff | Diff
Patch (110.91 KB, patch)
2011-05-24 12:34 PDT, Annie Sullivan
no flags Details | Formatted Diff | Diff
Patch (112.46 KB, patch)
2011-05-24 14:42 PDT, Annie Sullivan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Annie Sullivan 2011-05-23 18:11:00 PDT
The output would be much easier to read as markup.
Comment 1 Annie Sullivan 2011-05-23 18:13:19 PDT
Created attachment 94531 [details]
Patch
Comment 2 Ryosuke Niwa 2011-05-23 19:54:34 PDT
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 3 Annie Sullivan 2011-05-24 12:33:59 PDT
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.
Comment 4 Annie Sullivan 2011-05-24 12:34:52 PDT
Created attachment 94668 [details]
Patch
Comment 5 Ryosuke Niwa 2011-05-24 12:37:01 PDT
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 6 Ryosuke Niwa 2011-05-24 12:40:11 PDT
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 7 Annie Sullivan 2011-05-24 14:40:22 PDT
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.
Comment 8 Annie Sullivan 2011-05-24 14:42:16 PDT
Created attachment 94691 [details]
Patch
Comment 9 WebKit Commit Bot 2011-05-24 20:51:06 PDT
Comment on attachment 94691 [details]
Patch

Clearing flags on attachment: 94691

Committed r87262: <http://trac.webkit.org/changeset/87262>
Comment 10 WebKit Commit Bot 2011-05-24 20:51:12 PDT
All reviewed patches have been landed.  Closing bug.