RESOLVED FIXED 48287
Refactoring: Spellchecking related static functions could form a class
https://bugs.webkit.org/show_bug.cgi?id=48287
Summary Refactoring: Spellchecking related static functions could form a class
Hajime Morrita
Reported 2010-10-25 21:40:13 PDT
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.
Attachments
Patch (72.46 KB, patch)
2010-10-26 04:16 PDT, Hajime Morrita
no flags
Patch (72.61 KB, patch)
2010-10-26 04:18 PDT, Hajime Morrita
no flags
Patch (72.21 KB, patch)
2010-10-26 21:25 PDT, Hajime Morrita
no flags
Patch (71.90 KB, patch)
2010-10-28 05:52 PDT, Hajime Morrita
no flags
Patch (72.02 KB, patch)
2010-10-31 23:06 PDT, Hajime Morrita
no flags
Added an AllInOne entry for windows build. (72.58 KB, patch)
2010-10-31 23:22 PDT, Hajime Morrita
tkent: review+
Hajime Morrita
Comment 1 2010-10-26 04:16:21 PDT
Hajime Morrita
Comment 2 2010-10-26 04:18:17 PDT
Hajime Morrita
Comment 3 2010-10-26 04:21:33 PDT
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
Comment 4 2010-10-26 04:36:07 PDT
Eric Seidel (no email)
Comment 5 2010-10-26 06:17:09 PDT
Eric Seidel (no email)
Comment 6 2010-10-26 06:21:00 PDT
Jia Pu
Comment 7 2010-10-26 09:24:54 PDT
(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
Comment 8 2010-10-26 10:36:55 PDT
Hajime Morrita
Comment 9 2010-10-26 21:25:46 PDT
Hajime Morrita
Comment 10 2010-10-26 21:29:05 PDT
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 11 2010-10-26 21:38:23 PDT
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 12 2010-10-28 03:14:58 PDT
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
Comment 13 2010-10-28 05:52:08 PDT
Hajime Morrita
Comment 14 2010-10-28 05:54:49 PDT
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
Comment 15 2010-10-28 13:46:29 PDT
(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 16 2010-10-28 23:34:49 PDT
Comment on attachment 72175 [details] Patch ok
Hajime Morrita
Comment 17 2010-10-28 23:59:07 PDT
Mark Rowe (bdash)
Comment 18 2010-10-29 20:07:25 PDT
This has introduced crashes in tip of tree that appear to occur whenever typing in a text field. See bug 48708.
Mark Rowe (bdash)
Comment 19 2010-10-29 20:42:57 PDT
I rolled this out in r70970.
Hajime Morrita
Comment 20 2010-10-31 18:39:11 PDT
> I rolled this out in r70970. Thanks you for taking care and I'm sorry for my absence...
Hajime Morrita
Comment 21 2010-10-31 23:06:36 PDT
Hajime Morrita
Comment 22 2010-10-31 23:11:09 PDT
The original patch misplaced some parameters when replacing wit TextSpellingHelper (on Editor::advanceToNextMisspelling()) and the latest patch fixed it.
Hajime Morrita
Comment 23 2010-10-31 23:17:27 PDT
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
Comment 24 2010-10-31 23:22:04 PDT
Created attachment 72486 [details] Added an AllInOne entry for windows build.
Kent Tamura
Comment 25 2010-10-31 23:29:59 PDT
Comment on attachment 72486 [details] Added an AllInOne entry for windows build. ok, please check if this patch works manually :-)
Hajime Morrita
Comment 26 2010-10-31 23:36:18 PDT
Jia Pu
Comment 27 2010-11-01 10:25:45 PDT
> 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
Comment 28 2010-11-01 18:54:51 PDT
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.
Note You need to log in before you can comment on or make changes to this bug.