410928 - Remove nsAbMDBCardProperty

Status: RESOLVED FIXED

(MailNews Core :: Address Book, defect)

Description

[Joshua Cranmer [:jcranmer]]

A little hunting in the address book reveals that the only function in nsAbMDBCard not in nsAbMDBCardProperty is NotifyPropertyChanged, which is no longer used. This bug is to move all properties of nsAbMDBCardProperty into nsAbMDBCard and all uses of the former to the latter as well.

Comment 1

[Joshua Cranmer [:jcranmer]]

Attachment: Patch which removes nsAbMDBCard

This patch removes NotifyPropertyChanged from nsAbMDBCard and then moves everything from nsAbMDBCardProperty into nsAbMDBCard.

Comment 2

[Mark Banner (:standard8)]

Comment on attachment 295516 [details] [diff] [review]
Patch which removes nsAbMDBCard

I got this to apply (I had to alter one of the patch numbers).

I realise most of this is copy & pasted (and hence not your fault), the comments below are mainly made as it'd be nice to tidy the code up a bit more at the same time.

Could you remove the tabs and replace them by two-space indentation please.

+	nsCOMPtr<nsIAddrDatabase> mCardDatabase;  

this line also has extra whitespace on the end, I think its the only one.

--- /cvsroot/mozilla/mailnews/addrbook/src/nsAbMDBCard.cpp
+++ /cvsroot/mozilla/mailnews/addrbook/src/nsAbMDBCard.cpp

 #include "nsAbMDBCard.h"	 
 #include "nsIRDFService.h"
-#include "nsServiceManagerUtils.h"
+#include "nsIServiceManager.h"
 #include "nsRDFCID.h"
 #include "nsStringGlue.h"
+#include "nsAbBaseCID.h"
+#include "rdf.h"
 #include "nsCOMPtr.h"
-#include "nsAbBaseCID.h"
 
 #include "nsAddrDatabase.h"
 #include "nsIAddrBookSession.h"
+#include "nsIAbMDBDirectory.h"
+#include "nsILocalFile.h"

I just checked, all we actually need to include in this file with the changes is nsAbMDBCard.h, the others aren't necessary (I know some of them weren't yours).

 nsAbMDBCard::nsAbMDBCard(void)
 {
+	m_key = 0;
+	m_dbTableID = 0;
+	m_dbRowID = 0;
 }

Could we tidy this up and go with initialising them on the constructor line? Also, drop the void.

nsAbMDBCard::nsAbMDBCard(void) : m_key(0), ...

 nsAbMDBCard::~nsAbMDBCard(void)
 {
+	if (mCardDatabase)
+		mCardDatabase = nsnull;
 }

Not sure why they did this before, but a nsCOMPtr member doesn't need it. Ditto about dropping the void.

+NS_IMETHODIMP nsAbMDBCardProperty::SetStringAttribute(const char *name, const PRUnichar *value)
+NS_IMETHODIMP nsAbMDBCardProperty::GetStringAttribute(const char *name, PRUnichar **value)

Looks like you forgot to rename these.

+  if (dbRowID == m_dbRowID && dbTableID == m_dbTableID && key == m_key)
+    *result = PR_TRUE;
+  else
+    *result = PR_FALSE;

Not your fault, but we can simplify this to:

*result = (dbRowID ...

Comment 3

[Joshua Cranmer [:jcranmer]]

Attachment: Patch created from hand

Having decided that trying to excise the needed parts from one mega-patch by hand was too difficult, I wrote this one from scratch.

It should work for palmsync, but I am unable to build that directory, so I can't test it there.

I also fixed up some of the whitespace issues.

Comment 4

[Joshua Cranmer [:jcranmer]]

Attachment: Real patch

Sorry, but I forgot to make all the changes to the patch.

Here is the correct one....

Comment 5

[Mark Banner (:standard8)]

Comment on attachment 296708 [details] [diff] [review]
Real patch

 nsAbMDBCard::~nsAbMDBCard(void)
 {
+  if (mCardDatabase)
+    mCardDatabase = nsnull;

You forgot to remove this...

+NS_IMETHODIMP nsAbMDBCard::GetKey(PRUint32 *aKey)
+{
+  *aKey = m_key;
+    return NS_OK;
+  }

The indentation of the return just needs a quick tidy.

r=me with those fixed.

Comment 6

[Joshua Cranmer [:jcranmer]]

Comment on attachment 296708 [details] [diff] [review]
Real patch

sr=Neil over IRC with the caveat that the destructor is fixed.

Comment 7

[Joshua Cranmer [:jcranmer]]

Attachment: patch to checkin

Patch with Standard8's and Neil's problems corrected.
Almost posted without the destructor change...

Comment 8

[Mark Banner (:standard8)]

Checked in:

Checking in mailnews/addrbook/src/Makefile.in;
/cvsroot/mozilla/mailnews/addrbook/src/Makefile.in,v  <--  Makefile.in
new revision: 1.75; previous revision: 1.74
done
Checking in mailnews/addrbook/src/nsAbMDBCard.cpp;
/cvsroot/mozilla/mailnews/addrbook/src/nsAbMDBCard.cpp,v  <--  nsAbMDBCard.cpp
new revision: 1.16; previous revision: 1.15
done
Checking in mailnews/addrbook/src/nsAbMDBCard.h;
/cvsroot/mozilla/mailnews/addrbook/src/nsAbMDBCard.h,v  <--  nsAbMDBCard.h
new revision: 1.7; previous revision: 1.6
done
Checking in mailnews/addrbook/src/nsAbMDBDirectory.cpp;
/cvsroot/mozilla/mailnews/addrbook/src/nsAbMDBDirectory.cpp,v  <--  nsAbMDBDirectory.cpp
new revision: 1.79; previous revision: 1.78
done
Checking in mailnews/extensions/palmsync/src/nsAbIPCCard.h;
/cvsroot/mozilla/mailnews/extensions/palmsync/src/nsAbIPCCard.h,v  <--  nsAbIPCCard.h
new revision: 1.7; previous revision: 1.6
Removing mailnews/addrbook/src/nsAbMDBCardProperty.cpp;
/cvsroot/mozilla/mailnews/addrbook/src/nsAbMDBCardProperty.cpp,v  <--  nsAbMDBCardProperty.cpp
new revision: delete; previous revision: 1.30
done
Removing mailnews/addrbook/src/nsAbMDBCardProperty.h;
/cvsroot/mozilla/mailnews/addrbook/src/nsAbMDBCardProperty.h,v  <--  nsAbMDBCardProperty.h
new revision: delete; previous revision: 1.9

Comment 9

[[e-mail]]

i checked in the following change to fix bustage on the windows tbox.

http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&subdir=mozilla/mailnews/addrbook/src&command=DIFF_FRAMESET&file=nsAbMDBCard.h&rev1=1.7&rev2=1.8&root=/cvsroot

Comment 10

[Mark Banner (:standard8)]

(In reply to comment #9)
> i checked in the following change to fix bustage on the windows tbox.
> http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&subdir=mozilla/mailnews/addrbook/src&command=DIFF_FRAMESET&file=nsAbMDBCard.h&rev1=1.7&rev2=1.8&root=/cvsroot

That's the correct fix. Not sure how I missed that on the review.

This is fixed now.