[Webkit-unassigned] [Bug 55571] [Refactoring] Auto correction panel should be handled by its own class.

[bugzilla-daemon at webkit.org]

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





--- Comment #29 from Ryosuke Niwa <rniwa at webkit.org>  2011-03-23 01:58:25 PST ---
(From update of attachment 86423)
View in context: https://bugs.webkit.org/attachment.cgi?id=86423&action=review

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:25352
> +				A7005CCC1330C4BA000CC0BA /* SpellingCorrectionController.cpp in Sources */,

This one is out of order.

> Source/WebCore/editing/Editor.cpp:1056
> +    , m_spellingCorrector(new SpellingCorrectionController(frame))

We should call adoptPtr here.

> Source/WebCore/editing/Editor.cpp:2417
> +    if (!m_spellingCorrector->isAutomaticSpellingCorrectionEnabled())
>          return;

Why are you adding this line to removeSpellAndCorrectionMarkersFromWordsToBeEdited/updateMarkersForWordsAffectedByEditing?

> Source/WebCore/editing/SpellingCorrectionController.cpp:86
> +    if (isAutomaticSpellingCorrectionEnabled()) {

I know you're just copying but I'd prefer an early exist over a nested if.

> Source/WebCore/editing/SpellingCorrectionController.cpp:89
> +        if (type == CorrectionPanelInfo::PanelTypeCorrection)
> +            // If type is PanelTypeReversion, then the new range has been set. So we shouldn't clear it.
> +            m_correctionPanelInfo.rangeToBeReplaced.clear();

Need curly brackets here since comment + statement occupies 2 lines.

> Source/WebCore/editing/SpellingCorrectionController.cpp:105
> +    if (currentSelection != oldSelection) {

Ditto about early exit.

> Source/WebCore/editing/SpellingCorrectionController.cpp:135
> +    return !(m_frame->document()->markers()->hasMarkers(misspellingRange.get(), DocumentMarker::SpellCheckingExemption));

I don't think you need parenthesis before !

> Source/WebCore/editing/SpellingCorrectionController.cpp:186
> +    RefPtr<Range> correctionStartOffsetInParagraphAsRange = Range::create(paragraphRangeContainingCorrection->startContainer(ec)->document(), paragraphRangeContainingCorrection->startPosition(), paragraphRangeContainingCorrection->startPosition());

Wow! this is a really long statement.  Maybe the variable name is too verbose.

> Source/WebCore/editing/SpellingCorrectionController.cpp:215
> +        if (m_correctionPanelInfo.panelType == CorrectionPanelInfo::PanelTypeReversion)
> +            markers->addMarker(replacementRange.get(), markerTypesToAdd[i]);
> +        else
> +            markers->addMarker(replacementRange.get(), markerTypesToAdd[i], m_correctionPanelInfo.replacedString);

I know you're just copying but it seems silly to check this condition in each iteration.  We should just do:

String description;
if (m_correctionPanelInfo.panelType == CorrectionPanelInfo::PanelTypeReversion)
    description = m_correctionPanelInfo.replacedString;
for (size_t i = 0; i < size; ++i)
    markers->addMarker(replacementRange.get(), markerTypesToAdd[i], description);

Also, I'm surprised to learn that addMarker takes String instead of const String&.  It's probably a bug.

> Source/WebCore/editing/SpellingCorrectionController.cpp:313
> +        } else {
> +            if (!m_correctionPanelIsDismissedByEditor)

It seems like this should just be a single "else if".

> Source/WebCore/editing/SpellingCorrectionController.cpp:365
> +        if (((marker.type == DocumentMarker::Replacement && !marker.description.isNull()) || marker.type == DocumentMarker::Spelling) && static_cast<int>(marker.endOffset) == endOffset) {

This condition seems too long to fit in one line. Maybe split it into two lines or extract an inline function?

> Source/WebCore/editing/SpellingCorrectionController.cpp:383
> +void SpellingCorrectionController::respondToAppliedEditing(PassRefPtr<EditCommand> cmd)

Please spell out "command".

> Source/WebCore/editing/SpellingCorrectionController.cpp:454
> +SpellingCorrectionController::SpellingCorrectionController(Frame*) {}
> +SpellingCorrectionController::~SpellingCorrectionController() {}
> +void SpellingCorrectionController::startCorrectionPanelTimer(CorrectionPanelInfo::PanelType) {}
> +void SpellingCorrectionController::stopCorrectionPanelTimer() {}
> +void SpellingCorrectionController::dismiss(ReasonForDismissingCorrectionPanel) {}
> +void SpellingCorrectionController::show(PassRefPtr<Range>, const String&) {}
> +void SpellingCorrectionController::applyCorrectionPanelInfo(const Vector<DocumentMarker::MarkerType>&) {}
> +bool SpellingCorrectionController::applyAutocorrectionBeforeTypingIfAppropriate() { return false; }
> +void SpellingCorrectionController::respondToUnappliedSpellCorrection(const VisibleSelection&, const String&, const String&) {}
> +void SpellingCorrectionController::respondToAppliedEditing(PassRefPtr<EditCommand>) {}
> +void SpellingCorrectionController::respondToChangedSelection(const VisibleSelection&) {}
> +void SpellingCorrectionController::stopPendingCorrection(const VisibleSelection&) {}
> +void SpellingCorrectionController::applyPendingCorrection(const VisibleSelection&) {}
> +void SpellingCorrectionController::handleCorrectionPanelResult(const String&) {}
> +void SpellingCorrectionController::handleCancelOperation() {}
> +void SpellingCorrectionController::markRevsersed(PassRefPtr<Range>) {}
> +void SpellingCorrectionController::recordAutocorrectionResponseReversed(const String&, PassRefPtr<Range>) {}
> +bool SpellingCorrectionController::hasPendingCorrection() const { return false; }
> +bool SpellingCorrectionController::isSpellingMarkerAllowed(PassRefPtr<Range>) const { return true; }
> +bool SpellingCorrectionController::isShowingCorrectionPanel() { return false; }
> +bool SpellingCorrectionController::isAutomaticSpellingCorrectionEnabled() { return false; }

I still think we should put this in the header to take the advantage of optimizing compilers.

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