HasCardForEmailAddress/CardForEmailAddress should be a property on nsIAbDirectory

ASSIGNED
Assigned to

Status

MailNews Core
Address Book
ASSIGNED
12 years ago
8 years ago

People

(Reporter: Scott MacGregor, Assigned: Ian Neal)

Tracking

(Blocks: 1 bug)

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

12 years ago
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.
(Reporter)

Updated

12 years ago
Summary: HasCardForEmailAddress/CardForEmailAddress should be properties on nsIAbCard → HasCardForEmailAddress/CardForEmailAddress should be a property on nsIAbCard
(In reply to comment #0)
> Currently 
> HasCardForEmailAddress/CardForEmailAddress are methods on nsIAbMDBCard

Actually, you meant nsIAbMDBDirectory, and should be on nsIAbDirectory.
Keywords: helpwanted
OS: Windows XP → All
Hardware: PC → All
Summary: HasCardForEmailAddress/CardForEmailAddress should be a property on nsIAbCard → HasCardForEmailAddress/CardForEmailAddress should be a property on nsIAbDirectory
(Reporter)

Updated

11 years ago
Blocks: 363993
(Assignee)

Updated

11 years ago
Blocks: 180012
(Assignee)

Comment 2

11 years ago
Created attachment 271555 [details] [diff] [review]
Part 1 - Remove HasCardForEmailAddress - patch v0.2

I am splitting the work into multiple parts, the first being the removal of HasCardForEmailAddress.
Assignee: nobody → iann_bugzilla
Status: NEW → ASSIGNED
Attachment #271555 - Flags: review?(bugzilla)
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.
Attachment #271555 - Flags: review?(bugzilla) → review+
(Assignee)

Comment 4

11 years ago
Created attachment 271752 [details] [diff] [review]
Part 1 - Remove HasCardForEmailAddress with NS_RELEASE - patch v0.2a

Is this patch correct as far as NS_RELEASE, etc?
Attachment #271752 - Flags: review?(bugzilla)
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 on attachment 271752 [details] [diff] [review]
Part 1 - Remove HasCardForEmailAddress with NS_RELEASE - patch v0.2a

actually set the flag this time...
Attachment #271752 - Flags: review?(bugzilla) → review+
(Assignee)

Comment 7

11 years ago
Created attachment 272068 [details] [diff] [review]
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=
Attachment #271555 - Attachment is obsolete: true
Attachment #271752 - Attachment is obsolete: true
Attachment #272068 - Flags: superreview?(bienvenu)
Attachment #272068 - Flags: review+

Updated

11 years ago
Attachment #272068 - Flags: superreview?(bienvenu) → superreview+
(Assignee)

Comment 8

11 years ago
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
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

11 years ago
Created attachment 272323 [details] [diff] [review]
fix crash

Downloading messages from a POP3 server crashes in nsMsgDBFolder::CallFilterPlugins() because cardForAddress is uninitialized.
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.
Attachment #272323 - Flags: superreview?(bienvenu)
Attachment #272323 - Flags: review+

Updated

11 years ago
Attachment #272323 - Flags: superreview?(bienvenu) → superreview+
That patch works fine here.

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

Comment 13

11 years ago
Could someone please check this in?
Keywords: helpwanted → checkin-needed
(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.
Keywords: checkin-needed
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.
Blocks: 243631
Duplicate of this bug: 180012
Product: Core → MailNews Core
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).
Depends on: 449618
You need to log in before you can comment on or make changes to this bug.