Bug 50854

Summary:

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

Product:

WebKit

Reporter:

Ryosuke Niwa <rniwa>

Component:

HTML Editing

Assignee:

Ryosuke Niwa <rniwa>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, bweinstein, darin, ddkilzer, eae, enrica, eric, inferno, jberlin, pkasting

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Bug Depends on:

Bug Blocks:

52512

Attachments:

Description	Flags
Patch	none
Patch	darin: review+

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>