287832 - Clean up nsDirPrefs.cpp

Status: RESOLVED FIXED

(MailNews Core :: LDAP Integration, defect)

Description

[Mikael Parknert]

The code in nsDirPrefs.cpp need some clean-up.

Comment 1

[Mikael Parknert]

As a first step I will remove a bunch of obselete, unused functions. Also remove
the notification callbacks (not used), and clean up nsDirPrefs.h from non
existing function.

Comment 2

[Mark Banner (:standard8)]

Mikael emailed me a while ago, and said he wasn't able to work on Mozilla at the current time (Mikael - correct me if that's wrong now), so I'm taking this bug and seeing what I can do in the way of cleanup.

Comment 3

[Mark Banner (:standard8)]

Attachment: Part 1 v1 - Remove obsolete functions (checked into trunk)

First of a few patches (hopefully) - this removes all the obsolete functions from nsDirPrefs that I could find. There may be a few more yet, but I'll check it again once this patch in.

Comment 4

[David :Bienvenu]

Comment on attachment 204827 [details] [diff] [review]
Part 1 v1 - Remove obsolete functions (checked into trunk)

it'll be nice to see these go, thx.

Comment 5

[Mark Banner (:standard8)]

Note for me for next patch: idColumnAttribute in DIR_PrefId and columnAttributes in DIR Server are never really used - they are set to a default value, and attempted to be saved (but won't be because they are default) but never get set to anything else.

I bet there are other attributes not used. 

Currently waiting on bug 281666 before I do a patch for this.

Comment 6

[Mark Banner (:standard8)]

Attachment: Part 2 v1 - stop LDAP Change log using old nsDirPrefs attribute map stuff

As discussed with dmose on irc, this stops the LDAP change log replication using the nsDirPrefs attribute map and uses the newer nsIAbLDAPAttributeMap instead. Change Log replication isn't actually enabled by default at the moment, but I need this to enable us to remove all the old attribute map code from nsDirPrefs.

Comment 7

[Mark Banner (:standard8)]

Attachment: Part 3 v1 (checked in)

There's going to be a few more of these yet I think. This patch removes some attributes of servers that we no longer use.

Comment 8

[David :Bienvenu]

Comment on attachment 207020 [details] [diff] [review]
Part 3 v1 (checked in)

thx for the cleanup!

+  server->isOffline = (dirType == LDAPDirectory ? PR_TRUE : PR_FALSE);

this can just be

server>isOffline = (dirType == LDAPDirectory);

Comment 9

[Mark Banner (:standard8)]

adding link to bug 129326 - sort of the same as this one & needs to be considered as we do this cleanup.

Comment 10

[Mark Banner (:standard8)]

Attachment: Part 4 v1 (checked in)

Removes old filtering handling preferences and some other attributes and definitions that are no longer required. Still more to come after this one ;-)

Comment 11

[David :Bienvenu]

Comment on attachment 207204 [details] [diff] [review]
Part 4 v1 (checked in)

great, thx.

Comment 12

[Mark Banner (:standard8)]

Attachment: Part 5 v1 (depends on Part 2) (checked in)

This patch removes all the old ldap attribute handling from nsDirPrefs (this is now all contained within nsIAbLDAPAttributeMap).

This patch depends on the part 2 patch also attached to this bug being accepted (dmose: ping) as the part 2 patch removes the only bit of code that was still sort-of using it.

Comment 13

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 207009 [details] [diff] [review]
Part 2 v1 - stop LDAP Change log using old nsDirPrefs attribute map stuff

As discussed on IRC, please drop the "attributes" goop entirely and just do the default of requesting the entire set of attrs.  Someday, we should make the LDAP XPCOM SDK support LDAP_NO_ATTRS and use that here.  Otherwise, looks good.

Comment 14

[Mark Banner (:standard8)]

Attachment: Part 2 v2 - stop LDAP Change log using old nsDirPrefs attribute map stuff (checked in)

Update per Dan's comments.

Comment 15

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 207361 [details] [diff] [review]
Part 2 v2 - stop LDAP Change log using old nsDirPrefs attribute map stuff (checked in)

I'd vote for just using 0 instead of nsnull for the array, as that's reasonable C++.  Up to you, however.  Nice work; r=dmose.

Comment 16

[Mark Banner (:standard8)]

Attachment: Part 6 v1

A little more cleanup ;-)

Comment 17

[David :Bienvenu]

Comment on attachment 208012 [details] [diff] [review]
Part 6 v1

+  else
+    return 1;
+
+  return NS_OK;
 }

this seems odd - 1 isn't an nsresult.

Comment 18

[Mark Banner (:standard8)]

Attachment: Part 6 v2 (checked in)

Revised due to David's comment and we don't need that refCount code anyway - its not really used (Increment never called & all DIR_Servers initialised to 1, no refCount increments).

Comment 19

[Mark Banner (:standard8)]

Comment on attachment 208015 [details] [diff] [review]
Part 6 v2 (checked in)

This one is in now as well:

