Closed Bug 70611 Opened 24 years ago Closed 23 years ago

Fix attribute Get/Set etc. methods in nsLDAPURL.cpp

Categories

(Directory :: LDAP XPCOM SDK, defect, P2)

x86
Linux
defect

Tracking

(Not tracked)

VERIFIED FIXED
mozilla0.9.2

People

(Reporter: leif, Assigned: leif)

References

(Depends on 1 open bug)

Details

(Keywords: perf, Whiteboard: [PDT+] SR requested from mscott on 6/12)

Attachments

(6 files)

In order to meet crazy deadlines, we decided to punt on handling attribute lists properly in nsLDAPURL. We don't need this functionality for 6.5, but this must be fixed for 7.0 naturally. In addition to Get/Set() methods, I suggest we all add Add/Remove() methods as well, making it easy to add and remove single elements to the list of attributes.
Status: NEW → ASSIGNED
Priority: -- → P3
Target Milestone: --- → mozilla1.0
Hmmm, why didn't it assign this to me...
Assignee: chuang → leif
Status: ASSIGNED → NEW
Status: NEW → ASSIGNED
QA Contact: esther → yulian
Component: Address Book → LDAP XPCOM SDK
Product: MailNews → Directory
Target Milestone: mozilla1.0 → ---
Blocks: 75955
Blocks: 74098
Blocks: 17880
This is implemented, posting a patch here as soon as I've tested it.
Severity: minor → normal
Keywords: nsbeta1
Priority: P3 → P2
Target Milestone: --- → mozilla0.9.1
reassign to Olga as QA contact
QA Contact: yulian → olgac
Target Milestone: mozilla0.9.1 → mozilla0.9.2
This will make the user-experience worse for dial-in users hitting ldap servers which have large entries (ie certificates or jpeg attrs).
Keywords: nsbeta1perf
Target Milestone: mozilla0.9.2 → mozilla0.9.3
Target Milestone: mozilla0.9.3 → mozilla0.9.2
Dmose, can you gimme a review on this patch please?
I fixed one bug (was finally able to debug this beast, after nuking my entire build tree), and got a workaround for the casting issue (see bug 84551). Dmose, review please. -- Leif
Depends on: 84551
In general, it looks good (ie most of the commentary is just nitpicking, and should be very easy to fix). nsILDAPURL.idL -------------- + * Return all LDAP attributes currently set. If we return an empty + * array, which is the default, you should request all attributes. * Use of "we" and "you" is very confusing. Can you rephrase just referring to the URL that the resource describes without any specific personal point of view? + * Add one attribute to the array of attributes to request. If the + * attribute is only in our array, this becomes a noop s/only/already/ + /** + * Test if an attribute is in our list of attributes already, + * returning a boolean. + * + * @param aAttribute An LDAP attribute (e.g. "cn") + * @exception NS_ERROR_NULL_POINTER NULL pointer to GET method How about, instead: /** * Test if an attribute is in our list of attributes already. * * @param aAttribute An LDAP attribute (e.g. "cn") * * @return boolean truth value * * @exception NS_ERROR_NULL_POINTER NULL pointer passed to getter nsLDAPURL.cpp ------------- * If you only have to append a single character, it'll probably be faster to use .Append('a') than .Append("a") +nsresult nsLDAPURL::Init() Can you change this to be formatted like this: +nsresult +nsLDAPURL::Init() This makes it really easy to use grep to find function definitions by anchoring a search at the beginning of a line. + char *buf; + + spec.Append("?"); + while (index < count) { + buf = (mAttributes->CStringAt(index++))->ToNewCString(); + if (!buf) { + NS_ERROR("nsLDAPURL::GetSpec: out of memory "); + return NS_ERROR_OUT_OF_MEMORY; + } + spec.Append(buf); Can you avoid doing one allocation and one copy per attr by not using buf? i.e. spec.Append("?"); while (index < count) { spec.Append(mAttributes->CStringAt(index++)) nsLDAPURL::SetSpec() -------------------- + // Set the attributes array, need to count it first. + // + count = 0; Why not just declare "count" in this same statement so that gets intialized at the same time as declared, perhaps saving a cycle or two. nsLDAPURL::GetAttributes() -------------------------- As far as I can tell, you're constructing an array with a trailing NULL entry, like the LDAP C SDK uses. But this is an XPCOM-style array, which doesn't use the trailing NULL convention. Bug? + char **arr; Can you use a more meaningful variable name here? + arr = (char **)nsMemory::Alloc((*aCount + 1) * sizeof(char *)); Use a mozilla macro for a C++-style cast (NS_*_CAST), please. + // Loop through the string array, and build up the C-array. + // + while (index < *aCount) { Because the compiler can't know that you're the only accessor to the object of this pointer, it has to re-load *aCount into some register each iteration through the loop. You'll win a bit by using a local variable for aCount and then storing it once into *aCount at the end of the function. nsLDAPURL::SetAttributes(), RemoveAttribute(), HasAttribute() ------------------------------------------------------------- Why use the str temporary?
Attached patch Fixes as per reviewers comment (deleted) — Splinter Review
mscott, can you SR= this please? Thanks, -- Leif
Whiteboard: SR requested from mscott on 6/12
PDT+ per 6/12 mtg.
Whiteboard: SR requested from mscott on 6/12 → [PDT+] SR requested from mscott on 6/12
Looks good to me. Although I don't know why the summary says review requested from me yesterday when I was just emailed about it this morning! cc'ing me on the bug is not a valid way to request a review. Per the super reviewer's guidelines, you ALWAYS need to send me email directly. Thanks. sr=mscott
Thanks Scott. And yes, that was my fault, I forgot to send the two messages, dunno what I was thinking (I've been on honeymoon, my brain is fried). :( -- Leif
Dan/Scott: While testing this some more, I found a case where the previous patch did the wrong thing. If I had called url.spec = 'ldap://foo/dc=com?cn,mail'; and then call it again like url.spec = 'ldap://bar/dc=com'; then the old list of attributes is not cleared. This is a one line fix, I hope R= and SR= is still there? Thanks, -- leif The code affected was: + if (count) { + rv = SetAttributes(count, + NS_CONST_CAST(const char **, desc->lud_attrs)); + // This error could only be out-of-memory, so pass it up + // + if (NS_FAILED(rv)) { + return rv; + } + } else { + mAttributes->Clear(); + } Where I added the last else branch to clear the attributes list if none are specified in the URL.
r=dmose still applies.
a=blizzard on behalf of drivers for the trunk
my sr still applies.
Patch checked in.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
code is in, marking as verified
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: