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

RESOLVED FIXED

Status

MailNews Core
LDAP Integration
RESOLVED FIXED
13 years ago
10 years ago

People

(Reporter: standard8, Assigned: standard8)

Tracking

(Blocks: 1 bug)

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 6 obsolete attachments)

(Assignee)

Description

13 years ago
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.
(Assignee)

Comment 1

13 years ago
Created attachment 211584 [details] [diff] [review]
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.
(Assignee)

Comment 2

13 years ago
Created attachment 211762 [details] [diff] [review]
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.
Attachment #211584 - Attachment is obsolete: true
(Assignee)

Comment 3

13 years ago
Created attachment 212239 [details] [diff] [review]
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 ;-)
Attachment #211762 - Attachment is obsolete: true
(Assignee)

Comment 4

13 years ago
Created attachment 212525 [details] [diff] [review]
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.
(Assignee)

Updated

13 years ago
Summary: nsIAbLDAPReplicationQuery.idl should us nsIAb*Directory instead of DIR_Server as a member. → nsIAbLDAPReplicationQuery.idl should use nsIAb*Directory instead of DIR_Server as a member.
(Assignee)

Updated

13 years ago
Attachment #212239 - Attachment is obsolete: true
(Assignee)

Updated

13 years ago
Depends on: 328358
(Assignee)

Comment 5

13 years ago
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.
Attachment #212525 - Flags: review?(bienvenu)
(Assignee)

Comment 6

12 years ago
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.
Attachment #212525 - Attachment is obsolete: true
Attachment #212525 - Flags: review?(bienvenu)
(Assignee)

Comment 7

12 years ago
Created attachment 215303 [details] [diff] [review]
Patch v2

Updated patch to leave ldap ab migration intact for now.
Attachment #215303 - Flags: review?(bienvenu)

Comment 8

12 years ago
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.
Attachment #215303 - Flags: review?(bienvenu) → review+
(Assignee)

Comment 9

12 years ago
Created attachment 215668 [details] [diff] [review]
Patch v3

Addressed David's comments, carrying forward r+.
Attachment #215303 - Attachment is obsolete: true
Attachment #215668 - Flags: superreview?(neil)
Attachment #215668 - Flags: review+

Comment 10

12 years ago
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
Attachment #215668 - Flags: superreview?(neil) → superreview+
(Assignee)

Comment 11

12 years ago
Created attachment 215923 [details] [diff] [review]
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.
Attachment #215668 - Attachment is obsolete: true
Attachment #215923 - Flags: superreview+
Attachment #215923 - Flags: review+
(Assignee)

Comment 12

12 years ago
Created attachment 215925 [details] [diff] [review]
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.
Attachment #215925 - Flags: review?(bienvenu)
(Assignee)

Updated

12 years ago
Attachment #215923 - Attachment description: Patch v4 → Patch v4 (checked in)
(Assignee)

Updated

12 years ago
Status: NEW → ASSIGNED
(Assignee)

Updated

12 years ago
Blocks: 331557
(Assignee)

Comment 13

12 years ago
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.
Attachment #215925 - Flags: review?(bienvenu)
(Assignee)

Comment 14

12 years ago
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
(Assignee)

Updated

12 years ago
No longer depends on: 328358
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.