Bug 62680 – markup-dump conversion + rename: editing/deleting/5408255.html

• Component: HTML Editing
• Status: RESOLVED FIXED

[Wyatt Carss]

Test editing/deleting/5408255.html should be renamed to something more meaningful than the radr number and converted to a markup-dump.

[Wyatt Carss]

Created attachment 97202 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 97202 [details]
Patch

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

> LayoutTests/ChangeLog:8
> +        editing/deleting/5408255.html is now editing/deleting/delete-button-failure.html

delete-button-failure sounds too general.  Since this test is concerned with -webkit-user-select: none, I'd include that in the name.  Something like delete-button-with-select-none.

> LayoutTests/editing/deleting/delete-button-failure.html:26
> +    if(!deleteButton && !listElement)
> +        document.getElementById("test").innerHTML = "Success";

I don't think not having list item necessarily makes this test pass because it could be leaving other elements such as ul or text. r- because of this.

[Wyatt Carss]

Created attachment 97209 [details]
Patch

[Wyatt Carss]

Second changelog was inaccurate - didn't reflect updated filenames. Resubmitting.

[Wyatt Carss]

Created attachment 97212 [details]
Patch

[Wyatt Carss]

Created attachment 97224 [details]
Patch

[Wyatt Carss]

I tried svn revert'ing my directory and applying the earlier patch (with the move), and it failed, so I redid the change without the rename. After this lands, I'll update and submit a new patch -- possibly with multiple renames, as I plan to convert multiple bugs known only by their radr number.

[WebKit Review Bot]

Comment on attachment 97224 [details]
Patch

Attachment 97224 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8857124

[WebKit Review Bot]

Created attachment 97226 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Wyatt Carss]

(In reply to comment #9)
> Created an attachment (id=97226) [details]
> Archive of layout-test-results from ec2-cr-linux-01
> 
> The attached test failures were seen while running run-webkit-tests on the chromium-ews.
> Bot: ec2-cr-linux-01  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

It appears that I forgot to svn add the expected file -- I'll repatch.

[Ryosuke Niwa]

Comment on attachment 97224 [details]
Patch

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

> LayoutTests/editing/deleting/5408255.html:27
> +    if(!deleteButton && testContainer.innerHTML == "\n\n")
> +        testContainer.innerHTML = "Success";

I don't think the fact testContainer.innerHTML is identically equal to "\n\n" is significant.  Replacing testContainer's innerHTML by "Success" defeats the whole point of using dump-as-markup.  What I suggested was to print "FAIL" in the case deleteButton was still there.

[Ryosuke Niwa]

(In reply to comment #7)
> I tried svn revert'ing my directory and applying the earlier patch (with the move), and it failed, so I redid the change without the rename. After this lands, I'll update and submit a new patch -- possibly with multiple renames, as I plan to convert multiple bugs known only by their radr number.

That's because you probably had old .html files.  You should use Tools/Scripts/webkit-patch apply-attachment.

[Wyatt Carss]

(In reply to comment #12)
> (In reply to comment #7)
> > I tried svn revert'ing my directory and applying the earlier patch (with the move), and it failed, so I redid the change without the rename. After this lands, I'll update and submit a new patch -- possibly with multiple renames, as I plan to convert multiple bugs known only by their radr number.
> 
> That's because you probably had old .html files.  You should use Tools/Scripts/webkit-patch apply-attachment.

I did use that script, from a clean svn - but something messed up when applying the rename portion.

Anyway, I'm confused about what you suggested. As I understand this, the markup is most worth seeing if something is wrong. If the delete button still exists, it will be shown in the markup dump. If the test passes, the markup dump is kind of confusing, because it's just two newlines. So in the case that the test passes, I set the markup to be the word "Success" so that it's clear that this is the preferred case.

If the test fails, the markup is dumped without modification while the description remains above, identifying that only the word "Success" should have appeared.

I could conceivably dump the whole page markup instead of just what's inside the content-editable div. Then there would be meaningful output in both the pass and the fail case. I didn't want to insert another <div></div> unless it was necessary, in case it messed with the test. I think it would be needed to put something like "FAIL" in while preserving the content-editable div's innerHTML. Advice welcome on that!

[Ryosuke Niwa]

(In reply to comment #13)
> Anyway, I'm confused about what you suggested. As I understand this, the markup is most worth seeing if something is wrong.

Markup is worth seeing even when succeeded.  For example, I can see that result changing from \n\n to \n, <br>, or <div><br></div>, all of which are acceptable result.  And your check won't say "Success" in such cases. 

>If the delete button still exists, it will be shown in the markup dump.

Really?  As far as I understand it, delete button won't show up in the dump even if still existed at the time of markup dump.

> I think it would be needed to put something like "FAIL" in while preserving the content-editable div's innerHTML. Advice welcome on that!

I'm saying that you can just print "FAIL" in the the same editing host you're dumping where delete button is still there, and state that the editing host shouldn't contain any visible content.

[Wyatt Carss]

Created attachment 97514 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 97514 [details]
Patch

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

The patch looks good. Just one nit.

> LayoutTests/editing/deleting/5408255.html:26
> +    if(deleteButton)

Nit: space between "if" and "(".

[Wyatt Carss]

Created attachment 97521 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 97521 [details]
Patch

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

> LayoutTests/ChangeLog:10
> +        Tests if the UI delete button works while '-webkit-user-select: none' is applied. 
> +        Should see "Success" dumped as markup if the delete button worked.
> +        Otherwise, the portion of the content not deleted is dumped. Holding off on rename.

Did you forget to update the change log?

[Ryosuke Niwa]

Also, you should put cq? on the patch.

[Ryosuke Niwa]

Also, when you're intending to upload a new patch, remove r? flag instead of putting r-.  In general, only reviewer can put r+ or r-.

[Wyatt Carss]

Created attachment 97524 [details]
Patch

[WebKit Review Bot]

Comment on attachment 97524 [details]
Patch

Clearing flags on attachment: 97524

Committed r89092: <http://trac.webkit.org/changeset/89092>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.