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.
Created attachment 71864 [details] Patch
Created attachment 71865 [details] Patch
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.
Attachment 71865 [details] did not build on qt: Build output: http://queues.webkit.org/results/4745019
Attachment 71865 [details] did not build on mac: Build output: http://queues.webkit.org/results/4836017
Attachment 71865 [details] did not build on chromium: Build output: http://queues.webkit.org/results/4747020
(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.
Attachment 71865 [details] did not build on chromium: Build output: http://queues.webkit.org/results/4768019
Created attachment 71978 [details] Patch
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.
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?
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.
Created attachment 72175 [details] Patch
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.
(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.
Comment on attachment 72175 [details] Patch ok
Committed r70847: <http://trac.webkit.org/changeset/70847>
This has introduced crashes in tip of tree that appear to occur whenever typing in a text field. See bug 48708.
I rolled this out in r70970.
> I rolled this out in r70970. Thanks you for taking care and I'm sorry for my absence...
Created attachment 72485 [details] Patch
The original patch misplaced some parameters when replacing wit TextSpellingHelper (on Editor::advanceToNextMisspelling()) and the latest patch fixed it.
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.
Created attachment 72486 [details] Added an AllInOne entry for windows build.
Comment on attachment 72486 [details] Added an AllInOne entry for windows build. ok, please check if this patch works manually :-)
Committed r71009: <http://trac.webkit.org/changeset/71009>
> 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
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.