410158 - Merge nsIAddrBookSession into nsIAbManager

Status: RESOLVED FIXED

(MailNews Core :: Address Book, defect)

Description

[Mark Banner (:standard8)]

As nsIAddrBookSession and nsIAbManager are both services, and are both related to overall address book management and notifications, we should merge them into one to save having two locations to get everything from.

What we need to do is this:

- Move the functions from http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/mailnews/addrbook/public/nsIAddrBookSession.idl&rev=1.15&mark=55-117#47 into http://mxr.mozilla.org/seamonkey/source/mailnews/addrbook/public/nsIAbManager.idl and add documentation where missing.

- Move the members and functions from nsAddrBookSession.{h,cpp} into nsAbManager.{h,cpp} doing appropriate renaming.

- Only copy across the includes that we really need to.

- Fix all the callers (http://mxr.mozilla.org/seamonkey/search?string=nsIAddrBookSession) to use nsIAbManager. Note: there may be some chances for optimisation here where nsIAbManager is already in use.

So this should be reasonably simple for anyone with a build environment to do, hence helpwanted.

Comment 1

[[e-mail]]

Note that as a first stage you can just rename the functions in place i.e. nsAddrBookSession::AddAddressBookListener->nsAbManager::AddAddressBookListener

Comment 2

[Mark Banner (:standard8)]

Attachment: The fix

Merges nsIAddrBookSession and nsIAbManager. I decided that at the same time as merging them, it wasn't much difference to improve the listener structure, moving from an nsISupportsArray to an nsTObserverArray in a similar manner to what we did in nsMsgMailSession.

Most of this patch is just the appropriate renames. I found quite a few places where we were unnecessarily including nsIAddrBookSession or getting the service, so there's some nice cleanup there as well.

I've also added in a unit test for the listeners on nsAbManager, this is based on the nsMsgMailSession unit test.

Comment 3

[Ian Neal]

Comment on attachment 297769 [details] [diff] [review]
The fix

>Index: mailnews/extensions/palmsync/src/nsAbPalmSync.cpp
>===================================================================
>@@ -770,16 +769,17 @@ nsresult nsAbPalmHotSync::KeepCurrentSta
> 
>     if(!mPreviousABFile) 
>     {
>-        nsCOMPtr<nsIAddrBookSession> abSession = do_GetService(NS_ADDRBOOKSESSION_CONTRACTID, &rv);
>-        if(NS_FAILED(rv)) 
>-            return rv;
>-        rv = abSession->GetUserProfileDirectory(getter_AddRefs(mPreviousABFile));
>-        if(NS_SUCCEEDED(rv)) 
>-        {
>-            mPreviousABFile->AppendNative(previousLeafName);
>-            if(NS_FAILED(rv))
>-                return rv;
>-        }
>+      nsCOMPtr<nsIAbManager> abManager(do_GetService(NS_ABMANAGER_CONTRACTID, &rv));
>+      if (NS_FAILED(rv))
>+	return rv;
Nit: Tab in line above.
>+
>+      rv = abManager->GetUserProfileDirectory(getter_AddRefs(mPreviousABFile));
>+      if (NS_SUCCEEDED(rv))
>+      {
>+	mPreviousABFile->AppendNative(previousLeafName);
>+	if (NS_FAILED(rv))
>+	  return rv;
Nit: Tabs in the 3 lines above.
>+      }
>     }
> 
>     PRBool bExists=PR_FALSE;

Not sure if there are other tabs.

Comment 4

[[e-mail]]

Comment on attachment 297769 [details] [diff] [review]
The fix

>+  /**
>+   * Returns the user profile directory. NOTE: this should not be used
>+   * as it may go away soon.
>+   */
Almost worth doing that first, avoiding patching the same code twice ;-)

>+  Components.classes["@mozilla.org/abmanager;1"]
>+    .getService(Components.interfaces.nsIAbManager)
>+    .addAddressBookListener(gAddressBookAbListener,
>+			    Components.interfaces.nsIAbListener.directoryRemoved |
>+			    Components.interfaces.nsIAbListener.changed);
Nit: tabs; also it might be worth tweaking all these as follows:
const nsIAbListener = Components.interfaces.nsIAbListener;
Components.classes["@mozilla.org/abmanager;1"]
          .getService(Components.interfaces.nsIAbManager)
          .addAddressBookListener(gAddressBookAbListener,
                                  nsIAbListener.directoryRemoved |
                                  nsIAbListener.changed);

>+		  loginmgr \
Another patch methinks ;-)

>+#include "nsIAbManager.h"
> #include "nsDirPrefs.h"
> #include "nsAbBaseCID.h"
> #include "nsAddrDatabase.h"
>-#include "nsIAddrBookSession.h"
Any reason why you didn't edit the line in place in this file?

>+    abManager->NotifyItemPropertyChanged(this, "DirName", oldDirName.get(),
>                                          nsString(aDirName).get());
Looks like these notifications need to be AStringed at some point.

