Make editing/deleting/5390681.html and editing/deleting/delete-mixed-editable-content-001.html dump text instead of render tree
Created attachment 71529 [details]
Comment on attachment 71529 [details]
View in context: https://bugs.webkit.org/attachment.cgi?id=71529&action=review
Thanks for doing this!
This contains too little information. I can't tell what kind of DOM we're generating. When you're converting a test like this, you should be careful not to lose any information necessary to prevent regressions. See the changeset that added this test (http://trac.webkit.org/changeset/24945/trunk/LayoutTests/editing/deleting/5390681.html in this case) and make sure the expected result encompasses necessary information to prevent any regression. In this case particular, we want to avoid hitting an assert but we probably also want to know that we're not adding or removing inappropriate nodes. So it's good to dump the DOM. Including dump-as-markup.js and doing Markup.dump('div') will be a good way to achieve this. http://trac.webkit.org/changeset/70031 is a good example of this kind of test conversion.
This contains too little information too. We at least want to see the final DOM. Please include dump-as-markup.js and dump the contents of the div.
> - selectAllCommand();
> - deleteCommand();
> + selectAllCommand();
> + deleteCommand();
Great that you got rid of these tabs.
Created attachment 71702 [details]
Comment on attachment 71702 [details]
View in context: https://bugs.webkit.org/attachment.cgi?id=71702&action=review
> <p>This tests for a bug where expansion for smart delete would not consider editable boundaries. Only 'foo' should be deleted. You should see ' bar'. <b>There is a bug: while the non-editable space isn't deleted, deletion inserts a placeholder when it shouldn't.</b></p>
Please move this into Markup.description
> +if (window.layoutTestController)
> + layoutTestController.dumpAsText();
No need to manually call dumpAsText() since dump-as-markup.js automatically calls it.
> +Markup.dump(div, "div");
I don't think "div" is descriptive. You can either omit the second argument or replace it by something more descriptive.
> -<div style="border-style:solid; border-width:2px; border-color:red; width:200px;">
> +<div id="testContent" style="border-style:solid; border-width:2px; border-color:red; width:200px;">
You don't need have to add id here. You can just do Markup.dump(document.getElementsByTagName('div')).
> +Markup.dump(document.getElementById("testContent"), "testContent");
If you're adding the id, you should just do Markup.dump('testContent') because dump automatically calls getElementById when the first argument is string. Again, please remove the second argument as labeling the result by "testContent" doesn't add any useful information.
Created attachment 71704 [details]
(In reply to comment #5)
> Created an attachment (id=71704) [details]
LGTM. +tony, tkent,& ojan for the formal review.
Comment on attachment 71704 [details]
View in context: https://bugs.webkit.org/attachment.cgi?id=71704&action=review
> +| "
Could you add Markup.description() to show what is this test and how the resultant markup should be, please?
(In reply to comment #8)
> Could you add Markup.description() to show what is this test and how the resultant markup should be, please?
how -> what
(In reply to comment #9)
> (In reply to comment #8)
> > Could you add Markup.description() to show what is this test and how the resultant markup should be, please?
> how -> what
I looked up the original changeset (http://trac.webkit.org/browser/trunk/LayoutTests/editing/deleting/delete-mixed-editable-content-001.html) that added this test but it was not clear to me what the test was testing. We could guess that the test was only testing Mail should not crash but it maybe also testing the resultant DOM.
I don't know what the test is for; it was added 4 years ago filed in rdar. I could speculate something like
'Tests that SelectAll respects editability boundaries. Only "hello" should be selected and deleted; "12345" should remain.'
but that isn't really saying much more than can be concluded from the html + expected.txt.
ok, I think "Confirm no crash by ...." or something like that is enough.
Created attachment 71727 [details]
Comment on attachment 71727 [details]
Comment on attachment 71727 [details]
Clearing flags on attachment: 71727
Committed r70448: <http://trac.webkit.org/changeset/70448>
All reviewed patches have been landed. Closing bug.
http://trac.webkit.org/changeset/70448 might have broken Leopard Intel Debug (Tests)
The following tests are not passing: