476150 - Move to ParseString(nsCString, char, nsTArray<nsCString>&)

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[[e-mail]]

Bug 474759 was simply a build bustage fix, although I converted all the easy cases at the time. However we should obsolete the nsCStringArray versions.

Comment 1

[[e-mail]]

Attachment: "Easier" conversions

These are the changes that don't affect lines containing ParseString itself, instead they just change the type of the array. The rest is just fixing up the differences between the nsCStringArray and nsTArray<nsCString> API.

Comment 2

[[e-mail]]

Comment on attachment 359751 [details] [diff] [review]
"Easier" conversions

Joshua might like to look at the news changes, in particular m_currentXHDRIndex is now unsigned.

Comment 3

[Joshua Cranmer [:jcranmer]]

Comment on attachment 359751 [details] [diff] [review]
"Easier" conversions

I only looked at the news portion of this patch.

As it stands, the patch creates 3 signed/unsigned comparison/assignment warnings in nsNNTPNewsgroupList.cpp

r+ if you fix those to not warn.

Comment 4

[[e-mail]]

(In reply to comment #3)
> As it stands, the patch creates 3 signed/unsigned comparison/assignment
> warnings in nsNNTPNewsgroupList.cpp
Well you'll have to tell me where they are as VC7.1 doesn't find them.

Comment 5

[[e-mail]]

Attachment: Harder conversions (-w)

Interestingly I copied the "load all possible entries into an array and check that IndexOf returns the original index to eliminate duplicates" trick from nsSmtpService.cpp yet I actually removed that check from in that file in favour of the superior "simply ensure that all servers are created" check.

Comment 6

[David :Bienvenu]

Comment on attachment 359751 [details] [diff] [review]
"Easier" conversions

while you're here, this line could use wrapping:

+          nsCString &curPartToStrip = obj->options->state->partsToStrip[partIndex];
+          if (newPart.Find(curPartToStrip) == 0 && (newPart.Length() == curPartToStrip.Length() || newPart.CharAt(curPartToStrip.Length()) == '.'))

Comment 7

[Mark Banner (:standard8)]

Comment on attachment 359751 [details] [diff] [review]
"Easier" conversions

Sorry for the delay in getting to this. It looks fine, but doesn't compile:

/Users/moztest/comm/main/src/mailnews/base/src/nsMsgAccountManager.cpp: In member function ‘nsresult nsMsgAccountManager::AddVFListenersForVF(nsIMsgFolder*, const nsCString&, nsIRDFService*, nsIMsgDBService*)’:
/Users/moztest/comm/main/src/mailnews/base/src/nsMsgAccountManager.cpp:2927: error: no matching function for call to ‘ParseString(const nsCString&, char, nsCStringArray&)’
../../../mozilla/dist/include/msgbaseutil/nsMsgUtils.h:227: note: candidates are: PRBool ParseString(const char*, const char*, nsCStringArray&)
../../../mozilla/dist/include/msgbaseutil/nsMsgUtils.h:242: note:                 PRBool ParseString(const nsACString_internal&, char, nsTArray<nsCString>&)

I guess someone else is using ParseString now?

Comment 8

[[e-mail]]

Attachment: Including nsMsgAccountManager.cpp change

Sorry about that, I'd left that file out of the patch by mistake.

Comment 9

[Mark Banner (:standard8)]

Comment on attachment 363484 [details] [diff] [review]
Including nsMsgAccountManager.cpp change

That's better. r=me.

Comment 10

[[e-mail]]

Comment on attachment 363484 [details] [diff] [review]
Including nsMsgAccountManager.cpp change

Pushed changeset 1f0be0d2b89f to comm-central.

Comment 11

[Mark Banner (:standard8)]

Comment on attachment 360325 [details] [diff] [review]
Harder conversions (-w)

>         if (!accountList.IsEmpty()) {
>-          nsCStringArray existingAccountsArray;
>-          ParseString(accountList.get(), ACCOUNT_DELIMITER, existingAccountsArray);
>+          nsTArray<nsCString> accountsArray;
>+          ParseString(accountList, ACCOUNT_DELIMITER, accountsArray);
>+          PRUint32 i = accountsArray.Length();
...
>+          ParseString(appendAccountList, ACCOUNT_DELIMITER, accountsArray);

Ok, this is confusing when you forget that ParseString is an appending operation not a truncate & append.

Can you add a brief comment here that we append the appendAccountList onto the accountsArray?

>+          for (; i < accountsArray.Length(); i++) {
>+            if (accountsArray.IndexOf(accountsArray[i]) == i) {
>                 accountList.Append(',');
>                 accountList.Append(newAccount);

and a comment here that we're stripping out duplicates.

However, newAccount is just going to be an empty string which is wrong.

>diff -pruw comm.2a5433caac6e/mailnews/compose/src/nsMsgComposeService.cpp comm/mailnews/compose/src/nsMsgComposeService.cpp

>@@ -1401,30 +1400,21 @@ nsresult nsMsgComposeService::AddGlobalH
>         // Tokenize the data and add each html domain if it is not alredy there in
>         // the user's current html or plaintext domain lists
>-        char *newData = globalHtmlDomainList.BeginWriting();
>-        char *token = NS_strtok(DOMAIN_DELIMITER, &newData);
>+        globalHtmlDomainList.StripWhitespace();
>+        ParseString(globalHtmlDomainList, DOMAIN_DELIMITER, domainArray);
> 
>-        nsCAutoString htmlDomain;
>-        while (token) {
>-          if (token && *token) {
>-            htmlDomain.Assign(token);
>-            htmlDomain.StripWhitespace();
>-
>-            if (htmlDomainArray.IndexOf(htmlDomain) == -1  &&
>-                plaintextDomainArray.IndexOf(htmlDomain) == -1) {
>+        for (; i < domainArray.Length(); i++) {
>+          if (domainArray.IndexOf(domainArray[i]) == i) {
>               if (!newHtmlDomainList.IsEmpty())
>                 newHtmlDomainList += DOMAIN_DELIMITER;
>-              newHtmlDomainList += htmlDomain;
>-            }
>+            newHtmlDomainList += domainArray[i];

This change looks wrong. Before I think newHtmlDomainList would have included everything in globalHtmlDomainList that wasn't in currentHtmlDomainList or currentPlaintextDomainList. I think now it will include everything in globalHtmlDomainList, currentHtmlDomainList and currentPlaintextDomainList.

>-    //Get the pref in a tempServerList and then parse it to see if it has dupes.
>-    //if so remove the dupes and then create the serverList.

This duplicate checking seems to have gone awol.

>diff -pruw comm.2a5433caac6e/mailnews/local/src/nsPop3Protocol.cpp comm/mailnews/local/src/nsPop3Protocol.cpp
>         PRUint32 dateReceived = TimeInSecondsFromPRTime(PR_Now()); // if we don't find a date str, assume now.
...
>-        if (dateReceivedStr && !dateReceivedStr->IsEmpty())
>-          dateReceived = atoi(dateReceivedStr->get());
>+        if (lineElems.Length() > 2)
>+          dateReceived = atoi(lineElems[2].get());

Does ParseString ensure that elements aren't empty? e.g. "test string  " doesn't give you "test", "string" and ""

Comment 12

[[e-mail]]

(In reply to comment #11)
>(From update of attachment 360325 [details] [diff] [review])
>>                 accountList.Append(newAccount);
>newAccount is just going to be an empty string which is wrong.
D'oh. Fixed. And added some comments.

>This change looks wrong. Before I think newHtmlDomainList would have included
>everything in globalHtmlDomainList that wasn't in currentHtmlDomainList or
>currentPlaintextDomainList. I think now it will include everything in
>globalHtmlDomainList, currentHtmlDomainList and currentPlaintextDomainList.
I'll take another look at this. I did say these were harder ;-)

>>-    //Get the pref in a tempServerList and then parse it to see if it has dupes.
>>-    //if so remove the dupes and then create the serverList.
>This duplicate checking seems to have gone awol.
Yes, it was entirely unnecessary, as GetServerByKey (for once) does it already.

>Does ParseString ensure that elements aren't empty? e.g. "test string  "
>doesn't give you "test", "string" and ""
Correct. It's equivalent to string.split(char).filter(function(s) s) in JS.

Comment 13

[[e-mail]]

(In reply to comment #11)
>(From update of attachment 360325 [details] [diff] [review])
>>+        for (; i < domainArray.Length(); i++) {
>>+          if (domainArray.IndexOf(domainArray[i]) == i) {
>>               if (!newHtmlDomainList.IsEmpty())
>>                 newHtmlDomainList += DOMAIN_DELIMITER;
>>-              newHtmlDomainList += htmlDomain;
>>-            }
>>+            newHtmlDomainList += domainArray[i];
>This change looks wrong. Before I think newHtmlDomainList would have included
>everything in globalHtmlDomainList that wasn't in currentHtmlDomainList or
>currentPlaintextDomainList. I think now it will include everything in
>globalHtmlDomainList, currentHtmlDomainList and currentPlaintextDomainList.
You overlooked the fact that i is initialised to the length of the array of the currentHtml/PlaintextDomainList, so the elements we loop over are the new ones of the globalHtmlDomainList only.

Comment 14

[[e-mail]]

Attachment: Addressed review comments (!)

I tweaked some of the comments so that the way that the code only looks at the most recently added set of strings is clearer. (This applies to the first two changed files.) Also note that this patch isn't -w!

Comment 15

[[e-mail]]

Pushed changeset a0dd6d1dfd3d to comm-central.

I also removed the now obsolete version of the ParseString function.