Thunderbird:CodeCleanup

In preparation for Thunderbird and Seamonkey moving to frozen linkage/libxul/XULRunner, we need to remove our use of nsXPIDLStrings from mailnews (along with other changes). While we are doing this, we're taking the opportunity to modernize some of our core interfaces to use AString and ACString instead of wstring and string.

In starting to make these changes, we've noticed inefficient code patterns in our C++ code in mailnews. A lot of of these patterns can be improved as part of the AString changes we are making to the interfaces. By documenting these patterns and the associated code improvements, it will hopefully be easier for more folks to help out in the great mailnews code cleanup of 2007!

How To Help

• Read the rest of this document.
• Look at some of the patches we are working on in Bug 379070 to get a feel for what we are changing and cleaning up.
• I recommend keeping the focus of the changes as narrow as possible to avoid too much fan out.
• Pick a single interface like something in mailnews\base\public\search. Ask in the bug or e-mail mscott@mozilla.org for help in picking an interface to cleanup. Audit the interface making AString/ACString changes where they are appropriate.
• Change the interface ID.
• Fix the implementation for that interface, adjusting the methods to account for the new signatures.
• As you audit the implementation of the interface, examine the code method by method, looking for and correcting the patterns described in this document.
• After that's done, start fixing up the C++ callers of the APIs that you've changed. If a caller is using a nsXPIDLString, be sure to change that to nsString.
• Post a patch in Bug 379070 for the interface & implementation you've cleaned up.
• If you have any questions, just ask us in the bug or in the newsgroup!

IDL Changes

Many of the mailnews interfaces use string and wstring for string arguments. In many cases, we can convert these to AString and ACString. This change allows C++ callers to more efficiently manage the strings using nsString/nsCStrings. However it doesn't always make sense to do so. For instance in nsIMsgIdentity.idl, I found some string arguments that were passed directly to the pref service (which requires a char *), and all the call sites were passing in string literals. Changing that argument to ACString wouldn't have made sense. But in general, most things can be converted.

- attribute wstring key;
+ attribute AString key;

- string getChromePackageName(in string aExtensionName);
+ ACString getChromePackageName(in ACString aExtensionName);

Strings

First, familiarize yourself with: http://developer.mozilla.org/en/docs/XPCOM:Strings#Common_Patterns

We aren't trying to replace that document, just highlighting some of the patterns we see a lot of / improvements we can make.

