Closed
Bug 226756
Opened 21 years ago
Closed 21 years ago
[Spellcheck] Move unicode to charset convertors out of spellcheck glue into myspell
Categories
(Core :: DOM: Editor, defect)
Core
DOM: Editor
Tracking
()
RESOLVED
FIXED
People
(Reporter: mvl, Assigned: mvl)
Details
Attachments
(1 file, 2 obsolete files)
56.17 KB,
patch
|
dwitte
:
review+
|
Details | Diff | Splinter Review |
Internally, myspell uses a language specific charset. That's ok, because it
saves memory. A dictionary will fit in that charset.
But it gives all the code that lives in /extensions/spellcheck/src the task of
converting what they have in unicode to that charset. So there are multiple
places where the conversion is done. As a result, for example the personal
dictionary has to remember the current charset.
Moving the conversion into myspell reduces the amount of code, because there is
only one place that has to de the conversion.
Assignee | ||
Comment 1•21 years ago
|
||
This patch does the move.
I also remove the language attribute from the personal dictionary, to make it
stateless.
Comment 2•21 years ago
|
||
mvl,
Do you want to own spellcheck?
Comment 3•21 years ago
|
||
In mozPersonalDictionary::AddWord and ::RemoveWord, is there any reason to
allocate the temp and set mDirty if !mUnicodeTree?
These spellcheck cleanups have been great. Are they going to be rolled into a
giant patch at some point?
Comment 4•21 years ago
|
||
why bother rolling a giant patch?
mvl will land them all when 1.7a opens.
Assignee | ||
Comment 5•21 years ago
|
||
Comment on attachment 136301 [details] [diff] [review]
patch v1
Forgot to mention that this patch is on top of the two patches in bug 225749.
If you want a patch to apply, please let me know.
Nick: the temp car is not needed, i will remove it. You are right about mDirty,
i will change it to be set if(mUnicodeTree) only.
Attachment #136301 -
Flags: superreview?(alecf)
Attachment #136301 -
Flags: review?(mkaply)
Comment 6•21 years ago
|
||
Can you explain more on the stateless personal dictionary? Will I be able to
have German and Japanese words in one personal dictionary?
Assignee | ||
Comment 7•21 years ago
|
||
With stateless i mean that is doesn't have to remember the current language,
beacuse it will be passed in with every relevant call.
I want this, because if it is a service, and you spellcheck two documents at the
same time, it will get confused when the two documents are different languages.
You could solve that by making using do_CreateInstance instead of do_GetService,
but that means the dictionary is in memory twice.
At the moment, the pers dict doesn't do anything with the language, so there
should be no noticable difference. So if you have german and japanese words in
your dictionary, it will ok japanese words while you check a german document.
But that should be in a different bug.
Comment 8•21 years ago
|
||
Comment on attachment 136301 [details] [diff] [review]
patch v1
sorry for the delay, I'll try to get to this by 5-Dec
Comment 9•21 years ago
|
||
Comment on attachment 136301 [details] [diff] [review]
patch v1
+NS_IMETHODIMP mozMySpell::Suggest(const PRUnichar *aWord, PRUnichar
***aSuggestions, PRUint32 *aSuggestionCount)
{
- nsAutoString word(aword);
+ nsAutoString word(aWord);
I don't see 'word' used anywhere.. it could be elsewhere in the patch, but it
looks like you're rewriting this whole function..
Also, I see some places that nsIStringEnumerator might be helpful..not
necessary, maybe in a future cleanup?
other than that stuff, this seems ok.
sr=alecf
Attachment #136301 -
Flags: superreview?(alecf) → superreview+
Comment 10•21 years ago
|
||
oh! and don't forget to rev the IID of the interfaces you're changing!
Assignee | ||
Updated•21 years ago
|
Attachment #136301 -
Flags: review?(mkaply) → review?(dwitte)
Comment 11•21 years ago
|
||
Comment on attachment 136301 [details] [diff] [review]
patch v1
>+++ spellcheck/myspell/src/myspAffixmgr.cpp 2003-11-25 17:20:24.000000000 +0100
>-PRBool myspAffixMgr::check(const nsAFlatCString &word)
>+PRBool myspAffixMgr::check(const nsAFlatString &word)
> {
>+ nsresult rv = mEncoder->GetMaxLength(word.get(), inLength, &outLength);
>+ if (NS_FAILED(rv)|| rv == NS_ERROR_UENC_NOMAPPING) {
isn't that NS_ERROR covered by NS_FAILED(rv)?
>+ rv = mPersonalDictionary->Check(word.get(), PromiseFlatString(mLanguage).get(), &good);
you don't need that PromiseFlatString.
>+++ spellcheck/myspell/src/myspAffixmgr.h 2003-11-25 17:19:09.000000000 +0100
>- PRBool check(const nsAFlatCString &word);
>+ PRBool check(const nsAFlatString &word);
> nsString get_encoding();
wait, i thought you removed get_encoding() ?
>+++ spellcheck/myspell/src/myspSuggestmgr.cpp 2003-11-24 19:53:23.000000000 +0100
>- wlst=(char **)nsMemory::Alloc(sizeof(char *)*maxSug);
>+ wlst=(PRUnichar **)nsMemory::Alloc(sizeof(char *)*maxSug);
i know it doesn't matter, but please change this to |sizeof(PRUnichar *)| just
for consistency.
> for(i=0;i<maxSug;i++)
> wlst[i]=nsnull;
uh, |memset(wlst, nsnull, sizeof(PRUnichar*) * maxSug);| ?
>+nsresult myspSuggestMgr::badchar(PRUnichar ** wlst,const nsAFlatString &word, PRUint32 *ns)
> {
> char tmpc;
>- nsSharableCString candidate;
>+ nsSharableString candidate;
> PRBool cwrd;
> PRUint32 i,j,k;
> PRUint32 wl = word.Length();
> candidate.Assign(word);
>- nsACString::iterator candIt;
>+ nsAString::iterator candIt;
> for (i=0,candidate.BeginWriting(candIt); i < wl; i++,candIt++) {
> tmpc = *candIt;
this is completely broken... you're assigning a PRUnichar into a char, and
generally trying to mix types. also using an nsSharableString for |candidate|
is pointless since you call this method with an nsDependentString which is
inherently not sharable. so you'll copy at the outset anyway. since you know
it's a flat string you can also use nsASingleFragmentString::char_iterator
instead, which is more efficient.
at least some of the above comments apply to the rest of the methods in that
file too.
>+++ spellcheck/src/mozEnglishWordUtils.cpp 2003-11-24 20:03:34.000000000 +0100
>+NS_IMETHODIMP mozEnglishWordUtils::FromRootForm(const PRUnichar *aWord, const PRUnichar **iwords, PRUint32 icount, PRUnichar ***owords, PRUint32 *ocount)
> {
>+ for(PRUint32 i = 0; i < icount; ++i) {
>+ length = nsCRT::strlen(iwords[i]);
>+ tmpPtr[i] = (PRUnichar *) nsMemory::Alloc(sizeof(PRUnichar *) * (length + 1));
i think you want |sizeof(PRUnichar)| here no?
>+++ spellcheck/src/mozPersonalDictionary.cpp 2003-11-25 20:09:28.000000000 +0100
>- * Copy the tree to a waiiting aray of Unichar pointers.
>+ * Copy the tree to a waiting aray of Unichar pointers.
nice, but you missed "aray" :)
r-, because of the incorrect string fu.
Attachment #136301 -
Flags: review?(dwitte) → review-
Assignee | ||
Comment 12•21 years ago
|
||
Updated to review comments.
(sorry, can't produce an interdiff, because of the wacky way the previous diff
was created)
Attachment #136301 -
Attachment is obsolete: true
Assignee | ||
Comment 13•21 years ago
|
||
Comment on attachment 137794 [details] [diff] [review]
patch v2
Carrying forward sr, requesting r.
Attachment #137794 -
Flags: superreview+
Attachment #137794 -
Flags: review?(dwitte)
Assignee | ||
Comment 14•21 years ago
|
||
Updated again. Now also fix the try_string, to be in unicode too.
Attachment #137794 -
Attachment is obsolete: true
Updated•21 years ago
|
Attachment #137794 -
Flags: review?(dwitte)
Comment 15•21 years ago
|
||
Comment on attachment 137820 [details] [diff] [review]
patch v3
ok, looks fine :)
r=me
Attachment #137820 -
Flags: review+
Assignee | ||
Comment 16•21 years ago
|
||
Checked in.
The usage of string enumerator will be saved for a new bug.
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Comment 17•21 years ago
|
||
This might have caused bug 229215.
You need to log in
before you can comment on or make changes to this bug.
Description
•