WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(72.61 KB, patch)
2010-10-26 04:18 PDT
,
Hajime Morrita
no flags
Details
Formatted Diff
Diff
Patch
(72.21 KB, patch)
2010-10-26 21:25 PDT
,
Hajime Morrita
no flags
Details
Formatted Diff
Diff
Patch
(71.90 KB, patch)
2010-10-28 05:52 PDT
,
Hajime Morrita
no flags
Details
Formatted Diff
Diff
Patch
(72.02 KB, patch)
2010-10-31 23:06 PDT
,
Hajime Morrita
no flags
Details
Formatted Diff
Diff
Added an AllInOne entry for windows build.
(72.58 KB, patch)
2010-10-31 23:22 PDT
,
Hajime Morrita
tkent
: review+
Details
Formatted Diff
Diff
Show Obsolete
(5)
View All
Add attachment
proposed patch, testcase, etc.
Hajime Morrita
Comment 1
2010-10-26 04:16:21 PDT
Created
attachment 71864
[details]
Patch
Hajime Morrita
Comment 2
2010-10-26 04:18:17 PDT
Created
attachment 71865
[details]
Patch
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
Attachment 71865
[details]
did not build on qt: Build output:
http://queues.webkit.org/results/4745019
Eric Seidel (no email)
Comment 5
2010-10-26 06:17:09 PDT
Attachment 71865
[details]
did not build on mac: Build output:
http://queues.webkit.org/results/4836017
Eric Seidel (no email)
Comment 6
2010-10-26 06:21:00 PDT
Attachment 71865
[details]
did not build on chromium: Build output:
http://queues.webkit.org/results/4747020
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
Attachment 71865
[details]
did not build on chromium: Build output:
http://queues.webkit.org/results/4768019
Hajime Morrita
Comment 9
2010-10-26 21:25:46 PDT
Created
attachment 71978
[details]
Patch
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
Created
attachment 72175
[details]
Patch
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
Committed
r70847
: <
http://trac.webkit.org/changeset/70847
>
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
Created
attachment 72485
[details]
Patch
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
Committed
r71009
: <
http://trac.webkit.org/changeset/71009
>
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.
Top of Page
Format For Printing
XML
Clone This Bug