Trunk builds broken due to ParseString change

RESOLVED FIXED in Thunderbird 3.0b2

Status

defect
--
blocker
RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: standard8, Assigned: neil)

Tracking

({regression})

Trunk
Thunderbird 3.0b2
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

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.
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.
Severity: major → blocker
Posted patch WIP (obsolete) — Splinter Review
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?
Assignee: nobody → neil
Status: NEW → ASSIGNED
Attachment #358186 - Flags: review?(bugzilla)
(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.
Posted patch Fixed WIP (obsolete) — Splinter Review
(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
Attachment #358186 - Attachment is obsolete: true
Attachment #358205 - Flags: review?(bugzilla)
Attachment #358186 - Flags: review?(bugzilla)
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.
Attachment #358205 - Attachment is obsolete: true
Attachment #358297 - Flags: superreview?(bienvenu)
Attachment #358297 - Flags: review?(bugzilla)
Attachment #358205 - Flags: review?(bugzilla)
Attachment #358297 - Flags: review?(bugzilla) → review+
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."
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."
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.
Attachment #358811 - Flags: superreview?(bienvenu)
Attachment #358811 - Flags: review?(bugzilla)
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.
Attachment #358811 - Flags: review?(bugzilla)
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.
Attachment #358297 - Flags: superreview?(bienvenu) → superreview+
Comment on attachment 358811 [details] [diff] [review]
Follow-up patch :-(

I'll try this out once I've rebuilt the rest of my tree.
(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).
Keywords: regression
Attachment #358811 - Flags: superreview?(bienvenu) → superreview+
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.
Attachment #358811 - Flags: review?(bugzilla)
Attachment #358811 - Flags: review?(bugzilla) → review+
Pushed changeset b8383e5fa1e4 to comm-central.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Blocks: 476150
Target Milestone: --- → Thunderbird 3.0b2
You need to log in before you can comment on or make changes to this bug.