Closed Bug 409608 Opened 17 years ago Closed 17 years ago

Tidy up nsAbAddressCollecter and add some more tests

Categories

(MailNews Core :: Address Book, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: standard8, Assigned: standard8)

References

Details

(Keywords: memory-footprint, perf)

Attachments

(1 file, 2 obsolete files)

Looking at the address collector there are various functions/temporary variables that can be optimised to reduce footprint and help perf (not that the collector is very noticeable in its performance).

At the same time, I'd like to complete the unit tests for the main CollectAddress function.

Marking dependent on bug 409607 as I'd like to get that one done first as that'll clear a few things up.
Note to self/reviewers for later:

This section of code in the address collector is basically useless:

  if (m_database) {
    m_database->Commit(nsAddrDBCommitType::kSessionCommit);
    m_database->Close(PR_FALSE);
    m_database = nsnull;
  }

Reasons:

1) there is a cache of nsAddrDatabase (which m_database is).
2) Add and Modify Cards both do a commit after the change.
3) Close(PR_FALSE) actually ends up doing nothing.
4) nsnull just drops our reference to it which we'll do in either case this is used as one of them the address collector is going away, and the other one it is going to be replaced soon after anyway.
Attached patch The fix (obsolete) — Splinter Review
This tidies up the address collecter code to something that should be a bit more efficient and reasonable.

The only functional change is the addition of a check to ensure that the curAddress (current email address) is not empty before attempting to add to the address book. Although this is unlikely in the default profile situation, it would fix bug 65635 (I think that is SM only now), and I think its a good check to have in there anyway.

See comment #1 for why I've removed the database commit and close functions.

Most of the rest of the changes are based around removing nsresult return values where they aren't necessary, optimising parameter types and optimising the functions in general.

Also included are updates to the unit test to add testing of addition and modification of names and screen names on cards.
Attachment #294652 - Flags: superreview?(neil)
Attachment #294652 - Flags: review?(neil)
Comment on attachment 294652 [details] [diff] [review]
The fix

>+    if (curAddress.IsEmpty())
I'm wondering whether it's worth testing this using the pointer before creating the dependent string (which wastes a strlen in this case).

>+      continue;
As written, this is incorrect, as you won't advance to the next address.
One fix is to advance curName and curAddress (or curXXXPtr; I'm not sure it's worth renaming them yet) inside the for (;;).

>+      nsString displayName;
>+      // If we already have a display name, don't set the names on the card.
>+      if (NS_SUCCEEDED(existingCard->GetDisplayName(displayName)) &&
>+          displayName.IsEmpty() &&
>+          !unquotedName.IsEmpty())
Do you need to check the result of GetDisplayName?

>+  nsCString domain(Substring(aEmail, atPos + 1));
const nsACString& perhaps?

>+// Observers the collected address book pref in case it changes.
Typo: Observes

>-  NS_ASSERTION(pPrefBranchInt, "failed to get prefs");
>+  if (!pPrefBranchInt) {
>+    NS_ASSERTION(pPrefBranchInt, "failed to get prefs");
>+    return NS_OK;
>+  }

>+nsresult nsAbAddressCollecter::SetAbURI(const nsCString &aURI)
> {
>+  nsCString newURI(aURI.IsEmpty() ? nsDependentCString(kPersonalAddressbookUri) : aURI);
Rather than this I would make aURI a non-const string and simply assign kPersonalAddressbookUri to it if it's empty.

>-    m_database = nsnull;
I think it would be wise to leave this in just in case.

>+    if (collectChecker.AB instanceof Components.interfaces.nsIAbDirectory)
>+      do_check_false(collectChecker.AB.childCards.hasMoreElements(), false);
>+    else
>+      throw "collectChecker.AB is not an instance of the expected nsIAbDirectory";
You could simply write
collectChecker.AB.QueryInterface(Components.interfaces.nsIAbDirectory).childCards.hasMoreElements()
and let that throw instead.
Attachment #294652 - Flags: superreview?(neil)
Attachment #294652 - Flags: review?(neil)
Attachment #294652 - Flags: review-
(In reply to comment #3)
> (From update of attachment 294652 [details] [diff] [review])
> >+      continue;
> As written, this is incorrect, as you won't advance to the next address.
> One fix is to advance curName and curAddress (or curXXXPtr; I'm not sure it's
> worth renaming them yet) inside the for (;;).

I wanted to rename them just to make sure it was clear what they actually are when developing, especially as we're creating a nsDependentCString of one of them.
Attached patch The fix v2 (obsolete) — Splinter Review
This version fixes the nits. I've also extended the unit test to collect addition of more than address at a time (by adding all the possibilities at the same time).
Attachment #294652 - Attachment is obsolete: true
Attachment #294836 - Flags: superreview?(neil)
Attachment #294836 - Flags: review?(neil)
Comment on attachment 294836 [details] [diff] [review]
The fix v2

>+  for (PRUint32 i = 0; i < numAddresses; i++, curNamePtr += strlen(curNamePtr) + 1,
>+                                         curAddressPtr += strlen(curAddressPtr) + 1)
>   {
>+    // Don't allow collection of addresses with no email address, it makes
>+    // no sense. Whilst we should never get here in most normal cases, we
>+    // should still be careful.
>+    if (strlen(curAddressPtr) == 0)
>+      continue;
Sadly this isn't what I meant - too many strlens. However I've thought up a new solution to improve on this code and fix a bug that I've just noticed!
for (PRUint32 i = 0; i < numAdresses; i++)
{
  nsDependentCString curAddress(curAddressPtr);
  curAddressPtr += curAddress.Length() + 1;
  nsCString unquotedName;
  rv = pHeader->UnquotePhraseOrAddr(curNamePtr, PR_FALSE,
                                    getter_Copies(unquotedName));
  curNamePtr += strlen(curNamePtr) + 1;
  NS_ASSERTION(NS_SUCCEEDED(rv), "failed to unquote name");
  if (NS_FAILED(rv))
    continue;
  if (curAddress.IsEmpty())
    continue;
Attachment #294836 - Flags: superreview?(neil)
Attachment #294836 - Flags: superreview-
Attachment #294836 - Flags: review?(neil)
Attached patch The fix v3Splinter Review
Reduces strlens as suggested.
Attachment #294836 - Attachment is obsolete: true
Attachment #294951 - Flags: superreview?(neil)
Attachment #294951 - Flags: review?(neil)
Comment on attachment 294951 [details] [diff] [review]
The fix v3

Actually I think v2 fixed the other bug too, it was only unpatched and v1 that had the bug.
Attachment #294951 - Flags: superreview?(neil)
Attachment #294951 - Flags: superreview+
Attachment #294951 - Flags: review?(neil)
Attachment #294951 - Flags: review+
Patch checked in -> fixed.
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Flags: in-testsuite+
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: