Remove nsAbMDBCardProperty

RESOLVED FIXED

Status

MailNews Core
Address Book
RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: jcranmer, Assigned: jcranmer)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

11 years ago
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.
(Assignee)

Comment 1

11 years ago
Created attachment 295516 [details] [diff] [review]
Patch which removes nsAbMDBCard

This patch removes NotifyPropertyChanged from nsAbMDBCard and then moves everything from nsAbMDBCardProperty into nsAbMDBCard.
Attachment #295516 - Flags: review?(bugzilla)
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 ...
Attachment #295516 - Flags: review?(bugzilla) → review-
(Assignee)

Comment 3

11 years ago
Created attachment 296707 [details] [diff] [review]
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.
Attachment #295516 - Attachment is obsolete: true
Attachment #296707 - Flags: review?(bugzilla)
(Assignee)

Comment 4

11 years ago
Created attachment 296708 [details] [diff] [review]
Real patch

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

Here is the correct one....
Attachment #296707 - Attachment is obsolete: true
Attachment #296708 - Flags: review?(bugzilla)
Attachment #296707 - Flags: review?(bugzilla)
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.
Attachment #296708 - Flags: review?(bugzilla) → review+
(Assignee)

Comment 6

11 years ago
Comment on attachment 296708 [details] [diff] [review]
Real patch

sr=Neil over IRC with the caveat that the destructor is fixed.
Attachment #296708 - Flags: superreview+
(Assignee)

Comment 7

11 years ago
Created attachment 296845 [details] [diff] [review]
patch to checkin

Patch with Standard8's and Neil's problems corrected.
Almost posted without the destructor change...
Attachment #296708 - Attachment is obsolete: true
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
OS: Linux → All
Hardware: PC → All
(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.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.