Created attachment 79068 [details] Patch Reviewed by NOBODY (OOPS!). WebCore: The fix for Bug 50710 in r73799 introduced an off-by-one error when copying nodes to a local NodeVector for processing. A fix was attempted for Bug 50854 in r73818, but instead of stopping at the end offset, it iterates through all the sibling nodes because the loop variable (i) is never incremented. To clean this up, revert back to the code in r73799 and fix the off-by-one error. Test: fast/dom/Range/range-clone-contents.html * dom/Range.cpp: (WebCore::Range::processContents): Fix the loop that copies nodes to a local NodeVector by restoring the code from r73799 and fixing the off-by-one error. LayoutTests: * fast/dom/Range/range-clone-contents-expected.txt: Added. * fast/dom/Range/range-clone-contents.html: Added. --- 5 files changed, 124 insertions(+), 6 deletions(-)
Note to reviewers: Please verify the new algorithm matches the behavior of the original code. I've gone through the new algorithm, but this code has had bugs introduced through two rewrites and two reviews, so please study it carefully before giving an r+. This is the pre-r73799 code in processContents() in WebCore/dom/Range.cpp: Node* n = m_end.container()->firstChild(); if (n && m_end.offset()) { for (int i = 0; i + 1 < m_end.offset(); i++) { // skip to end.offset() Node* next = n->nextSibling(); if (!next) break; n = next; } Node* prev; for (; n; n = prev) { prev = n->previousSibling(); [...] } [...] } The fix is this code: Node* n = m_end.container()->firstChild(); if (n && m_end.offset()) { NodeVector nodes; for (int i = 0; i < m_end.offset() && n; i++, n = n->nextSibling()) nodes.append(n); for (int i = nodes.size() - 1; i >= 0; i--) { n = nodes[i].get(); [...] } [...] }
<rdar://problem/8789045>
Comment on attachment 79068 [details] Patch I'm not sure I understand the intermediate change before this very well, but this code looks right. You're still going to always append at least the end container's first child, since m_end.offset() won't be 0 in the inner loop. The new test looks good too, and Dave says this also passes the other editing tests, including the one that was failing for one of the earlier patches. r=me!
Committed r75882: <http://trac.webkit.org/changeset/75882>
(In reply to comment #4) > Committed r75882: <http://trac.webkit.org/changeset/75882> fast/dom/Range/range-clone-contents.html appears to be failing on the Windows 7 Release Test bots: http://build.webkit.org/results/Windows%207%20Release%20(Tests)/r75882%20(8257)/fast/dom/Range/range-clone-contents-pretty-diff.html
(In reply to comment #5) > (In reply to comment #4) > > Committed r75882: <http://trac.webkit.org/changeset/75882> > > fast/dom/Range/range-clone-contents.html appears to be failing on the Windows 7 Release Test bots: > > http://build.webkit.org/results/Windows%207%20Release%20(Tests)/r75882%20(8257)/fast/dom/Range/range-clone-contents-pretty-diff.html This is the exact erroneous behavior that the test is trying to fix (where the <img> tag is left in the range instead of being removed). It's as if Range.cpp wasn't compiled on the Windows 7 bot.
(In reply to comment #5) > (In reply to comment #4) > > Committed r75882: <http://trac.webkit.org/changeset/75882> > > fast/dom/Range/range-clone-contents.html appears to be failing on the Windows 7 Release Test bots: > > http://build.webkit.org/results/Windows%207%20Release%20(Tests)/r75882%20(8257)/fast/dom/Range/range-clone-contents-pretty-diff.html Same issue on the Windows XP box: http://build.webkit.org/results/Windows%20XP%20Debug%20(Tests)/r75882%20(24216)/fast/dom/Range/range-clone-contents-pretty-diff.html Oddly enough, I don't see any other bots failing on this test.
(In reply to comment #7) > (In reply to comment #5) > > (In reply to comment #4) > > > Committed r75882: <http://trac.webkit.org/changeset/75882> > > > > fast/dom/Range/range-clone-contents.html appears to be failing on the Windows 7 Release Test bots: > > [...] > Same issue on the Windows XP box: > [...] > Oddly enough, I don't see any other bots failing on this test. Filed Bug 52524 to cover this. Added the test to the Windows skip list: Committed r75889. <http://trac.webkit.org/changeset/75889>
(In reply to comment #8) > Filed Bug 52524 to cover this. Added the test to the Windows skip list: > > Committed r75889. <http://trac.webkit.org/changeset/75889> Turns out the buildbots just needed to do a clean build. Removed the test from the skip list: Committed r75970: <http://trac.webkit.org/changeset/75970>
*** Bug 54861 has been marked as a duplicate of this bug. ***