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

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Michiel van Leeuwen (email: mvl+moz@)]

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.

Comment 1

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: patch v1

This patch does the move.
I also remove the language attribute from the personal dictionary, to make it
stateless.

Comment 2

[Mike Kaply [:mkaply]]

mvl,

Do you want to own spellcheck?

Comment 3

[Nicholas Allen]

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

[[e-mail]]

why bother rolling a giant patch?

mvl will land them all when 1.7a opens.

Comment 5

[Michiel van Leeuwen (email: mvl+moz@)]

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.

Comment 6

[Mike Kaply [:mkaply]]

Can you explain more on the stateless personal dictionary? Will I be able to
have German and Japanese words in one personal dictionary?

Comment 7

[Michiel van Leeuwen (email: mvl+moz@)]

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

[Alec Flett]

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

[Alec Flett]

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

Comment 10

[Alec Flett]

oh! and don't forget to rev the IID of the interfaces you're changing!

Comment 11

[[e-mail]]

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.

Comment 12

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: patch v2

Updated to review comments.
(sorry, can't produce an interdiff, because of the wacky way the previous diff
was created)

Comment 13

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 137794 [details] [diff] [review]
patch v2

Carrying forward sr, requesting r.

Comment 14

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: patch v3

Updated again. Now also fix the try_string, to be in unicode too.

Comment 15

[[e-mail]]

Comment on attachment 137820 [details] [diff] [review]
patch v3

ok, looks fine :)

r=me

Comment 16

[Michiel van Leeuwen (email: mvl+moz@)]

Checked in.
The usage of string enumerator will be saved for a new bug.

Comment 17

[Oliver Klee]

This might have caused bug 229215.