Closed
Bug 379070
Opened 18 years ago
Closed 16 years ago
Remove nsXPIDLCString / nsXPIDLString from mailnews
Categories
(MailNews Core :: Backend, defect)
MailNews Core
Backend
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: mscott, Unassigned)
References
()
Details
Attachments
(62 files, 23 obsolete files)
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•18 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•18 years ago
|
||
Comment on attachment 263063 [details] [diff] [review]
convert nsIMsgIdentity
nice, looks OK to me...
Attachment #263063 -
Flags: superreview?(bienvenu) → superreview+
Comment 3•18 years ago
|
||
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 4•18 years ago
|
||
Comment on attachment 263063 [details] [diff] [review]
convert nsIMsgIdentity
I noticed that the old code deletes a pref when the string is null, while the new code also deletes it when the string is empty, is that a problem?
Comment 5•18 years ago
|
||
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•18 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•18 years ago
|
Attachment #263171 -
Flags: superreview?(bienvenu) → superreview+
Comment 7•18 years ago
|
||
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•18 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•18 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+
Comment 10•18 years ago
|
||
(In reply to comment #8)
>Did you see any string patterns you didn't like in my nsMsgIdentity.cpp changes?
I spotted just the one: using aSupportsString->GetData instead of ToString
Comment 11•18 years ago
|
||
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+
Comment 12•18 years ago
|
||
Attachment #263119 -
Attachment is obsolete: true
Reporter | ||
Comment 13•18 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•18 years ago
|
||
I didn't bump the interface ID because I did that in yesterday's checkin.
Comment 15•18 years ago
|
||
Comment on attachment 263385 [details] [diff] [review]
[checked in]more nsIMsgIdentity changes
good times!
Attachment #263385 -
Flags: superreview?(bienvenu) → superreview+
Comment 16•18 years ago
|
||
This appears to be causing the redness on MAC builds.
Reporter | ||
Updated•18 years ago
|
Attachment #263385 -
Attachment description: more nsIMsgIdentity changes → [checked in]more nsIMsgIdentity changes
Reporter | ||
Updated•18 years ago
|
Attachment #263171 -
Attachment description: Merged nsIdentity changes into Scott's patch → [checked in]Merged nsIdentity changes into Scott's patch
Reporter | ||
Updated•18 years ago
|
Attachment #263306 -
Attachment description: nsMsgIncomingServer pref cleanup updated for bitrot → [checked in]nsMsgIncomingServer pref cleanup updated for bitrot
Reporter | ||
Comment 17•18 years ago
|
||
nsIMsgAccount cleanup.
Attachment #263412 -
Flags: superreview?(bienvenu)
Comment 18•18 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•18 years ago
|
Attachment #263412 -
Attachment description: nsIMsgAccount → [checked in]nsIMsgAccount
Reporter | ||
Comment 19•18 years ago
|
||
Attachment #263528 -
Flags: superreview?(bienvenu)
Comment 20•18 years ago
|
||
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(¯o_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•18 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 22•18 years ago
|
||
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•18 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•18 years ago
|
||
nsDependentString is part of nsStringAPI so we can use it.
Reporter | ||
Comment 25•18 years ago
|
||
I updated this patch to include most of Neil's feedback for nsIMsgIdentity and nsIMsgAccountManager.
Comment 26•18 years ago
|
||
(In reply to comment #23)
>>> NS_IMETHODIMP \
>>>-nsMsgIdentity::Set##_postfix(const char *value) \
>>>+nsMsgIdentity::Set##_postfix(const nsACString& value) \
>>> { \
>>>- return setFolderPref(_prefname, value, _flag); \
>>>+ return setFolderPref(_prefname, nsCString(value), _flag); \
>>I didn't see any code in setFolderPref that needs value to be an nsCString so
>>you could probably change it to nsACString.
>setFolderPref is called by getFolderPref and I couldn't get getFolderPref to
>work with a nsACString, maybe I'm just being lazy there..
Since an nsCString can of couse be passed as a const nsACString& you don't need to change getFolderPref, so the only three changes are
a) declare and b) define setFolderPref as const nsACString& value
c) change the above back to setFolderPref(_prefname, value, _flag);
Comment 27•18 years ago
|
||
Comment on attachment 263653 [details] [diff] [review]
[checked in]updated nsIMsgAccountManager patch
> if (!uri.IsEmpty())
>- mCompFields->SetFcc(PL_strcasecmp(uri.get(), "nocopy://") ? uri.get() : "");
>+ mCompFields->SetFcc(uri.LowerCaseEqualsLiteral("nocopy://") ? "" : uri.get());
> else
> mCompFields->SetFcc("");
It occurs to me the IsEmpty() test only existed to avoid passing NULL to PL_strcasecmp, but of course that's not an issue for LowerCaseEqualsLiteral, so you can kill the isEmpty check.
Comment 28•18 years ago
|
||
Reporter | ||
Updated•18 years ago
|
Attachment #263653 -
Attachment description: updated nsIMsgAccountManager patch → [checked in]updated nsIMsgAccountManager patch
Reporter | ||
Updated•18 years ago
|
Attachment #263528 -
Attachment is obsolete: true
Reporter | ||
Updated•18 years ago
|
Updated•18 years ago
|
OS: Windows Vista → All
Hardware: PC → All
Reporter | ||
Comment 29•18 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•18 years ago
|
||
ugh, I just did nsImapIncomingServer.cpp - oh well...your patch is most likely a super-set of mine.
Comment 31•18 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•18 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•18 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•18 years ago
|
Attachment #264250 -
Flags: superreview?(bienvenu) → superreview+
Comment 34•18 years ago
|
||
Attachment #264283 -
Flags: superreview?(mscott)
Reporter | ||
Comment 35•18 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•18 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•18 years ago
|
Attachment #264253 -
Attachment description: remove most imap uses → [checked in]remove most imap uses
Comment 37•18 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•18 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•18 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•18 years ago
|
||
this makes things much happier
Attachment #264370 -
Flags: superreview?(mscott)
Reporter | ||
Comment 41•18 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•18 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 43•18 years ago
|
||
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•18 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•18 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.
Comment 46•18 years ago
|
||
imap login crashes for me though -- bug 380461
Comment 47•18 years ago
|
||
Attachment #264562 -
Flags: superreview?(mscott)
Comment 48•18 years ago
|
||
Haven't tried this, but I think it does the right thing...
Attachment #264563 -
Flags: superreview?(mscott)
Reporter | ||
Updated•18 years ago
|
Attachment #264563 -
Flags: superreview?(mscott) → superreview+
Comment 49•18 years ago
|
||
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•18 years ago
|
Attachment #264563 -
Attachment description: fix password regression → [checked in]fix password regression
Comment 50•18 years ago
|
||
I hope this doesn't cause any conflicts - I tried to keep it contained.
Attachment #264922 -
Flags: superreview?(mscott)
Reporter | ||
Comment 51•18 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•18 years ago
|
Attachment #264562 -
Flags: superreview?(mscott) → superreview+
Comment 52•18 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•18 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.
Comment 53•18 years ago
|
||
(In reply to comment #52)
> + aResult.AppendLiteral("]");
>
> UI think that can just be:
> aResult.Append(']'); since it's a single character.
>
> that didn't work for nsAString - append(char)
Try aResult.Append(PRUnichar(']'));
Comment 54•18 years ago
|
||
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•18 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•18 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•18 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•18 years ago
|
Attachment #265319 -
Attachment description: nsIMsg*Folder.idl → [checked in]nsIMsg*Folder.idl
Reporter | ||
Comment 58•18 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•18 years ago
|
||
nsIMsgWindow changes.
Attachment #265601 -
Flags: superreview?(bienvenu)
Updated•18 years ago
|
Attachment #265601 -
Attachment mime type: application/octet-stream → text/plain
Updated•18 years ago
|
Attachment #265601 -
Flags: superreview?(bienvenu) → superreview+
Reporter | ||
Updated•18 years ago
|
Attachment #265601 -
Attachment description: nsIMsgWindow → [checked in]nsIMsgWindow
Comment 60•18 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•18 years ago
|
||
this fixes the pop3 filter move action regression.
Attachment #265676 -
Flags: superreview?(mscott)
Reporter | ||
Comment 62•18 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•18 years ago
|
Attachment #265676 -
Attachment description: fix pop3 filter move action → [checked in]fix pop3 filter move action
Comment 63•18 years ago
|
||
Attachment #265683 -
Flags: review?
Updated•18 years ago
|
Attachment #265683 -
Flags: review? → review?(mscott)
Comment 64•18 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•18 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•18 years ago
|
Attachment #264562 -
Attachment description: remove xpidl strings from mime → [checked in]remove xpidl strings from mime
Comment 66•18 years ago
|
||
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•18 years ago
|
Attachment #265692 -
Flags: review?(mscott)
Reporter | ||
Comment 67•18 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•18 years ago
|
||
I think the remaining ws changes are sensible. Choose your favourite patch. ;)
Attachment #265694 -
Flags: review?(mscott)
Reporter | ||
Comment 69•18 years ago
|
||
nsIMsgFolderCache and nsIMsgFolderCacheElement changes.
Attachment #265710 -
Flags: superreview?(bienvenu)
Reporter | ||
Updated•18 years ago
|
Attachment #265692 -
Attachment is obsolete: true
Attachment #265692 -
Flags: review?(mscott)
Reporter | ||
Comment 70•18 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•18 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•18 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•18 years ago
|
Attachment #265710 -
Attachment description: nsIMsgFolderCacheElement and nsIMsgFolderCache → [checked in]nsIMsgFolderCacheElement and nsIMsgFolderCache
Reporter | ||
Comment 73•18 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
Comment 74•18 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.
Comment 75•18 years ago
|
||
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)
Comment 76•18 years ago
|
||
Attachment #265952 -
Flags: superreview?(mscott)
Reporter | ||
Comment 77•18 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•18 years ago
|
Attachment #265876 -
Flags: review?(mscott)
Updated•18 years ago
|
Attachment #265952 -
Attachment description: remove xpidl strings from view code → [checked in]remove xpidl strings from view code
Reporter | ||
Comment 78•18 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•18 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•18 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•18 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•18 years ago
|
Attachment #266018 -
Flags: superreview?(bienvenu) → superreview+
Reporter | ||
Updated•18 years ago
|
Attachment #266018 -
Attachment description: string cleanup for mailnews\base → [checked in]string cleanup for mailnews\base
Reporter | ||
Comment 82•18 years ago
|
||
Same string changes I just finished making to mailnews\base.
Attachment #266023 -
Flags: superreview?(bienvenu)
Updated•18 years ago
|
Attachment #266023 -
Flags: superreview?(bienvenu) → superreview+
Comment 83•18 years ago
|
||
(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•18 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.
Comment 85•18 years ago
|
||
Should we also start removing the "+" operator on strings. The "+" operator does not exist in frozen linkage.
This will mostly involve converting all "nsString writable = readable1 + readable2" to "nsString writable = readable1; writable.Append(readable2);"
Comment 86•18 years ago
|
||
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•18 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.
Comment 88•18 years ago
|
||
(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•18 years ago
|
Attachment #266023 -
Attachment description: string cleanup for mailnews\local → [checked in]string cleanup for mailnews\local
Reporter | ||
Updated•18 years ago
|
Attachment #265876 -
Attachment description: nsIImapHostSessionList Clean-up V1 → [checked in]nsIImapHostSessionList Clean-up V1
Reporter | ||
Comment 89•18 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•18 years ago
|
Attachment #266092 -
Flags: superreview?(bienvenu) → superreview+
Reporter | ||
Updated•18 years ago
|
Attachment #266092 -
Attachment description: string cleanup for mailnews\mime → [checked in]string cleanup for mailnews\mime
Comment 90•18 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•18 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•18 years ago
|
Attachment #266115 -
Attachment description: fix some issues with previous patches → [checked in]fix some issues with previous patches
Reporter | ||
Comment 92•18 years ago
|
||
Attachment #266157 -
Flags: superreview?(bienvenu)
Updated•18 years ago
|
Attachment #266157 -
Flags: superreview?(bienvenu) → superreview+
Comment 93•18 years ago
|
||
The first go-round for the nsIImapService cleanup. This is a white-space free patch.
Attachment #266227 -
Flags: review?(mscott)
Comment 94•18 years ago
|
||
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•18 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•18 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•18 years ago
|
||
First try of nsIMsgThread cleanup.
Attachment #266413 -
Flags: review?(mscott)
Comment 98•18 years ago
|
||
second try :)
Attachment #266413 -
Attachment is obsolete: true
Attachment #266414 -
Flags: review?(mscott)
Attachment #266413 -
Flags: review?(mscott)
Comment 99•18 years ago
|
||
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)
Comment 100•18 years ago
|
||
(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•18 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•18 years ago
|
Attachment #266157 -
Attachment description: string cleanup for mailnews\news → [checked in]string cleanup for mailnews\news
Reporter | ||
Comment 102•18 years ago
|
||
Attachment #266507 -
Flags: superreview?(bienvenu)
Updated•18 years ago
|
Attachment #266507 -
Flags: superreview?(bienvenu) → superreview+
Reporter | ||
Updated•18 years ago
|
Attachment #266507 -
Attachment description: string cleanup for mailnews\db → [checked in]string cleanup for mailnews\db
Reporter | ||
Comment 103•18 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•18 years ago
|
Attachment #266414 -
Attachment description: nsIMsgThread → [checked in]nsIMsgThread
Reporter | ||
Comment 104•18 years ago
|
||
Attachment #266540 -
Flags: superreview?(bienvenu)
Updated•18 years ago
|
Attachment #266540 -
Flags: superreview?(bienvenu) → superreview+
Reporter | ||
Updated•18 years ago
|
Attachment #266540 -
Attachment description: string cleanup for mailnews\extensions → [checked in]string cleanup for mailnews\extensions
Comment 105•18 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•18 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);
Reporter | ||
Comment 107•18 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•18 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•18 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
Updated•18 years ago
|
Comment 110•18 years ago
|
||
There's a lot more string cleanup needed here, but this gets rid of xpidl strings...
Attachment #266674 -
Flags: superreview?(mscott)
Comment 111•18 years ago
|
||
Comment on attachment 265319 [details] [diff] [review]
[checked in]nsIMsg*Folder.idl
>+ url.swap(*aURL);
XPCOM doesn't require the caller to init their out parameter (getter_AddRefs always does, but only because of an implementation detail), so generally you have to write *aURL = nsnull; first. Most people actually add this at the beginning, so that *aURL is always null on failure.
Comment 112•18 years ago
|
||
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•18 years ago
|
||
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)
Reporter | ||
Comment 114•18 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•18 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 116•18 years ago
|
||
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•18 years ago
|
||
Attachment #266821 -
Flags: superreview?(bienvenu)
Updated•18 years ago
|
Attachment #266821 -
Flags: superreview?(bienvenu) → superreview+
Comment 118•18 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•18 years ago
|
||
I think this is the last of the directories for pure string cleanup.
Attachment #266943 -
Flags: superreview?(bienvenu)
Comment 120•18 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•18 years ago
|
||
Attachment #266956 -
Flags: superreview?(bienvenu)
Updated•18 years ago
|
Attachment #266956 -
Flags: superreview?(bienvenu) → superreview+
Comment 122•18 years ago
|
||
(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•18 years ago
|
||
This is the cleanup of nsIMsgStatusFeedback.
Attachment #267001 -
Flags: review?(mscott)
Comment 124•18 years ago
|
||
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•18 years ago
|
Attachment #266821 -
Attachment description: string cleanup for mailnews\import → [checked in]string cleanup for mailnews\import
Reporter | ||
Updated•18 years ago
|
Attachment #266943 -
Attachment description: string cleanup for mailnews\mapi → [checked in]string cleanup for mailnews\mapi
Reporter | ||
Updated•18 years ago
|
Attachment #266956 -
Attachment description: string cleanup for mail\ → [checked in]string cleanup for mail\
Reporter | ||
Comment 125•18 years ago
|
||
Remove nsAdoptingCString, change nsPromiseFlat(C)String to PromiseFlat(C)String, remove nsPrintfCString.
Attachment #267089 -
Flags: superreview?(bienvenu)
Updated•18 years ago
|
Attachment #267089 -
Flags: superreview?(bienvenu) → superreview+
Reporter | ||
Updated•18 years ago
|
Attachment #267089 -
Attachment description: misc string changes in mozilla\mailnews → [checked in]misc string changes in mozilla\mailnews
Reporter | ||
Comment 126•18 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+
Comment 127•18 years ago
|
||
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 128•18 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
checked the patch mime-type
Attachment #267110 -
Attachment is patch: true
Attachment #267110 -
Attachment mime type: text/x-patch → text/plain
Comment 129•18 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 130•18 years ago
|
||
Comment on attachment 265710 [details] [diff] [review]
[checked in]nsIMsgFolderCacheElement and nsIMsgFolderCache
>- nsCStringKey hashKey(pathKey);
>-
>+ nsCStringKey hashKey(PromiseFlatCString(pathKey).get());
nsCStringKey will in fact accept an nsACString too, so these changes were unnecessary.
>-NS_IMPL_GETTER_STR(nsMsgFolderCacheElement::GetKey, m_folderKey)
>-NS_IMPL_SETTER_STR(nsMsgFolderCacheElement::SetKey, m_folderKey)
Are we missing NS_IMPL_G/SETTER_ACSTR macros?
>+ yarn.mYarn_Buf = (void *) PromiseFlatCString(propertyValue).get();
The PromiseFlatCString destructor will make this value stale.
> void nsMsgFolderCacheElement::SetMDBRow(nsIMdbRow *row)
> {
> if (m_mdbRow)
> NS_RELEASE(m_mdbRow);
>- m_mdbRow = row;
>- if (row)
>- NS_ADDREF(row);
>+ NS_IF_ADDREF(m_mdbRow = row);
> }
Why not make m_mdbRow an nsCOMPtr (or nsRefPtr)?
Comment 131•18 years ago
|
||
Comment on attachment 265694 [details] [diff] [review]
[checked in]nsMsgProgress
> nsCOMPtr<nsISupports> aSupports;
>- nsCOMPtr<nsIWebProgressListener> aProgressListener;
>+ nsCOMPtr<nsIWebProgressListener> progressListener;
> for (i = count - 1; i >= 0; i --)
> {
> m_listenerList->GetElementAt(i, getter_AddRefs(aSupports));
>- aProgressListener = do_QueryInterface(aSupports);
>- if (aProgressListener)
>- aProgressListener->OnStatusChange(aWebProgress, aRequest, aStatus, aMessage);
>+ progressListener = do_QueryInterface(aSupports);
>+ if (progressListener)
>+ progressListener->OnStatusChange(aWebProgress, aRequest, aStatus, aMessage);
> }
As you were touching this you could have taken the opportunity to convert to progressListener = do_QueryElementAt(m_listenerList, i); or better still convert m_listenerList to an nsCOMArray.
Comment 132•18 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
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•18 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•18 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•18 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•18 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•18 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•18 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•18 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•18 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•18 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•18 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•18 years ago
|
Attachment #267212 -
Flags: superreview?(bienvenu) → superreview+
Reporter | ||
Updated•18 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
Comment 142•18 years ago
|
||
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•18 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•18 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•18 years ago
|
Attachment #267283 -
Flags: superreview?(bienvenu) → superreview+
Comment 145•18 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•18 years ago
|
Attachment #267287 -
Flags: superreview? → superreview?(bienvenu)
Comment 146•18 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•18 years ago
|
Attachment #267293 -
Flags: review?(mscott)
Comment 147•18 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•18 years ago
|
||
This patch contains the conversion of nsMsgProgress::m_listenerList from nsISupportsArray* to nsCOMArray.
Updated•18 years ago
|
Attachment #267319 -
Flags: review?(mscott)
Comment 149•18 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•18 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•18 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•18 years ago
|
Attachment #267319 -
Flags: superreview?(neil)
Reporter | ||
Updated•18 years ago
|
Attachment #267283 -
Attachment description: start removing nsCRT:: for char * functions → [checked in]start removing nsCRT:: for char * functions
Reporter | ||
Updated•18 years ago
|
Attachment #267293 -
Attachment description: nsIMsgStatusFeedback → [checked in]nsIMsgStatusFeedback
Comment 152•18 years ago
|
||
Scott, lot of these nsCRT utilities might vanish once we migrate the code to use A(C)String. However, for testing purposes we could use something like this:
#ifdef MOZILLA_INTERNAL_API
#include "nsCRT.h"
#define NS_strlen nsCRT::strlen
#define NS_strdup nsCRT::strdup
#define NS_IsAsciiWhitespace nsCRT::IsAsciiSpace
#else
#include "nsCRTGlue.h"
#endif
or we could put this into msgCore.h or something temporarily.
Then, all nsCRT::strlen(char *) should be converted to strlen, and all nsCRT::strlen(PRUnichar *) should be converted to one of the above.
Currently I think these are the only functions from nsCRT that we need.
(In reply to comment #150)
> (From update of attachment 267110 [details] [diff] [review])
> Great work Prasad! I've landed the patch. What's next for you? I'm sure Mark
> could use help converting more address book APIs to use A(C)String.
>
> I'm trying to reduce our use of nsCRT methods for |char *| strings in the rest
> of mailnews. Once I'm done with that I'm going to start trying to build
> base\util without the internal APIs to see how far I can get.
>
Comment 153•18 years ago
|
||
nsEscape is another small thing that will need migration/conversion.
mailnews code uses four functions from this class... nsEscape, nsUnescape, nsEscapeHTML and nsEscapeHTML2. While nsEscape and nsUnescape now have an alternative frozen implementation (exposed through nsINetUtil - bug:372050), the other two functions have no frozen alternative.
We could move those functions to mailnews code (browser made a copy of these functions for itself) or could get that exposed through NetUtil too. These two functions are not dependent on rest of the nsEscape code, so either should be fine. Comments???
Scott: I have started working on migrating the addrbook APIs to A(C)String.
Comment 154•18 years ago
|
||
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+
Comment 155•18 years ago
|
||
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•18 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•18 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•18 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•18 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•18 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•18 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•18 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+
Comment 163•18 years ago
|
||
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•18 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•18 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•18 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•18 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•18 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.
Comment 169•18 years ago
|
||
Scott, I will do the nsEscape and nsUnescape part.
Reporter | ||
Comment 170•18 years ago
|
||
Comment 171•18 years ago
|
||
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•18 years ago
|
Attachment #267532 -
Attachment description: Make addrbook nsCRT:: free → [checked in]Make addrbook nsCRT:: free
Reporter | ||
Updated•18 years ago
|
Attachment #267588 -
Flags: superreview?(bienvenu)
Comment 172•18 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•18 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•18 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•18 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•18 years ago
|
||
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)
Comment 176•18 years ago
|
||
Scott, there seems to be a problem with the usage of ns(C)String::Find.
- if (FindInReadable(replyTo, start, end) == PR_FALSE) {
+ if (!replyToStr.Find(replyTo)) {
if (replyToStr.Length() > 0)
replyToStr.Append(',');
Both are not the same. The if statement should probably be
if (replyToStr.Find(replyTo) != -1)
and not
if (!replyToStr.Find(replyTo))
There are many places in this patch with the same mistake.
Indentation mistake:
+ PRUnichar * substr_start = strippedUCS2.BeginWriting();
Everything else looked fine to me.
(In reply to comment #175)
> Created an attachment (id=267677) [details]
> more string fixes in mailnews
>
> Prasad, I found a few more AssignWithConversion calls that I missed before. I
> also removed use of string iterators which aren't supported in the frozen API.
>
> Do you mind reviewing this patch? Thanks!
>
Comment 177•18 years ago
|
||
Reporter | ||
Comment 178•18 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 179•18 years ago
|
||
Comment on attachment 266092 [details] [diff] [review]
[checked in]string cleanup for mailnews\mime
> int MimeWriteAString(MimeObject *obj, const nsACString &string)
> {
>- const nsCString &flatString = PromiseFlatCString(string);
>- return MimeObject_write(obj, flatString.get(), flatString.Length(), PR_TRUE);
>+ return MimeObject_write(obj, nsCString(string).get(), string.Length(), PR_TRUE);
> }
Isn't this a backward step?
> if (mailCharset && *mailCharset) {
>- rv = nsMsgI18NConvertToUnicode(mailCharset, nsPromiseFlatCString(inputStr), lineSource);
>+ rv = nsMsgI18NConvertToUnicode(mailCharset, nsDependentCString(inputStr), lineSource);
> NS_ENSURE_SUCCESS(rv, -1);
> }
I wouldn't have thought this was legal.
(I would have used PromiseFlatCString without the ns.)
Comment 180•18 years ago
|
||
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•18 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•18 years ago
|
||
Here is go-round number one for the IMAP nsCRT:: removal.
Attachment #267773 -
Flags: review?(mscott)
Comment 183•18 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)
Comment 184•18 years ago
|
||
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 185•18 years ago
|
||
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 186•18 years ago
|
||
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)
Comment 187•18 years ago
|
||
(In reply to comment #178)
> Created an attachment (id=267763) [details]
> more string fixes in mail and mailnews
>
> Good catch Prasad. This fixes the problem with .Find and removes nsCRT from
> mail\components\migration. I also fixed a .Find call in nsImapProtocol.cpp that
> was also expecting a bool and not an index.
>
Scott, the patch looked absolutely fine. I don't have permissions to grant a review, so you will probably have to do it yourself :)
Comment 188•18 years ago
|
||
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•18 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+
Comment 190•17 years ago
|
||
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•17 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•17 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•17 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•17 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•17 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•17 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•17 years ago
|
Attachment #268011 -
Flags: superreview?(bienvenu) → superreview+
Reporter | ||
Comment 197•17 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•17 years ago
|
Attachment #268046 -
Flags: superreview?(bienvenu) → superreview+
Reporter | ||
Updated•17 years ago
|
Attachment #268011 -
Attachment description: remove nsCRT:: from mime → [checked in]remove nsCRT:: from mime
Reporter | ||
Updated•17 years ago
|
Attachment #268046 -
Attachment description: remove nsCString from nsIMsgCopyServiceListener → [checked in]remove nsCString from nsIMsgCopyServiceListener
Reporter | ||
Comment 198•17 years ago
|
||
Attachment #268054 -
Flags: superreview?(bienvenu)
Reporter | ||
Comment 199•17 years ago
|
||
remove nsA(C)FlatString use from mailnews.
Attachment #268056 -
Flags: superreview?(bienvenu)
Comment 200•17 years ago
|
||
(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•17 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•17 years ago
|
Attachment #268054 -
Flags: superreview?(bienvenu) → superreview+
Updated•17 years ago
|
Attachment #268056 -
Flags: superreview?(bienvenu) → superreview+
Comment 202•17 years ago
|
||
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+
Reporter | ||
Comment 203•17 years ago
|
||
Attachment #268267 -
Flags: superreview?
Updated•17 years ago
|
Attachment #268267 -
Flags: superreview? → superreview+
Reporter | ||
Comment 204•17 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•17 years ago
|
Attachment #268054 -
Attachment description: remove nsAdoptingString from nsMessenger → [checked in]remove nsAdoptingString from nsMessenger
Reporter | ||
Updated•17 years ago
|
Attachment #268056 -
Attachment description: remove nsAFlatString and nsAFlatCString from mailnews → [checked in]remove nsAFlatString and nsAFlatCString from mailnews
Reporter | ||
Comment 205•17 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•17 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 207•17 years ago
|
||
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.
Reporter | ||
Comment 208•17 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•17 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•17 years ago
|
||
Attachment #269031 -
Flags: review?(prasad)
Comment 211•17 years ago
|
||
Comment on attachment 269031 [details] [diff] [review]
replace FindInReadable & several more string iterators
In base/src/nsMsgDBView.cpp -----
> labelStr.Append((char) (label + '0'));
>- if (!FindInReadable(labelStr, keywords, nsCaseInsensitiveCStringComparator()))
>+ if (keywords.Find(labelStr, PR_TRUE) >= 0)
> FetchLabel(aHdr, tags);
That should be:
if (keywords.Find(labelStr, PR_TRUE) == -1)
This is the only mistake in this patch. I think we are looking for existing keywords here.
In base/util/nsMsgDBFolder.cpp -----
>- else if (StringBeginsWith(curLine, NS_LITERAL_CSTRING("Content-Transfer-Encoding:"),
>- nsCaseInsensitiveCStringComparator()))
>+ else if (curLine.Find("Content-Transfer-Encoding:", PR_TRUE) >= 0)
This should probably be left as it is till we do a String Comparator cleanup. The change you made does not ensure that "Content-Transfer-Encoding:" is at the beginning of curLine. You could use StringBeginsWith or probably even use Find and look for a return value of "0"
>+ const char * domain = trustedMailDomains.BeginReading();
>+ const char * domainEnd = trustedMailDomains.EndReading();
>+ const char * hostStart = host.BeginReading();
Minor thing: The spaces after '*'
In compose/src/nsMsgCompose.cpp -----
>+ PRInt32 wasOffset = subject.RFind(NS_LITERAL_STRING(" (was:"));
Just for the record, I think RFind isn't available in the frozen string API and we will have to find other ways to achieve this functionality.
Comment 212•17 years ago
|
||
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•17 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•17 years ago
|
Attachment #267773 -
Attachment is obsolete: true
Reporter | ||
Comment 214•17 years ago
|
||
Comment on attachment 268543 [details] [diff] [review]
IMAP nsCRT:: Removal V2
Nick, feel free to check this in at your leisure :).
Comment 215•17 years ago
|
||
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•17 years ago
|
Attachment #269098 -
Attachment description: replace FindInReadable & several more string iterators → [checked in]replace FindInReadable & several more string iterators
Comment 216•17 years ago
|
||
This is the string cleanup of nsIMessenger.
Attachment #269207 -
Flags: review?(mscott)
Comment 217•17 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•17 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.
Comment 219•17 years ago
|
||
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)
Comment 220•17 years ago
|
||
(In reply to comment #219)
> Created an attachment (id=269836) [details]
> Make addrbook and base nsEscape free
> The patch makes mailnews/addrbook and mailnews/base nsEscape free. It defines
> a few utility functions in nsMsgUtils and uses them across the code.
> There are changes in how the EscapeURL functions work - the versions from
> nsEscape.h appended the output to result, but the new ones (exposed through
> nsINetUtil) assign the value.
I'm going to jump in here with a couple of address book comments:
nsAddressBook::HandleContent
+ char *unescapedData = PL_strdup(unescapedDataStr.get());
Can't you use unescapedDataStr.get() in the call to EscapedVCardToAbCard and therefore drop the extra allocations etc?
Ditto in the EscapedVCardToAbCard function below that one.
Comment 221•17 years ago
|
||
>
> 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•17 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•17 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•17 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
Comment 225•17 years ago
|
||
Attachment #269971 -
Flags: superreview?(mscott)
Reporter | ||
Comment 226•17 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•17 years ago
|
Attachment #269971 -
Attachment description: fix regression from nsIMessenger patch → [checked in]fix regression from nsIMessenger patch
Comment 227•17 years ago
|
||
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•17 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•17 years ago
|
||
This patch changes the char* argument of GetMessageServiceFromURI to nsACString.
Attachment #270606 -
Flags: review?(mscott)
Reporter | ||
Updated•17 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•17 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•17 years ago
|
||
This is the cleanup of nsIMsgMessageService and its implementations nsNntpService, nsMailboxService and nsImapService. Whitespace changes are omitted. :)
Attachment #272049 -
Flags: review?(mscott)
Comment 232•17 years ago
|
||
Attachment #272059 -
Flags: superreview?(mscott)
Reporter | ||
Updated•17 years ago
|
Attachment #272059 -
Flags: superreview?(mscott) → superreview+
Comment 233•17 years ago
|
||
nominating for wanted-thunderbird3 to keep it on the radar.
Flags: wanted-thunderbird3?
Comment 234•17 years ago
|
||
Comment on attachment 272049 [details] [diff] [review]
[bitrotted] nsIMsgMessageService
This has bitrotted quite a bit, can you provide an updated patch?
Attachment #272049 -
Attachment filename: nsIMsgMessageService_04.diff → [bitrotted] nsIMsgMessageService_04.diff
Attachment #272049 -
Flags: review?(mscott)
Updated•17 years ago
|
Attachment #272049 -
Attachment description: nsIMsgMessageService → [bitrotted] nsIMsgMessageService
Attachment #272049 -
Attachment filename: [bitrotted] nsIMsgMessageService_04.diff → nsIMsgMessageService_04.diff
Comment 235•17 years ago
|
||
Comment on attachment 272049 [details] [diff] [review]
[bitrotted] nsIMsgMessageService
>- msgurl->SetUri(nsDependentCString(aImapURI).get());
Nice fix :-)
>- if (!PL_strncmp(aMessageURI, kNewsRootURI, kNewsRootURILen)) {
>+ if (aMessageURI.Equals(kNewsRootURI))
Shouldn't this use StringBeginsWith (and kill off kNewsRootURILen too).
Comment 236•17 years ago
|
||
Comment on attachment 269207 [details] [diff] [review]
[checked in]nsIMessenger
>- rv = NS_NewURI(getter_AddRefs(uri), aURL);
>+ rv = NS_NewURI(getter_AddRefs(uri), PromiseFlatCString(aURL).get());
Actually the char* version use an nsDependentCString... just pass aURL!
Assignee | ||
Updated•16 years ago
|
Product: Core → MailNews Core
Description
•