Bug 50854 – REGRESSION(r73799): editing/execCommand/4920488.html fails

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Ryosuke Niwa]

editing/execCommand/4920488.html fails on all platforms after http://trac.webkit.org/changeset/73799.

[Ryosuke Niwa]

Created attachment 76277 [details]
Patch

[Ryosuke Niwa]

Created attachment 76278 [details]
Patch

[Darin Adler]

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]

(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]

(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]

Sorry, that was my fault.  I should have spotted the "n" shadowing in my review.  :(

[Ryosuke Niwa]

Committed r73818: <http://trac.webkit.org/changeset/73818>

[Ryosuke Niwa]

(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)]

(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)]

(In reply to comment #9)
> Follow-up fix in Bug 52512.

Committed r75882: <http://trac.webkit.org/changeset/75882>