Remove nsXPIDLCString / nsXPIDLString from mailnews

RESOLVED FIXED

Status

defect
RESOLVED FIXED
12 years ago
11 years ago

People

(Reporter: mscott, Unassigned)

Tracking

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

()

Attachments

(62 attachments, 23 obsolete attachments)

32.75 KB, patch
neil
: review+
Bienvenu
: superreview+
Details | Diff | Splinter Review
53.06 KB, patch
mscott
: review+
Bienvenu
: superreview+
Details | Diff | Splinter Review
24.92 KB, patch
Details | Diff | Splinter Review
79.32 KB, patch
Bienvenu
: superreview+
Details | Diff | Splinter Review
23.03 KB, patch
Bienvenu
: superreview+
Details | Diff | Splinter Review
94.38 KB, patch
Details | Diff | Splinter Review
24.92 KB, patch
Details | Diff | Splinter Review
320.62 KB, patch
Bienvenu
: superreview+
Details | Diff | Splinter Review
127.21 KB, patch
mscott
: superreview+
Details | Diff | Splinter Review
69.30 KB, patch
mscott
: superreview+
Details | Diff | Splinter Review
2.01 KB, patch
mscott
: superreview+
Details | Diff | Splinter Review
29.57 KB, patch
mscott
: superreview+
Details | Diff | Splinter Review
2.08 KB, text/plain
mscott
: superreview+
Details
120.54 KB, patch
mscott
: superreview+
Details | Diff | Splinter Review
536.51 KB, patch
Bienvenu
: superreview+
Details | Diff | Splinter Review
34.66 KB, text/plain
Bienvenu
: superreview+
Details
1.26 KB, patch
mscott
: superreview+
Details | Diff | Splinter Review
8.68 KB, patch
mscott
: review+
Bienvenu
: superreview+
Details | Diff | Splinter Review
28.00 KB, patch
Bienvenu
: superreview+
Details | Diff | Splinter Review
12.07 KB, patch
mscott
: review+
Details | Diff | Splinter Review
54.96 KB, patch
mscott
: superreview+
Details | Diff | Splinter Review
65.96 KB, patch
Bienvenu
: superreview+
Details | Diff | Splinter Review
16.77 KB, patch
Bienvenu
: superreview+
Details | Diff | Splinter Review
6.67 KB, patch
Bienvenu
: superreview+
Details | Diff | Splinter Review
2.92 KB, patch
mscott
: superreview+
Details | Diff | Splinter Review
29.41 KB, patch
Bienvenu
: superreview+
Details | Diff | Splinter Review
52.67 KB, patch
mscott
: review+
Details | Diff | Splinter Review
22.65 KB, patch
Bienvenu
: superreview+
Details | Diff | Splinter Review
15.79 KB, patch
Bienvenu
: superreview+
Details | Diff | Splinter Review
166.82 KB, patch
mscott
: review+
Details | Diff | Splinter Review
141.92 KB, patch
mscott
: superreview+
Details | Diff | Splinter Review
39.82 KB, patch
Bienvenu
: superreview+
Details | Diff | Splinter Review
22.76 KB, patch
Bienvenu
: superreview+
Details | Diff | Splinter Review
13.26 KB, patch
Bienvenu
: superreview+
Details | Diff | Splinter Review
22.24 KB, patch
Bienvenu
: superreview+
Details | Diff | Splinter Review
147.19 KB, patch
standard8
: review+
mscott
: superreview+
Details | Diff | Splinter Review
18.81 KB, patch
Bienvenu
: superreview+
Details | Diff | Splinter Review
61.62 KB, patch
Bienvenu
: superreview+
Details | Diff | Splinter Review
41.15 KB, patch
Bienvenu
: superreview+
Details | Diff | Splinter Review
24.18 KB, patch
mscott
: review+
Details | Diff | Splinter Review
7.79 KB, patch
mscott
: review+
Details | Diff | Splinter Review
45.76 KB, patch
Details | Diff | Splinter Review
26.03 KB, patch
Bienvenu
: superreview+
Details | Diff | Splinter Review
31.05 KB, patch
standard8
: review+
mscott
: superreview+
Details | Diff | Splinter Review
129.64 KB, patch
Bienvenu
: superreview+
Details | Diff | Splinter Review
30.06 KB, patch
mscott
: review+
Details | Diff | Splinter Review
15.79 KB, patch
mscott
: review+
mscott
: superreview+
Details | Diff | Splinter Review
9.03 KB, patch
Details | Diff | Splinter Review
126.11 KB, patch
Bienvenu
: superreview+
Details | Diff | Splinter Review
10.46 KB, patch
Bienvenu
: superreview+
Details | Diff | Splinter Review
6.48 KB, patch
Bienvenu
: superreview+
Details | Diff | Splinter Review
15.56 KB, patch
Bienvenu
: superreview+
Details | Diff | Splinter Review
8.96 KB, patch
Bienvenu
: superreview+
Details | Diff | Splinter Review
53.80 KB, patch
mscott
: review+
Details | Diff | Splinter Review
13.68 KB, patch
prasad
: review+
Details | Diff | Splinter Review
85.46 KB, patch
mscott
: review+
Details | Diff | Splinter Review
1.16 KB, patch
mscott
: superreview+
Details | Diff | Splinter Review
29.91 KB, patch
mscott
: review+
Details | Diff | Splinter Review
29.41 KB, patch
mscott
: review+
Details | Diff | Splinter Review
101.69 KB, patch
Details | Diff | Splinter Review
636 bytes, patch
mscott
: superreview+
Details | Diff | Splinter Review
629 bytes, patch
neil
: review+
Details | Diff | Splinter Review
Reporter