>-  rv = abSession->NotifyItemPropertyChanged(item, nsnull, nsnull, nsnull);
>+  rv = abManager->NotifyItemPropertyChanged(item, nsnull, nsnull, nsnull);
(While researching the previous comment I noticed that the DS doesn't null-check the property name, but fortunately a card isn't a resource.)

>+  abListener newListener(aListener, aNotifyFlags);
>+  mListeners.AppendElementUnlessExists(newListener);
See below.

>+  PRInt32 index = mListeners.IndexOf(aListener);
>+  if (index != -1)
>+    mListeners.RemoveElementAt(index);
Why don't you use RemoveElement(aListener); directly?

>+  nsresult rv;
>+  nsCOMPtr<nsIFile> profileDir;
>+  nsCAutoString pathBuf;
>+
>+  rv = NS_GetSpecialDirectory(NS_APP_USER_PROFILE_50_DIR, getter_AddRefs(profileDir));
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  rv = profileDir->GetNativePath(pathBuf);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  return NS_NewNativeLocalFile(pathBuf, PR_TRUE, userDir);
Eww, this is some sucky code. Try this:
return CallQueryInterface(profileDir, userDir);
Of course, as the comment quoted above mentions, this should be inlined.

>+    int operator==(const abListener &aListener) const {
>+      return mListener == aListener.mListener &&
>+             mNotifyFlags == aListener.mNotifyFlags;
Note that the old append code only checked the listener, not the flags.

>Index: mailnews/addrbook/src/nsAbOSXCard.mm
Obviously I didn't compile this.

>+    do_check_eq(gAblAll.mReceived, Math.pow(2, numListenerOptions) - 1);
Don't use Math.pow for powers of 2, use (1 << numListenerOptions) - 1 instead
(I seem to remember seeing that on another test somewhere?)

>-  nsIAddrBookSession *abSession = nsnull;
>+  nsIAbManager *abManager = nsnull;
>   rv = proxyObjectManager->GetProxyForObject(NS_PROXY_TO_MAIN_THREAD,
>-                                             NS_GET_IID(nsIAddrBookSession),
>-                                             addrBookSession,
>+                                             NS_GET_IID(nsIAbManager),
>+                                             abMan,
>                                              NS_PROXY_SYNC,
>-                                             (void**)&abSession);
>+                                             (void**)&abManager);
> 
>   if (NS_SUCCEEDED(rv))
>-    rv = abSession->GetUserProfileDirectory(getter_AddRefs(dbPath));
>+    rv = abManager->GetUserProfileDirectory(getter_AddRefs(dbPath));
I wonder whether NS_GetSpecialDirectory is threadsafe?
If it was, inlining would be a perf win too :-)

Comment 5

[Mark Banner (:standard8)]

(In reply to comment #4)
> (From update of attachment 297769 [details] [diff] [review])
> >+  /**
> >+   * Returns the user profile directory. NOTE: this should not be used
> >+   * as it may go away soon.
> >+   */
> Almost worth doing that first, avoiding patching the same code twice ;-)

I would, but to get there its a lot more complicated at the moment.

> >+    abManager->NotifyItemPropertyChanged(this, "DirName", oldDirName.get(),
> >                                          nsString(aDirName).get());
> Looks like these notifications need to be AStringed at some point.

On my todo list, there's some others that I'd do at the same time, which is why I didn't do this one.

> >+  PRInt32 index = mListeners.IndexOf(aListener);
> >+  if (index != -1)
> >+    mListeners.RemoveElementAt(index);
> Why don't you use RemoveElement(aListener); directly?

Guess I was getting nsTObserverArray and nsIMutableArray mixed up...

> >Index: mailnews/addrbook/src/nsAbOSXCard.mm
> Obviously I didn't compile this.

I've got someone to do that for me.

> >-  nsIAddrBookSession *abSession = nsnull;
> >+  nsIAbManager *abManager = nsnull;
> >   rv = proxyObjectManager->GetProxyForObject(NS_PROXY_TO_MAIN_THREAD,
> >-                                             NS_GET_IID(nsIAddrBookSession),
> >-                                             addrBookSession,
> >+                                             NS_GET_IID(nsIAbManager),
> >+                                             abMan,
> >                                              NS_PROXY_SYNC,
> >-                                             (void**)&abSession);
> >+                                             (void**)&abManager);
> > 
> >   if (NS_SUCCEEDED(rv))
> >-    rv = abSession->GetUserProfileDirectory(getter_AddRefs(dbPath));
> >+    rv = abManager->GetUserProfileDirectory(getter_AddRefs(dbPath));
> I wonder whether NS_GetSpecialDirectory is threadsafe?
> If it was, inlining would be a perf win too :-)

Like I said above, I want to replace this all in some way so that we just create an nsIAbDirectory and get a nsIFile from it, still a little way to go, and this isn't perf critical.

Comment 6

[Mark Banner (:standard8)]

Attachment: The fix v2

Updated patch to address Neil's comments, carrying forward his r, and requesting sr.

Comment 7

[Mark Banner (:standard8)]

Attachment: The fix v3

Updated patch to include the changes to the unit test that are needed to keep it working after I implemented bug 413227.

Comment 8

[Mark Banner (:standard8)]

Patch checked in -> fixed.