Bug 50854 - REGRESSION(r73799): editing/execCommand/4920488.html fails
Summary: REGRESSION(r73799): editing/execCommand/4920488.html fails
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P1 Normal
Assignee: Ryosuke Niwa
URL:
Keywords:
Depends on:
Blocks: 52512
  Show dependency treegraph
 
Reported: 2010-12-10 16:01 PST by Ryosuke Niwa
Modified: 2011-01-15 12:45 PST (History)
10 users (show)

See Also:


Attachments
Patch (1.82 KB, patch)
2010-12-10 16:05 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Patch (1.81 KB, patch)
2010-12-10 16:06 PST, Ryosuke Niwa
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2010-12-10 16:01:11 PST
editing/execCommand/4920488.html fails on all platforms after http://trac.webkit.org/changeset/73799.
Comment 1 Ryosuke Niwa 2010-12-10 16:05:20 PST
Created attachment 76277 [details]
Patch
Comment 2 Ryosuke Niwa 2010-12-10 16:06:01 PST
Created attachment 76278 [details]
Patch
Comment 3 Darin Adler 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.
Comment 4 Ryosuke Niwa 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.
Comment 5 Darin Adler 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.
Comment 6 Adam Barth 2010-12-10 16:26:57 PST
Sorry, that was my fault.  I should have spotted the "n" shadowing in my review.  :(
Comment 7 Ryosuke Niwa 2010-12-10 16:30:29 PST
Committed r73818: <http://trac.webkit.org/changeset/73818>
Comment 8 Ryosuke Niwa 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().
Comment 9 David Kilzer (:ddkilzer) 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.
Comment 10 David Kilzer (:ddkilzer) 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>