[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 #35 from MORITA Hajime <morrita at google.com>  2011-03-24 22:56:54 PST ---
Hi Ryosuke, Jia, thank you for taking care of this.
I update the patch for address your feedback.

(The style error is a false positive. it mis-recognized a macro to a function.)

(In reply to comment #29)
> (From update of attachment 86423 [details])
> 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.
Fixed.

> 
> > Source/WebCore/editing/Editor.cpp:1056
> > +    , m_spellingCorrector(new SpellingCorrectionController(frame))
> 
> We should call adoptPtr here.
Fixed.

> 
> > 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.
Changed to early-exist style.

> 
> > 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.
Moved comment outside the if block

> 
> > Source/WebCore/editing/SpellingCorrectionController.cpp:105
> > +    if (currentSelection != oldSelection) {
> 
> Ditto about early exit.
Fixed.

> 
> > Source/WebCore/editing/SpellingCorrectionController.cpp:135
> > +    return !(m_frame->document()->markers()->hasMarkers(misspellingRange.get(), DocumentMarker::SpellCheckingExemption));
> 
> I don't think you need parenthesis before !
Removed the parenthesis.

> 
> > 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.
Made some variable names shorter, as Jia suggested.

> 
> > 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);
> 
Fixed.

> Also, I'm surprised to learn that addMarker takes String instead of const String&.  It's probably a bug.
This fix will be done by my upcoming DocumentMarkerController refactoring.

> 
> > Source/WebCore/editing/SpellingCorrectionController.cpp:313
> > +        } else {
> > +            if (!m_correctionPanelIsDismissedByEditor)
> 
> It seems like this should just be a single "else if".
Fixed.

> 
> > 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?
Divided the line.

> 
> > Source/WebCore/editing/SpellingCorrectionController.cpp:383
> > +void SpellingCorrectionController::respondToAppliedEditing(PassRefPtr<EditCommand> cmd)
> 
> Please spell out "command".
Done.

> 
> > 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.
Tried to put this inline. 
Please see SpellingCorrectionController.h.
Although it might not be a typical webkit way, I think it's worth 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.