Bug 50854

Summary: REGRESSION(r73799): editing/execCommand/4920488.html fails
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: HTML EditingAssignee: Ryosuke Niwa <rniwa>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, bweinstein, darin, ddkilzer, eae, enrica, eric, inferno, jberlin, pkasting
Priority: P1    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 52512    
Attachments:
Description Flags
Patch
none
Patch darin: review+

Ryosuke Niwa
Reported 2010-12-10 16:01:11 PST
editing/execCommand/4920488.html fails on all platforms after http://trac.webkit.org/changeset/73799.
Attachments
Patch (1.82 KB, patch)
2010-12-10 16:05 PST, Ryosuke Niwa 		no flags
DetailsFormatted DiffDiff
Patch (1.81 KB, patch)
2010-12-10 16:06 PST, Ryosuke Niwa 		darin: review+
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Ryosuke Niwa
Comment 1 2010-12-10 16:05:20 PST
Created attachment 76277 [details] Patch
Ryosuke Niwa
Comment 2 2010-12-10 16:06:01 PST
Created attachment 76278 [details] Patch
Darin Adler
Comment 3 2010-12-10 16:10:14 PST
Comment on attachment 76278 [details] Patch It would be *much* better if we had a regression test that was using Range directly rather than seeing this only as a side effect of the editing test. I worry that this rewrite of processContents broke other Range operations but we don’t have sufficient test coverage.
Ryosuke Niwa
Comment 4 2010-12-10 16:14:22 PST
(In reply to comment #3) > (From update of attachment 76278 [details]) > It would be *much* better if we had a regression test that was using Range directly rather than seeing this only as a side effect of the editing test. I worry that this rewrite of processContents broke other Range operations but we don’t have sufficient test coverage. Actually, the editing test directly called extractContents. Good thing I converted to a dump as text some time ago, and I was very familiar with the test. We might have missed otherwise. Anyway, thanks for the review. Will be landing.
Darin Adler
Comment 5 2010-12-10 16:18:12 PST
(In reply to comment #4) > Actually, the editing test directly called extractContents. Sure, but how much other coverage do we have for this Range function and the closely related functions that share the same back end code. I think we lucked out this time, but I fear we may have other bugs.
Adam Barth
Comment 6 2010-12-10 16:26:57 PST
Sorry, that was my fault. I should have spotted the "n" shadowing in my review. :(
Ryosuke Niwa
Comment 7 2010-12-10 16:30:29 PST
Committed r73818: <http://trac.webkit.org/changeset/73818>
Ryosuke Niwa
Comment 8 2010-12-10 16:31:48 PST
(In reply to comment #6) > Sorry, that was my fault. I should have spotted the "n" shadowing in my review. :( Shadowing was okay. The problem was that it stopped adding the first child (m_end.container()->firstChild()) when i + 1 >= m_end.offset().
David Kilzer (:ddkilzer)
Comment 9 2011-01-15 11:39:54 PST
(In reply to comment #8) > (In reply to comment #6) > > Sorry, that was my fault. I should have spotted the "n" shadowing in my review. :( > > Shadowing was okay. The problem was that it stopped adding the first child (m_end.container()->firstChild()) when i + 1 >= m_end.offset(). This fix does not increment the loop variable (i) in the do-while loop, so it ignores the end offset of the range and iterates through all children of the endContainer. Follow-up fix in Bug 52512.
David Kilzer (:ddkilzer)
Comment 10 2011-01-15 12:45:47 PST
(In reply to comment #9) > Follow-up fix in Bug 52512. Committed r75882: <http://trac.webkit.org/changeset/75882>
Note You need to log in before you can comment on or make changes to this bug.