If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Clean-up in spellchecker code

RESOLVED FIXED

Status

()

Core
Spelling checker
--
minor
RESOLVED FIXED
14 years ago
13 years ago

People

(Reporter: Michiel van Leeuwen (email: mvl+moz@), Assigned: Michiel van Leeuwen (email: mvl+moz@))

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

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.
(Assignee)

Comment 1

14 years ago
Created attachment 135508 [details] [diff] [review]
make nsIEditorSpellcheck use nsAString (Checked in)

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

Updated

14 years ago
Attachment #135508 - Flags: superreview?(kinmoz)
Attachment #135508 - Flags: review?(dwitte)
(Assignee)

Comment 2

14 years ago
Created attachment 135663 [details] [diff] [review]
make reading files use nsILineInputStream

No need for code duplication :)
(Assignee)

Comment 3

14 years ago
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)
Attachment #135663 - Attachment is obsolete: true

Comment 4

14 years ago
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.
Attachment #135508 - Flags: review?(dwitte) → review+
(Assignee)

Updated

14 years ago
Attachment #135508 - Flags: superreview?(kinmoz) → superreview?(alecf)
(Assignee)

Comment 5

14 years ago
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

14 years ago
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.
Attachment #135508 - Flags: superreview?(alecf) → superreview+
(Assignee)

Comment 7

14 years ago
Created attachment 136114 [details] [diff] [review]
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?)
(Assignee)

Updated

14 years ago
Attachment #136114 - Flags: superreview?(alecf)
Attachment #136114 - Flags: review?(timeless)

Comment 8

14 years ago
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
Attachment #136114 - Flags: superreview?(alecf) → superreview+
(Assignee)

Updated

14 years ago
Attachment #135508 - Attachment description: make nsIEditorSpellcheck use nsAString → make nsIEditorSpellcheck use nsAString (Checked in)

Comment 9

14 years ago
This might have caused bug 229215.

Updated

14 years ago
Attachment #136114 - Flags: review?(timeless) → review+
(Assignee)

Updated

14 years ago
Attachment #136114 - Attachment description: fix comments in idl files → fix comments in idl files (Checked in)

Updated

14 years ago
Component: Editor: Core → Spelling checker
QA Contact: bugzilla → core.spelling-checker
(Assignee)

Comment 10

13 years ago
I'm not planning any other cleanups
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.