379070 - Remove nsXPIDLCString / nsXPIDLString from mailnews

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Scott MacGregor]

In order to port to frozen linkages, we need to convert nsXPIDLCString / nsXPIDLString over to nsCString / nsString.

This work can be done in small chunks. 

I think it would be easier to do this work first and then do the all for one frozen linkages port after this.

While doing this work, I'd also like to update interfaces where possible to optimize their string use, using AString equivalents.

Comment 1

[Scott MacGregor]

Attachment: convert nsIMsgIdentity

I won't make everyone suffer from reviewing all these changes, but I think it's good to have some extra eyes on the first couple patches to help identify any bad string usage patterns.

In particular can you look at the changes in nsMsgIdentity.cpp.

This patch converts nsIMsgIdentity to use AString instead of wstring, adjusts the call sites, and removes nsXPIDLString from nsMsgIdentity.cpp.

Comment 2

[David :Bienvenu]

Comment on attachment 263063 [details] [diff] [review]
convert nsIMsgIdentity

nice, looks OK to me...

Comment 3

[[e-mail]]

Attachment: Convert pref usage

I've been meaning to attach this patch for some time... basically it converts nsMsgIdentity and nsMsgIncomingServer to use proper pref branches which means that you don't have to construct the pref strings inside the attribute methods.

Of course it doesn't take the proposed wstring -> AString changes into account.

Comment 4

[[e-mail]]

Comment on attachment 263063 [details] [diff] [review]
convert nsIMsgIdentity

I noticed that the old code deletes a pref when the string is null, while the new code also deletes it when the string is empty, is that a problem?

Comment 5

[[e-mail]]

Attachment: [checked in]Merged nsIdentity changes into Scott's patch

This also includes a couple of tweaks to avoid using getter_Copies.

This does not include the nsIMsgIncomingServer changes from attachment 263119 [details] [diff] [review].

Comment 6

[David :Bienvenu]

Comment on attachment 263119 [details] [diff] [review]
Convert pref usage

wow, good stuff.

+  if (NS_FAILED(mPrefBranch->GetCharPref(prefname, val)))
+    if (NS_FAILED(mDefPrefBranch->GetCharPref(prefname, val)))
+      *val = nsnull;

might be clear if it's of the form

if (a && b)
  *val = nsnull;

instead of

if a
  if b
    ...

This code has changed with the file spec removal stuff so there will be conflicts when you update (unless I've really screwed up somehow and missed those files)

Comment 7

[[e-mail]]

Comment on attachment 263171 [details] [diff] [review]
[checked in]Merged nsIdentity changes into Scott's patch

>+  rv = prefs->GetBranch("mail.smtpserver.default.", getter_AddRefs(mDefPrefBranch));
Oops. Guess where I copied this from ;-)

Comment 8

[Scott MacGregor]

Comment on attachment 263171 [details] [diff] [review]
[checked in]Merged nsIdentity changes into Scott's patch

wow Neil, that was very kind of you to merge my string changes into this patch. Looks good to me. Did you see any string patterns you didn't like in my nsMsgIdentity.cpp changes? Speak up now before I start doing similar changes in mailnews :)

Comment 9

[Scott MacGregor]

Comment on attachment 263119 [details] [diff] [review]
Convert pref usage

I needed to review this patch for the incoming server changes.

Comment 10

[[e-mail]]

