Closed Bug 226756 Opened 21 years ago Closed 21 years ago

[Spellcheck] Move unicode to charset convertors out of spellcheck glue into myspell


(Core :: DOM: Editor, defect)

Not set





(Reporter: mvl, Assigned: mvl)



(1 file, 2 obsolete files)

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.
Attached patch patch v1 (obsolete) — Splinter Review
This patch does the move.
I also remove the language attribute from the personal dictionary, to make it

Do you want to own spellcheck?
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?
why bother rolling a giant patch?

mvl will land them all when 1.7a opens.
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)
Can you explain more on the stateless personal dictionary? Will I be able to
have German and Japanese words in one personal dictionary?
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 on attachment 136301 [details] [diff] [review]
patch v1

sorry for the delay, I'll try to get to this by 5-Dec
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. 
Attachment #136301 - Flags: superreview?(alecf) → superreview+
oh! and don't forget to rev the IID of the interfaces you're changing!
Attachment #136301 - Flags: review?(mkaply) → review?(dwitte)
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-
Attached patch patch v2 (obsolete) — Splinter Review
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
Comment on attachment 137794 [details] [diff] [review]
patch v2

Carrying forward sr, requesting r.
Attachment #137794 - Flags: superreview+
Attachment #137794 - Flags: review?(dwitte)
Attached patch patch v3Splinter Review
Updated again. Now also fix the try_string, to be in unicode too.
Attachment #137794 - Attachment is obsolete: true
Attachment #137794 - Flags: review?(dwitte)
Comment on attachment 137820 [details] [diff] [review]
patch v3

ok, looks fine :)

Attachment #137820 - Flags: review+
Checked in.
The usage of string enumerator will be saved for a new bug.
Closed: 21 years ago
Resolution: --- → FIXED
This might have caused bug 229215. 
You need to log in before you can comment on or make changes to this bug.