[webkit-reviews] review denied: [Bug 54893] Remove CorrectionIndicator markers sooner. : [Attachment 83232] Proposed patch (v1)

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied jpu at apple.com's request for review:
Bug 54893: Remove CorrectionIndicator markers sooner.
https://bugs.webkit.org/show_bug.cgi?id=54893

Attachment 83232: Proposed patch (v1)
https://bugs.webkit.org/attachment.cgi?id=83232&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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?