Closed Bug 325958 Opened 15 years ago Closed 15 years ago

Misc Address Book tidying up

Categories

(MailNews Core :: Address Book, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: standard8, Assigned: standard8)

Details

(Keywords: fixed-seamonkey1.1a, fixed1.8.1)

Attachments

(3 files, 2 obsolete files)

There are some unused variables/functions and some unneeded includes that I'd like to clean up and remove from address book but don't really fit in any other bug at the moment. So this bug is for them.
This patch is mainly aimed at cleaning up the #includes a bit (there are some redundant ones/some changed ones - changed = #include -> interface). There is also the removal of a redundant function and a redundant variable from elsewhere.
Attachment #210757 - Flags: superreview?(dmose)
Attachment #210757 - Flags: review?(dmose)
According to lxr (http://lxr.mozilla.org/seamonkey/search?string=pref-addressbookOverlay.xul) pref-addressbookOverlay is only mentioned in the jar.mn file. I've done some other searching around and can't see where it would get hooked in by other means. The only thing I can think of is that it was from a time when address book may have almost become a seperate section in prefs. Therefore I guess its safe for us to remove it.
Attachment #210790 - Flags: superreview?(mscott)
Attachment #210790 - Flags: review?
Attachment #210790 - Flags: review? → review?(neil)
Comment on attachment 210790 [details] [diff] [review]
Remove pref-addressingOverlay.xul (checked in, trunk + branch)

Yeah, it looks as if mailPrefsOverlay.xul covers this.
Attachment #210790 - Flags: review?(neil) → review+
Attachment #210790 - Flags: superreview?(mscott) → superreview+
Attachment #210790 - Attachment description: Remove pref-addressingOverlay.xul → Remove pref-addressingOverlay.xul (checked in)
Fixes a small leak when we're modifying a directory.
Attachment #212227 - Flags: superreview?(bienvenu)
Attachment #212227 - Flags: review?(bienvenu)
Attachment #212227 - Flags: superreview?(bienvenu)
Attachment #212227 - Flags: superreview+
Attachment #212227 - Flags: review?(bienvenu)
Attachment #212227 - Flags: review+
Attachment #212227 - Attachment description: Fixes small leak in nsAbBSDirectory.cpp → Fixes small leak in nsAbBSDirectory.cpp (checked in)
Comment on attachment 212227 [details] [diff] [review]
Fixes small leak in nsAbBSDirectory.cpp (checked in, trunk + branch)

Requesting branch approval for this small memory leak fix patch. Should be low risk.
Attachment #212227 - Flags: approval-branch-1.8.1?(mscott)
Attachment #212227 - Flags: approval-branch-1.8.1?(mscott) → approval-branch-1.8.1+
Attachment #212227 - Attachment description: Fixes small leak in nsAbBSDirectory.cpp (checked in) → Fixes small leak in nsAbBSDirectory.cpp (checked in, trunk + branch)
Comment on attachment 210790 [details] [diff] [review]
Remove pref-addressingOverlay.xul (checked in, trunk + branch)

Requesting branch approval, Removes a redundant file that's no longer needed.
Attachment #210790 - Flags: approval-branch-1.8.1?(mscott)
Attachment #210790 - Flags: approval-branch-1.8.1?(mscott) → approval-branch-1.8.1+
Attachment #210790 - Attachment description: Remove pref-addressingOverlay.xul (checked in) → Remove pref-addressingOverlay.xul (checked in, trunk + branch)
Comment on attachment 210757 [details] [diff] [review]
idl tidy and a removal of function and unused variable

One of my other patches will probably break this one, I'll re-evaluate if I want to do all parts of this after that one has gone in.
Attachment #210757 - Attachment is obsolete: true
Attachment #210757 - Flags: superreview?(dmose)
Attachment #210757 - Flags: review?(dmose)
Attached patch Random tidy ups. (obsolete) — Splinter Review
Some more random tidy up and improvements in the code.
Attachment #214595 - Flags: superreview?(bienvenu)
Attachment #214595 - Flags: review?(bienvenu)
Comment on attachment 214595 [details] [diff] [review]
Random tidy ups.

a few nits:

saw this in a couple places:

-    if (!value)
+    if (mName.IsEmpty())
         *aName = 0;
     else
         *aName = ToNewCString(mName);

can just be
*aName = (mName.IsEmpty() ? 0 : ToNewCString(mName);

+      *matchFound = (conditionType == nsIAbBooleanConditionTypes::Exists) ?
+          PR_TRUE : PR_FALSE;

can just be *matchFound = (conditionType == nsIAb...);

-    if (!DIR_GetDirectories())
+    nsVoidArray *directories = DIR_GetDirectories();
+    if (!directories)
       return NS_ERROR_FAILURE;

I hate dropping errors like this, but I guess DIR_GetDirectories is going away eventually anyway.

sr=bienvenu with those nits addressed.
Attachment #214595 - Flags: superreview?(bienvenu)
Attachment #214595 - Flags: superreview+
Attachment #214595 - Flags: review?(bienvenu)
Attachment #214595 - Flags: review+
The patch I checked in after having addressed bienvenu's comments. Requesting approval for 1.8.1 branch to keep code in sync and enable the codesize to be reduced and improved a little - with removal of redundant variables and assigns  etc.
Attachment #214595 - Attachment is obsolete: true
Attachment #214698 - Flags: superreview+
Attachment #214698 - Flags: review+
Attachment #214698 - Flags: approval-branch-1.8.1?(mscott)
Ok, I think I've covered most of what I wanted to do initially at least. Any more changes can be covered under other bugs. Marking this as fixed.
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Attachment #214698 - Flags: approval-branch-1.8.1?(mscott) → approval-branch-1.8.1+
Attachment #214698 - Attachment description: Random tidy ups (checked in, trunk) → Random tidy ups (checked in, trunk + branch)
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.