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

[bugzilla-daemon at webkit.org]

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


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #83232|review?                     |review-
               Flag|                            |




--- Comment #2 from Darin Adler <darin at apple.com>  2011-02-22 14:07:51 PST ---
(From update of attachment 83232)
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.

> Source/WebCore/dom/DocumentMarkerController.h:74
> +    // This function clears the description even when the marker only partially follows in the specified range.
> +    void clearMarkerDescriptions(Range*, DocumentMarker::MarkerTypes);

You mean "partially falls" not "partially follows".

I think a better name for the function would have the word intersecting in it. So you wouldn’t need that comment so much.

> Source/WebCore/editing/Editor.cpp:1228
> +    if (text.length() == 1 && (category(text[0]) & (Punctuation_Dash | Punctuation_Open | Punctuation_Close | Punctuation_Connector | Punctuation_Other)) && !isAmbiguousBoundaryCharacter(text[0]))

I think the punctuation check here needs to be in a separate helper function. It can be in this file. But writing it out like that makes the code unnecessarily hard to read.

Also, can you use isPunct instead of category?

> 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?

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