Open Bug 359352 Opened 18 years ago Updated 2 years ago

HasCardForEmailAddress/CardForEmailAddress should be a property on nsIAbDirectory

Categories

(MailNews Core :: Address Book, defect)

defect

Tracking

(Not tracked)

ASSIGNED

People

(Reporter: mscott, Assigned: iannbugzilla)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 2 obsolete files)

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.
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
Blocks: 363993
Blocks: 180012
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+
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+
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+
Attachment #272068 - Flags: superreview?(bienvenu) → superreview+
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.
Attached patch fix crash (deleted) — Splinter Review
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+
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.
Could someone please check this in?
(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
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
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: