[Webkit-unassigned] [Bug 32131] WebCore::canHaveChildrenForEditing ReadAV at NULL (cd05b3b20e0f4c6b3afe5d165a1798aa)

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=32131





--- Comment #10 from Tony Chang (Google) <tony at chromium.org>  2010-01-18 22:04:24 PST ---
(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > > --- a/WebCore/editing/CompositeEditCommand.cpp
> > > > +++ b/WebCore/editing/CompositeEditCommand.cpp
> > > > @@ -946,10 +946,10 @@ void CompositeEditCommand::moveParagraphs(const VisiblePosition& startOfParagrap
> > > >      
> > > >      setEndingSelection(VisibleSelection(start, end, DOWNSTREAM));
> > > >      deleteSelection(false, false, false, false);
> > > > -
> > > >      ASSERT(destination.deepEquivalent().node()->inDocument());
> > > >  
> > > >      cleanupAfterDeletion();
> > > > +    ASSERT(destination.deepEquivalent().node()->inDocument());
> > > >  
> > > 
> > > I dont' understand why you moved the ASSERT here. In moveparagraph destination
> > > should always be valid after deleteSelection, because destination represents
> > > the
> > > new location to move to and deleteSelection only affects the old stuff.
> > 
> > I was removing a blank line and adding another ASSERT.  The destination needs
> > to be valid after the cleanup so we know where to insert paragraph into.
> > 
> > 
> The point I'm trying to make is that it is not necessary to move the ASSERT.
> Destination is a position that outside the selection being deleted. The ASSERT
> should verify that there is no overlap between destination and the selection to
> be deleted. We should not make unnecessary code changes.

Ok, I put back the blank line.

> > > I believe the 19087.html is failing because you are moving the end selection in
> > > a case where it is not necessary.
> > > You should try to find out why this test fails, I don't think you should just
> > > re-baseline the test without an explanation.
> > 
> > You're right, the bug is lower: it's in applyStyle.  In
> > ApplyStyleCommand::applyBlockStyle, it tries to save the selection, move the
> > paragraphs, then restore the selection.  In this test case, it fails to restore
> > the selection to the right place.  It looks like this might be a bug in
> > TextIterator::rangeLength and TextIterator::rangeFromLocationAndLength not
> > agreeing on how to restore a range?  I'm not sure what rangeLength is supposed
> > to represent.
> I don't agree. The test is failing because one line is left behind and I
> believe it has to do with moving the end selection. TextIterator computes
> correctly the position starting from an offset expressed in terms of number of
> visible characters. It could be off only if a layout is needed.

I was agreeing with your assessment that moving the end selection was wrong. 
I've posted a new patch that works around the bug in
TextIterator::rangeFromLocationAndLength.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.