359352 - HasCardForEmailAddress/CardForEmailAddress should be a property on nsIAbDirectory

Status: ASSIGNED

(MailNews Core :: Address Book, defect)

Description

[Scott MacGregor]

Spun out from Bug 357321.

Currently 
HasCardForEmailAddress/CardForEmailAddress are methods on nsIAbMDBCard

We should make these more generic and add them to nsIAbCard.

For the trunk, I think we can also remove HasCardForEmailAddress and just use CardForEmailAddress. Callers can test for the existence of the returned card. I didn't do this in my patch for Bug 357321 because that patch is going into 1.8.1 where we can't remove public interface methods easily.

Comment 1

[Mark Banner (:standard8)]

(In reply to comment #0)
> Currently 
> HasCardForEmailAddress/CardForEmailAddress are methods on nsIAbMDBCard

Actually, you meant nsIAbMDBDirectory, and should be on nsIAbDirectory.

Comment 2

[Ian Neal]

Attachment: Part 1 - Remove HasCardForEmailAddress - patch v0.2

I am splitting the work into multiple parts, the first being the removal of HasCardForEmailAddress.

Comment 3

[Mark Banner (:standard8)]

Comment on attachment 271555 [details] [diff] [review]
Part 1 - Remove HasCardForEmailAddress - patch v0.2

r=me via code inspection.

The only thing I'd be tempted to say is with this section:

-        PRBool cardExists = PR_FALSE;
+        nsCOMPtr<nsIAbCard> cardForAddress;
         // don't want to abort the rest of the scoring.
         if (!authorEmailAddress.IsEmpty())
         {
-          for (PRInt32 index = 0; index < whiteListDirArray.Count() && !cardExists; index++)
-            rv = whiteListDirArray[index]->HasCardForEmailAddress(authorEmailAddress.get(), &cardExists);
+          for (PRInt32 index = 0; index < whiteListDirArray.Count() && !cardForAddress; index++)
+            rv = whiteListDirArray[index]->CardForEmailAddress(authorEmailAddress.get(), getter_AddRefs(cardForAddress));
             if (NS_FAILED(rv))
-              cardExists = PR_FALSE;
+              cardForAddress = nsnull;
         }
-        if (cardExists)
+        if (cardForAddress)

Would it be faster to use nsIAbCard* cardForAddress;
...
->CardForEmailAddress(..., &cardForAddress)

etc, thus avoiding the overhead of nsCOMPtr and getter_AddRef functions in this case (we'd obviously need a NS_RELEASE in the cardForAddress existing case).

With the patch should be faster than it is at the moment anyway, so I'm not too worried. It is just a thought that came to mind.

Comment 4

[Ian Neal]

Attachment: Part 1 - Remove HasCardForEmailAddress with NS_RELEASE - patch v0.2a

Is this patch correct as far as NS_RELEASE, etc?

Comment 5

[Mark Banner (:standard8)]

Comment on attachment 271752 [details] [diff] [review]
Part 1 - Remove HasCardForEmailAddress with NS_RELEASE - patch v0.2a

r=me if you change the nsMsgSearchTerm.cpp instance to NS_IF_RELEASE as we're not guaranteed to have a valid pointer/card in that case (the nsMsgDBFolder.cpp is ok though).

Comment 6

[Mark Banner (:standard8)]

Comment on attachment 271752 [details] [diff] [review]
Part 1 - Remove HasCardForEmailAddress with NS_RELEASE - patch v0.2a

actually set the flag this time...

Comment 7

[Ian Neal]

Attachment: Part 1 - Remove HasCardForEmailAddress with NS_IF_RELEASE - patch v0.2b

Changes since v0.2a:
* NS_RELEASE -> NS_IF_RELEASE in nsMsgSearchTerm.cpp

Carrying forward r= and requesting sr=

Comment 8

[Ian Neal]

Comment on attachment 272068 [details] [diff] [review]
Part 1 - Remove HasCardForEmailAddress with NS_IF_RELEASE - patch v0.2b

Checking in (trunk)
mail/base/content/mailCommands.js;
new revision: 1.34; previous revision: 1.33
mail/base/content/msgHdrViewOverlay.js;
new revision: 1.92; previous revision: 1.91
mailnews/addrbook/public/nsIAbMDBDirectory.idl;
new revision: 1.10; previous revision: 1.9
mailnews/addrbook/src/nsAbMDBDirProperty.cpp;
new revision: 1.16; previous revision: 1.15
mailnews/addrbook/src/nsAbMDBDirectory.cpp;
new revision: 1.64; previous revision: 1.63
mailnews/addrbook/src/nsAbMDBDirectory.h;
new revision: 1.25; previous revision: 1.24
mailnews/base/resources/content/mailCommands.js;
new revision: 1.105; previous revision: 1.104
mailnews/base/search/src/nsMsgSearchTerm.cpp;
new revision: 1.144; previous revision: 1.143
mailnews/base/util/nsMsgDBFolder.cpp;
new revision: 1.319; previous revision: 1.318
done

Comment 9

[Caio Tiago Oliveira (asrail)]

Hey people...

the change on nsMsgDBFolder.cpp makes on of my accounts always segpipe just after getting the last message.

Some accounts works fine, but that one doesn't.

Comment 10

[Stephen Moehle]

Attachment: fix crash

Downloading messages from a POP3 server crashes in nsMsgDBFolder::CallFilterPlugins() because cardForAddress is uninitialized.

Comment 11

[Mark Banner (:standard8)]

Comment on attachment 272323 [details] [diff] [review]
fix crash

This looks the right fix so I'm jumping in and giving r=me, requesting sr.

Comment 12

[Caio Tiago Oliveira (asrail)]

That patch works fine here.

It crashed before because the code tried to release cardForAddress while it was a 0x01.

Comment 13

[Stephen Moehle]

Could someone please check this in?

Comment 14

[Mark Banner (:standard8)]

(In reply to comment #13)
> Could someone please check this in?
> 
Checked in:
/cvsroot/mozilla/mailnews/base/util/nsMsgDBFolder.cpp,v  <--  nsMsgDBFolder.cpp
new revision: 1.320; previous revision: 1.319

Thanks for the patch.

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Ian, is there any ongoing work for your second part here? It's a feature we need in bug 243631 to support a search in LDAP/OSX/Outlook address books.

Comment 17

[Joshua Cranmer [:jcranmer]]

These two bugs need to be linked together, but I think the bigger issue here is that we need to add implementations for non-MDB address books. Hence, bug 449618 (bumping it up) is "blocking" this bug (adding the impls).