[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 #12 from MORITA Hajime <morrita at google.com>  2011-03-17 00:49:00 PST ---
Ryoske, Jia, 
Thank you for quick feedback! 
I updated the patch to address that.

(In reply to comment #8)
> (From update of attachment 85922 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=85922&action=review
> 
> > Source/WebCore/editing/CorrectionPanelController.cpp:81
> > +CorrectionPanelController::~CorrectionPanelController()
> > +{
> > +#if SUPPORT_AUTOCORRECTION_PANEL
> > +    dismissCorrectionPanel(ReasonForDismissingCorrectionPanelIgnored);
> > +#endif
> > +}
> 
> It might be better to have EmptyCorrectionPanelController so that we don't have to put if-endif in all of these functions.
Makes sense, On the other hand, introducing virtual controller interface doesn't look usual in WebCore.
So I change it to return NULL if auto-correction is not enabled
(see SpellingCorrectioController::create()). 
But I'm not sure which is the wright way. What do you think?

> > Source/WebCore/editing/Editor.cpp:-2298
> > -#if SUPPORT_AUTOCORRECTION_PANEL
> >      // If this checking is only for showing correction panel, we shouldn't bother to mark misspellings.
> >      if (shouldShowCorrectionPanel)
> >          shouldMarkSpelling = false;
> > -#endif
> 
> Why is it safe to remove this if-endif?
> 
shouldShowCorrectionPanel never goes true unless SUPPORT_AUTOCORRECTION_PANEL.

> > Source/WebCore/editing/Editor.cpp:-3521
> > -#if SUPPORT_AUTOCORRECTION_PANEL
> >          // Don't check spelling and grammar if the change of selection is triggered by spelling correction itself.
> >          shouldCheckSpellingAndGrammar = !(options & SelectionController::SpellCorrectionTriggered);
> > -#endif
> 
> Ditto.
SelectionController::SpellCorrectionTriggered is only passed from SpellingCorrectionControlller.
thus only available when SUPPORT_AUTOCORRECTION_PANEL is given.

----

(In reply to comment #9)
> (From update of attachment 85922 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=85922&action=review
> 
> > Source/WebCore/editing/CorrectionPanelController.h:72
> > +class CorrectionPanelController {
> 
> We probably should call it SpellingCorrectionController, since it does more than just managing the panel UI. For instance, it also monitor undone autocorrection, and manager markers accordingly.
> 
It makes sense. I renamed it.

> > Source/WebCore/editing/CorrectionPanelController.h:85
> > +    void unappliedSpellCorrection(const VisibleSelection& selectionOfCorrected, const String& corrected, const String& correction);
> > +    void appliedEditing(PassRefPtr<EditCommand>);
> 
> Shall we call them respondToUnappliedSpellCorrection() and respondToAppliedEditing()?
Sure. Renamed.

> 
> > Source/WebCore/editing/Editor.cpp:1061
> >  Editor::~Editor()
> 
> I suppose we could remove the empty destructor now.
We need this because we moved #include for the controller to Editor.cpp 
to save the compilation time. (The destructor need to be on Editor.cpp in this case.)

> 
> > Source/WebCore/editing/Editor.cpp:2490
> > -void Editor::applyCorrectionPanelInfo(const Vector<DocumentMarker::MarkerType>& markerTypesToAdd)
> > +void Editor::prepareEditingWord(bool doNotRemoveIfSelectionAtWordBoundary)
> 
> Given the new function name, the argument name isn't very clear anymore. The purpose of this function is to remove markers on the word we are about to edit and that we have just edited. The argument indicates whether we want to handle a word if the selection is on word boundary. Maybe we could name it updateMarkersForWordsAffectedByEditing(bool onlyHandleWordsContainingSelection). We could also remove removeSpellAndCorrectionMarkersFromWordsToBeEdited(), since all prepareEditingWord() does is to call removeSpellAndCorrectionMarkersFromWordsToBeEdited().
> 
Renamed and made the two functions into one.

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