Bug 63359 – convert editing/deleting/5156801-2.html to dumpAsText and rename

• Component: HTML Editing
• Status: RESOLVED FIXED

[Wyatt Carss]

convert editing/deleting/5156801-2.html to dumpAsText and rename

[Wyatt Carss]

Created attachment 98550 [details]
Patch

[Wyatt Carss]

Created attachment 98557 [details]
Patch

[Wyatt Carss]

Renamed, converted, and rdar info included

[Ryosuke Niwa]

Comment on attachment 98557 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=98557&action=review

> LayoutTests/editing/deleting/delete-node-after-DOMNodeRemoved-2.html:2
> +<p>This tests for a crash when deleting the contents of a table cell. You should just see 'Cached' below.</p>

Please add rdar id.

> LayoutTests/editing/deleting/delete-node-after-DOMNodeRemoved-2.html:16
> +s = window.getSelection();
> +table = document.getElementById("table");
> +s.setPosition(table, table.childNodes.length);
> +s.modify("move", "backward", "character");
> +document.execCommand("Delete");
> +document.execCommand("Delete");
> +document.execCommand("Delete");
> +document.execCommand("Delete");
> +document.execCommand("Delete");
> +document.execCommand("Delete");
> +document.execCommand("Delete");

What is this test to do with DOMNodeRemoved?  That's a DOM level 3 event.  I don't see an event listener attached in this test case.  r- due to this.

[Wyatt Carss]

Created attachment 98755 [details]
Patch

[Wyatt Carss]

The rdar was present already; the diff shows the old file's deletion, and then the new file's creation with identical contents to the old. Then it shows the changes to the new file separately.

I've renamed the second test, because its only relation was through the rdar number - there's no reason I know of that these tests have to be kept together.

[Ryosuke Niwa]

Comment on attachment 98755 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=98755&action=review

> LayoutTests/editing/deleting/delete-table-cell-contents-expected.txt:4
> +
> +Cached

You should use dump-as-markup.js here because it's significant that "Cached" is inside a table cell.

[Wyatt Carss]

Created attachment 98774 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 98774 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=98774&action=review

> LayoutTests/editing/deleting/delete-table-cell-contents.html:24
> +if (window.layoutTestController) {
> +    Markup.description(description.textContent);
> +    Markup.dump("test");
> +}

You shouldn't wrap Markup in window.layoutTestController.  Markup takes care of that.

[Wyatt Carss]

Created attachment 98818 [details]
Patch

[WebKit Review Bot]

Comment on attachment 98818 [details]
Patch

Clearing flags on attachment: 98818

Committed r89883: <http://trac.webkit.org/changeset/89883>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.