nsCString value;
- PL_strcase(value.get(), "imap://");
+ StringBeginsWith(someString, NS_LITERAL_CSTRING("imap://");

The following applies to both nsXPIDLString and nsXPIDLCString. This class is going away when we use frozen linkage so we want to replace it with nsString and nsCString. If you are changing an interface such that somemethod now takes a nsAString / nsACString then you don't need the getter_Copies either:

- nsXPIDLString strValue;
- somemethod(getter_Copies(strValue));
+ nsString strValue;
+ somemethod(getter_Copies(strValue)); or somemethod(strValue);

nsString uniValue;
nsCString charValue;
- uniValue.Assign(NS_ConvertASCIItoUTF16(charValue));
+ CopyASCIItoUTF16(charValue, uniValue);

nsCString charValue;
nsString  uniValue;
- charValue.AssignWithConversion(uniValue);
+ LossyCopyUTF16toASCII(uniValue, charValue);

The append methods like AppendUTF8toUTF16 are not available in the frozen linkage world:

nsCString charValue;
nsString  uniValue;
- AppendUTF8toUTF16(charVAlue, uniValue);
+ uniValue.Append(NS_ConvertUTF8toUTF16(charValue));

nsCString charStr;
- PL_strchr(uri.get(), ' ')
+ uri.FindChar(' '); (kNotFound)

nsCString charStr;
- PL_strcasecmp(charStr.get(), "nocopy://")
+ charStr.LowerCaseEqualsLiteral("nocopy://")

nsCString charStr;
- nsCRT::strcmp(charStr.get(), charStr2.get()) == 0
+ charStr.Equals(charStr2)

nsCString charStr;
- if (nsCRT::strncmp("imap:", charStr, 5))
+ if (!StringBeginsWith(charStr, NS_LITERAL_CSTRING("imap:")))

Replacement of case-insensitive checking code:

- char *strA;
- char *strB;
- if (PL_strncasaecmp(strA, strB)) {
  ...
- }

+ nsACString strA;
+ nsACString strB;
+ if (strA.Equals(strB, nsCaseInsensitiveCStringComparator())) {
  ...
+ }
+ 
+ nsString strA;
+ nsString strB;
+ if (strA.Equals(strB, nsCaseInsensitiveStringComparator())) {
   ...
+ }

nsCOMPtr

Use swap to assign an object wrapped by a nsCOMPtr into a return variable. This saves the cost of a reference count. Don't do this when the nsCOMPtr object is a member variable of a class or you want to use

nsCOMPtr<nsIMsgIdentity> identity;
- NS_IF_ADDREF(*aIdentity = identity);
+ identity.swap(*aIdentity);
return NS_OK;

- *aIdentity = m_identity;
- NS_IF_ADDREF(*aIdentity);
+ NS_IF_ADDREF(*aIdentity = m_identity);

Validating Input Arguments & Return Values

Use NS_ENSURE_ARG_POINTER to validate argument pointers instead of explicitly validating the argument:

 NS_IMETHODIMP nsMsgAccount::GetIdentities(nsISupportsArray **_retval)
- if (!_retval) return NS_ERROR_NULL_POINTER;
+ NS_ENSURE_ARG_POINTER(_retval);

In many cases, you can use NS_ENSURE_SUCCESS instead of explicitly checking for a failure and returning:

- if (NS_FAILED(rv)) return rv;
+ NS_ENSURE_SUCCESS(rv, rv);

You can use NS_ENSURE_TRUE for things like:

- if (!m_identities) return NS_ERROR_FAILURE;
+ NS_ENSURE_TRUE(m_identities, NS_ERROR_FAILURE);

I see a surprising amount of return value code that looks like:

- if (!*_retval) return NS_ERROR_FAILURE;
- return NS_OK;
+ return (*retval) ? NS_OK : NS_ERROR_FAILURE

There's no need to check for a failure code and a null value:

nsCOMPtr<nsIMsgIncomingServer> server = do_QueryInterface(aNntpServer, &rv);
- if (NS_FAILED(rv)) return rv;
- if (!server) return NS_ERROR_FAILURE;
+ NS_ENSURE_SUCCESS(rv, rv);

nsISupportsArray

Use do_QueryElementAt to get an element from an nsISupportsArray.

nsCOMPtr<nsISupportsArray> identityArray;
- nsCOMPtr<nsIMsgIdentity> identity;
- rv = identityArray->QueryElementAt(index, NS_GET_IID(nsIMsgIdentity),
                                    (void **)getter_AddRefs(identity));
+ nsCOMPtr<nsIMsgIdentity> identity( do_QueryElementAt(identityArray, i, &rv));

nsCOMPtr<nsISupportsArray> identityArray;
-  nsCOMPtr<nsISupports> thisElement;
- identityArray->GetElementAt(index, getter_AddRefs(thisElement));
- nsCOMPtr<nsIMsgIdentity> identity = do_QueryInterface(thisElement, &rv);
+ nsCOMPtr<nsIMsgIdentity> identity( do_QueryElementAt(identityArray, i, &rv));

Other Things To Look For

• Remove trailing spaces
• Replace any tabs with spaces
• In most files we use a tab width of two spaces. Adjust 4 spaces tab widths.
• Some of the methods in our IDL files start with an upper case letter. When cleaning up an interface, change these to lower case. (Don't forget to search through http://mxr.mozilla.org and fix any JS callers of the API to also be lower case!)
• The more nsCRT/PL_strcmp routines that get replaced with nsString equivalents as part of the string cleanup, the easier it will be to make a later pass through the code to prepare for using frozen linkage where we won't get nsCRT routines anymore.
• Remove braces around single statement clauses:

if (test)
-{
   doSomething();
-}

• Local variables that begin with the letter 'a'. Variables that begin with the letter 'a' are reserved for function arguments and should not be used at the start of a local variable:

- nsCOMPtr<nsIWebProgressListener> aProgressListener;
+ nsCOMPtr<nsIWebProgressListener> progressListener;