Closed Bug 305434 Opened 14 years ago Closed 11 years ago

LDAP Prefs Service needs revising.

Categories

(MailNews Core :: Address Book, defect, P3)

x86
All
defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: standard8, Assigned: standard8)

References

Details

Attachments

(2 files, 1 obsolete file)

This bug originates from investigations during bug 58696 (see
https://bugzilla.mozilla.org/show_bug.cgi?id=58696#c34).

This will basically be a tidy up patch to hopefully reduce code base and memory
usage consisting of:

1) remove the hidden call to migrate from the constructor of nsILDAPPrefsService
and call it explictly where required.
2) There is some near duplicate code in:

http://lxr.mozilla.org/seamonkey/source/mailnews/addrbook/src/nsLDAPPrefsService.js#76
 (in the constructor)
and
http://lxr.mozilla.org/seamonkey/source/mailnews/addrbook/prefs/resources/content/pref-directory.js#141

which is sorting out the available functions. We could probably combine into one
function in nsILDAPPrefsService.

3) The constructor code could then be moved into migrate - as that is the only
bit that actually uses it, and we won't then need to store availDirectories in
nsILDAPPrefsService.
Blocks: 87660
This patch seperates out the migrate function from the constructor of the LDAP Prefs service. In doing this, we are able to clear down the constructor and not unnecessarily store a list of available directories.

There is still some duplication of processing of lists, but that's for the next patch.
Attachment #201334 - Flags: review?(bienvenu)
Comment on attachment 201334 [details] [diff] [review]
Seperate out the ldap migrate function from the constructor

how about calling the migrate method something like "migratePrefsIfNeeded" or something, since we will rarely actually migrate prefs.
Attachment #201334 - Flags: review?(bienvenu) → review+
Same as before, but changed name of migrate function as per David's suggestion.
Attachment #201334 - Attachment is obsolete: true
Attachment #201456 - Flags: superreview?(dmose)
Attachment #201456 - Flags: review+
Comment on attachment 201456 [details] [diff] [review]
Seperate out the ldap migrate function v2 (checked in)

>Index: mailnews/addrbook/resources/content/addressbook.js
>===================================================================
>+  if (ldapPrefs)
>+    ldapPrefs.migratePrefsIfNeeded();
>+

I tend to prefer bracing all "then" and "else" clauses, because it helps avoid mistakes in future edits of the code when someone wants to add another statement to the clause in question.  But up to you.

The other thing you might consider is that you use dump() in various places to report errors.  If you think it would help end users / sysadmins diagnose issues, you could alternately use Components.utils.reportError(), which will send a message to the JS console.
Attachment #201456 - Flags: superreview?(dmose) → superreview+
Comment on attachment 201456 [details] [diff] [review]
Seperate out the ldap migrate function v2 (checked in)

Checked in the first patch with dmose comments addressed.
/cvsroot/mozilla/mailnews/addrbook/prefs/resources/content/pref-directory.js,v  <--  pref-directory.js
new revision: 1.27; previous revision: 1.26
done
Checking in mailnews/addrbook/public/nsILDAPPrefsService.idl;
/cvsroot/mozilla/mailnews/addrbook/public/nsILDAPPrefsService.idl,v  <--  nsILDAPPrefsService.idl
new revision: 1.4; previous revision: 1.3
done
Checking in mailnews/addrbook/resources/content/addressbook.js;
/cvsroot/mozilla/mailnews/addrbook/resources/content/addressbook.js,v  <--  addressbook.js
new revision: 1.119; previous revision: 1.118
done
Checking in mailnews/addrbook/src/nsLDAPPrefsService.js;
/cvsroot/mozilla/mailnews/addrbook/src/nsLDAPPrefsService.js,v  <--  nsLDAPPrefsService.js
new revision: 1.19; previous revision: 1.18
done
Checking in mailnews/base/src/nsMessengerMigrator.cpp;
/cvsroot/mozilla/mailnews/base/src/nsMessengerMigrator.cpp,v  <--  nsMessengerMigrator.cpp
new revision: 1.116; previous revision: 1.115
done
Checking in mailnews/compose/resources/content/MsgComposeCommands.js;
/cvsroot/mozilla/mailnews/compose/resources/content/MsgComposeCommands.js,v  <--  MsgComposeCommands.js
new revision: 1.373; previous revision: 1.372
done
Checking in mail/components/addrbook/content/addressbook.js;
/cvsroot/mozilla/mail/components/addrbook/content/addressbook.js,v  <--  addressbook.js
new revision: 1.24; previous revision: 1.23
done
Checking in mail/components/compose/content/MsgComposeCommands.js;
/cvsroot/mozilla/mail/components/compose/content/MsgComposeCommands.js,v  <--  MsgComposeCommands.js
new revision: 1.81; previous revision: 1.80
done
Checking in mail/components/preferences/compose.js;
/cvsroot/mozilla/mail/components/preferences/compose.js,v  <--  compose.js
new revision: 1.7; previous revision: 1.6
Attachment #201456 - Attachment description: Seperate out the ldap migrate function v2 → Seperate out the ldap migrate function v2 (checked in)
Status: NEW → ASSIGNED
Priority: -- → P3
Product: Core → MailNews Core
Depends on: 408613
Depends on: 451381
Duplicate of this bug: 125821
Due to the various bugs that have fixed our preferences, we no longer need nsILDAPPrefsService :-)

Time for it to go.
Attachment #337712 - Flags: superreview?(bienvenu)
Attachment #337712 - Flags: review?(neil)
Attachment #337712 - Flags: superreview?(bienvenu) → superreview+
Comment on attachment 337712 [details] [diff] [review]
[checked in] Remove the LDAP Prefs Service

yay!
Attachment #337712 - Flags: review?(neil) → review+
Comment on attachment 337712 [details] [diff] [review]
[checked in] Remove the LDAP Prefs Service

You may (or may not) want to remove a couple of other references I spotted:
mailnews/mailnews.pkg
mozilla/modules/plugin/os2wrapper/moz_IDs_Input.lst
Attachment #337712 - Flags: approval-seamonkey2.0a1+
Comment on attachment 337712 [details] [diff] [review]
[checked in] Remove the LDAP Prefs Service

Checked in changeset id: 308:0d42e942671e
Attachment #337712 - Attachment description: Remove the LDAP Prefs Service → [checked in] Remove the LDAP Prefs Service
This is now fixed (you can't do many more revisions on it once you've removed it ;-) ).
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.