Changeset 148124 in webkit


Ignore:
Timestamp:
Apr 10, 2013 12:16:58 PM (11 years ago)
Author:
rniwa@webkit.org
Message:

Refactor Editor::markAndReplaceFor before fixing autocorrection bugs
​https://bugs.webkit.org/show_bug.cgi?id=114344

Reviewed by Enrica Casucci.

This patch refactors Editor::markAndReplaceFor so that we can start fixing bugs in a sane state.
Extracted isAutomaticTextReplacementType and correctSpellcheckingPreservingTextCheckingParagraph,
and made convenience local variables const.

In particular, shouldMarkSpelling used to be assigned of false when shouldShowCorrectionPanel was true
in a middle of a function but this was removed in favor of explicitly checking this condition later
since shouldMarkSpelling was only referenced once after the assignment.

  • editing/Editor.cpp:

(WebCore::isAutomaticTextReplacementType): Extracted.

(WebCore::correctSpellcheckingPreservingTextCheckingParagraph): Extracted. Used highestAncestor
and rangeFromLocationAndLength to match the rest of the up-to-date editing code.

(WebCore::Editor::markAndReplaceFor): See above.

Location:
trunk/Source/WebCore
Files:
2 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/WebCore/ChangeLog

    r148123 r148124  
     12013-04-10  Ryosuke Niwa  <rniwa@webkit.org>
     2
     3        Refactor Editor::markAndReplaceFor before fixing autocorrection bugs
     4        https://bugs.webkit.org/show_bug.cgi?id=114344
     5
     6        Reviewed by Enrica Casucci.
     7
     8        This patch refactors Editor::markAndReplaceFor so that we can start fixing bugs in a sane state.
     9        Extracted isAutomaticTextReplacementType and correctSpellcheckingPreservingTextCheckingParagraph,
     10        and made convenience local variables const.
     11
     12        In particular, shouldMarkSpelling used to be assigned of false when shouldShowCorrectionPanel was true
     13        in a middle of a function but this was removed in favor of explicitly checking this condition later
     14        since shouldMarkSpelling was only referenced once after the assignment.
     15
     16        * editing/Editor.cpp:
     17        (WebCore::isAutomaticTextReplacementType): Extracted.
     18
     19        (WebCore::correctSpellcheckingPreservingTextCheckingParagraph): Extracted.  Used highestAncestor
     20        and rangeFromLocationAndLength to match the rest of the up-to-date editing code.
     21
     22        (WebCore::Editor::markAndReplaceFor): See above.
     23
    1242013-04-08  Anders Carlsson  <andersca@apple.com>
    225

  • trunk/Source/WebCore/editing/Editor.cpp

    r146907 r148124  
    21342134}
    21352135
     2136static bool isAutomaticTextReplacementType(TextCheckingType type)
     2137{
     2138    switch (type) {
     2139    case TextCheckingTypeNone:
     2140    case TextCheckingTypeSpelling:
     2141    case TextCheckingTypeGrammar:
     2142        return false;
     2143    case TextCheckingTypeLink:
     2144    case TextCheckingTypeQuote:
     2145    case TextCheckingTypeDash:
     2146    case TextCheckingTypeReplacement:
     2147    case TextCheckingTypeCorrection:
     2148    case TextCheckingTypeShowCorrectionPanel:
     2149        return true;
     2150    }
     2151    ASSERT_NOT_REACHED();
     2152    return false;
     2153}
     2154
     2155static void correctSpellcheckingPreservingTextCheckingParagraph(TextCheckingParagraph& paragraph, PassRefPtr<Range> rangeToReplace, const String& replacement, int resultLocation, int resultLength)
     2156{
     2157    ContainerNode* scope = toContainerNode(highestAncestor(paragraph.paragraphRange()->startContainer()));
     2158
     2159    size_t paragraphLocation;
     2160    size_t paragraphLength;
     2161    TextIterator::getLocationAndLengthFromRange(scope, paragraph.paragraphRange().get(), paragraphLocation, paragraphLength);
     2162
     2163    applyCommand(SpellingCorrectionCommand::create(rangeToReplace, replacement));
     2164
     2165    // TextCheckingParagraph may be orphaned after SpellingCorrectionCommand mutated DOM.
     2166    // See <rdar://10305315>, http://webkit.org/b/89526.
     2167
     2168    RefPtr<Range> newParagraphRange = TextIterator::rangeFromLocationAndLength(scope, paragraphLocation, paragraphLength + replacement.length() - resultLength);
     2169
     2170    paragraph = TextCheckingParagraph(TextIterator::subrange(newParagraphRange.get(), resultLocation, replacement.length()), newParagraphRange);
     2171}
     2172
    21362173void Editor::markAndReplaceFor(PassRefPtr<SpellCheckRequest> request, const Vector<TextCheckingResult>& results)
    21372174{
    … …  
    21412178    TextCheckingParagraph paragraph(request->checkingRange(), request->paragraphRange());
    21422179
    2143     bool shouldMarkSpelling = textCheckingOptions & TextCheckingTypeSpelling;
    2144     bool shouldMarkGrammar = textCheckingOptions & TextCheckingTypeGrammar;
    2145     bool shouldMarkLink = textCheckingOptions & TextCheckingTypeLink;
    2146     bool shouldPerformReplacement = textCheckingOptions & TextCheckingTypeReplacement;
    2147     bool shouldShowCorrectionPanel = textCheckingOptions & TextCheckingTypeShowCorrectionPanel;
    2148     bool shouldCheckForCorrection = shouldShowCorrectionPanel || (textCheckingOptions & TextCheckingTypeCorrection);
     2180    const bool shouldMarkSpelling = textCheckingOptions & TextCheckingTypeSpelling;
     2181    const bool shouldMarkGrammar = textCheckingOptions & TextCheckingTypeGrammar;
     2182    const bool shouldMarkLink = textCheckingOptions & TextCheckingTypeLink;
     2183    const bool shouldPerformReplacement = textCheckingOptions & TextCheckingTypeReplacement;
     2184    const bool shouldShowCorrectionPanel = textCheckingOptions & TextCheckingTypeShowCorrectionPanel;
     2185    const bool shouldCheckForCorrection = shouldShowCorrectionPanel || (textCheckingOptions & TextCheckingTypeCorrection);
    21492186
    21502187    // Expand the range to encompass entire paragraphs, since text checking needs that much context.
    … …  
    21682205    }
    21692206
    2170     // If this checking is only for showing correction panel, we shouldn't bother to mark misspellings.
    2171     if (shouldShowCorrectionPanel)
    2172         shouldMarkSpelling = false;
    2173 
    21742207    int offsetDueToReplacement = 0;
    21752208
    21762209    for (unsigned i = 0; i < results.size(); i++) {
    2177         int spellingRangeEndOffset = paragraph.checkingEnd() + offsetDueToReplacement;
    2178         const TextCheckingResult* result = &results[i];
    2179         int resultLocation = result->location + offsetDueToReplacement;
    2180         int resultLength = result->length;
    2181         bool resultEndsAtAmbiguousBoundary = ambiguousBoundaryOffset >= 0 && resultLocation + resultLength == ambiguousBoundaryOffset;
     2210        const int spellingRangeEndOffset = paragraph.checkingEnd() + offsetDueToReplacement;
     2211        const TextCheckingType resultType = results[i].type;
     2212        const int resultLocation = results[i].location + offsetDueToReplacement;
     2213        const int resultLength = results[i].length;
     2214        const String& replacement = results[i].replacement;
     2215        const bool resultEndsAtAmbiguousBoundary = ambiguousBoundaryOffset >= 0 && resultLocation + resultLength == ambiguousBoundaryOffset;
    21822216
    21832217        // Only mark misspelling if:
    … …  
    21862220        // 3. The word in question doesn't end at an ambiguous boundary. For instance, we would not mark
    21872221        //    "wouldn'" as misspelled right after apostrophe is typed.
    2188         if (shouldMarkSpelling && result->type == TextCheckingTypeSpelling && resultLocation >= paragraph.checkingStart() && resultLocation + resultLength <= spellingRangeEndOffset && !resultEndsAtAmbiguousBoundary) {
     2222        if (shouldMarkSpelling && !shouldShowCorrectionPanel && resultType == TextCheckingTypeSpelling
     2223            && resultLocation >= paragraph.checkingStart() && resultLocation + resultLength <= spellingRangeEndOffset && !resultEndsAtAmbiguousBoundary) {
    21892224            ASSERT(resultLength > 0 && resultLocation >= 0);
    21902225            RefPtr<Range> misspellingRange = paragraph.subrange(resultLocation, resultLength);
    21912226            if (!m_alternativeTextController->isSpellingMarkerAllowed(misspellingRange))
    21922227                continue;
    2193             misspellingRange->startContainer()->document()->markers()->addMarker(misspellingRange.get(), DocumentMarker::Spelling, result->replacement);
    2194         } else if (shouldMarkGrammar && result->type == TextCheckingTypeGrammar && paragraph.checkingRangeCovers(resultLocation, resultLength)) {
     2228            misspellingRange->startContainer()->document()->markers()->addMarker(misspellingRange.get(), DocumentMarker::Spelling, replacement);
     2229        } else if (shouldMarkGrammar && resultType == TextCheckingTypeGrammar && paragraph.checkingRangeCovers(resultLocation, resultLength)) {
    21952230            ASSERT(resultLength > 0 && resultLocation >= 0);
    2196             for (unsigned j = 0; j < result->details.size(); j++) {
    2197                 const GrammarDetail* detail = &result->details[j];
    2198                 ASSERT(detail->length > 0 && detail->location >= 0);
    2199                 if (paragraph.checkingRangeCovers(resultLocation + detail->location, detail->length)) {
    2200                     RefPtr<Range> badGrammarRange = paragraph.subrange(resultLocation + detail->location, detail->length);
    2201                     badGrammarRange->startContainer()->document()->markers()->addMarker(badGrammarRange.get(), DocumentMarker::Grammar, detail->userDescription);
     2231            const Vector<GrammarDetail>& details = results[i].details;
     2232            for (unsigned j = 0; j < details.size(); j++) {
     2233                const GrammarDetail& detail = details[j];
     2234                ASSERT(detail.length > 0 && detail.location >= 0);
     2235                if (paragraph.checkingRangeCovers(resultLocation + detail.location, detail.length)) {
     2236                    RefPtr<Range> badGrammarRange = paragraph.subrange(resultLocation + detail.location, detail.length);
     2237                    badGrammarRange->startContainer()->document()->markers()->addMarker(badGrammarRange.get(), DocumentMarker::Grammar, detail.userDescription);
    22022238                }
    22032239            }
    22042240        } else if (resultLocation + resultLength <= spellingRangeEndOffset && resultLocation + resultLength >= paragraph.checkingStart()
    2205                     && (result->type == TextCheckingTypeLink
    2206                     || result->type == TextCheckingTypeQuote
    2207                     || result->type == TextCheckingTypeDash
    2208                     || result->type == TextCheckingTypeReplacement
    2209                     || result->type == TextCheckingTypeCorrection)) {
     2241            && isAutomaticTextReplacementType(resultType)) {
    22102242            // In this case the result range just has to touch the spelling range, so we can handle replacing non-word text such as punctuation.
    22112243            ASSERT(resultLength > 0 && resultLocation >= 0);
    22122244
    2213             if (shouldShowCorrectionPanel && (resultLocation + resultLength < spellingRangeEndOffset || result->type != TextCheckingTypeCorrection))
     2245            if (shouldShowCorrectionPanel && (resultLocation + resultLength < spellingRangeEndOffset || resultType != TextCheckingTypeCorrection))
    22142246                continue;
    2215 
    2216             int replacementLength = result->replacement.length();
    22172247
    22182248            // Apply replacement if:
    … …  
    22202250            // 2. The result doesn't end at an ambiguous boundary.
    22212251            //    (FIXME: this is required until 6853027 is fixed and text checking can do this for us
    2222             bool doReplacement = replacementLength > 0 && !resultEndsAtAmbiguousBoundary;
     2252            bool doReplacement = replacement.length() > 0 && !resultEndsAtAmbiguousBoundary;
    22232253            RefPtr<Range> rangeToReplace = paragraph.subrange(resultLocation, resultLength);
    2224             VisibleSelection selectionToReplace(rangeToReplace.get(), DOWNSTREAM);
    22252254
    22262255            // adding links should be done only immediately after they are typed
    22272256            int resultEnd = resultLocation + resultLength;
    2228             if (result->type == TextCheckingTypeLink
     2257            if (resultType == TextCheckingTypeLink
    22292258                && (selectionOffset > resultEnd + 1 || selectionOffset <= resultLocation))
    22302259                continue;
    … …  
    22342263
    22352264            String replacedString = plainText(rangeToReplace.get());
    2236             bool existingMarkersPermitReplacement = m_alternativeTextController->processMarkersOnTextToBeReplacedByResult(result, rangeToReplace.get(), replacedString);
     2265            const bool existingMarkersPermitReplacement = m_alternativeTextController->processMarkersOnTextToBeReplacedByResult(&results[i], rangeToReplace.get(), replacedString);
    22372266            if (!existingMarkersPermitReplacement)
    22382267                continue;
    … …  
    22452274                if (resultLocation + resultLength == spellingRangeEndOffset) {
    22462275                    // We only show the correction panel on the last word.
    2247                     m_alternativeTextController->show(rangeToReplace, result->replacement);
     2276                    m_alternativeTextController->show(rangeToReplace, replacement);
    22482277                    break;
    22492278                }
    … …  
    22522281            }
    22532282
     2283            VisibleSelection selectionToReplace(rangeToReplace.get(), DOWNSTREAM);
    22542284            if (selectionToReplace != m_frame->selection()->selection()) {
    22552285                if (!m_frame->selection()->shouldChangeSelection(selectionToReplace))
    … …  
    22572287            }
    22582288
    2259             if (result->type == TextCheckingTypeLink) {
     2289            if (resultType == TextCheckingTypeLink) {
    22602290                m_frame->selection()->setSelection(selectionToReplace);
    22612291                selectionChanged = true;
    22622292                restoreSelectionAfterChange = false;
    22632293                if (canEditRichly())
    2264                     applyCommand(CreateLinkCommand::create(m_frame->document(), result->replacement));
    2265             } else if (canEdit() && shouldInsertText(result->replacement, rangeToReplace.get(), EditorInsertActionTyped)) {
    2266                 Node* root = paragraph.paragraphRange()->startContainer();
    2267                 while (ContainerNode* parent = root->parentNode())
    2268                     root = parent;
    2269 
    2270                 int paragraphStartIndex = TextIterator::rangeLength(Range::create(m_frame->document(), root, 0, paragraph.paragraphRange()->startContainer(), paragraph.paragraphRange()->startOffset()).get());
    2271                 int paragraphLength = TextIterator::rangeLength(paragraph.paragraphRange().get());
    2272                 applyCommand(SpellingCorrectionCommand::create(rangeToReplace, result->replacement));
    2273                 // Recalculate newParagraphRange, since SpellingCorrectionCommand modifies the DOM, such that the original paragraph range is no longer valid. Radar: 10305315 Bugzilla: 89526
    2274                 RefPtr<Range> newParagraphRange = TextIterator::rangeFromLocationAndLength(toContainerNode(root), paragraphStartIndex, paragraphLength + replacementLength - resultLength);
    2275                 paragraph = TextCheckingParagraph(TextIterator::subrange(newParagraphRange.get(), resultLocation, replacementLength), newParagraphRange);
    2276                
     2294                    applyCommand(CreateLinkCommand::create(m_frame->document(), replacement));
     2295            } else if (canEdit() && shouldInsertText(replacement, rangeToReplace.get(), EditorInsertActionTyped)) {
     2296                correctSpellcheckingPreservingTextCheckingParagraph(paragraph, rangeToReplace, replacement, resultLocation, resultLength);
     2297
    22772298                if (AXObjectCache* cache = m_frame->document()->existingAXObjectCache()) {
    22782299                    if (Element* root = m_frame->selection()->selection().rootEditableElement())
    … …  
    22812302
    22822303                selectionChanged = true;
    2283                 offsetDueToReplacement += replacementLength - resultLength;
     2304                offsetDueToReplacement += replacement.length() - resultLength;
    22842305                if (resultLocation < selectionOffset) {
    2285                     selectionOffset += replacementLength - resultLength;
     2306                    selectionOffset += replacement.length() - resultLength;
    22862307                    if (ambiguousBoundaryOffset >= 0)
    22872308                        ambiguousBoundaryOffset = selectionOffset - 1;
    … …  
    22892310
    22902311                // Add a marker so that corrections can easily be undone and won't be re-corrected.
    2291                 if (result->type == TextCheckingTypeCorrection)
    2292                     m_alternativeTextController->markCorrection(paragraph.subrange(resultLocation, replacementLength), replacedString);
     2312                if (resultType == TextCheckingTypeCorrection)
     2313                    m_alternativeTextController->markCorrection(paragraph.subrange(resultLocation, replacement.length()), replacedString);
    22932314            }
    22942315        }
Note: See TracChangeset for help on using the changeset viewer.