Bug 48287 – Refactoring: Spellchecking related static functions could form a class

• Component: HTML Editing
• Status: RESOLVED FIXED

[Hajime Morrita]

In Editor.cpp, there are many static functions for spellchecking. They can form a class, namely RangeMarkerController.
This change will extract that class.

This is a preparation for Bug 41423. I'll start the bugfix after this cleanup.

[Hajime Morrita]

Created attachment 71864 [details]
Patch

[Hajime Morrita]

Created attachment 71865 [details]
Patch

[Hajime Morrita]

For safety, this change is rather mechanical: Just moving functions to the method of the class.

But this should be just a starting point. I'd like to do more cleanup after this patch land:
extracting similar code between functions to new method, move some parameter to instance variables, 
etc.

[Early Warning System Bot]

Attachment 71865 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4745019

[Eric Seidel (no email)]

Attachment 71865 [details] did not build on mac:
Build output: http://queues.webkit.org/results/4836017

[Eric Seidel (no email)]

Attachment 71865 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4747020

[Jia Pu]

(In reply to comment #0)
> In Editor.cpp, there are many static functions for spellchecking. They can form a class, namely RangeMarkerController.

This class seems do much more than just marker management like DocumentMarkerController does. Maybe we should consider a name that hints more at grammar and spelling checking.

[WebKit Review Bot]

Attachment 71865 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4768019

[Hajime Morrita]

Created attachment 71978 [details]
Patch

[Hajime Morrita]

Hi Jia, thank you for your feedback!

> > In Editor.cpp, there are many static functions for spellchecking. They can form a class, namely RangeMarkerController.
> 
> This class seems do much more than just marker management like DocumentMarkerController does. Maybe we should consider a name that hints more at grammar and spelling checking.

This makes sense. I renamed it to TextCheckingHelper in updated patch.

[Ryosuke Niwa]

Comment on attachment 71978 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=71978&action=review

> WebCore/editing/TextCheckingHelper.cpp:438
> +Vector<String> TextCheckingHelper::guessesForMisspelledOrUngrammaticalRange(bool checkGrammar, bool& misspelled, bool& ungrammatical) const
> +{

Will it be possible to split this function into two one that checks grammar and another that doesn't check grammar?

[Kent Tamura]

Comment on attachment 71978 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=71978&action=review

> WebCore/WebCore.xcodeproj/project.pbxproj:-21238
> -			developmentRegion = English;

Do not delete this line.  You need the latest Xcode to avoid the deletion.

> WebCore/editing/Editor.cpp:1661
> +    TextCheckingHelper checking(client(), spellingSearchRange);

The name "checking" looks curious to me.  Should it be "checkingHelper" or "checker"?

> WebCore/editing/Editor.cpp:2092
> +    TextCheckingHelper checking(client(), searchRange);

ditto.

> WebCore/editing/Editor.cpp:2177
> +    TextCheckingHelper checking(client(), grammarRange);

ditto.

>> WebCore/editing/TextCheckingHelper.cpp:438
>> +{
> 
> Will it be possible to split this function into two one that checks grammar and another that doesn't check grammar?

I think it will be done in the next step.

> WebCore/editing/TextCheckingHelper.h:38
> +    int findFirstGrammarDetail(const Vector<GrammarDetail>& grammarDetails, int badGrammarPhraseLocation, int /*badGrammarPhraseLength*/, int startOffset, int endOffset, bool markAll);

No need to comment out the argument name.

[Hajime Morrita]

Created attachment 72175 [details]
Patch

[Hajime Morrita]

Niwa-san, Kent-san, thank you for taking a look!

(In reply to comment #12)
> (From update of attachment 71978 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=71978&action=review
> 
> > WebCore/WebCore.xcodeproj/project.pbxproj:-21238
> > -			developmentRegion = English;
> 
> Do not delete this line.  You need the latest Xcode to avoid the deletion.
Recovered. And installed latest XCode...

> 
> > WebCore/editing/Editor.cpp:1661
> > +    TextCheckingHelper checking(client(), spellingSearchRange);
> 
> The name "checking" looks curious to me.  Should it be "checkingHelper" or "checker"?
> 
Agreed, renamed to checker

> >> WebCore/editing/TextCheckingHelper.cpp:438
> >> +{
> > 
> > Will it be possible to split this function into two one that checks grammar and another that doesn't check grammar?
> 
> I think it will be done in the next step.
+1 for Kent-san, there is a good number of refactoring ideas around.

> 
> > WebCore/editing/TextCheckingHelper.h:38
> > +    int findFirstGrammarDetail(const Vector<GrammarDetail>& grammarDetails, int badGrammarPhraseLocation, int /*badGrammarPhraseLength*/, int startOffset, int endOffset, bool markAll);
> 
> No need to comment out the argument name.
Removed the comment.

[Ryosuke Niwa]

(In reply to comment #14)
> > >> WebCore/editing/TextCheckingHelper.cpp:438
> > >> +{
> > > 
> > > Will it be possible to split this function into two one that checks grammar and another that doesn't check grammar?
> > 
> > I think it will be done in the next step.
> +1 for Kent-san, there is a good number of refactoring ideas around.

Ok. SGTM.

[Kent Tamura]

Comment on attachment 72175 [details]
Patch

ok

[Hajime Morrita]

Committed r70847: <http://trac.webkit.org/changeset/70847>

[Mark Rowe (bdash)]

This has introduced crashes in tip of tree that appear to occur whenever typing in a text field.  See bug 48708.

[Mark Rowe (bdash)]

I rolled this out in r70970.

[Hajime Morrita]

> I rolled this out in r70970.
Thanks you for taking care and I'm sorry for my absence...

[Hajime Morrita]

Created attachment 72485 [details]
Patch

[Hajime Morrita]

The original patch misplaced some parameters when replacing wit TextSpellingHelper
(on Editor::advanceToNextMisspelling()) and the latest patch fixed it.

[Hajime Morrita]

The original patch misplaced some parameters when replacing wit TextSpellingHelper
(on Editor::advanceToNextMisspelling()) and the latest patch fixed it.

Unfortunately, the crash isn't reproduceable on current release of Mac OS X because
it only happens in conjunction with "autocorrection panel" feature.
It might be OK just to re-land and watch what happens.
But a quick check by Apple folks for Bug 48708 would be preferable, of course.

[Hajime Morrita]

Created attachment 72486 [details]
Added an AllInOne entry for windows build.

[Kent Tamura]

Comment on attachment 72486 [details]
Added an AllInOne entry for windows build.

ok, please check if this patch works manually :-)

[Hajime Morrita]

Committed r71009: <http://trac.webkit.org/changeset/71009>

[Jia Pu]

> Unfortunately, the crash isn't reproduceable on current release of Mac OS X because
> it only happens in conjunction with "autocorrection panel" feature.
> It might be OK just to re-land and watch what happens.
> But a quick check by Apple folks for Bug 48708 would be preferable, of course.

I got undefined symbols when trying to build r71009 on the Mac OS X with autocorrection panel feature.

Undefined symbols:
  "__ZN7WebCore12SpellChecker18requestCheckingForEPNS_4NodeE", referenced from:
      __ZN7WebCore6Editor28replaceSelectionWithFragmentEN3WTF10PassRefPtrINS_16DocumentFragmentEEEbbb in Editor.o
  "__ZNK7WebCore12SpellChecker22canCheckAsynchronouslyEPNS_4NodeE", referenced from:
      __ZN7WebCore6Editor28replaceSelectionWithFragmentEN3WTF10PassRefPtrINS_16DocumentFragmentEEEbbb in Editor.o
  "__ZN7WebCore12SpellCheckerC1EPNS_12EditorClientE", referenced from:
      __ZN7WebCore6EditorC2EPNS_5FrameE in Editor.o
  "__ZN7WebCore12SpellChecker8didCheckEiRKN3WTF6VectorINS_19SpellCheckingResultELm0EEE", referenced from:
     -exported_symbol[s_list] command line option
  "__ZN7WebCore12SpellCheckerD1Ev", referenced from:
      __ZN7WebCore6EditorD2Ev in Editor.o
ld: symbol(s) not found
collect2: ld returned 1 exit status

[Hajime Morrita]

Hi Jia, thank you for trying this.
> I got undefined symbols when trying to build r71009 on the Mac OS X with autocorrection panel feature.
I suspect that you have a patch on Bug 40092 applied, which introduces WebCore::SpellChecker class.