Summary: | Refactoring: Spellchecking related static functions could form a class | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Hajime Morrita <morrita> | ||||||||||||||
Component: | HTML Editing | Assignee: | Hajime Morrita <morrita> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | darin, dglazkov, eric, jiapu.mail, mrowe, rniwa, webkit-ews, webkit.review.bot | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | PC | ||||||||||||||||
OS: | All | ||||||||||||||||
Attachments: |
|
Description
Hajime Morrita
2010-10-25 21:40:13 PDT
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 |