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)
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)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•24 years ago
|
Status: NEW → ASSIGNED
Priority: -- → P3
Target Milestone: --- → mozilla1.0
Assignee | ||
Comment 1•24 years ago
|
||
Hmmm, why didn't it assign this to me...
Assignee: chuang → leif
Status: ASSIGNED → NEW
Assignee | ||
Updated•24 years ago
|
Status: NEW → ASSIGNED
Updated•24 years ago
|
Component: Address Book → LDAP XPCOM SDK
Product: MailNews → Directory
Target Milestone: mozilla1.0 → ---
Assignee | ||
Comment 2•24 years ago
|
||
This is implemented, posting a patch here as soon as I've tested it.
Updated•24 years ago
|
Target Milestone: --- → mozilla0.9.1
Updated•23 years ago
|
Target Milestone: mozilla0.9.1 → mozilla0.9.2
Comment 4•23 years ago
|
||
This will make the user-experience worse for dial-in users hitting ldap servers
which have large entries (ie certificates or jpeg attrs).
Updated•23 years ago
|
Target Milestone: mozilla0.9.3 → mozilla0.9.2
Assignee | ||
Comment 5•23 years ago
|
||
Assignee | ||
Comment 6•23 years ago
|
||
Dmose, can you gimme a review on this patch please?
Assignee | ||
Comment 7•23 years ago
|
||
Assignee | ||
Comment 8•23 years ago
|
||
Assignee | ||
Comment 9•23 years ago
|
||
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
Assignee | ||
Comment 10•23 years ago
|
||
Comment 11•23 years ago
|
||
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?
Assignee | ||
Comment 12•23 years ago
|
||
Comment 13•23 years ago
|
||
Assignee | ||
Comment 14•23 years ago
|
||
mscott, can you SR= this please?
Thanks,
-- Leif
Whiteboard: SR requested from mscott on 6/12
Comment 15•23 years ago
|
||
PDT+ per 6/12 mtg.
Whiteboard: SR requested from mscott on 6/12 → [PDT+] SR requested from mscott on 6/12
Comment 16•23 years ago
|
||
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
Assignee | ||
Comment 17•23 years ago
|
||
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
Assignee | ||
Comment 18•23 years ago
|
||
Assignee | ||
Comment 19•23 years ago
|
||
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.
Comment 20•23 years ago
|
||
r=dmose still applies.
Comment 21•23 years ago
|
||
a=blizzard on behalf of drivers for the trunk
Comment 22•23 years ago
|
||
my sr still applies.
Assignee | ||
Comment 23•23 years ago
|
||
Patch checked in.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•