Description

12 years ago
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.
Reporter

Comment 1

12 years ago
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.
Assignee: nobody → mscott
Status: NEW → ASSIGNED
Attachment #263063 - Flags: superreview?(bienvenu)
Attachment #263063 - Flags: review?(neil)

Comment 2

12 years ago
Comment on attachment 263063 [details] [diff] [review]
convert nsIMsgIdentity

nice, looks OK to me...
Attachment #263063 - Flags: superreview?(bienvenu) → superreview+
Posted patch Convert pref usage (obsolete) — Splinter Review
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.
Attachment #263119 - Flags: superreview?(bienvenu)
Attachment #263119 - Flags: review?(mscott)
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?
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].
Attachment #263171 - Flags: superreview?(bienvenu)
Attachment #263171 - Flags: review?(mscott)

Comment 6

12 years ago
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)
Attachment #263119 - Flags: superreview?(bienvenu) → superreview+

Updated

12 years ago
Attachment #263171 - Flags: superreview?(bienvenu) → superreview+
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 ;-)
Reporter

Comment 8

12 years ago
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 :)
Attachment #263171 - Flags: review?(mscott) → review+
Reporter

Comment 9

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

I needed to review this patch for the incoming server changes.
Attachment #263119 - Flags: review?(mscott) → review+
(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 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.
Attachment #263063 - Flags: review?(neil) → review+
Reporter

Comment 13

12 years ago
Convert the rest of nsIMsgIdentity to ACString and related string cleanup from the effected call sites.
Attachment #263385 - Flags: superreview?(bienvenu)
Reporter

Comment 14

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

Comment 15

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

good times!
Attachment #263385 - Flags: superreview?(bienvenu) → superreview+
This appears to be causing the redness on MAC builds.
Reporter

Updated

12 years ago
Attachment #263385 - Attachment description: more nsIMsgIdentity changes → [checked in]more nsIMsgIdentity changes
Reporter

Updated

12 years ago
Attachment #263171 - Attachment description: Merged nsIdentity changes into Scott's patch → [checked in]Merged nsIdentity changes into Scott's patch
Reporter

Updated

12 years ago
Attachment #263306 - Attachment description: nsMsgIncomingServer pref cleanup updated for bitrot → [checked in]nsMsgIncomingServer pref cleanup updated for bitrot
Reporter

Comment 17

12 years ago
nsIMsgAccount cleanup.
Attachment #263412 - Flags: superreview?(bienvenu)

Comment 18

12 years ago
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();
Attachment #263412 - Flags: superreview?(bienvenu) → superreview+
Reporter

Updated

12 years ago
Attachment #263412 - Attachment description: nsIMsgAccount → [checked in]nsIMsgAccount
Reporter

Comment 19

12 years ago
Posted patch nsIMsgAccountManager changes (obsolete) — Splinter Review
Attachment #263528 - Flags: superreview?(bienvenu)
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

12 years ago
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);
Attachment #263528 - Flags: superreview?(bienvenu) → superreview+
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.
Reporter

Comment 23

12 years ago
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.
Reporter

Comment 24

12 years ago
nsDependentString is part of nsStringAPI so we can use it.
Reporter

Comment 25

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

Updated

12 years ago
Blocks: 306324
(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 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.
Reporter

Updated

12 years ago
Attachment #263653 - Attachment description: updated nsIMsgAccountManager patch → [checked in]updated nsIMsgAccountManager patch
Reporter

Updated

12 years ago
Attachment #263528 - Attachment is obsolete: true
OS: Windows Vista → All
Hardware: PC → All
Reporter

Comment 29

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

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

Comment 31

12 years ago
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...
Attachment #264253 - Flags: superreview?(mscott)
Reporter

Comment 32

12 years ago
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.
Attachment #264250 - Flags: superreview?(bienvenu)
Reporter

Comment 33

12 years ago
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.
Attachment #264253 - Flags: superreview?(mscott) → superreview+

Updated

12 years ago
Attachment #264250 - Flags: superreview?(bienvenu) → superreview+
Reporter

Comment 35

12 years ago
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.
Attachment #264250 - Attachment description: nsIMsg*IncomingServer.idl → [checked in] nsIMsg*IncomingServer.idl
Reporter

Comment 36

12 years ago
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 :)
Attachment #264283 - Flags: superreview?(mscott) → superreview+

