323181 - nsIAbLDAPReplicationQuery.idl should use nsIAb*Directory instead of DIR_Server as a member.

Status: RESOLVED FIXED

(MailNews Core :: LDAP Integration, defect)

Description

[Mark Banner (:standard8)]

nsIAbLDAPReplicationQuery.idl currently has as a member:

[noscript] readonly attribute DIR_Server replicationServerInfo;

This should become something more based on nsIAbDirectory so that we don't have to export nsDirPrefs.h, and because we are moving away from nsDirPrefs. If we use nsIAbDirectory, then this should help with future work in the nsDirPrefs area.

I've started putting some ideas together, but it needs a bit of work.

Comment 1

[Mark Banner (:standard8)]

Attachment: First Draft

This is the work so far - the normal replication is now working without using DIR_Server, though the change log replication (which we're currently not using) is not fully implemented. There is also some of a patch from bug 287832 (part 9) in this.

I'd have liked to have used nsACString for the new functions in the idls, but as nsIPrefBranch is based on strings, it seems to make more sense to leave the new interface functions as strings to cut down on conversions.

Comment 2

[Mark Banner (:standard8)]

Attachment: Second Draft

Changes in this version:

- Change Log entries fully implemented
- Shortened documentation in nsIAbDirectory.idl by using doxygen grouping.
- Lots more nsDirPrefs removals, these may or may not make the final patch (but will most likely be put in a follow-up patch on another bug).

Still to sort out:

- What happens to the pref branch if dirPrefId changes in nsIAbDirectory? Or even, should it be allowed to change once it has been initialised?
- if mInitialized is true (in some of the ldap change log/process replication files), does this also ensure that mDirectory is initialised (and valid?)? if so, can I just check for mInitialized and not mDirectory as well?
- Final cut of nsDirPrefs removals.

Comment 3

[Mark Banner (:standard8)]

Attachment: Third Draft

Almost there...

Changes in this version:
- Redundant prefs removed from nsDirPrefs
- we don't need to check for mDirectory as well as mInitialized - just mInitialized is ok.
- Prefs branch will get updated if dirPrefId changes.

To Do:

Tidy up & Full test on both SeaMonkey & Thunderbird for possible regressions that I haven't noticed yet ;-)

Comment 4

[Mark Banner (:standard8)]

Attachment: Patch v1

Ok, this is the full first version of the patch (tidied & tested in SeaMonkey). I've got Thunderbird to test still, but if that goes well I'll be requesting reviews.

Comment 5

[Mark Banner (:standard8)]

Comment on attachment 212525 [details] [diff] [review]
Patch v1

I've checked this works ok in Thunderbird and can't see any problems. I still need to fix bug 328358 before this can go in, but I'm going to ask for at least one review ahead of that as it won't change this patch (except maybe a small amount of bitrot) and I'd like to get some general feedback on the ideas I'm introducing in this one.

Comment 6

[Mark Banner (:standard8)]

Comment on attachment 212525 [details] [diff] [review]
Patch v1

Ok, I've decided not to rewrite the migration code before this goes in, in fact I think it may be easier the other way round. Also I think this patch was missing a function addition - opps, don't know how that got missed.

Comment 7

[Mark Banner (:standard8)]

Attachment: Patch v2

Updated patch to leave ldap ab migration intact for now.

Comment 8

[David :Bienvenu]

Comment on attachment 215303 [details] [diff] [review]
Patch v2

r=bienvenu with these nits:

+  long GetIntPref(in string aName, in long aDefaultValue);
+  boolean GetBoolPref(in string aName, in boolean aDefaultValue);
+  string GetStringPref(in string aName, in string aDefaultValue);


these should be getIntPref, getBoolPref, etc.

+NS_IMETHODIMP nsAbDirProperty::GetDirectoryPrefs(nsIPrefBranch * *aDirectoryPrefs)
+{

should be NS_ENSURE_ARG_POINTER(aDirectoryPrefs);

then you don't need
+  if (aDirectoryPrefs)

-
+  nsCOMPtr<nsIPrefBranch> mDirectoryPrefs;
+  nsCOMPtr<nsISupportsArray> m_AddressList;

might as well be consistent here - m_DirectoryPrefs, or convert the other var names.

Comment 9

[Mark Banner (:standard8)]

Attachment: Patch v3

Addressed David's comments, carrying forward r+.

Comment 10

[[e-mail]]

Comment on attachment 215668 [details] [diff] [review]
Patch v3

>+interface nsIPrefBranch;
Superfluous?

>+  /**
>+   * The id of the directory used in prefs e.g. "ldap_2.servers.pab"
>+   * Setting this will cause directoryPrefs to be updated.
>+   */
>+  attribute ACString dirPrefId;
[not required for sr] Typically for other mail prefs this is just the leaf name of the pref but I guess that's not so easy to arrange here. Even requiring the name to end in a . would be neater, as then you could rely on the branch to save the name for you i.e. SetDirPrefId would create the m_DirectoryPrefs branch and GetDirPrefId would call m_DirectoryPrefs->GetRoot(aDirPrefId). I suppose you could of course be really sneaky and null out the trailing . ;-)

>+   * Helper functions to set integer pref values
Nit: incorrect comment.

>+  //@{
>+  long getIntPref(in string aName, in long aDefaultValue);
>+  boolean getBoolPref(in string aName, in boolean aDefaultValue);
>+  string getStringPref(in string aName, in string aDefaultValue);
>+  //@}
Note that by comparison nsIMsgIncomingServer.idl uses getXXXValue. It would be nice if you could make palm sync call the new GetBoolPref, I mean GetBoolValue ;-) method too.

>+  //@{
>+  void setIntPref(in string aName, in long aValue);
>+  void setBoolPref(in string aName, in boolean aValue);
>+  void setStringPref(in string aName, in string aValue);
>+  //@}
I'm told this needs to go with @ingroup or @addtogroup

>+NS_IMETHODIMP nsAbDirProperty::GetStringPref(const char *aName,
>+                                             const char *aDefaultValue, 
>+                                             char * *aResult)
>+{
>+  NS_ENSURE_ARG_POINTER(aResult);
>+
>+  if (!m_DirectoryPrefs && NS_FAILED(InitDirectoryPrefs()))
>+    return NS_ERROR_NOT_INITIALIZED;
>+
>+  nsXPIDLCString value;
>+
>+  if (NS_SUCCEEDED(m_DirectoryPrefs->GetCharPref(aName, getter_Copies(value))))
>+  {
>+    /* unfortunately, there may be some prefs out there which look like this */
>+    if (value.EqualsLiteral("(null)")) 
>+      value = aDefaultValue;
>+  }
>+  else
>+    value = aDefaultValue;
Unfortunately this does extra copies. Would you mind switching these to *aResult = ToNewCString(aDefaultValue); and move the *aResult = ToNewCString(value); to an appropriate else clause?

>+  if(aChangeLogDN.IsEmpty()) 
And you were so good at fixing the spacing earlier :-)

>+  *aProtocolVersion = versionString.Equals("3") ?
EqualsLiteral

Comment 11

[Mark Banner (:standard8)]

Attachment: Patch v4 (checked in)

Patch for checkin addressing Neil's comments, except for the Palm Sync one, I think that's best done in a separate patch especially as I can't compile on windows.

Comment 12

[Mark Banner (:standard8)]

Attachment: Follow-up patch

Follow-up patch to address Neil's comments about PalmSync. David can you try/compile this please? You'll need Patch v4 applied unless I've checked it in by the time you review this.

Comment 13

[Mark Banner (:standard8)]

Comment on attachment 215925 [details] [diff] [review]
Follow-up patch

Cancelling review, this patch is going to be incorporated onto the one I'm very soon going to put onto bug 331557 and it'll save review work.

Comment 14

[Mark Banner (:standard8)]

The requested follow-up patch is now included in the "Remove c++ uses of GetDirectoryProperties" patch on bug 331557. I don't see any point in leaving this one open -> fixed.