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

RESOLVED FIXED

Status

defect
RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: neil, Assigned: neil)

Tracking

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

Assignee

Description

11 years ago
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.
Assignee

Comment 1

11 years ago
Posted patch "Easier" conversions (obsolete) — Splinter Review
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.
Assignee: nobody → neil
Status: NEW → ASSIGNED
Attachment #359751 - Flags: superreview?(bienvenu)
Attachment #359751 - Flags: review?(bugzilla)
Assignee

Comment 2

11 years ago
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.
Attachment #359751 - Flags: review?(Pidgeot18)
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.
Attachment #359751 - Flags: review?(Pidgeot18) → review+
Assignee

Comment 4

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

Comment 5

11 years ago
Posted patch Harder conversions (-w) (obsolete) — Splinter Review
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.
Attachment #360325 - Flags: superreview?(bienvenu)
Attachment #360325 - Flags: review?(bugzilla)

Comment 6

11 years ago
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()) == '.'))
Attachment #359751 - Flags: superreview?(bienvenu) → superreview+

Updated

11 years ago
Attachment #360325 - Flags: superreview?(bienvenu) → superreview+
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?
Attachment #359751 - Flags: review?(bugzilla) → review-
Assignee

Comment 8

11 years ago
Sorry about that, I'd left that file out of the patch by mistake.
Attachment #359751 - Attachment is obsolete: true
Attachment #363484 - Flags: superreview+
Attachment #363484 - Flags: review?(bugzilla)
Attachment #363484 - Flags: review?(bugzilla) → review+
Comment on attachment 363484 [details] [diff] [review]
Including nsMsgAccountManager.cpp change

That's better. r=me.
Assignee

Comment 10

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

Pushed changeset 1f0be0d2b89f to comm-central.
Attachment #360325 - Flags: review?(bugzilla) → review-
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 ""
Assignee

Comment 12

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

Comment 13

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

Comment 14

10 years ago
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!
Attachment #360325 - Attachment is obsolete: true
Attachment #364685 - Flags: review?(bugzilla)
Attachment #364685 - Flags: review?(bugzilla) → review+
Assignee

Comment 15

10 years ago
Pushed changeset a0dd6d1dfd3d to comm-central.

I also removed the now obsolete version of the ParseString function.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.