Checking in mailnews/addrbook/src/nsDirPrefs.h;
/cvsroot/mozilla/mailnews/addrbook/src/nsDirPrefs.h,v  <--  nsDirPrefs.h
new revision: 1.36; previous revision: 1.35
done
Checking in mailnews/addrbook/src/nsDirPrefs.cpp;
/cvsroot/mozilla/mailnews/addrbook/src/nsDirPrefs.cpp,v  <--  nsDirPrefs.cpp
new revision: 1.108; previous revision: 1.107

Comment 20

[Mark Banner (:standard8)]

Attachment: Part 7 v1 - Start using nsIPrefBranch properly

This is a more proof-of-concept patch that I'd like to extend to the rest of the nsDirPrefs (and possibly other address book) code, but I want to get someone's opinion on it first (note: if I get r+ then I'll be looking for sr+ and checkin)

The basic idea is to a) don't get nsIPrefBranch every time we want a pref, b) simplify getting ints & bools avoiding loads of function calls.

Comment 21

[Mark Banner (:standard8)]

Attachment: Remove some redundant attributes and other stuff (checked in).

Simply tidy up patch: This patch removes some unused function parameters and moves one function from being defined globally to just within nsDirPrefs.cpp as that is where it is used.

Comment 22

[David :Bienvenu]

Comment on attachment 208660 [details] [diff] [review]
Part 7 v1 - Start using nsIPrefBranch properly

+    if (NS_FAILED(prefBranch->GetBoolPref("replication.never", &prefBool)))
+      DIR_ForceFlag(server, DIR_REPLICATE_NEVER, kDefaultReplicateNever);
+    else
+      DIR_ForceFlag(server, DIR_REPLICATE_NEVER, prefBool);

you could use the ? operator, or just init prefBool to KDefaultReplicateNever and then ignore the return value of GetBoolPref, assuming it doesn't change the out param on failure (which it shouldn't).

In fact, there seems to be a lot of that pattern in the patch - do the DIR_Get functions change the out value on failure?

Comment 23

[Mark Banner (:standard8)]

Comment on attachment 208660 [details] [diff] [review]
Part 7 v1 - Start using nsIPrefBranch properly

(In reply to comment #22)
> In fact, there seems to be a lot of that pattern in the patch - do the DIR_Get
> functions change the out value on failure?
> 
The DIR_Get functions do take a default value to return. However in the int/bool cases I was going to try and remove the additional function call each time when there was just a choice of the pref value or the return value.

However, I'm going to cancel the review request  as firstly the patch will soon be bit-rotted, and secondly I'm not sure this is worth doing at the moment due to other back-end changes that will essentially make this part of clean up redundant, though probably in a slightly longer time scale.

Comment 24

[Mark Banner (:standard8)]

Attachment: Part 9 v1 (checked in)

Some more removals... The replication pref "description" isn't actually used anywhere. The replication pref "fileName" (not to be confused with the one of the same name defined in DIR_Server) is written to but never actually gets read from. Therefore we may as well just use the main DIR_Server fileName as that is the one that is being used (and it reduces code size :-) ).

Comment 25

[David :Bienvenu]

Comment on attachment 211502 [details] [diff] [review]
Part 9 v1 (checked in)

thx, looks good.

Comment 26

[Mark Banner (:standard8)]

Attachment: Part 10 Reduce definitions in nsDirPrefs.h (checked in)

Now that we have cleaned up some of nsDirPrefs and address book, there's a whole load of definitions (functions & defines) that don't really need to be in nsDirPrefs.h.

Therefore I'd like to remove them to the just nsDirPrefs.cpp so that a) it makes it clearer what is still used externally to nsDirPrefs (for the continuing clear up) and b) others don't start using them thinking that they are a good idea ;-) (not that this has happened... yet).

I checked this via lxr & compilation on my Linux box so we should be ok.

Comment 27

[Mark Banner (:standard8)]

Attachment: Part 11 - Remove more redundant/obsolete prefs from nsDirPrefs (checked in)

Another sweep of nsDirPrefs - all these removes either belong to prefs that we don't use anymore, or that don't need to be handled by nsDirPrefs.

Note that savingServer used to be in DIR_Server::flags - as it would be the only flag left, I've separated it out to a simple PRBool.

Comment 28

[Mark Banner (:standard8)]

Attachment: Part 12 - final clean up

This patch tidies up some functions (combine two into one where necessary, don't return values as they are never checked etc) and removes unnecessary includes.

This is the last patch I'm going to do on this bug, I'll mark it as fixed once the patch is approved and checked in.

I believe nsDirPrefs.h/.cpp include only what they need to now, it is a lot tidier and cleaner than when we started. Whilst I'd have liked to remove it altogether with this bug, I think the work that remains is best covered in other bugs as the restructuring work continues.

Comment 29

[David :Bienvenu]

Comment on attachment 281663 [details] [diff] [review]
Part 12 - final clean up

thx, Mark. I know this code just moved, but these PR_FREEIF's can just be PR_Free's:

+    PR_FREEIF(server->prefName);
+    PR_FREEIF(server->description);
+    PR_FREEIF(server->fileName);
+    PR_FREEIF(server->uri);

because we're freeing the server object in the next statement, so there's no use nulling out the pointers (which is the difference between PR_Free and PR_FREEIF)

Comment 30

[Mark Banner (:standard8)]

Patch checked in -> Fixed.

Any more work on nsDirPrefs.h/.cpp will be covered in other bugs.