[Webkit-unassigned] [Bug 48078] Editing a word with misspell or autocorrection underline should remove the underline when the editing changes the word.

[bugzilla-daemon at webkit.org]

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


mitz at webkit.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #71510|commit-queue?               |commit-queue-
               Flag|                            |




--- Comment #3 from mitz at webkit.org  2010-10-25 10:12:25 PST ---
(From update of attachment 71510)
View in context: https://bugs.webkit.org/attachment.cgi?id=71510&action=review

Seems like it should be possible to have tests to go with this change.

> WebCore/ChangeLog:13
> +        This patch part of on-going improvement of autocorrection feature on Mac OSX. When an editing
> +        occur, if it affect words (by deleting/inserting characters, spliting word, merging words) that
> +        has Spelling and/or CorrectionIndicator markers, we want to remove the markers. If subsequntial
> +        spelling checking found spelling error in newlly formed words, it will add the markers back in.
> +

Some typos in the above: “Mac OS X”, “When editing occurs”, “affects”, “that have Spelling”, “finds”, “newly”.

> WebCore/editing/Editor.cpp:145
> +#if PLATFORM(MAC) && !defined(BUILDING_ON_TIGER) && !defined(BUILDING_ON_LEOPARD) && !defined(BUILDING_ON_SNOW_LEOPARD)

Not for this patch, but we should consider defining a dedicated macro for all things autocorrection-related and using it instead of the above, for clarity and for other platforms that may want this behavior.

> WebCore/editing/Editor.cpp:146
> +    removeSpellAndCorrectionMarkersFromWordsToBeEdited(isSpaceOrNewline((event->data().characters())[0]));

Is it possible for event->data() to be zero-length?

> WebCore/editing/Editor.cpp:2935
> +    // We want to remove the markers from a word if an editing command will change the word. This can happend in one of

Typo: “happend”.

> WebCore/editing/Editor.cpp:2954
> +    VisiblePosition startOfFirstWord = startOfWord(startOfSelection, LeftWordIfOnBoundary).deepEquivalent();
> +    VisiblePosition endOfFirstWord = endOfWord(startOfSelection, LeftWordIfOnBoundary).deepEquivalent();
> +    // Last word is the word that begins before or on the end of selection
> +    VisiblePosition startOfLastWord = startOfWord(endOfSelection, RightWordIfOnBoundary).deepEquivalent();
> +    VisiblePosition endOfLastWord = endOfWord(endOfSelection, RightWordIfOnBoundary).deepEquivalent();

Why are you using deepEquivalent() here? You are going from a VisiblePosition to a Position and then back to a VisiblePosition.

> WebCore/editing/Editor.cpp:2988
> +    RefPtr<Document> document = m_frame->document();

Any reason to take a reference to the document here?

> WebCore/editing/Editor.cpp:2990
> +    for (TextIterator textIter(wordRange.get()); !textIter.atEnd(); textIter.advance()) {

Please don’t use an abbreviation in the name. “iterator”, or “textIterator” are good names for this variable.

> WebCore/editing/Editor.cpp:3000
> +                    document->markers()->removeMarkers(markerRange.get(), DocumentMarker::Spelling);
> +                    document->markers()->removeMarkers(markerRange.get(), DocumentMarker::CorrectionIndicator);

I don’t think it’s safe to call removeMarkers() while iterating over the markers vector!

> WebCore/editing/Editor.cpp:3015
> +            document->markers()->removeMarkers(node, 0, INT_MAX, DocumentMarker::Spelling);
> +            document->markers()->removeMarkers(node, 0, INT_MAX, DocumentMarker::CorrectionIndicator);

Using numeric_limits<int>::max() is preferred to INT_MAX. But this is not very elegant. Perhaps a version of removeMarkers() that takes a node and a marker type is needed.

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