449618 - Move cardForEmail to nsIAbDirectory

Status: RESOLVED FIXED

(MailNews Core :: Address Book, defect)

Description

[Joshua Cranmer [:jcranmer]]

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.

Comment 1

[Joshua Cranmer [:jcranmer]]

Attachment: Equivalent patch to repo changes

Meant to do this when I created the bug.

Comment 2

[Joshua Cranmer [:jcranmer]]

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...

Comment 3

[Joshua Cranmer [:jcranmer]]

Gotta remember to add my sr onto the CC list...

Comment 4

[Ian Neal]

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 5

[Mark Banner (:standard8)]

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.

Comment 6

[Joshua Cranmer [:jcranmer]]

Attachment: 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.

Comment 7

[Mark Banner (:standard8)]

(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.

Comment 8

[Joshua Cranmer [:jcranmer]]

Attachment: Patch, version 3

Sorry for the delay, but I've had some craziness over the past three days preventing me from working on this.

Comment 9

[Mark Banner (:standard8)]

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.

Comment 10

[Dan Mosedale (:dmosedale, :dmose)]

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

Comment 11

[Joshua Cranmer [:jcranmer]]

(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.

Comment 12

[Joshua Cranmer [:jcranmer]]

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.

Comment 13

[Mark Banner (:standard8)]

(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 14

[Serge Gautherie (:sgautherie)]

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 <[e-mail]> - 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
}}

Comment 15

[Joshua Cranmer [:jcranmer]]

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.

Comment 16

[Serge Gautherie (:sgautherie)]

(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>