Updated

12 years ago
Attachment #264253 - Attachment description: remove most imap uses → [checked in]remove most imap uses

Comment 37

12 years ago
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.
Attachment #264283 - Attachment description: remove nsImapProtocol.cpp uses (and a few related changes) → [checked in]remove nsImapProtocol.cpp uses (and a few related changes)
Reporter

Comment 38

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

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

12 years ago
this makes things much happier
Attachment #264370 - Flags: superreview?(mscott)
Reporter

Comment 41

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

thanks David!
Attachment #264370 - Flags: superreview?(mscott) → superreview+

Comment 42

12 years ago
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)
Attachment #264370 - Attachment description: fix subscribe ui and folder discovery → [checked in]fix subscribe ui and folder discovery
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

12 years ago
>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
Reporter

Comment 45

12 years ago
(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. 
Depends on: 380461
imap login crashes for me though -- bug 380461

Comment 48

12 years ago
Haven't tried this, but I think it does the right thing...
Attachment #264563 - Flags: superreview?(mscott)
Reporter

Updated

12 years ago
Attachment #264563 - Flags: superreview?(mscott) → superreview+
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)

Updated

12 years ago
Attachment #264563 - Attachment description: fix password regression → [checked in]fix password regression

Comment 50

12 years ago
I hope this doesn't cause any conflicts - I tried to keep it contained.
Attachment #264922 - Flags: superreview?(mscott)
Reporter

Comment 51

12 years ago
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!
Attachment #264922 - Flags: superreview?(mscott) → superreview+
Reporter

Updated

12 years ago
Attachment #264562 - Flags: superreview?(mscott) → superreview+

Comment 52

12 years ago
+    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...

Updated

