Closed Bug 413230 Opened 17 years ago Closed 16 years ago

Improve implementation of nsMsgCompose::CheckAndPopulateRecipients

Categories

(MailNews Core :: Backend, defect, P3)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3.0a3

People

(Reporter: standard8, Assigned: standard8)

References

Details

(Keywords: memory-footprint, perf)

Attachments

(3 files, 2 obsolete files)

Attached patch Fix Part 1 (diff -w) (obsolete) (deleted) — Splinter Review
The current implementation of nsMsgCompose::CheckAndPopulateRecipients basically relies on nsISupportsArrays where we've specifically created structures derived from nsISupports, where a struct and an nsTArray would do. There are some obvious gains on footprint to be made, and it will also improve perf, though that will only be really visible if you are sending emails to lots of recipients. As it is big and complicated (currently), I want to tackle it in several stages. The first one is the only defined one at this stage though, I'll work out the others as I go along. So, the Part 1 patch does the following: - Changes the interface in nsIMsgCompose to use a AString rather than a wstring, changes the capitalisation of "checkAndPop..." to match what most interfaces use, changes the argument names to start with "a", and updates the documentation. - Drops the existing "class nsMsgRecipient : public nsISupports" structure, and replaces it with "struct nsMsgRecipient". - nsMsgCompFields::SplitRecipientsEx is then updated to use one nsTArray<nsMsgRecipient> rather than two nsIMsgRecipientArray. - This then simplifies greatly some of the loops. - Most of the other changes are then just tidy ups. I've also included a unit test for the function, that tests various options and lookups from the address books.
Attachment #298124 - Flags: review?(neil)
Attached patch Fix Part 1 (normal patch) (obsolete) (deleted) — Splinter Review
Comment on attachment 298124 [details] [diff] [review] Fix Part 1 (diff -w) >+ nsMsgRecipient(const nsString &aAddress, const nsString &aEmail) : >+ mAddress(aAddress), mEmail(aEmail), >+ mPreferFormat(nsIAbPreferMailFormat::unknown), mProcessed(PR_FALSE) {} Instead of using this constructor I think it would be better to create the nsMsgRecipient in advance and pass its members in place. For example: rv = ConvertToUnicode("UTF-8", pAddresses, msgRecipient.mAddress); >+ virtual ~nsMsgRecipient() {} This doesn't need to be virtual. (It didn't before, either...)
Attachment #298124 - Flags: review?(neil) → review+
Addressed Neil's comments, carrying forward his r, requesting sr.
Attachment #298124 - Attachment is obsolete: true
Attachment #298125 - Attachment is obsolete: true
Attachment #298990 - Flags: superreview?(mscott)
Attachment #298990 - Flags: review+
Attached patch The fix v2 (normal patch) (deleted) — Splinter Review
Comment on attachment 298990 [details] [diff] [review] [checked in] The fix v2 (diff -w) nice work Mark.
Attachment #298990 - Flags: superreview?(mscott) → superreview+
Attachment #298990 - Attachment description: The fix v2 (diff -w) → [checked in] The fix v2 (diff -w)
This patch: - drops nsISupportsArray from GetABDirectories, replacing with an nsCOMArray<nsIAbDirectory> - parses the RDF Service into GetABDirectories (rather than re-creating it each time). - drops the searchSubDirectory parameter (it was always true) As before, still much more to tidy up, this is more an intermediate patch whilst I'm looking at improving some other areas.
Attachment #299561 - Flags: superreview?(neil)
Attachment #299561 - Flags: review?(neil)
Comment on attachment 299561 [details] [diff] [review] [checked in] Part 2 - Drop nsISupportsArray from nsMsgCompose::GetABDirectories >- nsCOMPtr<nsISupports> item; >- addrbookDirArray->GetElementAt(k, getter_AddRefs(item)); >+ nsCOMPtr<nsIAbDirectory> item(addrbookDirArray[k]); > > // Avoid recursive mailing lists > if (abDirectory && (item == abDirectory)) > { > stillNeedToSearch = PR_FALSE; > break; > } > >- abDirectory = do_QueryInterface(item, &rv); >- if (NS_FAILED(rv)) >- return rv; >+ abDirectory.swap(item); So, I looked at the bug where you added this and failed to understand it :-( But I think I'd prefer if you removed the item temporary and just used addrbookDirArray[k] in the test instead (and abDirectory = of course).
Attachment #299561 - Flags: superreview?(neil)
Attachment #299561 - Flags: superreview+
Attachment #299561 - Flags: review?(neil)
Attachment #299561 - Flags: review+
Comment on attachment 299561 [details] [diff] [review] [checked in] Part 2 - Drop nsISupportsArray from nsMsgCompose::GetABDirectories Checked in with comment addressed.
Attachment #299561 - Attachment description: Part 2 - Drop nsISupportsArray from nsMsgCompose::GetABDirectories → [checked in] Part 2 - Drop nsISupportsArray from nsMsgCompose::GetABDirectories
Depends on: 414432
Priority: -- → P3
Product: Core → MailNews Core
There were possibly some other tidy ups that I was thinking of doing (e.g. remove nsMsgMailList and replace by something sane), however given the age of this bug, I think we'll resolve it as fixed and deal with those in separate bugs once we get round to them.
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.0a3
Depends on: 542947
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: