Closed
Bug 410928
Opened 17 years ago
Closed 17 years ago
Remove nsAbMDBCardProperty
Categories
(MailNews Core :: Address Book, defect)
MailNews Core
Address Book
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: jcranmer, Assigned: jcranmer)
Details
Attachments
(1 file, 3 obsolete files)
14.32 KB,
patch
|
Details | Diff | Splinter Review |
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•17 years ago
|
||
This patch removes NotifyPropertyChanged from nsAbMDBCard and then moves everything from nsAbMDBCardProperty into nsAbMDBCard.
Attachment #295516 -
Flags: review?(bugzilla)
Comment 2•17 years ago
|
||
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•17 years ago
|
||
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•17 years ago
|
||
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 5•17 years ago
|
||
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•17 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•17 years ago
|
||
Patch with Standard8's and Neil's problems corrected. Almost posted without the destructor change...
Attachment #296708 -
Attachment is obsolete: true
Comment 8•17 years ago
|
||
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•17 years ago
|
||
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
Updated•17 years ago
|
OS: Linux → All
Hardware: PC → All
Comment 10•17 years ago
|
||
(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
Closed: 17 years ago
Resolution: --- → FIXED
Updated•16 years ago
|
Product: Core → MailNews Core
You need to log in
before you can comment on or make changes to this bug.
Description
•