83360 - improve LDAP datasource usefulness with XUL templates

Status: RESOLVED INCOMPLETE

(Directory Graveyard :: LDAP XPCOM SDK, defect, P3)

Description

[Dan Mosedale (:dmosedale, :dmose)]

Last week, peterv and I got to talking about how we could make the LDAP
datasource more useful with XUL templates.  After a bunch of discussion, we came
up with a theory:

First, add a "DN" arc, so it's easy to tell what an entry's distinguished name is.

Second, add a "recursiveChild" arc, which is intended to be used as a
containment arc when you want to descend the entire trees below any entries that
are returned by your search (the default "child" arc, as implemented, does not
recursively descend retrieved entries, because this seems counter to the
intended semantics of ldap: search URIs).  

Peter hacked like a madman and implemented this stuff; I'm attaching his patch
here.  I'm on the hook to review the patch, but I probably won't get to that for
a few days, as I need to work my way through reviewing various addressbook/LDAP
integration changes first.

Comment 1

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: LDAP datasource patch, v1

Comment 2

[Peter Van der Beken [:peterv]]

Let's get this out of my Netscape bug list.

Comment 3

[Rostislav Matl]

Peter, thanks for your work, it works nicely.
Only one thing seems doesn't work correctly 
- the folder icon is not displayed after loading
all data, the icon appears after first "refresh"
(after resizinf window, refocusing it,...).

Comment 4

[Dan Mosedale (:dmosedale, :dmose)]

peter, if you can get me a new patch that works with the CVS head (probably just
needs tweaks to SearchExt calls), I'll review it tout suite.

Comment 5

[Peter Van der Beken [:peterv]]

Attachment: LDAP datasource patch, v2

Comment 6

[Asa Dotzler [:asa]]

pushing out. 0.9.2 is done. 

Comment 7

[Jeff Hooker]

per eclient triage removing nsenterprise keyword

Comment 8

[Peter Van der Beken]

Giving dmose more time.

Comment 9

[Peter Van der Beken]

Attachment: LDAP datasource patch, v3

Chèvre!

Comment 10

[Dan Mosedale (:dmosedale, :dmose)]

I suck for not reviewing this much much sooner.

Comments:

a) One line of the patch sez:

+ XXX Could we just return true here

returning true would probably be ok, since every entry had better have
a dn.  But the way you've written it is probably even better, since it
allows for extra error checking if the caller so desires it (most
datasource users will never even call this routine with the kNC_DN arc anyway).
OTOH, if (say) the XUL template builder calls this routine frequently,
the current impl might slow down things slightly.  It's up to you.
:-)

b) towards the end of GetTargets:

+            // Error or the attribute was not there.
+            // XXX Do we need to do something about the real errors?
+            // XXX Just returning empty array for now.

It seems to me that if the attribute is not there, returning an empty
array is appropriate, but if there's a real error, an exception should
be thrown (and allowed to propagate to the caller of GetTargets).

Both of these are just my thoughts on this; you've got r=dmose: feel free to
check this patch in as-is, or after adjusting the patch as per these comments. 
Your choice.

Anyway, all the comments that you added to the code were great; they made
reviewing this patch much easier than it otherwise would have been. 

Comment 11

[Peter Van der Beken]

Thank you Dan! I addressed your first comment in the patch that i checked in
(just check for delegate == null). I didn't do the second yet, though I plan to,
it's a border condition that no one should be hitting anyway.

Comment 12

[Dan Mosedale (:dmosedale, :dmose)]

This fix has been checked in, right?

Comment 13

[Peter Van der Beken]

I'm working on a new version of the datasource, but it won't be ready by 1.0.
Moving this out (also because NS managment might freak out seeing this bug on my
list *sigh*).