I have noticed that there are a number of places where nsAutoString is used as the type for a member variable. We need to review these cases and determine if they are really necessary or if nsString would work fine. In many cases this could be causing unnecessary object bloat. In cases where it is valid, we should comment the code to say it was intentional.

Moving to M15.

Moving non-essential, non-beta2 and performance-related bugs to M17.

moving to browser product and adding arch keyword

Reassigning to Rickg since he has a source scanning tool that can spot this easily.

Reassigning to my friend Warren.

Thanks Rick! This makes the task so much easier.

rickg, could you run the report again on the current trunk? "what would warren do" (tm) ;-) once the report gets generated? who is a good owner for this now? maybe thesteve??

Comment on attachment 172796 [details] [diff] [review] mailnews patch This is better than nothing, although as you're touching it... >Index: mailnews/compose/src/nsSmtpService.h >- nsCAutoString mServerKeyList; >+ nsCString mServerKeyList; IMHO this is unnecessary, we only have the one service. >Index: mailnews/base/src/nsMessengerWinIntegration.h >- nsAutoString mAppName; >- nsAutoString mEmailPrefix; >+ nsString mAppName; I'm undecided about this one as it's only used on XP. >+ nsString mEmailPrefix; Some trailing spaces crept in... >Index: mailnews/mapi/mapihook/src/nsMapiRegistryUtils.h >- nsCAutoString m_thisApp ; >- nsAutoString m_brand ; >- nsAutoString m_vendor ; >- nsAutoString m_versionNo ; >+ nsCString m_thisApp ; Gosh, this string is dreadfully abused (not your fault). >+ nsString m_brand ; >+ nsString m_vendor ; These two would work better as nsXPIDLStrings, to save copying. >+ nsString m_versionNo ; This one doesn't seem to be used at all...

As Mikael is not currently working on Mozilla, I've updated his patch which had (r+ and sr+) with respect to Neil's comments and produced this new one. Requesting reviews again just to make sure I've got it right. David or Neil, could you check this compiles on windows for me? Thanks.

Comment on attachment 206922 [details] [diff] [review] mailnews patch v2 >- void getVarValue(const PRUnichar * varName, nsAutoString & result); >+ void getVarValue(const PRUnichar * varName, nsString & result); Actually my idea was to have either a PRUnichar * result or an nsXPIDLString & result either way saving on the temporary value.

Comment on attachment 206922 [details] [diff] [review] mailnews patch v2 Ok, I see it clearer now with Neil's new comment. I'll update this soon.

Revised patch, hopefully understanding Neil's comments correctly ;-) Neil, can you check this compiles on windows as well please?

Just spoken with Neil over IRC, hopefully this is what he wants...

Comment on attachment 207854 [details] [diff] [review] mailnews patch v4 (checked in) >+ nsCString m_thisApp ; >+ nsXPIDLString m_brand ; >+ nsXPIDLString m_vendor ; Any reason for the ^^ extra spaces? While you're removing them, kill the spaces before ; too.

Comment on attachment 207854 [details] [diff] [review] mailnews patch v4 (checked in) Requesting sr, I'll address Neil's review comments on check in.

Comment on attachment 207854 [details] [diff] [review] mailnews patch v4 (checked in) thx, sr=bienvenu

Comment on attachment 207854 [details] [diff] [review] mailnews patch v4 (checked in) This patch is now checked in. I won't be doing any more on this bug as I was just seeing the mailnews patch through. I'll reassign to default owner as well in a minute seeing that Mikael isn't working on it either.

mass reassigning to nobody.

I ran $ hg locate '*.h' | xargs egrep 'nsC?AutoString\s+\w+\s*;' and looked through the results. Some classes do store nsC?AutoString's as members, but it doesn't look like any class which is instantiated a substantial number of times does. Let's close this 11-year-old bug.