413230 - Improve implementation of nsMsgCompose::CheckAndPopulateRecipients

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect, P3)

Description

[Mark Banner (:standard8)]

Attachment: Fix Part 1 (diff -w)

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.

Comment 1

[Mark Banner (:standard8)]

Attachment: Fix Part 1 (normal patch)

Comment 2

[[e-mail]]

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...)

Comment 3

[Mark Banner (:standard8)]

Attachment: [checked in] The fix v2 (diff -w)

Addressed Neil's comments, carrying forward his r, requesting sr.

Comment 4

[Mark Banner (:standard8)]

Attachment: The fix v2 (normal patch)

Comment 5

[Scott MacGregor]

Comment on attachment 298990 [details] [diff] [review]
[checked in] The fix v2 (diff -w)

nice work Mark.

Comment 6

[Mark Banner (:standard8)]

Attachment: [checked in] Part 2 - Drop nsISupportsArray from nsMsgCompose::GetABDirectories

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.

Comment 7

[[e-mail]]

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).

Comment 8

[Mark Banner (:standard8)]

Comment on attachment 299561 [details] [diff] [review]
[checked in] Part 2 - Drop nsISupportsArray from nsMsgCompose::GetABDirectories

Checked in with comment addressed.

Comment 9

[Mark Banner (:standard8)]

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.