Summary: | markup-dump conversion + rename: editing/deleting/5408255.html | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Wyatt Carss <wcarss> | ||||||||||||||||||
Component: | HTML Editing | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||
Severity: | Enhancement | CC: | dglazkov, rniwa, wcarss, webkit.review.bot | ||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||
Hardware: | All | ||||||||||||||||||||
OS: | All | ||||||||||||||||||||
Attachments: |
|
Description
Wyatt Carss
2011-06-14 17:14:33 PDT
Created attachment 97202 [details]
Patch
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. Created attachment 97209 [details]
Patch
Second changelog was inaccurate - didn't reflect updated filenames. Resubmitting. Created attachment 97212 [details]
Patch
Created attachment 97224 [details]
Patch
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. Comment on attachment 97224 [details] Patch Attachment 97224 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8857124 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
(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. 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. (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. (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! (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. Created attachment 97514 [details]
Patch
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 "(". Created attachment 97521 [details]
Patch
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? Also, you should put cq? on the patch. 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-. Created attachment 97524 [details]
Patch
Comment on attachment 97524 [details] Patch Clearing flags on attachment: 97524 Committed r89092: <http://trac.webkit.org/changeset/89092> All reviewed patches have been landed. Closing bug. |