The default bug view has changed. See this FAQ.

Move cardForEmail to nsIAbDirectory

RESOLVED FIXED in mozilla1.9.1a2

Status

MailNews Core
Address Book
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: jcranmer, Assigned: jcranmer)

Tracking

(Blocks: 3 bugs)

Trunk
mozilla1.9.1a2
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

9 years ago
This bug is to refactor cardForEmail upwards.

http://hg.mozilla.org/users/Pidgeot18_gmail.com/ab_rewrite/index.cgi/rev/d6d492617b7b has my work here so far.
(Assignee)

Comment 1

9 years ago
Created attachment 332755 [details] [diff] [review]
Equivalent patch to repo changes

Meant to do this when I created the bug.
(Assignee)

Comment 2

9 years ago
Comment on attachment 332755 [details] [diff] [review]
Equivalent patch to repo changes

I can guarantee that things work for the following uses of cardForEmailAddress:

Allow remote content checking
Searching "From in <address book>"

This one appears to work, but I'm not fully certain the codepath is being used:
Spam whitelisting

I'm not sure the code path invoking the message header view overlay is invoked at all--or at least detectable from a UI standpoint.

Therefore, requesting review...
Attachment #332755 - Flags: superreview?(dmose)
Attachment #332755 - Flags: review?(bugzilla)
(Assignee)

Comment 3

9 years ago
Gotta remember to add my sr onto the CC list...
Version: unspecified → Trunk
(Assignee)

Updated

9 years ago
Blocks: 359352

Comment 4

9 years ago
Comment on attachment 332755 [details] [diff] [review]
Equivalent patch to repo changes

>--- a/mail/base/content/mailWindowOverlay.js
>+++ b/mail/base/content/mailWindowOverlay.js
>@@ -2552,23 +2552,25 @@
>-    addrbook = enumerator.getNext();
>+    addrbook = enumerator.getNext()
Ooops!
Comment on attachment 332755 [details] [diff] [review]
Equivalent patch to repo changes

>diff --git a/mail/base/content/mailWindowOverlay.js b/mail/base/content/mailWindowOverlay.js
>-    addrbook = enumerator.getNext();
>-    if (addrbook instanceof Components.interfaces.nsIAbMDBDirectory)
>+    addrbook = enumerator.getNext()
>+    addrbook.QueryInterface(Components.interfaces.nsIAbDirectory);
>+    try {
>       cardForEmailAddress = addrbook.cardForEmailAddress(authorEmailAddress);
>+    } catch (e) {}
>   }

So although I said to return NS_ERROR_NOT_IMPLEMENTED, I think we could probably just null the result and return NS_OK. Then we won't have to have try/catch statements like this which may cause us to miss other errors.

I'd like to see what Dan thinks of that idea.

Please also fix the missing ; though actually, could you make the first two lines into:

addrbooks = enumerator.getNext()
                      .QueryInterface(Components.interfaces.nsIAbDirectory);