(In reply to comment #8)
>Did you see any string patterns you didn't like in my nsMsgIdentity.cpp changes?
I spotted just the one: using aSupportsString->GetData instead of ToString

Comment 11

[[e-mail]]

Comment on attachment 263063 [details] [diff] [review]
convert nsIMsgIdentity

>+      macro_tmpstr->ToString(getter_Copies(macro_oldStr)); \
For the record, r=me except using GetData instead of ToString as noted.

Comment 12

[[e-mail]]

Attachment: [checked in]nsMsgIncomingServer pref cleanup updated for bitrot

Comment 13

[Scott MacGregor]

Attachment: [checked in]more nsIMsgIdentity changes

Convert the rest of nsIMsgIdentity to ACString and related string cleanup from the effected call sites.

Comment 14

[Scott MacGregor]

I didn't bump the interface ID because I did that in yesterday's checkin.

Comment 15

[David :Bienvenu]

Comment on attachment 263385 [details] [diff] [review]
[checked in]more nsIMsgIdentity changes

good times!

Comment 16

[Bill Gianopoulos [:WG9s]]

This appears to be causing the redness on MAC builds.

Comment 17

[Scott MacGregor]

Attachment: [checked in]nsIMsgAccount

nsIMsgAccount cleanup.

Comment 18

[David :Bienvenu]

Comment on attachment 263412 [details] [diff] [review]
[checked in]nsIMsgAccount

+  if (m_identities)
+    return NS_ERROR_FAILURE;
+  else
 	return createIdentities();


this can be just

return (m_identites) ? NS_ERROR_FAILURE : createIdenties();

Comment 19

[Scott MacGregor]

Attachment: nsIMsgAccountManager changes

Comment 20

[[e-mail]]

Comment on attachment 263385 [details] [diff] [review]
[checked in]more nsIMsgIdentity changes

>     PRBool needDummyHeader =
>-      PL_strcasestr(saveListener->m_templateUri, "mailbox://") 
>+      PL_strcasestr(saveListener->m_templateUri.get(), "mailbox://") 
>       != nsnull;
>     PRBool canonicalLineEnding =
>-      PL_strcasestr(saveListener->m_templateUri, "imap://")
>+      PL_strcasestr(saveListener->m_templateUri.get(), "imap://")
>       != nsnull;
You can't convert these into StringBeginsWith or .Find() or something?

> 			char *macro_oldStr = nsnull; \
> 			macro_rv = macro_spec->GetUnixStyleFilePath(&macro_oldStr);	\
>     		if (NS_SUCCEEDED(macro_rv)) { \
>-				MACRO_OBJECT->MACRO_METHOD(macro_oldStr); \
>+				MACRO_OBJECT->MACRO_METHOD(nsCString(macro_oldStr)); \
>             } \
>             PR_FREEIF(macro_oldStr); \
This should be
nsCString macro_oldStr;
macro_rv = macro_spec->GetUnixStyleFilePath(getter_Copies(macro_oldStr));
if (NS_SUCCEEDED(macro_rv)) {
  MACRO_OBJECT->MACRO_METHOD(macro_oldStr);
}

>-  rv = createKeyedIdentity(key, _retval);
>+  rv = createKeyedIdentity(nsCString(key), _retval);
Since key is known to be nonnull you should prefer nsDependentCString(key)

> #define NS_IMPL_IDPREF_STR(_postfix, _prefname) \
> NS_IMETHODIMP                                   \
>-nsMsgIdentity::Get##_postfix(char **retval)     \
>+nsMsgIdentity::Get##_postfix(nsACString& retval)     \
> {                                               \
>   return GetCharAttribute(_prefname, retval);   \
> }                                               \
> NS_IMETHODIMP                                   \
>-nsMsgIdentity::Set##_postfix(const char *value) \
>+nsMsgIdentity::Set##_postfix(const nsACString& value) \
> {                                               \
>   return SetCharAttribute(_prefname, value);    \
And I'd gone to all the trouble of lining these \s up too...

>+nsMsgIdentity::Get##_postfix(nsACString& retval)                \
> {                                                          \
>-  return getFolderPref(_prefname, retval, _flag);          \
>+  nsresult rv;                                             \
>+  nsCString folderPref;                                    \
>+  rv = getFolderPref(_prefname, folderPref, _flag);        \
>+  retval = folderPref;                                     \
>+  return rv;                                               \
> }                                                          \
Hmm, a bit ugly though...

> NS_IMETHODIMP                                              \
>-nsMsgIdentity::Set##_postfix(const char *value)            \
>+nsMsgIdentity::Set##_postfix(const nsACString& value)      \
> {                                                          \
>-  return setFolderPref(_prefname, value, _flag);           \
>+  return setFolderPref(_prefname, nsCString(value), _flag); \
I didn't see any code in setFolderPref that needs value to be an nsCString so you could probably change it to nsACString.

>+  uri.Truncate(0);
Actually nsStringAPI only supports Truncate(), not Truncate(0) (or any other number).

>-      if (PL_strchr(uri, ' ') != nsnull)
>+      if (PL_strchr(uri.get(), ' ') != nsnull)
.Find() again?

>+      rv = identity->GetEscapedVCard(escapedVCard);
> 
>       if (NS_SUCCEEDED(rv) && !escapedVCard.IsEmpty()) 
>       {
>           nsCString vCardUrl;
>           vCardUrl = "data:text/x-vcard;charset=utf-8;base64,";
>-          char *unescapedData = PL_strdup(escapedVCard);
>+          char *unescapedData = ToNewCString(escapedVCard);
>           if (!unescapedData)
>               return NS_ERROR_OUT_OF_MEMORY;
>           nsUnescape(unescapedData);
Hmm... is there an nsCString version of nsUnescape?

>+          mCompFields->SetFcc(PL_strcasecmp(uri.get(), "nocopy://") ? uri.get() : "");
uri.LowerCaseEqualsLiteral("nocopy://") ? "" : uri.get();

>+    fullName.Assign(NS_ConvertASCIItoUTF16(realName));
CopyASCIItoUTF116(realName, fullName);

Comment 21

[David :Bienvenu]

Comment on attachment 263528 [details] [diff] [review]
nsIMsgAccountManager changes

whew, that's a lot.

One thing to keep in mind (you know this, but sometimes we get stuck in our NS_ENSURE_SUCCESS(rv, rv) rut :-) )

-  if (NS_FAILED(rv)) return PR_TRUE;
+  if (NS_FAILED(rv))
+    return PR_TRUE;

If it's really an unexpected error that we might want to appear on the debug console, you can write:

NS_ENSURE_SUCCESS(rv, PR_TRUE);

Comment 22

[[e-mail]]

Comment on attachment 263528 [details] [diff] [review]
nsIMsgAccountManager changes

>+    if (!findEntry.account)
>+      break;
>+    else
>     findEntry.account = nsnull;
>+  } while (1);
This is a bit of an odd construct; I think it would be more obvious to write do { findEntry.account = nsnull; ... } while (findEntry.account);

>+  server->SetKey(nsCString(key).get());
>+  server->SetType(nsCString(type).get());
>+  server->SetUsername(nsCString(username).get());
>+  server->SetHostName(nsCString(hostname).get());
Maybe you should convert nsIIncomingServer to nsACString first?

>-    mAccountKeyList += ",";
>-    mAccountKeyList += key;
>+    mAccountKeyList.AppendLiteral(",");
>+    mAccountKeyList.Append(key);
For a single character mAccountKeyList.Append(',') is better.

>   findServerEntry serverInfo;
>   // hostname might be blank, pass "" instead
>+  serverInfo.hostname = hostname;
>   // username might be blank, pass "" instead
>+  serverInfo.username = username;
>   // port is initialized to zero if not specified in the url,
>   // so use it no matter what
>   serverInfo.port = port;
>   // type might be blank, pass "" instead
>+  serverInfo.type = type;
>   serverInfo.useRealSetting = aRealFlag;
Those comments are obsolete, also it might be worth writing a constructor for findServerEntry.

>+nsMsgAccountManager::FindRealServer(const nsACString& username,
>+                                    const nsACString& hostname,
>+                                    const nsACString& type,
>                                 PRInt32 port,
>                                 nsIMsgIncomingServer** aResult)
I wonder why this didn't use InternalFindServer?

>   // NOTE: DO NOT REPLACE PL_strcmp OR PL_strcasecmp below because one or more
>   // of these items may be null
>-  if ((!*entry->type || (PL_strcmp(entry->type, thisType)==0)) &&
>-      (!*entry->hostname || (PL_strcasecmp(entry->hostname, thisHostname)==0)) &&
>+  if ((entry->type.IsEmpty() || thisType.Equals(entry->type)) &&
>+      (entry->hostname.IsEmpty() || thisHostname.Equals(entry->hostname, nsCaseInsensitiveCStringComparator())) &&
>       (!(entry->port != 0) || (entry->port == thisPort)) &&
>-      (!*entry->username || (PL_strcmp(entry->username, thisUsername)==0)))
>+      (entry->username.IsEmpty() || thisUsername.Equals(entry->username)))
The comment is out of date?

>   // treat "" as a wild card, so if the caller passed in "" for the desired attribute
>   // treat it as a match
>-  PRBool checkType = PL_strcmp(entry->type, "");
>-  PRBool checkHostname = PL_strcmp(entry->hostname,"");
>-  PRBool checkUsername = PL_strcmp(entry->username,"");
>-  if ((!checkType || (PL_strcmp(entry->type, thisType)==0)) &&
>-      (!checkHostname || (PL_strcasecmp(entry->hostname, thisHostname)==0)) &&
>-      (!checkUsername || (PL_strcmp(entry->username, thisUsername)==0)))
>+  if ((!entry->type.IsEmpty()     || thisType.Equals(entry->type)) &&
>+      (!entry->hostname.IsEmpty() || thisHostname.Equals(entry->hostname, nsCaseInsensitiveCStringComparator())) &&
>+      (!entry->username.IsEmpty() || thisUsername.Equals(entry->username)))
!PL_strcmp(entry->type, "") should map to entry->type.IsEmpty() (without a !)

>+  nsresult rv = pMgr->FindServer( nsCString((const char *)pBytes), nsCString(pServerName), NS_LITERAL_CSTRING("imap"), getter_AddRefs(in));
Prefer nsDependentCString [for now] as it avoid copying. (Obviously also applies to the other similar occurrances.)

>           if (aAccountKey && *aAccountKey)
>           {
>             nsCOMPtr <nsIMsgAccount> account;
>-            acctMgr->GetAccount(aAccountKey, getter_AddRefs(account));
>+            acctMgr->GetAccount(nsCString(aAccountKey), getter_AddRefs(account));
nsDependentCString again.

Comment 23

[Scott MacGregor]

Thanks for the feedback Neil.

(In reply to comment #20)
> >-  rv = createKeyedIdentity(key, _retval);
> >+  rv = createKeyedIdentity(nsCString(key), _retval);
> Since key is known to be nonnull you should prefer nsDependentCString(key)

I'm under the impression that nsDependentCString doesn't do anything special in the frozen linkage world so I'm using nsCString here in preparation. 

 
> > NS_IMETHODIMP                                              \
> >-nsMsgIdentity::Set##_postfix(const char *value)            \
> >+nsMsgIdentity::Set##_postfix(const nsACString& value)      \
> > {                                                          \
> >-  return setFolderPref(_prefname, value, _flag);           \
> >+  return setFolderPref(_prefname, nsCString(value), _flag); \
> I didn't see any code in setFolderPref that needs value to be an nsCString so
> you could probably change it to nsACString.

setFolderPref is called by getFolderPref and I couldn't get getFolderPref to work with a nsACString, maybe I'm just being lazy there..
 
> >+  uri.Truncate(0);
> Actually nsStringAPI only supports Truncate(), not Truncate(0) (or any other
> number).

good catch!

> 
> >-      if (PL_strchr(uri, ' ') != nsnull)
> >+      if (PL_strchr(uri.get(), ' ') != nsnull)
> .Find() again?
> 
> >+      rv = identity->GetEscapedVCard(escapedVCard);
> > 
> >       if (NS_SUCCEEDED(rv) && !escapedVCard.IsEmpty()) 
> >       {
> >           nsCString vCardUrl;
> >           vCardUrl = "data:text/x-vcard;charset=utf-8;base64,";
> >-          char *unescapedData = PL_strdup(escapedVCard);
> >+          char *unescapedData = ToNewCString(escapedVCard);
> >           if (!unescapedData)
> >               return NS_ERROR_OUT_OF_MEMORY;
> >           nsUnescape(unescapedData);
> Hmm... is there an nsCString version of nsUnescape?

I'm not aware of one. We're going to end up fixing this anyway when we make the frozen linkage pass since nsEscape.h nsUnescape isn't part of frozen linkages I believe.

I've made the rest of your suggestions to my tree and will be checking them in with the nsIMsgAccountManager changes.

Comment 24

[Scott MacGregor]

nsDependentString is part of nsStringAPI so we can use it.

Comment 25

[Scott MacGregor]

Attachment: [checked in]updated nsIMsgAccountManager patch

I updated this patch to include most of Neil's feedback for nsIMsgIdentity and nsIMsgAccountManager.

Comment 26

[[e-mail]]

(In reply to comment #23)
>>> NS_IMETHODIMP                                              \
>>>-nsMsgIdentity::Set##_postfix(const char *value)            \
>>>+nsMsgIdentity::Set##_postfix(const nsACString& value)      \
>>> {                                                          \
>>>-  return setFolderPref(_prefname, value, _flag);           \
>>>+  return setFolderPref(_prefname, nsCString(value), _flag); \
>>I didn't see any code in setFolderPref that needs value to be an nsCString so
>>you could probably change it to nsACString.
>setFolderPref is called by getFolderPref and I couldn't get getFolderPref to
>work with a nsACString, maybe I'm just being lazy there..
Since an nsCString can of couse be passed as a const nsACString& you don't need to change getFolderPref, so the only three changes are
a) declare and b) define setFolderPref as const nsACString& value
c) change the above back to setFolderPref(_prefname, value, _flag);

Comment 27

[[e-mail]]

Comment on attachment 263653 [details] [diff] [review]
[checked in]updated nsIMsgAccountManager patch

>         if (!uri.IsEmpty())
>-          mCompFields->SetFcc(PL_strcasecmp(uri.get(), "nocopy://") ? uri.get() : "");
>+          mCompFields->SetFcc(uri.LowerCaseEqualsLiteral("nocopy://") ? "" : uri.get());
>         else
>           mCompFields->SetFcc("");
It occurs to me the IsEmpty() test only existed to avoid passing NULL to PL_strcasecmp, but of course that's not an issue for LowerCaseEqualsLiteral, so you can kill the isEmpty check.

Comment 28

[[e-mail]]

Attachment: FindServer cleanup as alluded to earlier

Comment 29

[Scott MacGregor]

Attachment: [checked in] nsIMsg*IncomingServer.idl

I haven't been able to test this patch as much as I want yet. Some of the code has some temporary string stuff in it until additional interfaces get converted (like nsIMsgFolder).

This patch converts all of the incoming server interfaces & nsIImapServerSink.

Comment 30

[David :Bienvenu]

ugh, I just did nsImapIncomingServer.cpp - oh well...your patch is most likely a super-set of mine.

Comment 31

[David :Bienvenu]

Attachment: [checked in]remove most imap uses

this gets rid of xpidlstring in imap, except for nsImapMailFolder.cpp

you can ignore most of the nsImapIncomingServer.cpp changes, since you probably have them in your tree already...

Comment 32

[Scott MacGregor]

Comment on attachment 264250 [details] [diff] [review]
[checked in] nsIMsg*IncomingServer.idl

Sorry David, I thought i mentioned that I was doing nsIMsgIncomingServer next but I see I didn't. I'm trying to do all of the top level interfaces (identity, accounts, server, folders, msg hdrs) first. I'll be starting in on nsIMsgFolder next I think.

Comment 33

[Scott MacGregor]

Comment on attachment 264253 [details] [diff] [review]
[checked in]remove most imap uses

sr=me if you let me land my nsIMsg*IncomingServer changes first :)

FYI, I'll be doing nsIMsgFolder next.

Comment 34

[David :Bienvenu]

Attachment: [checked in]remove nsImapProtocol.cpp uses (and a few related changes)

Comment 35

[Scott MacGregor]

Comment on attachment 264250 [details] [diff] [review]
[checked in] nsIMsg*IncomingServer.idl

this is checked in now. Sorry for all the merge conflicts David :).

Neil, as always I'm standing by to incorporate any addition corrections / suggestions. 

Oh and this patch also added the last round of suggestions from Neil for the identity changes.

Comment 36

[Scott MacGregor]

Comment on attachment 264283 [details] [diff] [review]
[checked in]remove nsImapProtocol.cpp uses (and a few related changes)

sorry for the conflicts you're going to get now :)

Comment 37

[David :Bienvenu]

Comment on attachment 264283 [details] [diff] [review]
[checked in]remove nsImapProtocol.cpp uses (and a few related changes)

I also checked in a couple fixes for problems introduced in previous patches - we crashed if imap logging was turned on, and we were calling LowerCaseEqualsLiteral but not passing in a lower case literal :-)

IMAP subscribe UI and folder discovery is broken - I don't know when that broke, but it was before I checked in :-) - I'll investigate.

Comment 38

[Scott MacGregor]

Thanks for cleaning up after me David. I can debug the subscribe and folder discovery issues later today if you want to pass that off.

Comment 39

[David :Bienvenu]

I'll try to figure it out - it could have been some of the filespec stuff, though I know I tested that at some point...

Comment 40

[David :Bienvenu]

Attachment: [checked in]fix subscribe ui and folder discovery

this makes things much happier

Comment 41

[Scott MacGregor]

Comment on attachment 264370 [details] [diff] [review]
[checked in]fix subscribe ui and folder discovery

thanks David!

Comment 42

[David :Bienvenu]

Comment on attachment 264370 [details] [diff] [review]
[checked in]fix subscribe ui and folder discovery

also landed a temporary fix for crash doing acl, and fixed myrights handling (check for empty user name as well as null, since using CString prevents the null string from surviving)

Comment 43

[[e-mail]]

Comment on attachment 264250 [details] [diff] [review]
[checked in] nsIMsg*IncomingServer.idl

>-NS_IMPL_GETSET(nsMsgIncomingServer, ServerBusy, PRBool, m_serverBusy)
Why this change (expanding the macro)?

>+  aResult.Append(localStoreType);
You don't seem to clear aResult, so perhaps this should be .Assign (or just read the localStoreType into aResult directly).

>+      nsCString escapedUsername;
>         *((char **)getter_Copies(escapedUsername)) =
>-            nsEscape(username, url_XAlphas);
>-//            nsEscape(username, url_Path);
>+          nsEscape(username.get(), url_XAlphas);
If I didn't know that nsEscape will have to be changed anyway I would have suggested using .Adopt instead of abusing getter_Copies.

>+    mDefPrefBranch->GetCharPref(prefname,getter_Copies(tmpVal));
Nit: space after comma

>   if (supportsString)
>-    return supportsString->ToString(val);
>-
>-  *val = nsnull;
>+    return supportsString->GetData(val);
>   return NS_OK;
*val = nsnull; becomes val.Truncate(); rather than ignoring it.

>+    rootFolder->SetPrettyName(nsPromiseFlatString(value).get());
This should say PromiseFlatString (no ns) or be consistent with SetCharValue and use nsString directly.

>+nsMsgIncomingServer::ToString(nsAString& aResult) {
You missed the chance to put the { on its own line ;-)

>+  aResult.AppendLiteral("[nsIMsgIncomingServer: ");
Again, you didn't clear aResult, so shouldn't you use AssignLiteral?

>+      nsString uniPassword;
>+      if (!aPassword.IsEmpty())
>+        CopyASCIItoUTF16(aPassword, uniPassword);
> 
>       PRUint32 savePasswordType = PasswordProtectLocalCache() ? nsIAuthPrompt::SAVE_PASSWORD_FOR_SESSION : nsIAuthPrompt::SAVE_PASSWORD_PERMANENTLY;
>+      rv = dialog->PromptPassword(PromiseFlatString(aPromptTitle).get(), PromiseFlatString(aPromptMessage).get(),
>         NS_ConvertASCIItoUTF16(serverUri).get(), savePasswordType,
>+        getter_Copies(uniPassword), okayValue);
I'm not sure what was going in here before the patch, but getter_Copies always overwrites uniPassword so there's no point copying aPassword to it?

>+  nsCString type;
>+  rv = GetType(type);
>     NS_ENSURE_SUCCESS(rv, rv);
> 
>     nsCAutoString contractid(NS_MSGPROTOCOLINFO_CONTRACTID_PREFIX);
>     contractid.Append(type);
Would Insert()ing the prefix would be better?

My eyes blurred over when I got to imap so I'll just assume it's ok ;-)

Comment 44

[David :Bienvenu]

>I'm not sure what was going in here before the patch, but getter_Copies always
>overwrites uniPassword so there's no point copying aPassword to it?

Crud, I think that's going to break the feature where we pre-populate the password dialog with the password that we last sent - so you'll get ********* in the case where a logon failed for some reason, but the password worked before.  So even though we're passing in a char **, we really mean it to be an in out param

Comment 45

[Scott MacGregor]

(In reply to comment #43)
> (From update of attachment 264250 [details] [diff] [review])
> >-NS_IMPL_GETSET(nsMsgIncomingServer, ServerBusy, PRBool, m_serverBusy)
> Why this change (expanding the macro)?

Good question. NS_IMPL_GETSET is defined in xpcom obsolete which we are trying to remove our dependency on, so I expanded the macro. I suppose we could also try to move the macro from xpcom obsolete to somewhere in mailnews instead of expanding the macros...Although now that I look at it, I see it's defined in a file called nsISupportsObsolete which is still built by xpcom\base.

I'll try to incorporate your feedback into my next patch. Thanks as always Neil.

> 
> My eyes blurred over when I got to imap so I'll just assume it's ok ;-)

heh, I know the feeling. 

Comment 46

[Tuukka Tolvanen (sp3000)]

imap login crashes for me though -- bug 380461

Comment 47

[David :Bienvenu]

Attachment: [checked in]remove xpidl strings from mime

Comment 48

[David :Bienvenu]

Attachment: [checked in]fix password regression

Haven't tried this, but I think it does the right thing...

Comment 49

[[e-mail]]

Comment on attachment 264562 [details] [diff] [review]
[checked in]remove xpidl strings from mime

>@@ -2024,25 +2024,25 @@ MimeGetStringByID(PRInt32 stringID)
>     if (NS_SUCCEEDED(res) && (nsnull != sBundleService)) 
>     {
>       res = sBundleService->CreateBundle(propertyURL, getter_AddRefs(stringBundle));
>     }
>   }
> 
>   if (stringBundle)
>   {
>-    nsXPIDLString v;
>+    nsString v;
>     res = stringBundle->GetStringFromID(stringID, getter_Copies(v));
> 
>     if (NS_SUCCEEDED(res)) 
>       tempString = ToNewUTF8String(v);
>   }
> 
>   if (!tempString)
>-    tempString = nsCRT::strdup(resultString);
>+    tempString = strdup(resultString);
> 
>   return tempString;
> }
Not your bug but this should probably say tempString = ToNewCString(NS_LITERAL_CSTRING("???")); or maybe something along the lines of this:
nsString v;
if (stringBundle)
  stringBundle->GetStringFromID(stringID, getter_Copies(v);

if (v.IsEmpty())
  v.AssignLiteral("???");

return ToNewUTF8String(v);
(either way removing the now redundant resultString)

Comment 50

[David :Bienvenu]

Attachment: [checked in]remove from base/search, and clean up import service a bit.

I hope this doesn't cause any conflicts - I tried to keep it contained.

Comment 51

[Scott MacGregor]

Comment on attachment 264922 [details] [diff] [review]
[checked in]remove from base/search, and clean up import service a bit.

-            if (PL_strcasecmp(folderUri,oldFolderUri) == 0 ) //local
+             if (folderUri.Equals(oldFolderUri)) //local

Do we need to pass in nsCaseInsensitiveCStringComparator() here?

+          filter->GetTerm(i, &attrib, nsnull,nsnull,nsnull, arbitraryHeader);

spaces after commas.

is there a string API routine like StringBeginsWith ?

I think when we move to the frozen linkage string  API, auto string maps straight to nsString, so I'm not sure if it matters if we declare auto strings anymore:

+          nsAutoString nullCharset, folderCharset;

+    aResult = NS_ConvertUTF8toUTF16(mValue.string);

could this be:
CopyUTF8toUTF16(mValue.string, aResult);

+        aResult.Append(tempInt);

if you figure out how to append an integer to a nsAString without going through a nsString first, let me know!

+    aResult.AppendLiteral("]");

UI think that can just be:
   aResult.Append(']'); since it's a single character.

need to bump the interface ID in nsIImapServerSink.idl.

good stuff!

Comment 52

[David :Bienvenu]

+    aResult.AppendLiteral("]");

UI think that can just be:
   aResult.Append(']'); since it's a single character.

that didn't work for nsAString - append(char) is private, apparently, for AString. I fixed the other comments, though, except for StringsBeginWith...

Comment 53

[[e-mail]]

(In reply to comment #52)
> +    aResult.AppendLiteral("]");
> 
> UI think that can just be:
>    aResult.Append(']'); since it's a single character.
> 
> that didn't work for nsAString - append(char)
Try aResult.Append(PRUnichar(']'));

Comment 54

[[e-mail]]

Comment on attachment 264922 [details] [diff] [review]
[checked in]remove from base/search, and clean up import service a bit.

> void nsImportGenericMail::GetMailboxName( PRUint32 index, nsISupportsString *pStr)
> {
> 	if (m_pMailboxes) {
> 		nsCOMPtr<nsIImportMailboxDescriptor> box(do_QueryElementAt(m_pMailboxes, index));
> 		if (box) {
>-			nsXPIDLString name;
>+			nsAutoString name;
As with the other cases, a regular nsString would have sufficed here, no?

Comment 55

[Scott MacGregor]

Attachment: [checked in]nsIMsg*Folder.idl

This patch updates our nsIMsgFolder interfaces. Unfortunately I'm pretty sure this is going to break some things due to the size of the patch. I've successfully completed basic smoke tests for IMAP, POP, News and RSS including account creation, sending, fetching, deleting, moving/copying & filters. 

I also incorporated some incoming server suggestions from Neil.

Comment 56

[David :Bienvenu]

Comment on attachment 265319 [details] [diff] [review]
[checked in]nsIMsg*Folder.idl

wow, great work, Scott.

+  NS_ENSURE_SUCCESS(rv, rv);
+  return aFolderArray ? folderCompactor->CompactAll(aFolderArray, aMsgWindow, aCompactOfflineAlso, aOfflineFolderArray) :
+                        folderCompactor->CompactAll(folderArray, aMsgWindow, aCompactOfflineAlso, aOfflineFolderArray);

Can the ? operator be moved into the first arg of CompactAll? Maybe with a .get() on the comptr cuz some compilers are picky with the ? operator.

 #ifdef DEBUG_bienvenu
-  if (!nsCRT::strcasecmp((const char *) onlineName, "Sent"))
-    printf("loading folder cache elem for %s flags = %lx", (const char *) onlineName, mFlags);
-  else if (!nsCRT::strcasecmp((const char *) onlineName, "INBOX"))
-    printf("loading folder cache elem for %s flags = %lx", (const char *) onlineName, mFlags);
+  if (!nsCRT::strcasecmp(onlineName.get(), "Sent"))
+    printf("loading folder cache elem for %s flags = %lx", onlineName.get(), mFlags);
+  else if (!nsCRT::strcasecmp(onlineName.get(), "INBOX"))
+    printf("loading folder cache elem for %s flags = %lx", onlineName.get(), mFlags);
 #endif


This can either go away or use EqualsLowerCaseLiteral...


-
     rv = aHdrXferInfo->GetHeader(i, getter_AddRefs(headerInfo));
     NS_ENSURE_SUCCESS(rv, rv);
-    if (!headerInfo)
-      break;

this part worries me a little - there's some weird problems with this code - does GetHeaders always return an error when it can't get a headerInfo?

Comment 57

[Scott MacGregor]

(In reply to comment #56)
> (From update of attachment 265319 [details] [diff] [review])
> wow, great work, Scott.
hah, let's save that compliment till after we see how much I broke :).

> -
>      rv = aHdrXferInfo->GetHeader(i, getter_AddRefs(headerInfo));
>      NS_ENSURE_SUCCESS(rv, rv);
> -    if (!headerInfo)
> -      break;
> 
> this part worries me a little - there's some weird problems with this code -
> does GetHeaders always return an error when it can't get a headerInfo?

This was a good catch. Many of the problems I had debugging this patch came back to me being to agressive about assuming methods were returning failure codes when the return value was null. 

I thought about landing this patch a couple times today but I think I'm going to wait until the compose window crash / regression issue is cleared up in order to avoid piling on more regressions.

Comment 58

[Scott MacGregor]

For future reference, as I've forgotten this fact many times in the past too, the Mac compiler won't let you do this:
undoMsgTxn->SetTransactionType(mFlags & MSG_FOLDER_FLAG_TRASH ? nsIMessenger::eDeleteMsg : nsIMessenger::eMoveMsg);

it gets very unhappy with the enums in that syntax.

Comment 59

[Scott MacGregor]

Attachment: [checked in]nsIMsgWindow

nsIMsgWindow changes.

Comment 60

[David :Bienvenu]

POP3 filters on incoming mail seem to be broken after rebuilding - they work if you run after the fact. I'll look into it...

Comment 61

[David :Bienvenu]

Attachment: [checked in]fix pop3 filter move action

this fixes the pop3 filter move action regression.

Comment 62

[Scott MacGregor]

Comment on attachment 265676 [details] [diff] [review]
[checked in]fix pop3 filter move action

thanks david.

Comment 63

[Joerg Bornemann]

Attachment: nsMsgProgress

Comment 64

[David :Bienvenu]

Comment on attachment 265683 [details] [diff] [review]
nsMsgProgress

we use 2 space indent usually, so converting the indentation to 4 spaces is a step in the wrong direction...

Comment 65

[David :Bienvenu]

Comment on attachment 265683 [details] [diff] [review]
nsMsgProgress

We do appreciate you helping out - and a patch that doesn't have all the whitespace changes would be very helpful.

Comment 66

[Joerg Bornemann]

Attachment: nsMsgProgress

OK, whitespaces back to normal. Apparently I've read the space-adjustment-sentence on the CodeCleanUp-Page too fast. :-)

Comment 67

[Scott MacGregor]

Comment on attachment 265692 [details] [diff] [review]
nsMsgProgress

almost, you forgot to put the 'w' in your cvs diff command so we don't see the white space changes :)

Comment 68

[Joerg Bornemann]

Attachment: [checked in]nsMsgProgress

I think the remaining ws changes are sensible. Choose your favourite patch. ;)

Comment 69

[Scott MacGregor]

Attachment: [checked in]nsIMsgFolderCacheElement and nsIMsgFolderCache

nsIMsgFolderCache and nsIMsgFolderCacheElement changes.

Comment 70

[Scott MacGregor]

Comment on attachment 265694 [details] [diff] [review]
[checked in]nsMsgProgress

-    nsCOMPtr<nsIWebProgressListener> aProgressListener;
+    nsCOMPtr<nsIWebProgressListener> progressListener;
     for (i = count - 1; i >= 0; i --)
     {
       m_listenerList->GetElementAt(i, getter_AddRefs(aSupports));
-      aProgressListener = do_QueryInterface(aSupports);
-      if (aProgressListener)
-        aProgressListener->OnStatusChange(aWebProgress, aRequest, aStatus, aMessage);
+      progressListener = do_QueryInterface(aSupports);
+      if (progressListener)
+        progressListener->OnStatusChange(aWebProgress, aRequest, aStatus, aMessage);

Can we use do_QueryElementAt here and get rid of the aSupports variable? I think you've done that in a couple other places in this file.

No need to attach a new patch, I can tweak that when I check this in for you.

Asking David for sr since I already reviewed most of this yesterday over email. Thanks again for the patch!

Comment 71

[David :Bienvenu]

Comment on attachment 265710 [details] [diff] [review]
[checked in]nsIMsgFolderCacheElement and nsIMsgFolderCache

There are a couple places where you now assume that  yarn's are null terminated - MDB doesn't guarantee that. It may be that Mork yarns are always null-terminated. If any problems appear, this would be one assumption to check.

-			char *result = (char *) PR_Malloc(yarn.mYarn_Fill + 1);
-			if (result)
-			{
-				memcpy(result, yarn.mYarn_Buf, yarn.mYarn_Fill);
-				result[yarn.mYarn_Fill] = '\0';
-			}
-			else
-				err = NS_ERROR_OUT_OF_MEMORY;
-
-			*resultPtr = result;
+      resultStr.Assign((const char *)yarn.mYarn_Buf, yarn.mYarn_Fill);
 			hdrCell->Release(); // always release ref

Comment 72

[David :Bienvenu]

Comment on attachment 265694 [details] [diff] [review]
[checked in]nsMsgProgress

looks good, thx, modulo Scott's do_QueryElementAt comment

Comment 73

[Scott MacGregor]

Comment on attachment 265694 [details] [diff] [review]
[checked in]nsMsgProgress

Patch checked in. Thank you Joerg!

Comment 74

[Nick Kreeger]

Attachment: [checked in]nsIImapHostSessionList Clean-up V1

Here is the patch for |nsIImapHostSessionList|. I didn't see any string changes, but I did notice some alignment issues.

This interface doesn't have an IDL definition.

Comment 75

[Prasad Sunkari [:prasad]]

Attachment: convert nsIAbAddressCollecter

Scott, can you comment on this patch, so that I go ahead and do the same with other files in addrbook.  Just for the purpose of review, I am also attaching the complete addrbook conversion to Frozen linkage.

Comment 76

[David :Bienvenu]

Attachment: [checked in]remove xpidl strings from view code

Comment 77

[Scott MacGregor]

Comment on attachment 265952 [details] [diff] [review]
[checked in]remove xpidl strings from view code

nice!

Comment 78

[Scott MacGregor]

Comment on attachment 265876 [details] [diff] [review]
[checked in]nsIImapHostSessionList Clean-up V1

This patch looks like white space & brace removal for single line statements. Looks good to me!

Comment 79

[Scott MacGregor]

Attachment: [checked in]string cleanup for mailnews\base

This patch does the following to all the code in mailnews\base:

1) Replace nsXPIDLCString / nsXPIDLString (does not exist in frozen linkage) with nsString equivalents.
2) Replace PromiseFlatString / PromiseFlatCString (does not exist in frozen linkage) with nsString equivalents
3) Replace callers of nsString::Truncate that were passing in non zero value to use nsString::SetLength (truncate doesn't take an argument).
4) Replace AppendUTF8toUTF16 (does not exist in frozen linkage) and other friends with NS_Convert equivalents.
5) Replace nsString::AssignWithConversion (does not exist in frozen linkage) with the appropriate LossyCopyUTF16toASCII / CopyASCIItoUTF16 method.

Comment 80

[Scott MacGregor]

Comment on attachment 266018 [details] [diff] [review]
[checked in]string cleanup for mailnews\base

see the previous comment for a description. We should probably do this same exercise for each directory in mailnews.

Comment 81

[Scott MacGregor]

Comment on attachment 265943 [details] [diff] [review]
convert nsIAbAddressCollecter

Wow, this looked really good to me Prasad. And I'm sure Mark will appreciate converting this interface over to AString.

Only one comment:

+  rv = SetAbURI(prefVal.IsEmpty() ? nsCString(kPersonalAddressbookUri) : prefVal);

should that be NS_LITERAL_CSTRING(kPersonalAddressbookUri) ?

Asking Mark for a module owner review.

Comment 82

[Scott MacGregor]

Attachment: [checked in]string cleanup for mailnews\local

Same string changes I just finished making to mailnews\base.

Comment 83

[Prasad Sunkari [:prasad]]

(In reply to comment #77)
> (From update of attachment 265952 [details] [diff] [review])
> nice!
> 

I am not able to build thunderbird unless I reverse 265952 patch... is it only with me or anyone else found the same too?

Most of the issues are with using nsAString where nsString is required.

Eg. getter_Copies() and  .Adopt()

Comment 84

[Scott MacGregor]

(In reply to comment #83)
> 
> I am not able to build thunderbird unless I reverse 265952 patch... is it only
> with me or anyone else found the same too?
> 
> Most of the issues are with using nsAString where nsString is required.
> 
> Eg. getter_Copies() and  .Adopt()

lol, I was telling David earlier today that I was having this same problem but all the tinderbox were building with it. I eventually had to do a clobber build and then things compiled. 

Comment 85

[Prasad Sunkari [:prasad]]

Should we also start removing the "+" operator on strings.  The "+" operator does not exist in frozen linkage.

This will mostly involve converting all "nsString writable = readable1 + readable2" to "nsString writable = readable1; writable.Append(readable2);"

Comment 86

[Prasad Sunkari [:prasad]]

Attachment: Partial string cleanup for mailnews/addrbook

This patch replaces nsXPIDL(C)String, PromiseFlat(C)String, Truncate(nn), Append*To*,(Lossy)AssignWithConversion and the '+' operator in mailnews/addrbook with equivalents as in scott's patch.

There are a few conflicts with the "convert nsIAbAddressCollector" patch, so I am obsoleting it and will resubmit it.

Comment 87

[David :Bienvenu]

Yes, I think we should remove the '+' operator on strings when we encounter them...at some point, we'll have to let the compiler find the remaining issues.

Comment 88

[Mark Banner (:standard8)]

(In reply to comment #86)
> Created an attachment (id=266058) [details]
> Partial string cleanup for mailnews/addrbook

Thanks for doing this. As I expect Scott will probably ask me for to review this as well, I'll just leap in here with some comments.

-                               utf8Email, getter_Copies(fullAddress));
+                               utf8Email.get(), getter_Copies(fullAddress));
       if (!fullAddress.IsEmpty())
       {
         /* We need to convert back the result from UTF-8 to Unicode */
         fullAddrStr = nsCRT::strdup(NS_ConvertUTF8toUTF16(fullAddress.get()).get());
       }

The nsCRT::strdup line looks wrong, do you really need fullAddress.get()? Can you not just use fullAddress?

-            mParser->MakeFullAddressWString (pDisplayNameStr, pDisplayNameStr, getter_Copies(pEmailStr[0]));  
+            mParser->MakeFullAddressWString (pDisplayNameStr.get(), pDisplayNameStr.get(), getter_Copies(pEmailStr[0]));  

As you're touching this line, please drop the space beween the function name and opening brackets (there's one or two other instances of this where you're touching lines that could do with fixing as well please).

   nsCAutoString fileNamePref;
-  fileNamePref = prefName + NS_LITERAL_CSTRING(".filename");
+  fileNamePref  = prefName;
+  fileNamePref += NS_LITERAL_CSTRING(".filename");

This would be better as:

nsCAutoString fileNamePref(prefName);
fileNamePref.AppendLiteral(".filename");

   nsCAutoString URI;
-  URI = NS_LITERAL_CSTRING("moz-abmdbdirectory://") + fileName;
+  URI  = NS_LITERAL_CSTRING("moz-abmdbdirectory://");
+  URI += fileName;

Similary this would be better as (note in this case, please replace the text with the constant we've already got defined as well):

nsCAutoString URI(NS_LITERAL_CSTRING(kMDBDirectoryRoot);
URI.Append(fileName);

The above two patterns you've used in several places, please change the other locations as well in a similar manner. There's also other places where you've used += that would be better served with Append and AppendNative.

-    if (!(const PRUnichar*)prevSearchString || ((const PRUnichar*)prevSearchString)[0] == 0)
+    if (prevSearchString.IsEmpty() || prevSearchString.IsEmpty())

Those two if comparisons say the same thing...

 NS_IMETHODIMP nsAbDirProperty::CopyMailList(nsIAbDirectory* srcList)
 {
-  nsXPIDLString str;
+  nsString str;
 	srcList->GetDirName(getter_Copies(str));
-	SetDirName(str);
+	SetDirName(str.get());

As you're here, and modifying this small function, please reformat the whole function to use two-space indentation rather than tabs (the 'new' lines shouldn't use tabs anyway).

-  return m_DirectoryPrefs->SetCharPref(aName, PromiseFlatCString(aValue).get());
+  return m_DirectoryPrefs->SetCharPref(aName, nsCString(aValue).get());

I'm fairly sure you should just be able to do aValue.get(), there may be other places you've done this as well...

-    if (nsCRT::strcasecmp (name, "card:nsIAbCard") != 0)
+    if (! name.Equals("card:nsIAbCard", nsCaseInsensitiveCStringComparator()))

There should be no space between ! and name.

-                AppendUTF16toUTF8(nsDependentString(iter.get(), 1), *aValue); //XXXjag poke me about string generators
+                aValue->Append(NS_ConvertUTF16toUTF8(nsDependentString(iter.get(),1)));

I think something like aValue->AppendWithConversion(iter.get(), 1) should work better here.

-    rv = aMessage->GetValues(PromiseFlatCString(aAttrName).get(), &numVals, 
+    rv = aMessage->GetValues(nsCString(aAttrName).get(), &numVals, 
                              &values);

aAttrName.get() should be sufficient here

-    AppendUTF16toUTF8(values[0], aValue);
+    aValue.Append(NS_ConvertUTF16toUTF8(values[0]));

Use AppendWithConversion again. Ditto in other places.

     if (bIsMailList) {
-        nsXPIDLString pListName;
+        nsString pListName;
       rv = list->GetDirName(getter_Copies(pListName));

Indentation is wrong, please fix

-        aSource.Truncate(offset) ; 
+        aSource.SetLength(offset) ; 

Drop the space before the ; please (ditto in other places where you've changed the lines).

Comment 89

[Scott MacGregor]

Attachment: [checked in]string cleanup for mailnews\mime

David already removed xpidlstring from mime, this removes some of the other trouble spots we've already done for local & base.

Comment 90

[David :Bienvenu]

Attachment: [checked in]fix some issues with previous patches

The original problem was the nsCString(propertyValue) one, but deleting my panacea.dat exposed a few other issues...

Comment 91

[Scott MacGregor]

Comment on attachment 266115 [details] [diff] [review]
[checked in]fix some issues with previous patches

thanks David, I'll keep an eye out for that nsCString pattern in the future.

Comment 92

[Scott MacGregor]

Attachment: [checked in]string cleanup for mailnews\news

Comment 93

[Nick Kreeger]

Attachment: nsIImapService cleanup V1

The first go-round for the nsIImapService cleanup. This is a white-space free patch.

Comment 94

[Nick Kreeger]

Attachment: nsIImapService cleanup V1

Updated patch, I broke the quoting code by my replacement of |PL_strstr()|. I needed to add a |!= kNotFound| here:

if (messageURI.Find(NS_LITERAL_CSTRING("&type=application/x-message-display")) != kNotFound)

Comment 95

[Philip Chee]

I'm getting a crash every time I try to fetch new nntp messages (Talkback ID TB32578768X).

Stack Signature	 nsNntpService::FetchMessage aa8774b6
Product ID	MozillaTrunk
Build ID	2007052708
Trigger Time	2007-05-27 21:54:14.0
Platform	Win32
Operating System	Windows NT 5.1 build 2600
Module	msgnews.dll + (0000439f)
URL visited	
User Comments	Fetching nntp articles.
Since Last Crash	12 sec
Total Uptime	146 sec
Trigger Reason	Access violation
Source File, Line No.	d:\builds\tinderbox\seamonkeytrunk\winnt_5.2_clobber\mozilla\mailnews\news\src\nsnntpservice.cpp, line 394
Stack Trace 	
nsNntpService::FetchMessage  [mozilla/mailnews/news/src/nsnntpservice.cpp, line 394]
nsNewsDownloader::DownloadNext  [mozilla/mailnews/news/src/nsnewsdownloader.cpp, line 146]
nsNewsDownloader::DownloadArticles  [mozilla/mailnews/news/src/nsnewsdownloader.cpp, line 74]
nsMsgSearchSession::TimerCallback  [mozilla/mailnews/base/search/src/nsmsgsearchsession.cpp, line 539]
nsTimerImpl::Fire  [mozilla/xpcom/threads/nstimerimpl.cpp, line 383]
NS_ProcessNextEvent_P  [mozilla/xpcom/build/nsthreadutils.cpp, line 227]
nsBaseAppShell::Run  [mozilla/widget/src/xpwidgets/nsbaseappshell.cpp, line 154]
main  [mozilla/xpfe/bootstrap/nsapprunner.cpp, line 1684]
WinMain  [mozilla/xpfe/bootstrap/nsapprunner.cpp, line 1708]
kernel32.dll + 0x16d4f (0x7c816d4f)

 CVS Blame points to <http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/mailnews/news/src/nsNntpService.cpp&mark=394&rev=#394>
which points to this bug.

Comment 96

[Scott MacGregor]

Good catch Philip. aURL can be null and we need to handle that. I've checked in the fix for that.

Comment 97

[Joerg Bornemann]

Attachment: nsIMsgThread

First try of nsIMsgThread cleanup.

Comment 98

[Joerg Bornemann]

Attachment: [checked in]nsIMsgThread

second try :)

Comment 99

[Prasad Sunkari [:prasad]]

Attachment: Partial string cleanup for addrbook and migration of nsIAddressCollector to AString/ACString

Mark, I fixed the patch based on your comments... but there are a few things I wanted to discuss.

1. 

-  return m_DirectoryPrefs->SetCharPref(aName,PromiseFlatCString(aValue).get());
+  return m_DirectoryPrefs->SetCharPref(aName, nsCString(aValue).get());

The compilation failed when I used aValue.get() because aValue is of type nsACString and get() is not a function defined in nsACString (it is a function in nsCString) - I am using GCC4.1.3.  Am I missing something?

2. 

- AppendUTF16toUTF8(nsDependentString(iter.get(), 1), *aValue);
+ aValue->Append(NS_ConvertUTF16toUTF8(nsDependentString(iter.get(),1)));

I could have used aValue->AppendWithConversion(iter.get(), 1), but the function AppendWithConversion will not be available in the frozen linkage, so I preferred using Append with NS_ConvertUTF16toUTF8.


Thanks,

Comment 100

[Mark Banner (:standard8)]

(In reply to comment #99)
> Created an attachment (id=266443) [details]
> 1. 
> -  return
> m_DirectoryPrefs->SetCharPref(aName,PromiseFlatCString(aValue).get());
> +  return m_DirectoryPrefs->SetCharPref(aName, nsCString(aValue).get());
> The compilation failed when I used aValue.get() because aValue is of type
> nsACString and get() is not a function defined in nsACString (it is a function
> in nsCString) - I am using GCC4.1.3.  Am I missing something?

No you're right. I was assuming it was possible without first checking.

> 2. 
> - AppendUTF16toUTF8(nsDependentString(iter.get(), 1), *aValue);
> + aValue->Append(NS_ConvertUTF16toUTF8(nsDependentString(iter.get(),1)));
> I could have used aValue->AppendWithConversion(iter.get(), 1), but the function
> AppendWithConversion will not be available in the frozen linkage, so I
> preferred using Append with NS_ConvertUTF16toUTF8.

Again you're right, I'd forgotten that when originally looking at this patch.

I'll try and look at the patch in full in the next day or two.

Comment 101

[Scott MacGregor]

Comment on attachment 266258 [details] [diff] [review]
nsIImapService cleanup V1

I think nsEscape returns a newly allocated string, so I think this new code might be leaking a string buffer:

-  *folderName = nsEscape(onlineName.get(), url_Path);
+  folderName = nsEscape(onlineName.get(), url_Path);

This might need to be something like:

folderName.Adopt(nsEscape(...));

I would have probably left the command argument in nsImapService::FolderCommand alone (as a char *) instead of a nsCString since every caller passes in a string literal anyway instead of making all the callers use NS_LITERAL_CSTRING.

This looked funny to me, I think it's the cast going on here:

+    aImapMailFolder->HasMsgOffline(atoi(((nsDependentCString)messageIdentifierList).get()), &useLocalCache);  

I wonder if that should instead be:

HasMsgOffline(atoi(nsCString(messageIdentitiferList).get())), ...);

In NS_IMETHODIMP nsImapService::CopyMessage:

if (!aSrcMailboxURI || !aMailboxCopy) return rv;

can instead by
NS_ENSURE_ARG_POINTER(aSrcMailboxURI);
NS_ENSURE_ARG_POINTER(aMailboxCopy);

and then we don't need to initialize rv to NS_ERROR_NULL_POINTER.

ditto for this:

   if (!keys || !aMailboxCopy) 
     return NS_ERROR_NULL_POINTER;
in ::CopyMessages.

In  nsImapService::FetchMessage, I'd remove these lines:

   NS_ASSERTION (aImapUrl && aImapMailFolder &&  aImapMessage,"Oops ... null pointer");
   if (!aImapUrl || !aImapMailFolder || !aImapMessage)
       return NS_ERROR_NULL_POINTER;

and replace then with NS_ENSURE_ARG_POINTER.

ditto for nsImapService::GetHeaders, nsImapService::GetBodyStart, nsImapService::FolderCommand, nsImapService::Biff,  nsImapService::DeleteMessages and anywhere else we are asserting on method arguments.

In nsImapService::SetImapUrlSink, we can remove the assertion on the method arguments since we are using NS_ENSURE_ARG_POINTER and we don't need to initialize rv.

remove the argument assertion in nsImapService::DiscoverAllFolders, nsImapService::DiscoverAllAndSubscribedFolders,nsImapService::DiscoverChildren.

In nsImapService::DiscoverChildren, 

         if (NS_SUCCEEDED (rv1) && hierarchySeparator != kOnlineHierarchySeparatorUnknown &&
           uriDelimiter != hierarchySeparator)
+        {
           aImapUrl->SetOnlineSubDirSeparator((char)hierarchySeparator);
-
+        }

We don't need the braces around the single statement in the if clause. I would change this part:

rv = NS_ERROR_NULL_POINTER; 

to NS_ERROR_FAILURE since it's detecting that the folderPath string is empty (not a null pointer) now. 

we can remove the assertion at the top of nsImapService::OnlineMessageCopy.

+    nsCOMPtr<nsIURI> msgUrlUri = do_QueryInterface(msgUrl);
+    msgUrlUri.swap(*aURL);

is the QI here really necessary? nsIMsgMailNewsUrl inherits nsIURI, I would have thought it would just work.

This:
+      if (!escapedNewName) 
+        return NS_ERROR_OUT_OF_MEMORY;

could be 

NS_ENSURE_TRUE(escapedNewName, NS_ERROR_OUT_OF_MEMORY); 

since wed expect an out of memory condition to be unexpected.

In GetListofFoldersWithPath, 
+  NS_ENSURE_SUCCESS(rv, rv);
+  if (!rootMsgFolder) 
+    return NS_ERROR_FAILURE;

could be
NS_ENSURE_TRUE(NS_SUCCEEDED(rv) && rootMsgFolder, NS_ERROR_FAILURE);

This:

   rv = DiscoverChildren(NS_GetCurrentThread(), msgFolder, listener, folderPath, nsnull);
-  if (NS_FAILED(rv)) return rv;
+  NS_ENSURE_SUCCESS(rv, rv);
   
   return NS_OK;

could just be

return DiscoverChildren(...);

ditto for GetListOfFoldersOnServer

Fix those nits and r=mscott

Nice work Nick!

Comment 102

[Scott MacGregor]

Attachment: [checked in]string cleanup for mailnews\db

Comment 103

[Scott MacGregor]

Comment on attachment 266414 [details] [diff] [review]
[checked in]nsIMsgThread

Jorg sent me a cvs diff -u patch which I reviewed via e-mail. Here are the comments & tweaks I made to the patch before checking it in:

I found one bug with the patch:

In nsMsgThread::GetRootHdr:

-    nsresult ret = GetChildHdrForKey(m_threadRootKey, result, resultIndex);
-    if (NS_SUCCEEDED(ret) && *result)
-      return ret;
-    else
-    {
+    nsresult rv = GetChildHdrForKey(m_threadRootKey, result, resultIndex);
+    NS_ENSURE_SUCCESS(rv, rv);

The logic here is wrong as we are no longer returning when *result gets a value from GetChildHdrForKey. when I ran thunderbird with the patch for the first time, I got a bunch of assertions here which is how I spotted it :)

The fix:

    nsresult rv = GetChildHdrForKey(m_threadRootKey, result, resultIndex);
    if (NS_SUCCEEDED(rv) && *result)
      return rv;

And I fixed a few minor nits as well:

1) I trimmed some trailing spaces in nsMsgThread.cpp
2)

+  //do I have to release hdrRow?
+  rv = m_mdbTable->PosToRow(m_mdbDB->GetEnv(), aIndex, &hdrRow);
+  NS_ENSURE_SUCCESS(rv, rv);

I was worried that PosToRow might return NS_OK when hdrRow is null so I changed it to:

  NS_ENSURE_TRUE(NS_SUCCEEDED(rv) && hdrRow, NS_ERROR_FAILURE);

just to be safe.

3) In EnumerateMessage I changed:

+  NS_ADDREF(e);
+  *result = e;

to NS_ADDREF(*result = e);

4) In ReparentMsgsWithInvalidParent,
-    ret  = GetChildHdrAt(childIndex, getter_AddRefs(curChild));
-    if (NS_SUCCEEDED(ret) && curChild)
+    rv  = GetChildHdrAt(childIndex, getter_AddRefs(curChild));
+    if (NS_SUCCEEDED(rv))

I put back the && curChild just in case GetChildHdrAt returns NS_OK with a null curChild.

Comment 104

[Scott MacGregor]

Attachment: [checked in]string cleanup for mailnews\extensions

Comment 105

[Nick Kreeger]

Attachment: [checked in]nsIImapService cleanup V3

Should meet all of mscott's comments.

Comment 106

[Nick Kreeger]

(In reply to comment #105)
> Created an attachment (id=266581) [details]
> nsIImapService cleanup V3
> 
> Should meet all of mscott's comments.
> 


Oh, forgot to mention that GCC 3.3 (Mac OS X variety) did not like:
    msgUrl.swap(*aURL);

So I stuck with:
    nsCOMPtr<nsIURI> msgUrlUri = do_QueryInterface(msgUrl);
    msgUrlUri.swap(*aURL);

Comment 107

[Scott MacGregor]

Comment on attachment 266581 [details] [diff] [review]
[checked in]nsIImapService cleanup V3

I forgot to tell you to bump the interface ID on nsIImapService.idl since we are changing the interface.

I'll let you land the patch. Nice work Nick!

Comment 108

[Nick Kreeger]

(In reply to comment #107)
> (From update of attachment 266581 [details] [diff] [review])
> I forgot to tell you to bump the interface ID on nsIImapService.idl since we
> are changing the interface.
> 
> I'll let you land the patch. Nice work Nick!
> 

New UUID (sorry, I forgot to as well):
[scriptable, uuid(16d88469-a6ba-4389-ae75-e306373afc2a)]

Comment 109

[Nick Kreeger]

Comment on attachment 266581 [details] [diff] [review]
[checked in]nsIImapService cleanup V3

Checked the |nsIImapService| patch into HEAD.

Comment 110

[David :Bienvenu]

Attachment: [checked in]remove xpidl strings from compose/src

There's a lot more string cleanup needed here, but this gets rid of xpidl strings...

Comment 111

[[e-mail]]

Comment on attachment 265319 [details] [diff] [review]
[checked in]nsIMsg*Folder.idl

>+  url.swap(*aURL);
XPCOM doesn't require the caller to init their out parameter (getter_AddRefs always does, but only because of an implementation detail), so generally you have to write *aURL = nsnull; first. Most people actually add this at the beginning, so that *aURL is always null on failure.

Comment 112

[[e-mail]]

Comment on attachment 265319 [details] [diff] [review]
[checked in]nsIMsg*Folder.idl

>+  mReadSet = nsMsgKeySet::Create(nsPromiseFlatCString(newsrcLine).get());
PromiseFlatCString doesn't have an ns prefix (nsPromiseFlatString is internal).

Comment 113

[Nick Kreeger]

Attachment: nsIMailboxSpec cleanup V1

Here is the first go-round for |nsIMailboxSpec|. 

The class is defined in nsImapCore.h and defined in nsImapUtils.cpp.

Could I suggest moving some things around, such as keep the #define's and the typedef's in nsImapCore.h and move the class definitions to perhaps nsImapUtils.h?

Comment 114

[Scott MacGregor]

Comment on attachment 266674 [details] [diff] [review]
[checked in]remove xpidl strings from compose/src

Is StringBeginsWith case insensitive?

-  if (!PL_strncasecmp (url, "news:", 5) ||
-    !PL_strncasecmp (url, "snews:", 6) ||
-    !PL_strncasecmp (url, "IMAP:", 5) ||
-    !PL_strncasecmp (url, "mailbox:", 8))
+  if (StringBeginsWith (url, NS_LITERAL_CSTRING("news:")) ||
+    StringBeginsWith (url, NS_LITERAL_CSTRING("snews:")) ||
+    StringBeginsWith (url, NS_LITERAL_CSTRING("IMAP:")) ||
+    StringBeginsWith (url, NS_LITERAL_CSTRING("mailbox:")))

I think this can just be:

+        dialogMessage.Append(PRUnichar('\n');

I suspect you just haven't made the Assign/AppendWithConversion pass yet:

-        unicodeString.AssignWithConversion((const char*)str);
+        unicodeString.AssignWithConversion(str);

-    *aResultArray = smtpServerResources;
-    NS_ADDREF(*aResultArray);
+    NS_ADDREF(*aResultArray = smtpServerResources);

I think we can use .swap there.

Can this use .swap?

+        NS_IF_ADDREF(*aResult = server);

Comment 115

[Scott MacGregor]

Comment on attachment 266763 [details] [diff] [review]
nsIMailboxSpec cleanup V1

I like your idea of moving the class declaration into nsImapUtils.h and out of nsImapCore.

Can we use nsString::Find here?

+          PL_strstr(adoptedBoxSpec->mAllocatedPathName.get(), GetTrashFolderName()))

i.e. adoptedBoxSpec->mAllocatedPathName.Find(...)

Here:

-                trashExists = !PL_strncasecmp(adoptedBoxSpec->allocatedPathName, serverTrashName, 6) && /* "INBOX/" */
-                              !PL_strcmp(adoptedBoxSpec->allocatedPathName + 6, serverTrashName + 6);
+                trashExists = !PL_strncasecmp(adoptedBoxSpec->mAllocatedPathName.get(), serverTrashName, 6) && /* "INBOX/" */
+                              !PL_strcmp(adoptedBoxSpec->mAllocatedPathName.get() + 6, serverTrashName + 6);

I think we can use StringBeginsWith  and .Equals

This can be .Equals too:
+                trashExists = (PL_strcmp(serverTrashName, adoptedBoxSpec->mAllocatedPathName.get()) == 0);

This:
-        m_deletableChildren->AppendElement((void*)
-          nsCRT::strdup(adoptedBoxSpec->allocatedPathName));
-        PR_FREEIF(adoptedBoxSpec->hostName);
+        m_deletableChildren->AppendElement((void*)nsCRT::strdup(adoptedBoxSpec->mAllocatedPathName.get()));

can just be:

AppendElement((void *) ToNewCString(adoptedBoxSpec->mAllocatedPathName));

These:
+    boxSpec->mAllocatedPathName.Assign("");
+    boxSpec->mHostName.Assign("");;

can be replaced with calls to string.Truncate();

I think this leaks allocatedPathStr:

+    char *allocatedPathStr;
+    boxSpec->mConnection->GetCurrentUrl()->AllocateCanonicalPath(boxname, boxSpec->mHierarchySeparator, &allocatedPathStr);
+    boxSpec->mAllocatedPathName.Assign(allocatedPathStr);

Instead use the getter_Copies macro on mAllocatedPathName:
    boxSpec->mConnection->GetCurrentUrl()->AllocateCanonicalPath(boxname, boxSpec->mHierarchySeparator, getter_Copies(boxSpec->mAllocatedPathName));

I think that getter_Copies pattern can be used in several places where you are assigning into mAllocaedPathName.

mAllocatedPathName is a nsCString right? So you don't need the strdup here, you can just assign the string into the nsCString and it does the copy. The strdup causes us to leak a copy of mailboxNameToConvert.
+  returnSpec->mAllocatedPathName = strdup(mailboxNameToConvert);

If mHostName is a nsCString you don't want the ToNewCString call here. Just assign GetImapHostname() into mHostName. nsCString handles the copy.
+            boxSpec->mHostName = ToNewCString(GetImapHostName());

This can be .Truncate():

+    returnSpec->mHostName.Assign("");

This can use .Truncate and there's an extra ';':

+    boxSpec->mHostName.Assign("");;

Change the name of this argument back to hostname:

+nsImapURI2FullName(const char* rootURI, const char* mHostName, const char* uriStr,

the m before the variable name implies that it's a member of a class.

It doesn't matter much, but you can use the = operator for this type of expression:

+  aAllocatedPathName.Assign(mAllocatedPathName);

mAllocatedPathName = aAllocatedPathName;

ditto for things like:

+  mUnicharPathName.Assign(aCopy.mUnicharPathName);

Other than it looks good!

Comment 116

[Mark Banner (:standard8)]

Comment on attachment 266443 [details] [diff] [review]
Partial string cleanup for addrbook and migration of nsIAddressCollector to AString/ACString

Sorry for the delay in this.

You've still got a few function arguments with:

'const nsAString& aAddress' instead of 'const nsAString &aAddress'

I'd prefer nsCString(aName).get() rather than ((nsCString)aName).get()

-    rv = GetCardFromAttribute(kPriEmailColumn, curAddress, getter_AddRefs(existingCard));
+    rv = GetCardFromAttribute(NS_LITERAL_CSTRING(kPriEmailColumn), nsCString(curAddress), getter_AddRefs(existingCard));

curAddress is a char* pointer, therefore you should be using nsDependentCString as its more efficient, same goes for curName.

+  int atPos = aEmail.FindChar('@');
+  if (atPos == kNotFound) 
     return NS_OK;
-  
-  const char *domain = atPos + 1;
- 
-  if (!domain)
+    
+  if (atPos + 1 == aEmail.Length()) // Check for empty domain
     return NS_OK;

I think you can make this a lot simpler by doing:

if (atPos == -1 || (atPos + 1 == aEmail.Length()))
  return NS_OK;

(I've a feeling kNotFound isn't available on frozen api).

+  nsCString domain;
+  nsCString(aEmail).Mid(domain, atPos + 1, aEmail.Length() - (atPos + 1));
+  if (domain.IsEmpty())     // Redundant
+    return NS_OK; 

If its redundant you shouldn't need to do it. Can't you use Right() here as well instead of Mid? If you used Right you could drop the if (atPos + 1 == aEmail.Length()) check and keep the domain.IsEmpty one

 -  if (strcmp(domain,"aol.com") && 
-      strcmp(domain,"cs.com") && 
-      strcmp(domain,"netscape.net"))
-    return NS_OK;
+  if (domain.Equals("aol.com") || domain.Equals("cs.com") || domain.Equals("netscape.net")) {

I agree that the first version was over kill on the short line lengths, but please try and keep line lengths to < 80 chars (move the last domain.Equals to the next line).

+    nsCString(aEmail).Left(userName, atPos);

Seeing as you could have to do nsCString(aEmail) in a couple of places, you could just copy it to a new variable at the top of the funtion and use that.

+    nsCString(aFullName).Mid(lastName, index + 1, aFullName.Length() - (index + 1));

Right() again.

-  rv = SetAbURI(prefVal.IsEmpty() ? kPersonalAddressbookUri : prefVal.get());
+  rv = SetAbURI(prefVal.IsEmpty() ? nsDependentCString(kPersonalAddressbookUri) : prefVal);

NS_LITERAL_CSTRING for constants (2 places).

-	nsXPIDLString str;
+	nsString str;

 NS_IMETHODIMP nsAbCardProperty::Copy(nsIAbCard* srcCard)
 {
   NS_ENSURE_ARG_POINTER(srcCard);
 
Spaces please!

-      bridgeURI = NS_LITERAL_CSTRING(kLDAPDirectoryRoot) + prefName;
+      bridgeURI =  NS_LITERAL_CSTRING(kLDAPDirectoryRoot);
+      bridgeURI += prefName;

An extra space slipped in there.

I'm assuming we'll have to go through a second patch to remove the "=" on strings? (I'm happy to do it that way)

Thanks for the update. Its only really the ns(I)AbAddressCollector changes that are causing the r- at the moment. So separate it into two if you wish. I should have time over the weekend to review another update.

Comment 117

[Scott MacGregor]

Attachment: [checked in]string cleanup for mailnews\import

Comment 118

[David :Bienvenu]

Comment on attachment 266674 [details] [diff] [review]
[checked in]remove xpidl strings from compose/src

I believe I addressed all the very helpful comments in what I checked in - fixed all the AssignWithConversions, the StringBeginsWith, and the swaps, where I could.

Comment 119

[Scott MacGregor]

Attachment: [checked in]string cleanup for mailnews\mapi

I think this is the last of the directories for pure string cleanup.

Comment 120

[David :Bienvenu]

Comment on attachment 266943 [details] [diff] [review]
[checked in]string cleanup for mailnews\mapi

yay!

Comment 121

[Scott MacGregor]

Attachment: [checked in]string cleanup for mail\

Comment 122

[Prasad Sunkari [:prasad]]

(In reply to comment #116)
> (From update of attachment 266443 [details] [diff] [review])
>
> (I've a feeling kNotFound isn't available on frozen api).

right, kNotFound isn't available in frozen api

> -  rv = SetAbURI(prefVal.IsEmpty() ? kPersonalAddressbookUri : prefVal.get());
> +  rv = SetAbURI(prefVal.IsEmpty() ?
> nsDependentCString(kPersonalAddressbookUri) : prefVal);

The first thing I did here was to use NS_LITERAL_CSTRING, but the compiler (GCC 4.2 on Linux) did not like it.  It said - /nsAbAddressCollecter.cpp:311: error: no match for ternary 'operator?:' in '(prefVal.nsCString::<anonymous>.nsACString_internal::IsEmpty() != 0) ? (const nsDependentCString&)((const nsDependentCString*)(& nsDependentCString(((const char*)"moz-abmdbdirectory://abook.mab"), 30u))) : prefVal'

> 
> I'm assuming we'll have to go through a second patch to remove the "=" on
> strings? (I'm happy to do it that way)
> 

Does that mean that we should be using "Assign" or some silimar function instead of '=' on strings?

Comment 123

[Joerg Bornemann]

Attachment: nsIMsgStatusFeedback

This is the cleanup of nsIMsgStatusFeedback.

Comment 124

[Nick Kreeger]

Attachment: nsIMailboxSpec cleanup V2

Should address all of mscott's comments.

I moved the two classes out of nsImapCore.h and into nsImapUtils.h and nsImapProtocol.h.

Comment 125

[Scott MacGregor]

Attachment: [checked in]misc string changes in mozilla\mailnews

Remove nsAdoptingCString, change nsPromiseFlat(C)String to PromiseFlat(C)String, remove nsPrintfCString.

Comment 126

[Scott MacGregor]

Comment on attachment 267001 [details] [diff] [review]
nsIMsgStatusFeedback

+    return m_statusFeedback->ShowStatusString (nsString(aMsg));

I think this should be nsDependentString not nsString.

Neil was telling me the other day that we can use PromiseFlatString here:

+  return OnStatusChange(nsnull, nsnull, NS_OK, nsString(aStatus).get());

when we move to the frozen string API PromiseFlatString is typdefed as a nsString.

I know we got some bad luck with this with the thread patch, but when getting a service using do_GetService, we can use NS_ENSURE_SUCCESS(rv, rv);

+  nsresult rv;
+  nsCOMPtr<nsIStringBundleService> bundleService =
+    do_GetService(NS_STRINGBUNDLE_CONTRACTID, &rv);
 
-    if (NS_SUCCEEDED(rv))
-        bundleService->CreateBundle("chrome://messenger/locale/messenger.properties",
-                                    getter_AddRefs(mBundle));
+  if (NS_SUCCEEDED(rv))
+    bundleService->CreateBundle("chrome://messenger/locale/messenger.properties",
+  

Patch looks really good, great work! Would you like me to change the nsString's to nsDependentStrings and use PromiseFlatString in the few places I mentioned above before I check this in or would you like to make those changes yourself?

Comment 127

[Prasad Sunkari [:prasad]]

Attachment: [checked in]Partial string cleanup for addrbook and migration of nsIAddressCollector to AString/ACString - with corrections suggested by Mark

Mark, I fixed most of the mistakes you pointed out.  One thing I could no do is suing NS_LITERAL_CSTRING instead of nsDependentString for a constant in one place.  GCC simply refuses to accept NS_LITERAL_CSTRING.

Heres what GCC said: nsAbAddressCollecter.cpp:311: error:
no match for ternary 'operator?:' in
'(prefVal.nsCString::<anonymous>.nsACString_internal::IsEmpty() != 0) ? (const
nsDependentCString&)((const nsDependentCString*)(& nsDependentCString(((const
char*)"moz-abmdbdirectory://abook.mab"), 30u))) : prefVal'

Otherwise, all other things are corrected.

Comment 128

[Prasad Sunkari [:prasad]]

Comment on attachment 267110 [details] [diff] [review]
[checked in]Partial string cleanup for addrbook and migration of nsIAddressCollector to AString/ACString - with corrections suggested by Mark

checked the patch mime-type

Comment 129

[Joerg Bornemann]

(In reply to comment #126)

> Patch looks really good, great work! Would you like me to change the nsString's
> to nsDependentStrings and use PromiseFlatString in the few places I mentioned
> above before I check this in or would you like to make those changes yourself?

I'll make the changes and will submit a new patch. 

Comment 130

[[e-mail]]

Comment on attachment 265710 [details] [diff] [review]
[checked in]nsIMsgFolderCacheElement and nsIMsgFolderCache

>-  nsCStringKey hashKey(pathKey);
>-  
>+  nsCStringKey hashKey(PromiseFlatCString(pathKey).get());
nsCStringKey will in fact accept an nsACString too, so these changes were unnecessary.


>-NS_IMPL_GETTER_STR(nsMsgFolderCacheElement::GetKey, m_folderKey)
>-NS_IMPL_SETTER_STR(nsMsgFolderCacheElement::SetKey, m_folderKey)
Are we missing NS_IMPL_G/SETTER_ACSTR macros?

>+        yarn.mYarn_Buf = (void *) PromiseFlatCString(propertyValue).get();
The PromiseFlatCString destructor will make this value stale.

> void nsMsgFolderCacheElement::SetMDBRow(nsIMdbRow	*row) 
> {
>   if (m_mdbRow)
>     NS_RELEASE(m_mdbRow);
>-  m_mdbRow = row;
>-  if (row)
>-    NS_ADDREF(row);
>+  NS_IF_ADDREF(m_mdbRow = row);
> }
Why not make m_mdbRow an nsCOMPtr (or nsRefPtr)?

Comment 131

[[e-mail]]

Comment on attachment 265694 [details] [diff] [review]
[checked in]nsMsgProgress

>     nsCOMPtr<nsISupports> aSupports;
>-    nsCOMPtr<nsIWebProgressListener> aProgressListener;
>+    nsCOMPtr<nsIWebProgressListener> progressListener;
>     for (i = count - 1; i >= 0; i --)
>     {
>       m_listenerList->GetElementAt(i, getter_AddRefs(aSupports));
>-      aProgressListener = do_QueryInterface(aSupports);
>-      if (aProgressListener)
>-        aProgressListener->OnStatusChange(aWebProgress, aRequest, aStatus, aMessage);
>+      progressListener = do_QueryInterface(aSupports);
>+      if (progressListener)
>+        progressListener->OnStatusChange(aWebProgress, aRequest, aStatus, aMessage);
>     }
As you were touching this you could have taken the opportunity to convert to progressListener = do_QueryElementAt(m_listenerList, i); or better still convert m_listenerList to an nsCOMArray.

Comment 132

[Mark Banner (:standard8)]

Comment on attachment 267110 [details] [diff] [review]
[checked in]Partial string cleanup for addrbook and migration of nsIAddressCollector to AString/ACString - with corrections suggested by Mark

This looks ok now, r=me. Please get sr from David or Scott before checking in.

Comment 133

[Scott MacGregor]

Comment on attachment 267015 [details] [diff] [review]
nsIMailboxSpec cleanup V2

I'm probably just missing something but:
-      m_runningUrl->AllocateServerPath(mb->GetMailboxName(),
+      m_runningUrl->AllocateServerPath(nsDependentCString(mb->GetMailboxName()).get(),

isn't:

nsDependentCString(mb->GetMailboxName()).get() == mb->GetMailboxName() ?

ditto here:

-      m_runningUrl->AllocateServerPath(mb->GetMailboxName(),
+      m_runningUrl->AllocateServerPath(nsDependentCString(mb->GetMailboxName()).get(),

+            boxSpec->mHostName = ToNewCString(GetImapHostName());

mHostName is a nsCString so it will allocate a buffer on its own, no need to call ToNewCString. In fact that line leaks a string as is.

-          if (FolderNeedsACLInitialized(mb->GetMailboxName()))
+          if (FolderNeedsACLInitialized(nsDependentCString(mb->GetMailboxName()).get()))

-            m_runningUrl->AllocateServerPath(mb->GetMailboxName(), mb->GetDelimiter(), &onlineName);
+            m_runningUrl->AllocateServerPath(nsDependentCString(mb->GetMailboxName()).get(), 
+                                             mb->GetDelimiter(), &onlineName);

Everything else looked good to me.

asking david for an sr.

Comment 134

[Scott MacGregor]

Comment on attachment 267110 [details] [diff] [review]
[checked in]Partial string cleanup for addrbook and migration of nsIAddressCollector to AString/ACString - with corrections suggested by Mark

While I don't think it matters too much, I'm starting to use PromiseFlat(C)String instead of nsCString for things like:

-  rv = pHeader->ParseHeaderAddresses(nsnull, aAddress, &names, &addresses, &numAddresses);
+  rv = pHeader->ParseHeaderAddresses(nsnull, nsCString(aAddress).get(), &names, &addresses, &numAddresses);

even though PromiseFlat(C)String is typedefed to ns(C)String...

Patch looks great Prasad, this is such a huge help!

Comment 135

[Scott MacGregor]

Attachment: [checked in]remove use of NS_LINEBREAK and NS_LINEBREAK_LEN

NS_LINEBREAK and NS_LINEBREAK_LEN are defined in nsCRT.h and won't be available to us going forward. I replaced all call sites with MSG_LINEBREAK which we were already using in some places along with defining MSG_LINEBREAK so it doesn't depend on NS_LINEBREAK.

Next up is to figure out what to do about nsCRT::CR and nsCRT::LF...hmmmm

Comment 136

[Scott MacGregor]

Comment on attachment 267200 [details] [diff] [review]
[checked in]remove use of NS_LINEBREAK and NS_LINEBREAK_LEN

this patch also contained:

nsCStringKey optimizations suggested by Neil, removal of several tmpStr variables now that we can use nsA(C)Strings with the getter_Copies macro.

Comment 137

[David :Bienvenu]

Comment on attachment 267200 [details] [diff] [review]
[checked in]remove use of NS_LINEBREAK and NS_LINEBREAK_LEN

-    if (nsCRT::strncmp(body+i, NS_LINEBREAK, NS_LINEBREAK_LEN)) {
+    if (nsCRT::strncmp(body+i, MSG_LINEBREAK, MSG_LINEBREAK_LEN)) {
 

while you're here, can this just use strncmp, since it's char *, and nsCRT is going away?

Comment 138

[Scott MacGregor]

It looks like /browser/ code converted spots where it used nsCRT::LF to '\n' and nsCRT::CR to '\r'. Any objections to us doing something similar or should we add a typedef in msgCore.h and use that instead?

Comment 139

[David :Bienvenu]

I'm happy with '\n' and '\r' - those chars haven't changed in decades. It's hard to imagine them changing now

Comment 140

[Scott MacGregor]

Comment on attachment 267015 [details] [diff] [review]
nsIMailboxSpec cleanup V2

I meant to add r=mscott if you fix the problems I pointed out in my last comment (or point out what I missed with the nsDependentCString use case).

Comment 141

[Scott MacGregor]

Attachment: [checked in]replace nsCRT::CR and nsCRT::LF with '\r' and '\n' respectively

I did a global search and replace for mailnews & mail. I then did a basic smoketest  on the results.

Comment 142

[Prasad Sunkari [:prasad]]

Scott or someone, can you check in the patch (already reviewed and super-reviewed)

(In reply to comment #134)
> (From update of attachment 267110 [details] [diff] [review])
> While I don't think it matters too much, I'm starting to use
> PromiseFlat(C)String instead of nsCString for things like:
> 
> -  rv = pHeader->ParseHeaderAddresses(nsnull, aAddress, &names, &addresses,
> &numAddresses);
> +  rv = pHeader->ParseHeaderAddresses(nsnull, nsCString(aAddress).get(),
> &names, &addresses, &numAddresses);
> 
> even though PromiseFlat(C)String is typedefed to ns(C)String...
> 
> Patch looks great Prasad, this is such a huge help!
> 

Comment 143

[David :Bienvenu]

Comment on attachment 267015 [details] [diff] [review]
nsIMailboxSpec cleanup V2

+// This class is currently only used for the one-time upgrade
+// process from a LIST view to the subscribe model.
+// It basically contains the name of a mailbox and whether or not
+// its children have been listed.

I don't think the first two lines of this comment are right...looking at the code, I think we use this class in several situations, not just in a one-time upgrade. I know you just moved this comment, but might as well fix it :-)

-NS_IMPL_GETSET(nsImapMailboxSpec, Folder_UIDVALIDITY, PRInt32, folder_UIDVALIDITY)
-NS_IMPL_GETSET(nsImapMailboxSpec, NumMessages, PRInt32, number_of_messages)
-NS_IMPL_GETSET(nsImapMailboxSpec, NumUnseenMessages, PRInt32, number_of_unseen_messages)

why get rid of the NS_IMPL_GETSETs? AFAIK, we can still use them...

+      notSelectedSpec->mFlagState = NULL;

nit:
should replace with nsnull for consistency

other than these things, and Scott's comments, seems fine to me, thx!

Comment 144

[Scott MacGregor]

Attachment: [checked in]start removing nsCRT:: for char * functions

Many of the nsCRT methods that act on char * aren't available in frozen linkage. We should be using strdup, strcmp, etc., directly.

I made a pass to fix this for base, extensions & db. I want to check in what I have so far to make sure the functions I'm using compile properly on all platforms before I go further. 

While I was here, I cleaned up some string abuse in nsMsgPrintEngine.cpp

Comment 145

[Nick Kreeger]

Attachment: 267015: nsIMailboxSpec cleanup V3

+ m_runningUrl->AllocateServerPath(nsDependentCString(mb->GetMailboxName()).get(), mb->GetDelimiter(), &onlineName);

IIRC: The |AllocateServerPath()| function takes a |char *| for the first param. Rather than using a |nsDependentCString|, I switched it over to use this pattern: |PromiseFlatCString(aACString).get()|.

This patch should meet the comments of mscott and bienvenu.

Comment 146

[Joerg Bornemann]

Attachment: [checked in]nsIMsgStatusFeedback

Patch with scotts comments applied. I'll submit a separate patch with the conversion of nsMsgProgress::m_listenerList to a nsCOMArray.

Comment 147

[David :Bienvenu]

Comment on attachment 267287 [details] [diff] [review]
267015: nsIMailboxSpec cleanup V3

thx, Nick, sr=bienvenu, modulo the two things we talked about on irc:

+                trashExists = !StringBeginsWith(adoptedBoxSpec->mAllocatedPathName, nsDependentCString(serverTrashName), nsCaseInsensitiveCStringComparator()) &&  /* "INBOX/" */
+                              !pathName.Equals(nsDependentCString(serverTrashName + 6));

need to remove the !'s

and revert the getter/setters for mFlagState back the way they were...

Comment 148

[Joerg Bornemann]

Attachment: [checked in]nsMsgProgress nsISupportsArray removal

This patch contains the conversion of nsMsgProgress::m_listenerList from nsISupportsArray* to nsCOMArray.

Comment 149

[Nick Kreeger]

Attachment: [checked in]nsIMailboxSpec cleanup V4

Updated patch to address the comments via IRC:

I did change the if statement above the troubled INBOX string checking from |!PL_strncasecmp()| to |StringBeginsWith()|.

If there are no complaints, I will check this patch in.

Comment 150

[Scott MacGregor]

Comment on attachment 267110 [details] [diff] [review]
[checked in]Partial string cleanup for addrbook and migration of nsIAddressCollector to AString/ACString - with corrections suggested by Mark

Great work Prasad! I've landed the patch. What's next for you? I'm sure Mark could use help converting more address book APIs to use A(C)String. 

I'm trying to reduce our use of nsCRT methods for |char *| strings in the rest of mailnews. Once I'm done with that I'm going to start trying to build base\util without the internal APIs to see how far I can get.

Comment 151

[Scott MacGregor]

Comment on attachment 267293 [details] [diff] [review]
[checked in]nsIMsgStatusFeedback

this updated patch looks great. I'm going to run with it for a little bit and then I'll check it in later today.

Comment 152

[Prasad Sunkari [:prasad]]

Scott, lot of these nsCRT utilities might vanish once we migrate the code to use A(C)String. However, for testing purposes we could use something like this:

#ifdef MOZILLA_INTERNAL_API
  #include "nsCRT.h"
  #define NS_strlen nsCRT::strlen
  #define NS_strdup nsCRT::strdup
  #define NS_IsAsciiWhitespace nsCRT::IsAsciiSpace
#else
  #include "nsCRTGlue.h"
#endif

or we could put this into msgCore.h or something temporarily.

Then, all nsCRT::strlen(char *) should be converted to strlen, and all nsCRT::strlen(PRUnichar *) should be converted to one of the above.
Currently I think these are the only functions from nsCRT that we need.

(In reply to comment #150)
> (From update of attachment 267110 [details] [diff] [review])
> Great work Prasad! I've landed the patch. What's next for you? I'm sure Mark
> could use help converting more address book APIs to use A(C)String. 
> 
> I'm trying to reduce our use of nsCRT methods for |char *| strings in the rest
> of mailnews. Once I'm done with that I'm going to start trying to build
> base\util without the internal APIs to see how far I can get.
> 

Comment 153

[Prasad Sunkari [:prasad]]

nsEscape is another small thing that will need migration/conversion.

mailnews code uses four functions from this class... nsEscape, nsUnescape, nsEscapeHTML and nsEscapeHTML2.  While nsEscape and nsUnescape now have an alternative frozen implementation (exposed through nsINetUtil - bug:372050), the other two functions have no frozen alternative.

We could move those functions to mailnews code (browser made a copy of these functions for itself) or could get that exposed through NetUtil too.  These two functions are not dependent on rest of the nsEscape code, so either should be fine. Comments???

Scott: I have started working on migrating the addrbook APIs to A(C)String.

Comment 154

[[e-mail]]

Comment on attachment 267319 [details] [diff] [review]
[checked in]nsMsgProgress nsISupportsArray removal

>+  PRInt32 i;
> 
>+  for (i = m_listenerList.Count() - 1; i >= 0; i --)
>   {
>+    m_listenerList[i]->OnProgressChange(aWebProgress, aRequest, aCurSelfProgress, aMaxSelfProgress, aCurTotalProgress, aMaxTotalProgress);
>   }
I'm not sure what mail/news house style is but I would have written these as
for (PRInt32 i = m_listenerList.Count() - 1; i >= 0; i --)
  m_listenerList[i]->On... etc.

Comment 155

[Prasad Sunkari [:prasad]]

Attachment: Replaced all nsCRT calls with the corresponding functions from libc and nsCRTGlue

Scott, tried replacing all instances of nsCRT with corresponding functions from libc (string.h/ctype.h) and from nsCRTGlue.  The code compiled without any problems and passed the brief functionality test that I did.

This patch frees addrbook of nsCRT.h

I used GCC4.2 on linux to compile it.

Comment 156

[Scott MacGregor]

(In reply to comment #152)
> Scott, lot of these nsCRT utilities might vanish once we migrate the code to
> use A(C)String. However, for testing purposes we could use something like this:
> 
> #ifdef MOZILLA_INTERNAL_API
>   #include "nsCRT.h"
>   #define NS_strlen nsCRT::strlen
>   #define NS_strdup nsCRT::strdup
>   #define NS_IsAsciiWhitespace nsCRT::IsAsciiSpace
> #else
>   #include "nsCRTGlue.h"
> #endif
> 
> or we could put this into msgCore.h or something temporarily.
> 
> Then, all nsCRT::strlen(char *) should be converted to strlen, and all
> nsCRT::strlen(PRUnichar *) should be converted to one of the above.
> Currently I think these are the only functions from nsCRT that we need.

Putting something like this in msgCore.h appeals to me. What do you think David?

Prasad, we also use nsCRT::strtok quite heavily hopefully the arguments for that map to NS_strtok so we can use the same macro trick. 

Although, I noticed after you wrote this comment you got the address book to load nsCRTGlue.h directly while using MOZILLA_INTERNAL_API. Maybe we should just go that route instead of dealing with the macros?

Comment 157

[David :Bienvenu]

yes, I'm fine with putting something like this into msgCore.h

For most things, I'm fine with replacing nsCRT::strdup with strdup, etc. I do remember there being an issue with one of these set of methods which did a null check and others didn't - but maybe that was PL_strdup, PL_strlen, etc...

iirc, nsCRT::strtok is different from strtok - I don't know if it's the same as NS_strtok.

Comment 158

[Nick Kreeger]

Comment on attachment 267334 [details] [diff] [review]
[checked in]nsIMailboxSpec cleanup V4

Checked the nsIMailboxSpec cleanup into TRUNK.

Comment 159

[Scott MacGregor]

Just a heads up for folks converting nsCRT::strdup to NS_strdup. I believe nsCRT::strdup could handle a null input buffer, NS_strdup does not. I've already found a couple crashes when converting mailnews\base to use nsCRTGlue.

Comment 160

[Scott MacGregor]

Attachment: [checked in]make mailnews\base nsCRT:: free!

Follow prasad's lead on the address book and remove all nsCRT:: methods from mailnews\base. For methods that take PRUnichar, use the NS_ equivalents in nsCRTGlue.h.

Comment 161

[Scott MacGregor]

Comment on attachment 267423 [details] [diff] [review]
Replaced all nsCRT calls with the corresponding functions from libc and nsCRTGlue

minusing only because this didn't build for me. I think we just need to convert this:

-            if (!nsCRT::strcasecmp( sLDIFFields[i], field))
+            if (!strcasecmp( sLDIFFields[i], field))

to PL_strcasecmp

Comment 162

[David :Bienvenu]

Comment on attachment 267482 [details] [diff] [review]
[checked in]make mailnews\base nsCRT:: free!

? operator for this and the next couple methods?

+  if (nntpCommand)
+    return NS_strdup(nntpCommand);
+  else
+    return nsnull;

Comment 163

[Prasad Sunkari [:prasad]]

Attachment: Make addrbook nsCRT:: free

Scott, made the change you suggested.

This should build fine for you too now.

Comment 164

[Scott MacGregor]

Comment on attachment 267482 [details] [diff] [review]
[checked in]make mailnews\base nsCRT:: free!

checked in with David's suggestions.

Comment 165

[Scott MacGregor]

Comment on attachment 267319 [details] [diff] [review]
[checked in]nsMsgProgress nsISupportsArray removal

I incorporated Neil's suggestion into your patch and checked this in. Thanks again for the patch!

Comment 166

[Scott MacGregor]

Comment on attachment 267519 [details] [diff] [review]
Make addrbook nsCRT:: free

I had to make some more changes to get this patch to build on Windows:

* several more strcasecmp methods needed to be PL_strcasecmp.

* change strndup to PL_strndup.

* Replaced use of CRLF (which is defined in nsCRT.h) with "\r\n".

This: 

-        fullAddrStr = nsCRT::strdup(NS_ConvertUTF8toUTF16(fullAddress.get()).get());
+        fullAddrStr = NS_strdup(NS_ConvertUTF8toUTF16(fullAddress.get()).get());

can be:

fullAddrStr = ToNewUnicode(NS_ConvertUTF8toUTF16(fullAddress));

I didn't make this change but can't this:

utf8Email.Adopt(ToNewUTF8String(nsDependentString(pNotesStr)));

be 

utf8Email = pNotesStr;

I'm going to attach a new patch with those small changes and will ask Mark if he wants to look over them.

Comment 167

[Scott MacGregor]

Attachment: [checked in]Make addrbook nsCRT:: free

Hi Mark, did you want to review these changes from Prasad? I made a few tweaks to his original patch (see the comment 166).

Comment 168

[Scott MacGregor]

Prasad, do you want to take on fixing consumers of nsEscape, nsUnescape in mailnews? I'll keep driving forward with the nsCRT removal for the rest of mailnews.

Comment 169

[Prasad Sunkari [:prasad]]

Scott, I will do the nsEscape and nsUnescape part.

Comment 170

[Scott MacGregor]

Attachment: [checked in]remove nsCRT:: from compose, local, extensions, db, import

Comment 171

[Mark Banner (:standard8)]

Comment on attachment 267532 [details] [diff] [review]
[checked in]Make addrbook nsCRT:: free

r=me, I'll check this in in a few minutes.

Comment 172

[David :Bienvenu]

Comment on attachment 267588 [details] [diff] [review]
[checked in]remove nsCRT:: from compose, local, extensions, db, import

-            const char * query = strrchr(path.get(), '?');
-            if (query && nsCRT::strncasecmp(path.get(), originalPath, query - path.get()) == 0)
+            PRInt32 pos = path.RFindChar('?');
+            if (pos != -1)
+            {
+              path.SetLength(pos);
+              if (StringBeginsWith(path, nsDependentCString(originalPath), nsCaseInsensitiveCStringComparator()))


This isn't quite the same - I can't tell if it's right or not. Previously, we would compare the chars up the ? - I'm not sure what originalPath looks like, so I'm not sure how this compares.

Comment 173

[Scott MacGregor]

For coordination purposes here's the scoop on whose working on what.

These items need to be completed before we can start trying to compile without MOZILLA_INTERNAL_API:

* prasad --> nsEscape cleanup in mailnews
* mcsmurf --> remove nsCRT methods from mailnews\news
* kreeger --> remove nsCRT methods from mailnews\imap
* mscott --> remove nsCRT methods from mailnews\mime (done, still testing)

These items can continue independently, and don't block our attemps to compile without MOZILLA_INTERNAL_API
* Joerg --> continuing to convert APIs in mailnews\base\public to use A(C)String. 
* Damitha --> is starting to help out with updating some of our APIs as well.
* prasad --> hacking on address book APIs.

Comment 174

[Scott MacGregor]

Comment on attachment 267334 [details] [diff] [review]
[checked in]nsIMailboxSpec cleanup V4

Nick, this line in nsImapProtocol.cpp didn't look right to me:

        if (GetDeleteIsMoveToTrash() && // don't set the Trash flag
            // if not using the Trash model
            !onlineTrashFolderExists &&
            adoptedBoxSpec->mAllocatedPathName.Find(GetTrashFolderName()));
{
  ...
}

(Note the semicolon at the end of the if statement).

I'm gonna remove the ';' in my next patch....

Comment 175

[Scott MacGregor]

Attachment: more string fixes in mailnews

Prasad, I found a few more AssignWithConversion calls that I missed before. I also removed use of string iterators which aren't supported in the frozen API.

Do you mind reviewing this patch? Thanks!

Comment 176

[Prasad Sunkari [:prasad]]

Scott, there seems to be a problem with the usage of ns(C)String::Find.

-        if (FindInReadable(replyTo, start, end) == PR_FALSE) {
+        if (!replyToStr.Find(replyTo)) {
           if (replyToStr.Length() > 0)
             replyToStr.Append(',');

Both are not the same.  The if statement should probably be
         if (replyToStr.Find(replyTo) != -1)
and not
         if (!replyToStr.Find(replyTo))
There are many places in this patch with the same mistake.

Indentation mistake:
+  PRUnichar * substr_start = strippedUCS2.BeginWriting();

Everything else looked fine to me.  

(In reply to comment #175)
> Created an attachment (id=267677) [details]
> more string fixes in mailnews
> 
> Prasad, I found a few more AssignWithConversion calls that I missed before. I
> also removed use of string iterators which aren't supported in the frozen API.
> 
> Do you mind reviewing this patch? Thanks!
> 

Comment 177

[Frank Wein [:mcsmurf]]

Attachment: Remove nsCRT from news/

Comment 178

[Scott MacGregor]

Attachment: [checked in]more string fixes in mail and mailnews

Good catch Prasad. This fixes the problem with .Find and removes nsCRT from mail\components\migration. I also fixed a .Find call in nsImapProtocol.cpp that was also expecting a bool and not an index.

Comment 179

[[e-mail]]

Comment on attachment 266092 [details] [diff] [review]
[checked in]string cleanup for mailnews\mime

> int MimeWriteAString(MimeObject *obj, const nsACString &string)
> {
>-  const nsCString &flatString = PromiseFlatCString(string);
>-  return MimeObject_write(obj, flatString.get(), flatString.Length(), PR_TRUE);
>+  return MimeObject_write(obj, nsCString(string).get(), string.Length(), PR_TRUE);
> }
Isn't this a backward step?

>         if (mailCharset && *mailCharset) {
>-          rv = nsMsgI18NConvertToUnicode(mailCharset, nsPromiseFlatCString(inputStr), lineSource);
>+          rv = nsMsgI18NConvertToUnicode(mailCharset, nsDependentCString(inputStr), lineSource);
>           NS_ENSURE_SUCCESS(rv, -1);
>         }
I wouldn't have thought this was legal.
(I would have used PromiseFlatCString without the ns.)

Comment 180

[[e-mail]]

Comment on attachment 266157 [details] [diff] [review]
[checked in]string cleanup for mailnews\news

>       nsAutoString rateStr;
>-      rateStr.AppendWithConversion(rate_buf);
>+      rateStr.Append(NS_ConvertASCIItoUTF16(rate_buf));
Since rateStr is blank an append is equivalent to a copy i.e.
CopyASCIItoUTF16(rate_buf, rateStr);
however since you only need it for a .get() then
NS_ConvertASCIItoUTF16 rateStr(rate_buf);
also works.

Comment 181

[Scott MacGregor]

I've incorporated Neil's comments from comment 179 and comment 180 into the next round of string changes I'll be checking in. Thanks Neil!

Comment 182

[Nick Kreeger]

Attachment: IMAP nsCRT:: Removal V1

Here is go-round number one for the IMAP nsCRT:: removal.

Comment 183

[Joerg Bornemann]

Attachment: [checked in]GetExistingFolder string cleanup

This patch changes GetExistingFolder from nsMsgUtils to take a const nsCString& as argument instead of a const char*.

Comment 184

[Prasad Sunkari [:prasad]]

Attachment: nsEscape cleanup from mailnews/addrbook and mailnews/base

nsEscape removed from mailnews/base and mailnews/addrbook.

This patch does the following:
1. Exposes nsEscapeURL and nsUnescapeURL through nsINetUtil.  This part will probably need review from [e-mail]
2. Adds utility functions to nsMsgUtils.
3. Replaces all usages of nsEscape from base and mailnews.

Comment 185

[Prasad Sunkari [:prasad]]

Comment on attachment 267850 [details] [diff] [review]
nsEscape cleanup from mailnews/addrbook and mailnews/base

Changed review to mscott and addl.review to cbiesinger.

Christian: there are changes to nsINetUtil that you will want to review.

Comment 186

[Prasad Sunkari [:prasad]]

Comment on attachment 267850 [details] [diff] [review]
nsEscape cleanup from mailnews/addrbook and mailnews/base

Changed review to mscott and addl.review to cbiesinger.

Christian: there are changes to nsINetUtil that you will want to review.

Comment 187

[Prasad Sunkari [:prasad]]

(In reply to comment #178)
> Created an attachment (id=267763) [details]
> more string fixes in mail and mailnews
> 
> Good catch Prasad. This fixes the problem with .Find and removes nsCRT from
> mail\components\migration. I also fixed a .Find call in nsImapProtocol.cpp that
> was also expecting a bool and not an index.
> 

Scott, the patch looked absolutely fine.  I don't have permissions to grant a review, so you will probably have to do it yourself :)

Comment 188

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 267850 [details] [diff] [review]
nsEscape cleanup from mailnews/addrbook and mailnews/base

please don't do the netutil changes as part of this bug. there's at least one existing bug asking for that functionality.

Comment 189

[Scott MacGregor]

Comment on attachment 267763 [details] [diff] [review]
[checked in]more string fixes in mail and mailnews

thanks for the detailed review prasad.

Comment 190

[Prasad Sunkari [:prasad]]

Attachment: Make addrbook and base nsEscape free. Moved changes to nsINetUtil to another Bug.

This patch removes all usages of nsEscape.  It defines utility function in nsMsgUtils and uses them across the addrbook and base code.  Please see if this is okay.

The patch copies nsEscapeHTML and nsEscapeHTML2 from nsEscape to nsMsgUtils.

The patch needs EscapeURL and UnescapeURL to be exposed from nsINetUtil and hence depends on https://bugzilla.mozilla.org/show_bug.cgi?id=350932

Comment 191

[Scott MacGregor]

Attachment: [checked in]remove nsCRT methods from mailnews\news

Thanks for the patch mcsmurf! I took your patch and applied Neil's suggestion to use nsCStringArray instead of strtok in nsNNTPService.cpp. 

Asking Neil for a code review.

Comment 192

[Scott MacGregor]

Comment on attachment 267797 [details] [diff] [review]
[checked in]GetExistingFolder string cleanup

patch looks good and I've checked it in too. Thanks again Jörg! I believe I already e-mailed you another interface to tackle right?

Comment 193

[Scott MacGregor]

Comment on attachment 267773 [details] [diff] [review]
IMAP nsCRT:: Removal V1

I think this:

+  if (!PL_strcmp(fNextToken, "NIL"))

can just use strcmp

This isn't you, but:

     nsIMAPBodypartMessage *message = new nsIMAPBodypartMessage(NULL, NULL, PR_TRUE,
-                                                               nsCRT::strdup("message"), nsCRT::strdup("rfc822"),
+                                                               strdup("message"), strdup("rfc822"),

but this is a bit unusual . Usually the callee is responsible for making copies of any input arguments and not the caller. Maybe we could file a follow up bug fix to investigate the string management here a bit.

I think we can use the nsCString search routines here:

+    if ((PL_strcasecmp(tokenStr.get(), "INBOX")==0) && (strcmp(tokenStr.get(), "INBOX") != 0))

patch looks good to me other than those minor comments. Asking David for an sr. I'll try to run with this patch tomorrow and will report back. Thanks Nick!

Comment 194

[David :Bienvenu]

Comment on attachment 267773 [details] [diff] [review]
IMAP nsCRT:: Removal V1

this looks good, modulo Scott's comments. I would like to run with this first, however, and first, before I do that, I need to figure out why html mail is broken on the trunk.

Comment 195

[David :Bienvenu]

Comment on attachment 267773 [details] [diff] [review]
IMAP nsCRT:: Removal V1

nm, html mail is fine :-) I'll try running with this a bit

Comment 196

[Scott MacGregor]

Attachment: [checked in]remove nsCRT:: from mime

I've been running this patch for 5 days or so now. The only problem I ran into was that libmime uses utf-8 strings in many places and isspace gets very unhappy when given a utf-8 string. So I used IS_SPACE instead which is defined in msgCore.h and eventually maps to:

(((((intn)(VAL)) & 0x7f) == ((intn)(VAL))) && isspace((intn)(VAL)) )

Comment 197

[Scott MacGregor]

Attachment: [checked in]remove nsCString from nsIMsgCopyServiceListener

remove nsCString from nsIMsgCopyServiceListener and nsIMsgSend. nsIMsgSend makes me want to cry...what are you gonna do..

convert the nsCString to a ACString....

Comment 198

[Scott MacGregor]

Attachment: [checked in]remove nsAdoptingString from nsMessenger

Comment 199

[Scott MacGregor]

Attachment: [checked in]remove nsAFlatString and nsAFlatCString from mailnews

remove nsA(C)FlatString use from mailnews.

Comment 200

[Frank Wein [:mcsmurf]]

(In reply to comment #191)
> Created an attachment (id=267925) [details]
> remove nsCRT methods from mailnews\news

Did you change the if check in nsNewsFolder.cpp intentionally back to nsCRT::CR and ::LF? I replaced those with '\r' and '\n' in my patch.

Comment 201

[Scott MacGregor]

(In reply to comment #200)
> Did you change the if check in nsNewsFolder.cpp intentionally back to nsCRT::CR
> and ::LF? I replaced those with '\r' and '\n' in my patch.

nsCRT::CR and nsCRT::LF were removed from mailnews as part of another patch dedicated towards fixing this specific issue across mailnews. And that patch has a that already landed which is why it isn't in the news diff.

Comment 202

[[e-mail]]

Comment on attachment 267925 [details] [diff] [review]
[checked in]remove nsCRT methods from mailnews\news

>-      nsAutoString rateStr;
>-      rateStr.Append(NS_ConvertASCIItoUTF16(rate_buf));
>-
>       nsAutoString numGroupsStr;
>       numGroupsStr.AppendInt(mNumGroupsListed);
> 
>-      const PRUnichar *formatStrings[3] = { numGroupsStr.get(), bytesStr.get(), rateStr.get() };
>+      const PRUnichar *formatStrings[3] = { numGroupsStr.get(), bytesStr.get(), NS_ConvertASCIItoUTF16(rate_buf).get() };
This isn't legal but the rest looks OK.

Comment 203

[Scott MacGregor]

Attachment: [checked in]don't use string iterators in MsgFindKeyword

Comment 204

[Scott MacGregor]

Comment on attachment 267925 [details] [diff] [review]
[checked in]remove nsCRT methods from mailnews\news

patch checked in. Frank, thanks so much for the patch!

Comment 205

[Scott MacGregor]

Looks like we need to remove our use of nsHashTable.h from mailnews as well. I filed Bug 384490 to track that instead of doing it in this bug.

nsRDFResource.h also needs ported to frozen linkage and I've filed that as Bug 384346.

Comment 206

[Nick Kreeger]

Attachment: IMAP nsCRT:: Removal V2

Followup patch for IMAP nsCRT removal.

This patch addresses the comments form mscott. I have filed a followup bug for investigating the string duplication calls at bug 384210.

Comment 207

[[e-mail]]

Comment on attachment 266581 [details] [diff] [review]
[checked in]nsIImapService cleanup V3

>-  return imapService->SetMessageFlags(m_thread, this, this, url, uids, flags, PR_TRUE);
>+  return imapService->SetMessageFlags(m_thread, this, this, url, nsCAutoString(uids), flags, PR_TRUE);
Not nsDependentCString?

>+  nsCAutoString messageURI(aMessageURI);
>   nsCAutoString msgKey;
>   nsCAutoString mimePart;
>   nsCAutoString	folderURI;
>   nsMsgKey key;
>   
>-  rv = DecomposeImapURI(aMessageURI, getter_AddRefs(folder), getter_Copies(msgKey));
>+  rv = DecomposeImapURI(messageURI, getter_AddRefs(folder), msgKey);
Not nsDependentCString again?

>+    aImapMailFolder->HasMsgOffline(atoi(nsCString(messageIdentifierList).get()), &useLocalCache);
I wish internal and external strings had the same API - under the external API you can use messageIdentifierList.ToInteger(&rv);

>+    nsCAutoString messageIdString;
>+    messageIdString.AppendInt(msgKey);
>+    aMsgKey = messageIdString;
Here's another case where the external API allows you to AppendInt directly without needing the temporary string (which should be an nsCString to avoid copying the result).

>+  rv = nsParseImapMessageURI(nsDependentCString(aMessageURI).get(), folderURI, aMsgKey, nsnull);
Should be PromiseFlatCString(aMessageURI).get()

>-nsresult 
>-nsImapService::CreateStartOfImapUrl(const char * aImapURI, nsIImapUrl ** imapUrl,
>+nsresult nsImapService::CreateStartOfImapUrl(const nsACString &aImapURI, 
>+                                             nsIImapUrl **imapUrl,
>                                     nsIMsgFolder* aImapMailFolder,
>                                     nsIUrlListener * aUrlListener,
>-                                    nsCString & urlSpec, 
>+                                             nsACString &urlSpec, 
>                                     PRUnichar &hierarchyDelimiter)
This is an internal (notxpcom) method, so nsCString & is fine...
>-    urlSpec.AppendInt(port);
>+    nsCAutoString portStr;
>+    portStr.AppendInt(port);
>+    urlSpec.Append(portStr);
...which avoids this change.

>   // create a protocol instance to handle the request.
>-  // NOTE: once we start working with multiple connections, this step will be much more complicated...but for now
>+  // NOTE: once we start working with multiple connections, 
>+  //       this step will be much more complicated...but for now
>   // just create a connection and process the request.
Interesting gap ;-)

>+  aScheme.Assign("imap");
AssignLiteral

>-    aImapUrl->QueryInterface(NS_GET_IID(nsIURI), (void **) _retval);
>+    nsCOMPtr<nsIURI> imapUri = do_QueryInterface(aImapUrl);
>+    imapUri.swap(*aRetVal);
My preference is rv = CallQueryInterface(aImapURL, _retval);
Note that imapUri.swap(*aRetVal) assumes *aRetVal is already null.

Comment 208

[Scott MacGregor]

Comment on attachment 268543 [details] [diff] [review]
IMAP nsCRT:: Removal V2

can you remove this before you check in?

+    //if (strA.Equals(strB, nsCaseInsensitiveCStringComparator())) {

looks great to me. Thanks Nick!

Comment 209

[Scott MacGregor]

Comment on attachment 268267 [details] [diff] [review]
[checked in]don't use string iterators in MsgFindKeyword

This was checked in but I forgot to mark it as such.

Comment 210

[Scott MacGregor]

Attachment: replace FindInReadable & several more string iterators

Comment 211

[Prasad Sunkari [:prasad]]

Comment on attachment 269031 [details] [diff] [review]
replace FindInReadable & several more string iterators


In base/src/nsMsgDBView.cpp -----

>     labelStr.Append((char) (label + '0'));
>-    if (!FindInReadable(labelStr, keywords, nsCaseInsensitiveCStringComparator()))
>+    if (keywords.Find(labelStr, PR_TRUE) >= 0)
>       FetchLabel(aHdr, tags);

That should be:
if (keywords.Find(labelStr, PR_TRUE) == -1)
This is the only mistake in this patch.  I think we are looking for existing keywords here.

 
In base/util/nsMsgDBFolder.cpp -----

>-      else if (StringBeginsWith(curLine, NS_LITERAL_CSTRING("Content-Transfer-Encoding:"),
>-                           nsCaseInsensitiveCStringComparator()))
>+      else if (curLine.Find("Content-Transfer-Encoding:", PR_TRUE) >= 0)

This should probably be left as it is till we do a String Comparator cleanup.  The change you made does not ensure that "Content-Transfer-Encoding:" is at the beginning of curLine.  You could use StringBeginsWith or probably even use Find and look for a return value of "0"

>+  const char * domain = trustedMailDomains.BeginReading();
>+  const char * domainEnd = trustedMailDomains.EndReading();
>+  const char * hostStart = host.BeginReading();

Minor thing: The spaces after '*'

In compose/src/nsMsgCompose.cpp -----
>+      PRInt32 wasOffset = subject.RFind(NS_LITERAL_STRING(" (was:"));

Just for the record, I think RFind isn't available in the frozen string API and we will have to find other ways to achieve this functionality.

Comment 212

[Prasad Sunkari [:prasad]]

Comment on attachment 269031 [details] [diff] [review]
replace FindInReadable & several more string iterators

Refer to the comments above.
https://bugzilla.mozilla.org/show_bug.cgi?id=379070#c211

Comment 213

[Scott MacGregor]

Attachment: [checked in]replace FindInReadable & several more string iterators

Thanks for the great review prasad. This patch should address your comments.

Comment 214

[Scott MacGregor]

Comment on attachment 268543 [details] [diff] [review]
IMAP nsCRT:: Removal V2

Nick, feel free to check this in at your leisure :).

Comment 215

[Prasad Sunkari [:prasad]]

Comment on attachment 269098 [details] [diff] [review]
[checked in]replace FindInReadable & several more string iterators

The patch looks good now.

Along with the code migration, it also seems to fix a hidden bug here:

-    if (StringBeginsWith(buf, NS_LITERAL_CSTRING("Content-Type:")) && FindInReadable(buf, NS_LITERAL_CSTRING("text/html")))
+    if (StringBeginsWith(buf, NS_LITERAL_CSTRING("Content-Type:")) && buf.Find("text/html") >= 0)

Comment 216

[Joerg Bornemann]

Attachment: [checked in]nsIMessenger

This is the string cleanup of nsIMessenger.

Comment 217

[Nick Kreeger]

Comment on attachment 268543 [details] [diff] [review]
IMAP nsCRT:: Removal V2

Checked in a merged version of the path to the current trunk, plus I removed that extra comment.

Comment 218

[David :Bienvenu]

Comment on attachment 269098 [details] [diff] [review]
[checked in]replace FindInReadable & several more string iterators

this breaks search and filters - nsString::Find's second arg is an offset to start at, not whether or not the search is case insensitive. So we're starting at offset 1 :-(

I'll try to figure out a fix.

Comment 219

[Prasad Sunkari [:prasad]]

Attachment: Make addrbook and base nsEscape free

The patch makes mailnews/addrbook and mailnews/base nsEscape free.  It defines a few utility functions in nsMsgUtils and uses them across the code.

There are changes in how the EscapeURL functions work - the versions from nsEscape.h appended the output to result, but the new ones (exposed through nsINetUtil) assign the value.

Comment 220

[Mark Banner (:standard8)]

(In reply to comment #219)
> Created an attachment (id=269836) [details]
> Make addrbook and base nsEscape free
> The patch makes mailnews/addrbook and mailnews/base nsEscape free.  It defines
> a few utility functions in nsMsgUtils and uses them across the code.
> There are changes in how the EscapeURL functions work - the versions from
> nsEscape.h appended the output to result, but the new ones (exposed through
> nsINetUtil) assign the value.

I'm going to jump in here with a couple of address book comments:

nsAddressBook::HandleContent

+            char *unescapedData = PL_strdup(unescapedDataStr.get());

Can't you use unescapedDataStr.get() in the call to EscapedVCardToAbCard and therefore drop the extra allocations etc?

Ditto in the EscapedVCardToAbCard function below that one.

Comment 221

[Prasad Sunkari [:prasad]]

> 
> I'm going to jump in here with a couple of address book comments:
> 
> nsAddressBook::HandleContent
> 
> +            char *unescapedData = PL_strdup(unescapedDataStr.get());
> 
> Can't you use unescapedDataStr.get() in the call to EscapedVCardToAbCard and
> therefore drop the extra allocations etc?
> 
> Ditto in the EscapedVCardToAbCard function below that one.
> 

You are right mark, I missed that.  I will update the patch once Scott reviews the rest of the patch too.

Comment 222

[Scott MacGregor]

Comment on attachment 269207 [details] [diff] [review]
[checked in]nsIMessenger

Joerg, a couple small things:

   rv = filePicker->Show(&dialogResult);
-  if (NS_FAILED(rv) || dialogResult == nsIFilePicker::returnCancel)
-    goto done;
+  NS_ENSURE_SUCCESS(rv, rv);
+  NS_ENSURE_TRUE(dialogResult != nsIFilePicker::returnCancel, rv);

I didn't think it made sense to assert if the user cancels the save as dialog so I changed this to just be:

if (NS_FAILED(rv) || dialogresult == nsIFilePicker::returnCancel)
  return rv;

* I don't think the string logic is quite right here:

-  if (mMsgWindow && (!strncmp(aUri, "file:", 5) || PL_strstr(aUri, "type=application/x-message-display")))
+  if (mMsgWindow && (!strncmp(PromiseFlatCString(aUri).get(), "file:", 5) || 
+                     aUri.Equals(NS_LITERAL_CSTRING("type=application/x-message-display"))))


I changed it to be:

  if (mMsgWindow && (StringBeginsWith(aUri, NS_LITERAL_CSTRING("file:")) ||
                     aUri.Find(NS_LITERAL_CSTRING("type=application/x-message-display")) >= -1))

Everything else looked good. I'm going to keep running with this patch for a little while longer before I check it in. Great work!

Comment 223

[Scott MacGregor]

Comment on attachment 269836 [details] [diff] [review]
Make addrbook and base nsEscape free

+        char *unescapedData = PL_strdup(unescapedDataStr.get());

can be ToNewCString(unescapedDataStr);

These:
+  if (!nu)
+    return NS_ERROR_OUT_OF_MEMORY;

can be:

nsresult rv;
nsCOMPtr<nsINetUtil> nu = do_GetService(NS_NETUTIL_CONTRACTID, &rv);
NS_ENSURE_SUCCESS(rv, rv);

Instead of calling this MsgEscapeHTML2, I wonder if we could call it MsgEscapeW or MsgEscapeHTMLUnichar, since the arguments are different, maybe we can just call it MsgEscapeHTML...

aSourceBufferLen = nsCRT::strlen(aSourceBuffer); // ...then I will

should be NS_strlen(aSourceBuffer)

everything else looks good.

Comment 224

[Scott MacGregor]

Comment on attachment 269207 [details] [diff] [review]
[checked in]nsIMessenger

I checked thie patch in with my additional changes. Thanks for the patch!

Comment 225

[David :Bienvenu]

Attachment: [checked in]fix regression from nsIMessenger patch

Comment 226

[Scott MacGregor]

Comment on attachment 269971 [details] [diff] [review]
[checked in]fix regression from nsIMessenger patch

that's what I get for making a change to the original patch.

Comment 227

[Prasad Sunkari [:prasad]]

Attachment: [checked in]Make addrbook and base nsEscape free - updated as per comments by Scott and Mark

This patch attempts to remove use of functions form nsEscape.h in mailnews/addrbook and mailnews/base.

nsEscape gets replaced by MsgEscapeString
nsUnescape gets replaced by MsgUnescapeString
NS_EscapeURL gets replaced by MsgEscapeURL
  - Change in symantics: The result is not appended, 
    but assigned to the out string.
NS_UnescapeURL gets replaced by MsgUnescapeString
  - Change in symantics: The result is not appended,
    but assigned to the out string.
nsEscapeHTML and nsEscapeHTML2 should get replaced by MsgEscapeHTML

Comment 228

[Scott MacGregor]

Comment on attachment 269995 [details] [diff] [review]
[checked in]Make addrbook and base nsEscape free - updated as per comments by Scott and Mark

looks good Prasad, I'll get this landed later today.

Comment 229

[Joerg Bornemann]

Attachment: [checked in]GetMessageServiceFromURI char* -> nsACString

This patch changes the char* argument of GetMessageServiceFromURI to nsACString.

Comment 230

[Scott MacGregor]

Comment on attachment 270606 [details] [diff] [review]
[checked in]GetMessageServiceFromURI char* -> nsACString

this looks great and I just checked it. thanks again for the patch!

Comment 231

[Joerg Bornemann]

Attachment: [bitrotted] nsIMsgMessageService

This is the cleanup of nsIMsgMessageService and its implementations nsNntpService, nsMailboxService and nsImapService. Whitespace changes are omitted. :)

Comment 232

[David :Bienvenu]

Attachment: fix regression Neil pointed out

Comment 233

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

nominating for wanted-thunderbird3 to keep it on the radar.

Comment 234

[Magnus Melin [:mkmelin]]

Comment on attachment 272049 [details] [diff] [review]
[bitrotted] nsIMsgMessageService

This has bitrotted quite a bit, can you provide an updated patch?

Comment 235

[[e-mail]]

Comment on attachment 272049 [details] [diff] [review]
[bitrotted] nsIMsgMessageService

>-    msgurl->SetUri(nsDependentCString(aImapURI).get());
Nice fix :-)

>-    if (!PL_strncmp(aMessageURI, kNewsRootURI, kNewsRootURILen)) {
>+  if (aMessageURI.Equals(kNewsRootURI))
Shouldn't this use StringBeginsWith (and kill off kNewsRootURILen too).

Comment 236

[[e-mail]]

Comment on attachment 269207 [details] [diff] [review]
[checked in]nsIMessenger

>-  rv = NS_NewURI(getter_AddRefs(uri), aURL);
>+  rv = NS_NewURI(getter_AddRefs(uri), PromiseFlatCString(aURL).get());
Actually the char* version use an nsDependentCString... just pass aURL!