474759 - Trunk builds broken due to ParseString change

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Mark Banner (:standard8)]

Bug 466622 move ParseString from nsCStringArray to be a standalone function. This has broken various places in mailnews when building with mozilla-central (as opposed to comm-central):

/buildbot/comm-central-trunk-bloat-linux/build/mailnews/base/src/nsMsgAccountManager.cpp: In member function ‘virtual nsresult nsMsgAccountManager::LoadAccounts()’:
/buildbot/comm-central-trunk-bloat-linux/build/mailnews/base/src/nsMsgAccountManager.cpp:1266: error: ‘class nsCStringArray’ has no member named ‘ParseString’
/buildbot/comm-central-trunk-bloat-linux/build/mailnews/base/src/nsMsgAccountManager.cpp: In member function ‘nsresult nsMsgAccountManager::AddVFListenersForVF(nsIMsgFolder*, const char*, nsIRDFService*, nsIMsgDBService*)’:
/buildbot/comm-central-trunk-bloat-linux/build/mailnews/base/src/nsMsgAccountManager.cpp:2924: error: ‘class nsCStringArray’ has no member named ‘ParseString’

I've not investigated a fix yet, but this may involve the use of MOZILLA_1_9_1_BRANCH in various places.

Comment 1

[Dan Mosedale (:dmosedale, :dmose)]

I think this is actually a blocker, because it completely blocks noticing any other regressions on the trunk.  Certainly, we can get away without fixing it for a little bit, but if that length time creeps into weeks, we've got a problem.

Comment 2

[[e-mail]]

Attachment: WIP

I thought I'd provide 5 methods:
1. (const char*, const char*, nsCStringArray&) for compatibility
2. (const char*, char, nsCStringArray&)      }
3. (const char*, char, nsTArray<nsCString>&) } to aid migration
4. (const nsCString&, char, nsCStringArray&) }
5. (const nsCString&, char, nsTArray<nsCString>&) for 1.9.1
Thoughts?

Comment 3

[Mark Banner (:standard8)]

(In reply to comment #2)
> I thought I'd provide 5 methods:
> 1. (const char*, const char*, nsCStringArray&) for compatibility
> 2. (const char*, char, nsCStringArray&)      }
> 3. (const char*, char, nsTArray<nsCString>&) } to aid migration
> 4. (const nsCString&, char, nsCStringArray&) }
> 5. (const nsCString&, char, nsTArray<nsCString>&) for 1.9.1
> Thoughts?

I think this sounds good, possibly overdoing it with 2 & 3, but at least we'll have a good way to migrate through and it will get us going again.

Comment 4

[[e-mail]]

Attachment: Fixed WIP

(In reply to comment #2)
> I thought I'd provide 5 methods:
> 1. (const char*, const char*, nsCStringArray&) [still used 6 times]
> 2. (const char*, char, nsCStringArray&) [converted 7 cases]
> 3. (const char*, char, nsTArray<nsCString>&) not used
> 4. (const nsCString&, char, nsCStringArray&) [converted 13 cases]
> 5. (const nsCString&, char, nsTArray<nsCString>&) not used yet of course

Comment 5

[[e-mail]]

Attachment: Includes Enumerator

I also decided to convert as many consumers over to nsACString as I could i.e. those for which the second parameter could easily be converted to a character.

Comment 6

[Phil Ringnalda (:philor)]

The http://hg.mozilla.org/comm-central/rev/176d86062c81 checkin for this broke Remove All Tags - removing one by one by toggling the individual menu items still works, but context menu or Message menu - Tag - Remove All Tags is now "Remove One Tag if there's two or more, or do nothing if there's just one."

Comment 7

[Phil Ringnalda (:philor)]

Oh, apparently I meant that it's now "Remove the Important tag if that's either the only one or one of several, but don't remove any others."

Comment 8

[[e-mail]]

Attachment: Follow-up patch :-(

I got my APIs mixed up ... Substring(char*, char*) are start and end, but Substring(char*, uint, uint) are start and length...

It's safe to pass in (-1 - start) as the length, as it's cast to unsigned and then reduced to the actual length, resulting in the tail of the string.

Comment 9

[Mark Banner (:standard8)]

Comment on attachment 358811 [details] [diff] [review]
Follow-up patch :-(

I'm not convinced this is fully right still. I'm getting multiple (1 per tag):

###!!! ASSERTION: space only keyword: 'keywords.IsEmpty() || keywords.CharAt(0) != ' '', file /Users/moztest/comm/main/src/mailnews/base/util/nsMsgDBFolder.cpp, line 5348

when I remove all tags.

Comment 10

[David :Bienvenu]

Comment on attachment 358297 [details] [diff] [review]
Includes Enumerator

retroactive sr. A few comments:

-nsresult nsMsgSearchTerm::MatchKeyword(const char *keywordList, PRBool *pResult)
+nsresult nsMsgSearchTerm::MatchKeyword(const nsACString& keywordList, PRBool *pResult)

while you're here, this should be NS_IMETHODIMP, I think.

A couple comments in the ParseString methods to the effect that on failure, we clearing out the partial results would be helpful...I also didn't realize that ParseString would add to an existing array of strings, instead of clearing it out - I presume the old method work that way as well. Might be worth commenting in nsMsgUtils.h.

Comment 11

[David :Bienvenu]

Comment on attachment 358811 [details] [diff] [review]
Follow-up patch :-(

I'll try this out once I've rebuilt the rest of my tree.

Comment 12

[[e-mail]]

(In reply to comment #10)
> (From update of attachment 358297 [details] [diff] [review])
> -nsresult nsMsgSearchTerm::MatchKeyword(const char *keywordList, PRBool
> *pResult)
> +nsresult nsMsgSearchTerm::MatchKeyword(const nsACString& keywordList, PRBool
> *pResult)
> while you're here, this should be NS_IMETHODIMP, I think.
That makes most of that file wrong :-(

> A couple comments in the ParseString methods to the effect that on failure, we
> clearing out the partial results would be helpful...I also didn't realize that
> ParseString would add to an existing array of strings, instead of clearing it
> out - I presume the old method work that way as well. Might be worth commenting
> in nsMsgUtils.h.
OK, I'll check in a comment fix along with attachment 358811 [details] [diff] [review] (or whichever).

Comment 13

[David :Bienvenu]

Comment on attachment 358811 [details] [diff] [review]
Follow-up patch :-(

I'm not seeing assertions w/ my simple tests. But we should figure out what Standard8 is seeing, if you haven't already.

Comment 14

[[e-mail]]

Pushed changeset b8383e5fa1e4 to comm-central.