12 years ago
Attachment #264922 - Attachment description: remove from base/search, and clean up import service a bit. → [checked in]remove from base/search, and clean up import service a bit.
(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 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?
Reporter

Comment 55

12 years ago
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.
Attachment #265319 - Flags: superreview?(bienvenu)

Comment 56

12 years ago
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?
Attachment #265319 - Flags: superreview?(bienvenu) → superreview+
Reporter

Comment 57

12 years ago
(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.
Reporter

Updated

12 years ago
Attachment #265319 - Attachment description: nsIMsg*Folder.idl → [checked in]nsIMsg*Folder.idl
Reporter

Comment 58

12 years ago
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.
Reporter

Comment 59

12 years ago
nsIMsgWindow changes.
Attachment #265601 - Flags: superreview?(bienvenu)

Updated

12 years ago
Attachment #265601 - Attachment mime type: application/octet-stream → text/plain

Updated

12 years ago
Attachment #265601 - Flags: superreview?(bienvenu) → superreview+
Reporter

Updated

12 years ago
Attachment #265601 - Attachment description: nsIMsgWindow → [checked in]nsIMsgWindow

Comment 60

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

12 years ago
this fixes the pop3 filter move action regression.
Attachment #265676 - Flags: superreview?(mscott)
Reporter

Comment 62

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

thanks david.
Attachment #265676 - Flags: superreview?(mscott) → superreview+

Updated

12 years ago
Attachment #265676 - Attachment description: fix pop3 filter move action → [checked in]fix pop3 filter move action

Comment 63

12 years ago
Posted patch nsMsgProgress (obsolete) — Splinter Review
Attachment #265683 - Flags: review?

Updated

12 years ago
Attachment #265683 - Flags: review? → review?(mscott)

Comment 64

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

12 years ago
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.

Updated

12 years ago
Attachment #264562 - Attachment description: remove xpidl strings from mime → [checked in]remove xpidl strings from mime

Comment 66

12 years ago
Posted patch nsMsgProgress (obsolete) — Splinter Review
OK, whitespaces back to normal. Apparently I've read the space-adjustment-sentence on the CodeCleanUp-Page too fast. :-)
Attachment #265683 - Attachment is obsolete: true
Attachment #265683 - Flags: review?(mscott)

Updated

12 years ago
Attachment #265692 - Flags: review?(mscott)
Reporter

Comment 67

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

12 years ago
I think the remaining ws changes are sensible. Choose your favourite patch. ;)
Attachment #265694 - Flags: review?(mscott)
Reporter

Comment 69

12 years ago
nsIMsgFolderCache and nsIMsgFolderCacheElement changes.
Attachment #265710 - Flags: superreview?(bienvenu)
Reporter

Updated

12 years ago
Attachment #265692 - Attachment is obsolete: true
Attachment #265692 - Flags: review?(mscott)
Reporter

Comment 70

12 years ago
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!
Attachment #265694 - Flags: review?(mscott) → review+

Comment 71

12 years ago
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
Attachment #265710 - Flags: superreview?(bienvenu) → superreview+

Comment 72

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

looks good, thx, modulo Scott's do_QueryElementAt comment
Attachment #265694 - Flags: superreview+
Reporter

Updated

12 years ago
Attachment #265710 - Attachment description: nsIMsgFolderCacheElement and nsIMsgFolderCache → [checked in]nsIMsgFolderCacheElement and nsIMsgFolderCache
Reporter

Comment 73

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

Patch checked in. Thank you Joerg!
Attachment #265694 - Attachment description: nsMsgProgress → [checked in]nsMsgProgress
Depends on: 381797

Comment 74

12 years ago
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.
Posted patch convert nsIAbAddressCollecter (obsolete) — Splinter Review
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.
Attachment #265943 - Flags: review?(mscott)
Reporter

Comment 77

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

nice!
Attachment #265952 - Flags: superreview?(mscott) → superreview+

Updated

12 years ago
Attachment #265876 - Flags: review?(mscott)

Updated

12 years ago
Attachment #265952 - Attachment description: remove xpidl strings from view code → [checked in]remove xpidl strings from view code
Reporter

Comment 78

12 years ago
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!
Attachment #265876 - Flags: review?(mscott) → review+
Reporter

Comment 79

12 years ago
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.
Reporter

Comment 80

12 years ago
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.
Attachment #266018 - Flags: superreview?(bienvenu)
Reporter

Comment 81

12 years ago
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.
Attachment #265943 - Flags: superreview+
Attachment #265943 - Flags: review?(mscott)
Attachment #265943 - Flags: review?(bugzilla)

Updated

12 years ago
Attachment #266018 - Flags: superreview?(bienvenu) → superreview+
Reporter

Updated

12 years ago
Attachment #266018 - Attachment description: string cleanup for mailnews\base → [checked in]string cleanup for mailnews\base
Reporter

Comment 82

12 years ago
Same string changes I just finished making to mailnews\base.
Attachment #266023 - Flags: superreview?(bienvenu)

Updated

12 years ago
Attachment #266023 - Flags: superreview?(bienvenu) → superreview+
(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()
Reporter

Comment 84

12 years ago
(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. 
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);"
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.
Attachment #265943 - Attachment is obsolete: true
Attachment #266058 - Flags: review?(mscott)
Attachment #265943 - Flags: review?(bugzilla)

Comment 87

12 years ago
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.
(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).
Reporter

Updated

12 years ago
Attachment #266023 - Attachment description: string cleanup for mailnews\local → [checked in]string cleanup for mailnews\local
Reporter

Updated

12 years ago
Attachment #265876 - Attachment description: nsIImapHostSessionList Clean-up V1 → [checked in]nsIImapHostSessionList Clean-up V1
Reporter

Comment 89

12 years ago
David already removed xpidlstring from mime, this removes some of the other trouble spots we've already done for local & base.
Attachment #266092 - Flags: superreview?(bienvenu)

Updated

12 years ago
Attachment #266092 - Flags: superreview?(bienvenu) → superreview+
Reporter

Updated

12 years ago
Attachment #266092 - Attachment description: string cleanup for mailnews\mime → [checked in]string cleanup for mailnews\mime

Comment 90

12 years ago
The original problem was the nsCString(propertyValue) one, but deleting my panacea.dat exposed a few other issues...
Attachment #266115 - Flags: superreview?(mscott)
Reporter

Comment 91

12 years ago
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.
Attachment #266115 - Flags: superreview?(mscott) → superreview+

Updated

12 years ago
Attachment #266115 - Attachment description: fix some issues with previous patches → [checked in]fix some issues with previous patches
Reporter

Comment 92

12 years ago
Attachment #266157 - Flags: superreview?(bienvenu)

Updated

12 years ago
Attachment #266157 - Flags: superreview?(bienvenu) → superreview+

Comment 93

12 years ago
Posted patch nsIImapService cleanup V1 (obsolete) — Splinter Review
The first go-round for the nsIImapService cleanup. This is a white-space free patch.
Attachment #266227 - Flags: review?(mscott)

Comment 94

12 years ago
Posted patch nsIImapService cleanup V1 (obsolete) — Splinter Review
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)
Attachment #266227 - Attachment is obsolete: true
Attachment #266258 - Flags: review?(mscott)
Attachment #266227 - Flags: review?(mscott)

Comment 95

12 years ago
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.

Reporter

Comment 96

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

Comment 97

12 years ago
Posted patch nsIMsgThread (obsolete) — Splinter Review
First try of nsIMsgThread cleanup.
Attachment #266413 - Flags: review?(mscott)

Comment 98

12 years ago
second try :)
Attachment #266413 - Attachment is obsolete: true
Attachment #266414 - Flags: review?(mscott)
Attachment #266413 - Flags: review?(mscott)
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,
Attachment #266058 - Attachment is obsolete: true
Attachment #266443 - Flags: review?(bugzilla)
Attachment #266058 - Flags: review?(mscott)
(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.
Reporter

Comment 101

12 years ago
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!
Reporter

Updated

12 years ago
Attachment #266157 - Attachment description: string cleanup for mailnews\news → [checked in]string cleanup for mailnews\news
Reporter

Comment 102

12 years ago
Attachment #266507 - Flags: superreview?(bienvenu)

Updated

12 years ago
Attachment #266507 - Flags: superreview?(bienvenu) → superreview+
Reporter

Updated

12 years ago
Attachment #266507 - Attachment description: string cleanup for mailnews\db → [checked in]string cleanup for mailnews\db
Reporter

Comment 103

12 years ago
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.
Attachment #266414 - Flags: review?(mscott) → review+
Reporter

Updated

12 years ago
Attachment #266414 - Attachment description: nsIMsgThread → [checked in]nsIMsgThread
Reporter

Comment 104

12 years ago
Attachment #266540 - Flags: superreview?(bienvenu)

Updated

12 years ago
Attachment #266540 - Flags: superreview?(bienvenu) → superreview+
Reporter

Updated

12 years ago
Attachment #266540 - Attachment description: string cleanup for mailnews\extensions → [checked in]string cleanup for mailnews\extensions

Comment 105

12 years ago
Should meet all of mscott's comments.
Attachment #266258 - Attachment is obsolete: true
Attachment #266581 - Flags: review?(mscott)
Attachment #266258 - Flags: review?(mscott)

Comment 106

12 years ago
(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);
Depends on: 382441
Reporter

Comment 107

12 years ago
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!
Attachment #266581 - Flags: review?(mscott) → review+

Comment 108

12 years ago
(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

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

Checked the |nsIImapService| patch into HEAD.
Attachment #266581 - Attachment description: nsIImapService cleanup V3 → [checked in]nsIImapService cleanup V3
Blocks: 382504
No longer blocks: 382504
Depends on: 382504

Comment 110

12 years ago
There's a lot more string cleanup needed here, but this gets rid of xpidl strings...
Attachment #266674 - Flags: superreview?(mscott)
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 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

12 years ago
Posted patch nsIMailboxSpec cleanup V1 (obsolete) — Splinter Review
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?
Attachment #266763 - Flags: review?(mscott)
Depends on: 382640
Reporter

Comment 114

12 years ago
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);
Attachment #266674 - Flags: superreview?(mscott) → superreview+
Reporter

Comment 115

12 years ago
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!
Attachment #266763 - Flags: review?(mscott) → review-
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.
Attachment #266443 - Flags: review?(bugzilla) → review-
Reporter

Comment 117

12 years ago
Attachment #266821 - Flags: superreview?(bienvenu)

Updated

12 years ago
Attachment #266821 - Flags: superreview?(bienvenu) → superreview+

Comment 118

12 years ago
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.
Attachment #266674 - Attachment description: remove xpidl strings from compose/src → [checked in]remove xpidl strings from compose/src
Reporter

Comment 119

12 years ago
I think this is the last of the directories for pure string cleanup.
Attachment #266943 - Flags: superreview?(bienvenu)

Comment 120

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

yay!
Attachment #266943 - Flags: superreview?(bienvenu) → superreview+
Reporter

Comment 121

12 years ago
Attachment #266956 - Flags: superreview?(bienvenu)

Updated

12 years ago
Attachment #266956 - Flags: superreview?(bienvenu) → superreview+
(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

12 years ago
Posted patch nsIMsgStatusFeedback (obsolete) — Splinter Review
This is the cleanup of nsIMsgStatusFeedback.
Attachment #267001 - Flags: review?(mscott)

Comment 124

12 years ago
Posted patch nsIMailboxSpec cleanup V2 (obsolete) — Splinter Review
Should address all of mscott's comments.

I moved the two classes out of nsImapCore.h and into nsImapUtils.h and nsImapProtocol.h.
Attachment #266763 - Attachment is obsolete: true
Attachment #267015 - Flags: review?(mscott)
Reporter

Updated

12 years ago
Attachment #266821 - Attachment description: string cleanup for mailnews\import → [checked in]string cleanup for mailnews\import
Reporter

Updated

12 years ago
Attachment #266943 - Attachment description: string cleanup for mailnews\mapi → [checked in]string cleanup for mailnews\mapi
Reporter

Updated

12 years ago
Attachment #266956 - Attachment description: string cleanup for mail\ → [checked in]string cleanup for mail\
Reporter

Comment 125

12 years ago
Remove nsAdoptingCString, change nsPromiseFlat(C)String to PromiseFlat(C)String, remove nsPrintfCString.
Attachment #267089 - Flags: superreview?(bienvenu)

Updated

12 years ago
Attachment #267089 - Flags: superreview?(bienvenu) → superreview+
Reporter

Updated

12 years ago
Attachment #267089 - Attachment description: misc string changes in mozilla\mailnews → [checked in]misc string changes in mozilla\mailnews
Reporter

Comment 126

12 years ago
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?
Attachment #267001 - Flags: review?(mscott) → review+
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.
Attachment #266443 - Attachment is obsolete: true
Attachment #267110 - Flags: review?(bugzilla)
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
Attachment #267110 - Attachment is patch: true
Attachment #267110 - Attachment mime type: text/x-patch → text/plain

Comment 129

12 years ago
(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 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 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 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.
Attachment #267110 - Flags: review?(bugzilla) → review+
Reporter

Comment 133

12 years ago
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.
Attachment #267015 - Flags: superreview?(bienvenu)
Reporter

Comment 134

12 years ago
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!
Attachment #267110 - Flags: superreview+
Reporter

Comment 135

12 years ago
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
Attachment #267200 - Flags: superreview?(bienvenu)
Reporter

Comment 136

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

12 years ago
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?
Attachment #267200 - Flags: superreview?(bienvenu) → superreview+
Reporter

Comment 138

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

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

Updated

12 years ago
Attachment #267200 - Attachment description: remove use of NS_LINEBREAK and NS_LINEBREAK_LEN → [checked in]remove use of NS_LINEBREAK and NS_LINEBREAK_LEN
Reporter

Comment 140

12 years ago
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).
Attachment #267015 - Flags: review?(mscott) → review+
Reporter

Comment 141

12 years ago
I did a global search and replace for mailnews & mail. I then did a basic smoketest  on the results.
Attachment #267212 - Flags: superreview?(bienvenu)

Updated

12 years ago
Attachment #267212 - Flags: superreview?(bienvenu) → superreview+
Reporter

Updated

12 years ago
Attachment #267212 - Attachment description: replace nsCRT::CR and nsCRT::LF with '\r' and '\n' respectively → [checked in]replace nsCRT::CR and nsCRT::LF with '\r' and '\n' respectively
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

12 years ago
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!
Attachment #267015 - Flags: superreview?(bienvenu) → superreview+
Reporter

Comment 144

12 years ago
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
Attachment #267283 - Flags: superreview?(bienvenu)

Updated

12 years ago
Attachment #267283 - Flags: superreview?(bienvenu) → superreview+

Comment 145

12 years ago
+ 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.
Attachment #267015 - Attachment is obsolete: true
Attachment #267287 - Flags: superreview?

Updated

12 years ago
Attachment #267287 - Flags: superreview? → superreview?(bienvenu)

Comment 146

12 years ago
Patch with scotts comments applied. I'll submit a separate patch with the conversion of nsMsgProgress::m_listenerList to a nsCOMArray.
Attachment #267001 - Attachment is obsolete: true

Updated

12 years ago
Attachment #267293 - Flags: review?(mscott)

Comment 147

12 years ago
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...
Attachment #267287 - Flags: superreview?(bienvenu) → superreview+

Comment 148

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

Updated

12 years ago
Attachment #267319 - Flags: review?(mscott)

Comment 149

12 years ago
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.
Attachment #267287 - Attachment is obsolete: true
Reporter

Comment 150

12 years ago
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.
Attachment #267110 - Attachment description: Partial string cleanup for addrbook and migration of nsIAddressCollector to AString/ACString - with corrections suggested by Mark → [checked in]Partial string cleanup for addrbook and migration of nsIAddressCollector to AString/ACString - with corrections suggested by Mark
Reporter

Comment 151

12 years ago
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.
Attachment #267293 - Flags: review?(mscott) → review+
Reporter

Updated

12 years ago
Attachment #267319 - Flags: superreview?(neil)
Reporter

Updated

12 years ago
Attachment #267283 - Attachment description: start removing nsCRT:: for char * functions → [checked in]start removing nsCRT:: for char * functions
Reporter

Updated

12 years ago
Attachment #267293 - Attachment description: nsIMsgStatusFeedback → [checked in]nsIMsgStatusFeedback
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.
> 

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 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.
Attachment #267319 - Flags: superreview?(neil) → superreview+
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.
Attachment #267423 - Flags: review?(mscott)
Reporter

Comment 156

12 years ago
(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

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

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

Checked the nsIMailboxSpec cleanup into TRUNK.
Attachment #267334 - Attachment description: nsIMailboxSpec cleanup V4 → [checked in]nsIMailboxSpec cleanup V4
Reporter

Comment 159

12 years ago
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.
Reporter

Comment 160

12 years ago
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.
Attachment #267482 - Flags: superreview?(bienvenu)
Reporter

Comment 161

12 years ago
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
Attachment #267423 - Flags: review?(mscott) → review-

Comment 162

12 years ago
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;
Attachment #267482 - Flags: superreview?(bienvenu) → superreview+
Posted patch Make addrbook nsCRT:: free (obsolete) — Splinter Review
Scott, made the change you suggested.

This should build fine for you too now.
Attachment #267423 - Attachment is obsolete: true
Attachment #267519 - Flags: review?(mscott)
Reporter

Comment 164

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

checked in with David's suggestions.
Attachment #267482 - Attachment description: make mailnews\base nsCRT:: free! → [checked in]make mailnews\base nsCRT:: free!
Reporter

Comment 165

12 years ago
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!
Attachment #267319 - Attachment description: nsMsgProgress nsISupportsArray removal → [checked in]nsMsgProgress nsISupportsArray removal
Attachment #267319 - Flags: review?(mscott) → review+
Reporter

Comment 166

12 years ago
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.
Attachment #267519 - Attachment is obsolete: true
Attachment #267519 - Flags: review?(mscott)
Reporter

Comment 167

12 years ago
Hi Mark, did you want to review these changes from Prasad? I made a few tweaks to his original patch (see the comment 166).
Attachment #267532 - Flags: superreview+
Attachment #267532 - Flags: review?(bugzilla)
Reporter

Comment 168

12 years ago
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.
Scott, I will do the nsEscape and nsUnescape part.
Comment on attachment 267532 [details] [diff] [review]
[checked in]Make addrbook nsCRT:: free

r=me, I'll check this in in a few minutes.
Attachment #267532 - Flags: review?(bugzilla) → review+
Reporter

Updated

12 years ago
Attachment #267532 - Attachment description: Make addrbook nsCRT:: free → [checked in]Make addrbook nsCRT:: free
Reporter

Updated

12 years ago
Attachment #267588 - Flags: superreview?(bienvenu)

Comment 172

12 years ago
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.
Attachment #267588 - Flags: superreview?(bienvenu) → superreview+
Reporter

Updated

12 years ago
Attachment #267588 - Attachment description: remove nsCRT:: from compose, local, extensions, db, import → [checked in]remove nsCRT:: from compose, local, extensions, db, import
Reporter

Comment 173

12 years ago
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.
Reporter

Comment 174

12 years ago
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....
Reporter

Comment 175

12 years ago
Posted patch more string fixes in mailnews (obsolete) — Splinter Review
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!
Attachment #267677 - Flags: review?(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!
> 

Posted patch Remove nsCRT from news/ (obsolete) — Splinter Review
Reporter

Comment 178

12 years ago
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.
Attachment #267677 - Attachment is obsolete: true
Attachment #267763 - Flags: review?(prasad)
Attachment #267677 - Flags: review?(prasad)
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 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.
Reporter

Comment 181

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

12 years ago
Posted patch IMAP nsCRT:: Removal V1 (obsolete) — Splinter Review
Here is go-round number one for the IMAP nsCRT:: removal.
Attachment #267773 - Flags: review?(mscott)

Comment 183

12 years ago
This patch changes GetExistingFolder from nsMsgUtils to take a const nsCString& as argument instead of a const char*.
Attachment #267797 - Flags: review?(mscott)
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 cbiesinger@gmx.at
2. Adds utility functions to nsMsgUtils.
3. Replaces all usages of nsEscape from base and mailnews.
Attachment #267850 - Flags: review?(bugzilla)
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.
Attachment #267850 - Flags: review?(bugzilla) → review?(mscott)
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.
Attachment #267850 - Flags: review?(cbiesinger)
(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 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.
Reporter

Comment 189

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

thanks for the detailed review prasad.
Attachment #267763 - Attachment description: more string fixes in mail and mailnews → [checked in]more string fixes in mail and mailnews
Attachment #267763 - Flags: review?(prasad) → review+
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
Attachment #267850 - Attachment is obsolete: true
Attachment #267922 - Flags: review?(mscott)
Attachment #267850 - Flags: review?(mscott)
Attachment #267850 - Flags: review?(cbiesinger)
Reporter

Comment 191

12 years ago
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.
Attachment #267761 - Attachment is obsolete: true
Attachment #267925 - Flags: superreview?(neil)
Reporter

Comment 192

12 years ago
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?
Attachment #267797 - Attachment description: GetExistingFolder string cleanup → [checked in]GetExistingFolder string cleanup
Attachment #267797 - Flags: superreview+
Attachment #267797 - Flags: review?(mscott)
Attachment #267797 - Flags: review+
Reporter

Comment 193

12 years ago
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!
Attachment #267773 - Flags: superreview?(bienvenu)
Attachment #267773 - Flags: review?(mscott)
Attachment #267773 - Flags: review+

Comment 194

12 years ago
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.
Attachment #267773 - Flags: superreview?(bienvenu) → superreview+

Comment 195

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

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

Comment 196

12 years ago
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)) )
Attachment #268011 - Flags: superreview?(bienvenu)

Updated

12 years ago
Attachment #268011 - Flags: superreview?(bienvenu) → superreview+
Reporter

Comment 197

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

convert the nsCString to a ACString....
Attachment #268046 - Flags: superreview?(bienvenu)

Updated

12 years ago
Attachment #268046 - Flags: superreview?(bienvenu) → superreview+
Reporter

Updated

12 years ago
Attachment #268011 - Attachment description: remove nsCRT:: from mime → [checked in]remove nsCRT:: from mime
Reporter

Updated

12 years ago
Attachment #268046 - Attachment description: remove nsCString from nsIMsgCopyServiceListener → [checked in]remove nsCString from nsIMsgCopyServiceListener
Reporter

Comment 198

12 years ago
Attachment #268054 - Flags: superreview?(bienvenu)
Reporter

Comment 199

12 years ago
remove nsA(C)FlatString use from mailnews.
Attachment #268056 - Flags: superreview?(bienvenu)
(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.
Reporter

Comment 201

12 years ago
(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.

Updated

12 years ago
Attachment #268054 - Flags: superreview?(bienvenu) → superreview+

Updated

12 years ago
Attachment #268056 - Flags: superreview?(bienvenu) → superreview+
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.
Attachment #267925 - Flags: superreview?(neil) → superreview+

Updated

12 years ago
Attachment #268267 - Flags: superreview? → superreview+
Reporter

Updated

12 years ago
Depends on: 384346
Reporter

Comment 204

12 years ago
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!
Attachment #267925 - Attachment description: remove nsCRT methods from mailnews\news → [checked in]remove nsCRT methods from mailnews\news
Reporter

Updated

12 years ago
Attachment #268054 - Attachment description: remove nsAdoptingString from nsMessenger → [checked in]remove nsAdoptingString from nsMessenger
Reporter

Updated

12 years ago
Attachment #268056 - Attachment description: remove nsAFlatString and nsAFlatCString from mailnews → [checked in]remove nsAFlatString and nsAFlatCString from mailnews
Reporter

Updated

12 years ago
Depends on: 384490
Reporter

Comment 205

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

12 years ago
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.
Attachment #268543 - Flags: review?(mscott)
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.
Depends on: 384701
Reporter

Comment 208

12 years ago
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!
Attachment #268543 - Flags: review?(mscott) → review+
Reporter

Comment 209

12 years ago
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.
Attachment #268267 - Attachment description: don't use string iterators in MsgFindKeyword → [checked in]don't use string iterators in MsgFindKeyword
Reporter

Comment 210

12 years ago
Attachment #269031 - Flags: review?(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.
Reporter

Updated

12 years ago
Depends on: 384979
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
Attachment #269031 - Flags: review?(prasad) → review-
Reporter

Comment 213

12 years ago
Thanks for the great review prasad. This patch should address your comments.
Attachment #269031 - Attachment is obsolete: true
Attachment #269098 - Flags: review?(prasad)
Reporter

Updated

12 years ago
Attachment #267773 - Attachment is obsolete: true
Reporter

Comment 214

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

Nick, feel free to check this in at your leisure :).
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)
Attachment #269098 - Flags: review?(prasad) → review+
Reporter

Updated

12 years ago
Attachment #269098 - Attachment description: replace FindInReadable & several more string iterators → [checked in]replace FindInReadable & several more string iterators

Comment 216

12 years ago
This is the string cleanup of nsIMessenger.
Attachment #269207 - Flags: review?(mscott)

Comment 217

12 years ago
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.
Attachment #268543 - Attachment filename: imap_nsCRT_v2.diff → [checked in]imap_nsCRT_v2.diff

Comment 218

12 years ago
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.
Blocks: 385486
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.
Attachment #267922 - Attachment is obsolete: true
Attachment #269836 - Flags: review?(mscott)
Attachment #267922 - Flags: review?(mscott)
(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.
> 
> 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.
Reporter

Comment 222

12 years ago
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!
Attachment #269207 - Flags: review?(mscott) → review+
Reporter

Comment 223

12 years ago
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.
Attachment #269836 - Flags: review?(mscott) → review+
Reporter

Comment 224

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

I checked thie patch in with my additional changes. Thanks for the patch!
Attachment #269207 - Attachment description: nsIMessenger → [checked in]nsIMessenger
Reporter

Comment 226

12 years ago
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.
Attachment #269971 - Flags: superreview?(mscott) → superreview+

Updated

12 years ago
Attachment #269971 - Attachment description: fix regression from nsIMessenger patch → [checked in]fix regression from nsIMessenger patch
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
Attachment #269836 - Attachment is obsolete: true
Attachment #269995 - Flags: review?(mscott)
Reporter

Comment 228

12 years ago
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.
Attachment #269995 - Flags: review?(mscott) → review+

Comment 229

12 years ago
This patch changes the char* argument of GetMessageServiceFromURI to nsACString.
Attachment #270606 - Flags: review?(mscott)
Reporter

Updated

12 years ago
Attachment #269995 - Attachment description: Make addrbook and base nsEscape free - updated as per comments by Scott and Mark → [checked in]Make addrbook and base nsEscape free - updated as per comments by Scott and Mark
Reporter

Comment 230

12 years ago
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!
Attachment #270606 - Attachment description: GetMessageServiceFromURI char* -> nsACString → [checked in]GetMessageServiceFromURI char* -> nsACString
Attachment #270606 - Flags: review?(mscott) → review+

Comment 231

12 years ago