225749 - Clean-up in spellchecker code

Status: RESOLVED FIXED

(Core :: Spelling checker, defect)

Description

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

I came across a lot of code in spellchecker that doesn't follow mozilla coding
guidelines, or is just 'not nice' in general.

I'm planning to put some patches in this bug to clean up random places.

This is really just clean up, no change of functionality or anything.

Comment 1

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

Attachment: make nsIEditorSpellcheck use nsAString (Checked in)

nsIEditorSpellcheck uses const &nsString, which makes it hard to pass a
substring for example.

Comment 2

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

Attachment: make reading files use nsILineInputStream

No need for code duplication :)

Comment 3

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

Comment on attachment 135663 [details] [diff] [review]
make reading files use nsILineInputStream

Come to think of it, this is wrong.
the file we are reading isn't utf8 or utf16, but something else, like
ISO-8859-1

And it should be stored in memory in the same encoding (not ascii as i do here)

Comment 4

[[e-mail]]

Comment on attachment 135508 [details] [diff] [review]
make nsIEditorSpellcheck use nsAString (Checked in)

>Index: editor/composer/src/nsEditorSpellCheck.cpp
>===================================================================
> nsEditorSpellCheck::CheckCurrentWord(const PRUnichar *aSuggestedWord,
>+                                     PRBool *aIsMisspelled)
> {
>   nsAutoString suggestedWord(aSuggestedWord);
>   DeleteSuggestedWordList();
>+  return mSpellChecker->CheckWord(nsDependentString(aSuggestedWord),
>+                                  aIsMisspelled, &mSuggestedWordList);

remove the |nsAutoString suggestedWord|.

> nsEditorSpellCheck::ReplaceWord(const PRUnichar *aMisspelledWord,
>+                                const PRUnichar *aReplaceWord,
>+                                PRBool           allOccurrences)
> {
>-  nsAutoString misspelledWord(aMisspelledWord);
>   nsAutoString replaceWord(aReplaceWord);
>+  return mSpellChecker->Replace(nsDependentString(aMisspelledWord),
>+                                nsDependentString(aReplaceWord), allOccurrences);

here too

>Index: extensions/spellcheck/src/mozSpellChecker.cpp
>===================================================================
> NS_IMETHODIMP 
>+mozSpellChecker::CheckWord(const nsAString &aWord, PRBool *aIsMisspelled, nsStringArray *aSuggestions)
> {
>       PRUnichar **words;
>       nsAutoString temp;
>       
>+      mSpellCheckingEngine->Suggest(PromiseFlatString(aWord).get(), &words, &count);
>       for(i=0;i<count;i++){
>         temp.Assign(words[i]);
>         aSuggestions->AppendString(temp);

remove the |nsAutoString temp|, and just do this:

aSuggestions->AppendString(nsDependentString(words[i]));

> NS_IMETHODIMP 
>+mozSpellChecker::GetCurrentDictionary(nsAString &aDictionary)
> {
>   nsXPIDLString dictname;
>   nsresult res;
>   res=mSpellCheckingEngine->GetDictionary(getter_Copies(dictname));
>   if(NS_SUCCEEDED(res))
>+    aDictionary = dictname;

remove the |if(NS_SUCCEEDED(res))|, it's unnecessary. (we should probably
propagate the failure code too, but *shrug*)

>Index: extensions/spellcheck/src/mozSpellChecker.h
>===================================================================

sigh, when are we going to have an idl instead of redeclaring all this crap? ;)

r=dwitte with nits fixed.

Comment 5

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

Comment on attachment 135508 [details] [diff] [review]
make nsIEditorSpellcheck use nsAString (Checked in)

dwitte: thanks for the review, i will fix those nits.
alecf, if you sr, consider those nits fixed :)

Comment 6

[Alec Flett]

Comment on attachment 135508 [details] [diff] [review]
make nsIEditorSpellcheck use nsAString (Checked in)

don't forget to rev the IID for nsISpellChecker and any other interfaces you
change!

sr=alecf as long as you rev the IID.

Comment 7

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

Attachment: fix comments in idl files (Checked in)

This patch fixes the comments in the .idl files to use javadoc style, and the
someMethod style for methods (instead of SomeMethod).

I will rev the IID's when checking in, but not now, as I am planning to change
the interfaces some more. (does the capitalization change require a IID change
anyway?)

Comment 8

[Alec Flett]

Comment on attachment 136114 [details] [diff] [review]
fix comments in idl files (Checked in)

sr=alecf assuming we've made sure there's no JS that needs to be updated!
sr=alecf

Comment 9

[Oliver Klee]

This might have caused bug 229215.

Comment 10

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

I'm not planning any other cleanups