[Webkit-unassigned] [Bug 54893] Remove CorrectionIndicator markers sooner.

[bugzilla-daemon at webkit.org]

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





--- Comment #3 from jpu at apple.com  2011-02-22 14:40:39 PST ---
(In reply to comment #2)
> (From update of attachment 83232 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=83232&action=review
> 
> > Source/WebCore/dom/DocumentMarkerController.cpp:580
> > +    for (TextIterator markedText(range); !markedText.atEnd(); markedText.advance()) {
> > +        RefPtr<Range> textPiece = markedText.range();
> > +        unsigned startOffset = textPiece->startOffset();
> > +        unsigned endOffset = textPiece->endOffset();
> > +        MarkerMapVectorPair* vectorPair = m_markers.get(textPiece->startContainer());
> > +        if (!vectorPair)
> > +            continue;
> 
> It seems wrong to iterate a range by using the TextIterator. We can easily iterate all the nodes in a range and remove the markers found in those nodes. There’s no real reason to involve the text iterator. TextIterator does more work that unneeded and potentially expensive. The code in DocumentMarkerController::setMarkersActive is an example of how to do this.

I was confused by the existing usage of TextIterator in DocumentMarkerController::addMarker. I assumed that TextIterator is preferred, since it seems prevent iterating on nodes that are not really leaf text nodes. I will change this loop to iterating on node directly.


> 
> > Source/WebCore/editing/Editor.h:405
> > +    bool m_hasCorrectionIndicatorMarkers;
> > +    bool m_editingCommandShouldRemoveCorrectionIndicators;
> 
> I don’t see initial values for these in the constructor. They are needed.
> 
> Using boolean state like this is really awkward. The more of these we add the more spaghetti-like the code becomes. Is there some other way to do it?

Yes. I realize this issue. I did think about an alternative for each of these two variables. 

m_hasCorrectionIndicatorMarkers serves the purpose of caching, since we want to find out quickly if there isn't a marker with this value. I could move this sort of caching into DocumentMarkerController class.

m_editingCommandShouldRemoveCorrectionIndicator is very much a property of edit command. So we could add getter and setter for this property in EditCommand. 

Let know what you think about these alternatives.

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