>diff --git a/mail/base/content/msgHdrViewOverlay.js b/mail/base/content/msgHdrViewOverlay.js
>--- a/mail/base/content/msgHdrViewOverlay.js
>+++ b/mail/base/content/msgHdrViewOverlay.js
>@@ -977,17 +977,17 @@
> 
>   if (!gPersonalAddressBookDirectory)
>   {
>     var dirs = Components.classes["@mozilla.org/abmanager;1"]
>                          .getService(Components.interfaces.nsIAbManager).directories;
>     while (dirs.hasMoreElements) {
>       var dir = dirs.getNext().QueryInterface(Components.interfaces.nsIAbDirectory);
>       if (dir.URI == kPersonalAddressbookUri) {
>-        gPersonalAddressBookDirectory = dir.QueryInterface(Components.interfaces.nsIAbMDBDirectory);
>+        gPersonalAddressBookDirectory = dir;
>         break;
>       }
>     }

Firstly, please file a bug on this function only looking at the personal address book. I don't see any reason it should do.

Secondly (for now), we can just use .getDirectory(kPersonalAddressbookUri) and forget about the looping etc.

>diff --git a/mailnews/addrbook/public/nsIAbDirectory.idl b/mailnews/addrbook/public/nsIAbDirectory.idl
>+
>+  /** 
>+   * Returns the address card for the specified email address if found.
>+   *
>+   * @param  emailAddress The email address to find in either the primary or
>+   *                      secondary email address fields. If email address is
>+   *                      empty, the database won't be searched and the function
>+   *                      will return as if no card was found.
>+   * @return              An nsIAbCard if one was found, else returns NULL.
>+   */
>+  nsIAbCard cardForEmailAddress(in AUTF8String emailAddress);

If we keep the exception (NS_ERROR_NOT_IMPLEMENTED) I think you should mention it hear. If we don't keep the exception, then we need to mention that address book types where it is not implemented should/will return NULL and not throw an exception.

>diff --git a/mailnews/addrbook/src/nsAbDirProperty.cpp b/mailnews/addrbook/src/nsAbDirProperty.cpp
>+NS_IMETHODIMP nsAbDirProperty::CardForEmailAddress(const nsACString &aEmailAddress,
>+                                                   nsIAbCard ** aAbCard)
>+{ return NS_ERROR_NOT_IMPLEMENTED; }
>+

See notes above.

>diff --git a/mailnews/addrbook/test/unit/test_collection.js b/mailnews/addrbook/test/unit/test_collection.js
>--- a/mailnews/addrbook/test/unit/test_collection.js
>+++ b/mailnews/addrbook/test/unit/test_collection.js
>@@ -259,18 +259,17 @@
> 
>   // XXX Getting all directories ensures we create all ABs because the
>   // address collecter can't currently create ABs itself (bug 314448).
>   var temp = abManager.directories;
> 
>   // Get the actual AB for the collector so we can check cards have been
>   // added.
>   collectChecker.AB =
>-    abManager.getDirectory(prefService.getCharPref("mail.collect_addressbook"))
>-             .QueryInterface(Components.interfaces.nsIAbMDBDirectory);
>+    abManager.getDirectory(prefService.getCharPref("mail.collect_addressbook"));
> 

There's a QI to nsIAbDirectory further down (using the CAB variable). I think you should be able to drop that QI and the CAB variable.

>@@ -354,17 +358,17 @@
>     return;
> 
>   var tmpMsgHdr = messenger.messageServiceFromURI(tmpMsgURI).messageURIToMsgHdr(tmpMsgURI);
>   var spamSettings = tmpMsgHdr.folder.server.spamSettings;
> 
>   // if enabled in the spam settings, retrieve whitelist addressbook
>   var whiteListDirectory = null;
>   if (spamSettings.useWhiteList && spamSettings.whiteListAbURI)
>-    whiteListDirectory = RDF.GetResource(spamSettings.whiteListAbURI).QueryInterface(Components.interfaces.nsIAbMDBDirectory);
>+    whiteListDirectory = RDF.GetResource(spamSettings.whiteListAbURI).QueryInterface(Components.interfaces.nsIAbDirectory);

If its easy, can you switch this to nsIAbManager? If not, file a bug on it and we'll do it later.

>diff --git a/mailnews/base/search/public/nsMsgSearchTerm.h b/mailnews/base/search/public/nsMsgSearchTerm.h
> class nsMsgSearchTerm : public nsIMsgSearchTerm

>-    nsCOMPtr <nsIAbMDBDirectory> mDirectory;
>+    nsCOMPtr <nsIAbDirectory> mDirectory;

Yay! This will partially fix bug 437908. Except that if I try it out (forgetting that OS X isn't implemented yet, it crashes.

nsMsgSearchTerm::MatchInAddressBook isn't checking the rv:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/mailnews/base/search/src/nsMsgSearchTerm.cpp&rev=1.164&mark=935-937#923

If we change to returning NS_OK, and null, then we can probably just drop assignment to rv. If we don't, then we definitely need to check rv there.

Whichever we do, please check the return/result checking of all the cardForEmailAddress calls.

>diff --git a/mailnews/base/util/nsMsgDBFolder.cpp b/mailnews/base/util/nsMsgDBFolder.cpp
>--- a/mailnews/base/util/nsMsgDBFolder.cpp
>+++ b/mailnews/base/util/nsMsgDBFolder.cpp
>-        nsCOMPtr<nsIAbMDBDirectory> whiteListDirectory = do_QueryInterface(resource, &rv);
>+        nsCOMPtr<nsIAbDirectory> whiteListDirectory = do_QueryInterface(resource, &rv);

This looks like it could do with being on two lines.
Attachment #332755 - Flags: review?(bugzilla) → review-
(Assignee)

Updated

9 years ago
Blocks: 437908
Blocks: 325046
(Assignee)

Comment 6

9 years ago
Created attachment 333336 [details] [diff] [review]
Patch, version 2

This patch is updated per Standard8's comments. I've removed the RDF service in lieu of the address book manager in two places, and have taken the liberty of removing related includes where possible. Per discussion in IRC, dmose argued that returning NOT_IMPLENTED on error is better than returning null, so I have changed it back to do that.
Attachment #332755 - Attachment is obsolete: true
Attachment #333336 - Flags: superreview?(dmose)
Attachment #333336 - Flags: review?(bugzilla)
Attachment #332755 - Flags: superreview?(dmose)
(In reply to comment #5)
> >diff --git a/mail/base/content/msgHdrViewOverlay.js b/mail/base/content/msgHdrViewOverlay.js
> >--- a/mail/base/content/msgHdrViewOverlay.js
> >+++ b/mail/base/content/msgHdrViewOverlay.js
> >@@ -977,17 +977,17 @@
> > 
> >   if (!gPersonalAddressBookDirectory)
> >   {
> >     var dirs = Components.classes["@mozilla.org/abmanager;1"]
> >                          .getService(Components.interfaces.nsIAbManager).directories;
> >     while (dirs.hasMoreElements) {
> >       var dir = dirs.getNext().QueryInterface(Components.interfaces.nsIAbDirectory);
> >       if (dir.URI == kPersonalAddressbookUri) {
> >-        gPersonalAddressBookDirectory = dir.QueryInterface(Components.interfaces.nsIAbMDBDirectory);
> >+        gPersonalAddressBookDirectory = dir;
> >         break;
> >       }
> >     }
> 
> Secondly (for now), we can just use .getDirectory(kPersonalAddressbookUri) and
> forget about the looping etc.

You missed this addressing this comment.
 
> >diff --git a/mailnews/addrbook/public/nsIAbDirectory.idl b/mailnews/addrbook/public/nsIAbDirectory.idl
> >+
> >+  /** 
> >+   * Returns the address card for the specified email address if found.
> >+   *
> >+   * @param  emailAddress The email address to find in either the primary or
> >+   *                      secondary email address fields. If email address is
> >+   *                      empty, the database won't be searched and the function
> >+   *                      will return as if no card was found.
> >+   * @return              An nsIAbCard if one was found, else returns NULL.
> >+   */
> >+  nsIAbCard cardForEmailAddress(in AUTF8String emailAddress);
> 
> If we keep the exception (NS_ERROR_NOT_IMPLEMENTED) I think you should mention
> it hear. If we don't keep the exception, then we need to mention that address
> book types where it is not implemented should/will return NULL and not throw an
> exception.

ditto.

   if (mDirectory)
   {
     nsIAbCard* cardForAddress;
     rv = mDirectory->CardForEmailAddress(nsDependentCString(aAddress),
                                          &cardForAddress);
+    if (NS_FAILED(rv) && rv != NS_ERROR_NOT_IMPLEMENTED)
+      return rv;
     if ((m_operator == nsMsgSearchOp::IsInAB && cardForAddress) || (m_operator == nsMsgSearchOp::IsntInAB && !cardForAddress))
       *pResult = PR_TRUE;
     NS_IF_RELEASE(cardForAddress);

You need to initialise cardForAddress to nsnull because a NS_ERROR_NOT_IMPLEMENTED case won't do it for us.
Attachment #333336 - Flags: review?(bugzilla) → review-
(Assignee)

Comment 8

9 years ago
Created attachment 333846 [details] [diff] [review]
Patch, version 3

Sorry for the delay, but I've had some craziness over the past three days preventing me from working on this.
Attachment #333336 - Attachment is obsolete: true
Attachment #333846 - Flags: review?(bugzilla)
Attachment #333336 - Flags: superreview?(dmose)
Blocks: 450724
Comment on attachment 333846 [details] [diff] [review]
Patch, version 3

-  nsCOMPtr<nsIAbDirectory> mCachedTopLevelAb;

Sigh. I didn't notice this before. This will regress bug 379828, however I'll try and fix it in bug 403256 before or soon after this bug goes in.

r=me.
Attachment #333846 - Flags: review?(bugzilla) → review+
(Assignee)

Updated

9 years ago
Attachment #333846 - Flags: superreview?(dmose)
Blocks: 381555
Comment on attachment 333846 [details] [diff] [review]
Patch, version 3

>diff --git a/mail/base/content/mailWindowOverlay.js b/mail/base/content/mailWindowOverlay.js
>--- a/mail/base/content/mailWindowOverlay.js
>+++ b/mail/base/content/mailWindowOverlay.js
>@@ -2552,23 +2552,25 @@
>   // search through all of our local address books looking for a match.
>   var enumerator = Components.classes["@mozilla.org/abmanager;1"]
>                              .getService(Components.interfaces.nsIAbManager)
>                              .directories;
>   var cardForEmailAddress;
>   var addrbook;
>   while (!cardForEmailAddress && enumerator.hasMoreElements())
>   {
>-    addrbook = enumerator.getNext();
>-    if (addrbook instanceof Components.interfaces.nsIAbMDBDirectory)
>+    addrbook = enumerator.getNext()
>+                         .QueryInterface(Components.interfaces.nsIAbDirectory);

Won't this throw when it hits the first LDAP directory, causing the loop to abort? 

Also, on a somewhat related note, we should be using the NS_IMPL_ISUPPORTS?_CI macros whenever possible on C++ objects (e.g. the addressbooks themselves) so that we have classinfo, which gets us interface flattening in JS for free.

Looks like the mailnews/ version of this change has the same issue.

sr=dmose with the throwing issue addressed
Attachment #333846 - Flags: superreview?(dmose) → superreview+
(Assignee)

Comment 11

9 years ago
(In reply to comment #10)
> Won't this throw when it hits the first LDAP directory, causing the loop to
> abort? 

This is as a result of a misunderstanding, so no changes resulted (discussed via IRC).

Checked in, changeset fad5a882c378.
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
(Assignee)

Comment 12

9 years ago
Reopening, as it was segfaulting in tests for no obvious reason. I can't reproduce it locally, so I'll have to wait until gozer gets in tomorrow.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Updated

9 years ago
Status: REOPENED → ASSIGNED
(In reply to comment #12)
> Reopening, as it was segfaulting in tests for no obvious reason. I can't
> reproduce it locally, so I'll have to wait until gozer gets in tomorrow.

I've pinged gozer about the missing bots. However, you could try landing again, just leave it in for more than one cycle as I expect that will probably fix the problem.
Comment on attachment 333846 [details] [diff] [review]
Patch, version 3

>diff --git a/mailnews/base/resources/content/junkCommands.js b/mailnews/base/resources/content/junkCommands.js
>@@ -353,18 +357,20 @@
>-  if (spamSettings.useWhiteList && spamSettings.whiteListAbURI)
>-    whiteListDirectory = RDF.GetResource(spamSettings.whiteListAbURI).QueryInterface(Components.interfaces.nsIAbMDBDirectory);
>+  if (spamSettings.useWhiteList && spamSettings.whiteListAbURI) {
>+    whiteListDirectory = Components.classes["@mozilla.org/abmanager;1"]
>+                                   .getService(Components.interfaces.nsIAbManager)
>+                                   .getDirectory(spamSettings.whiteListAbURI);

{{
4f44120fd2a8: Bug 449618 -- Move cardForEmailAddress to nsIAbDirectory r=Standard8, sr=dmose
Joshua Cranmer <Pidgeot18@gmail.com> - Sat, 16 Aug 2008 10:54:28 -0400 - rev 111
}}
causes
<http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1218901560.1218902810.4620.gz>
{{
Linux nye Depend bloat on 2008/08/16 08:46:00

JavaScript error: chrome://messenger/content/junkCommands.js, line 490: missing } after function body
}}
(Assignee)

Comment 15

9 years ago
Clobbering the builds made it work.

Checking in was revision 4f44120fd2a8.

> {{
> Linux nye Depend bloat on 2008/08/16 08:46:00
> 
> JavaScript error: chrome://messenger/content/junkCommands.js, line 490: missing
> } after function body
> }}

This was fixed with revision 4e6a70a1f0bd.
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago9 years ago
Resolution: --- → FIXED
(In reply to comment #15)
> This was fixed with revision 4e6a70a1f0bd.

V.Fixed on this part:
<http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1218914580.1218915427.7363.gz&fulltext=1>

Updated

9 years ago
Target Milestone: --- → mozilla1.9.1a2
Depends on: 436677
No longer depends on: 436677
You need to log in before you can comment on or make changes to this bug.