Closed
Bug 633150
Opened 12 years ago
Closed 12 years ago
Change nsILDAPURL::setAttributes to take a string rather than an array
Categories
(MailNews Core :: LDAP Integration, defect)
MailNews Core
LDAP Integration
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 3.3a3
People
(Reporter: standard8, Assigned: standard8)
References
Details
Attachments
(1 file, 6 obsolete files)
38.10 KB,
patch
|
standard8
:
review+
Bienvenu
:
superreview+
|
Details | Diff | Splinter Review |
In bug 452232, I currently have one nsISupports parameter in which to pass the required attributes. Currently, we pass an array into nsILDAPURL::setAttributes. If we were to change this to a comma-separated string, it would make the whole process of getting attributes from/to places a lot easier. Although this means we will need a bit more processing to join and to separate out the attributes in various places, I think the easier to use interface makes it worthwhile.
Attachment #511346 -
Flags: superreview?(bienvenu)
Attachment #511346 -
Flags: review?(neil)
Assignee | ||
Comment 1•12 years ago
|
||
Minor change, fixes a couple of unused variables that I'd left hanging around.
Attachment #511346 -
Attachment is obsolete: true
Attachment #511356 -
Flags: superreview?(bienvenu)
Attachment #511356 -
Flags: review?(neil)
Attachment #511346 -
Flags: superreview?(bienvenu)
Attachment #511346 -
Flags: review?(neil)
Comment 2•12 years ago
|
||
Comment on attachment 511356 [details] [diff] [review] The fix v2 >@@ -100,7 +100,7 @@ nsLDAPURL::GetPathInternal(nsCString &aP > if (!mDN.IsEmpty()) > aPath.Append(mDN); > >- PRUint32 count = mAttributes.Count(); >+ PRUint32 count = mAttributes.Length(); > if (count) > { > aPath.Append('?'); >@@ -108,7 +108,7 @@ nsLDAPURL::GetPathInternal(nsCString &aP Not the ideal number of lines of context ;-) >+ rv = SetAttributeArray(const_cast<const char **>(desc->lud_attrs)); Since this is an internal method, you can drop the const. > NS_IMETHODIMP nsLDAPURL::GetAttributes(PRUint32 *aCount, char ***_retval) So, the getter and setter are now out of sync? Annoying. Unless there's a possiblity of changing to attribute ACString attributes; >+ cArray = static_cast<char **>(nsMemory::Alloc(length * sizeof(char *))); Nit: could switch to NS_Alloc >+ const char** attributes = aAttributes; Don't need to copy it, just use aAttributes. >+ if (!ParseString(aAttributes, ',', mAttributes)) >+ return NS_ERROR_FAILURE; Not NS_ERROR_OUT_OF_MEMORY? >+ getAllCardAttributes: function getAllCardAttributes() { >+ var attrs = ""; > for each (var attrArray in this.mPropertyMap) { >+ if (attrArray.length) { >+ if (attrs) >+ attrs = [attrs, attrArray].join(","); >+ else >+ attrs = attrArray.join(","); >+ } > } Not sure what this is trying to do. Probably easist would be to join the array once at the end, instead of trying to do it piecemeal.
Assignee | ||
Comment 3•12 years ago
|
||
(In reply to comment #2) > > NS_IMETHODIMP nsLDAPURL::GetAttributes(PRUint32 *aCount, char ***_retval) > So, the getter and setter are now out of sync? Annoying. Unless there's a > possiblity of changing to attribute ACString attributes; The c-sdk requires the array format, so we need to translate it somewhere. We could probably have a GetAttributesArray function, as well as a GetAttributes that isn't used.
Comment 4•12 years ago
|
||
(In reply to comment #3) > (In reply to comment #2) > > > NS_IMETHODIMP nsLDAPURL::GetAttributes(PRUint32 *aCount, char ***_retval) > > So, the getter and setter are now out of sync? Annoying. Unless there's a > > possibility of changing to attribute ACString attributes; > > The c-sdk requires the array format, so we need to translate it somewhere. We > could probably have a GetAttributesArray function, as well as a GetAttributes > that isn't used. I would be fine with this, particularly if the array had a null terminator, although there's also the possibility of getting nsLDAPOperation::SearchExt to create the array format. Oh, and maybe GetPathInternal could share code with GetAttributes? void nsLDAPURL::AppendAttributesTo(nsACString& _retval) { for (PRUint32 index = 0; index < mAttributes.Length(); index++) { if (index) _retval.Append(','); _retval.Append(mAttributes[index]); } } /* ... */ NS_IMETHODIMP nsLDAPURL::GetAttributes(nsACString& _retval) { _retval.Truncate(); AppendAttributesTo(_retval); return NS_OK; }
Comment 5•12 years ago
|
||
Comment on attachment 511356 [details] [diff] [review] The fix v2 clearing review request since I think an other patch is coming.
Attachment #511356 -
Flags: superreview?(bienvenu)
Attachment #511356 -
Flags: review?(neil)
Assignee | ||
Comment 6•12 years ago
|
||
Ok, this was a bit more wide-ranging than I would have liked, but I think this is probably much better now. I've changed nsLDAPURL to store the attributes as a comma separated string and not provide an array alternative. This had a few knock-on effects of changing storage formats elsewhere (or not actually storing at all where not necessary). This will change the LDAP sync query stuff to not return all elements when particular elements are not present in the card, but I think that is probably fine.
Attachment #511356 -
Attachment is obsolete: true
Attachment #512258 -
Flags: review?(neil)
Comment 7•12 years ago
|
||
Comment on attachment 512258 [details] [diff] [review] The fix v3 >+nsresult nsLDAPURL::SetAttributeArray(char** aAttributes) >+{ >+ mAttributes.Truncate(); >+ PRBool addedItem = PR_FALSE; >+ >+ while (aAttributes && *aAttributes) > { >+ if (addedItem) Nit: can use !mAttributes.IsEmpty() instead >+ mAttributes.Append(nsDependentCString(*aAttributes)); Nit: can probably Append(*aAttributes) directly >+ // Check to see if the attribute is already stored. If it is, then also >+ // check to see if it is the last attribute in the string, or if the next >+ // character is a comma, this means we won't match substrings. I think you also need to check whether the previous character is a comma. I couldn't find many callers of this method, so the simplest way of doing this is probably to prefix and suffix a comma to mAttributes and aAttribute and do the find on those. (I have a vague memory that we did or do do that somewhere else but I can't seem to find where.) [In theory you could keep the string internally with the commas prefixed and suffixed which would make removing an attribute cleaner since you no longer have any edge cases but since nobody actually removes an attribute it probably makes no sense to do that.] >+ PRInt32 pos = mAttributes.Find(nsCString(aAttribute), >+ CaseInsensitiveCompare); Nit: use PromiseFlatCString instead of nsCString >+ PRInt32 pos = mAttributes.Find(nsCString(aAttribute)); Was this supposed to be case insensitive?
Assignee | ||
Comment 8•12 years ago
|
||
Updated addressing Neil's comments and fixing the removeAttribute and providing a couple more tests for it.
Attachment #512258 -
Attachment is obsolete: true
Attachment #513084 -
Flags: review?(neil)
Attachment #512258 -
Flags: review?(neil)
Comment 9•12 years ago
|
||
Comment on attachment 513084 [details] [diff] [review] The fix v4 >+ if (pos >= 0 && mAttributes.Length() != pos + aAttribute.Length() && >+ mAttributes[pos + aAttribute.Length()] != ',') Oops, I hadn't noticed that this test is wrong. If your attributes are "CommonName,SecondaryEmail" then AddAttribute("SecondaryEmail") adds it again, while AddAttribute("Common") does nothing. Note that you need to fix the HasAttribute test before you copy it, see below, since the test is so broken that AddAttribute("mail") actually works! >+ else if (mAttributes[pos + aAttribute.Length()] == ',' && >+ (pos == 0 || mAttributes[pos - 1] == ',')) RemoveAttribute("Common"), RemoveAttribute("mail") all do nothing as expected. >+ *_retval = pos >= 0 && (mAttributes.Length() == pos + aAttribute.Length() || >+ mAttributes[pos + aAttribute.Length()] == ','); But this is missing the (pos == 0 || mAttributes[pos - 1] == ',') clause, which means that HasAttribute("Name"), HasAttribute("mail") return true. Apparently line 293 has trailing whitespace.
Assignee | ||
Comment 10•12 years ago
|
||
Ok, a few more tests, plus this time, I've changed the internal storage to have a comma on start and end. That saves the complex if statements.
Attachment #513084 -
Attachment is obsolete: true
Attachment #513430 -
Flags: review?(neil)
Attachment #513084 -
Flags: review?(neil)
Comment 11•12 years ago
|
||
Comment on attachment 513430 [details] [diff] [review] The fix v5 >+ NS_ASSERTION(mAttributes[0] == ',' && >+ mAttributes[mAttributes.Length() - 1] == ',', Nit: .First() and .Last() [this happens in a few places] >+ "ERROR: mAttributes does not being and end with a comma"); Typo: begin Nit: Probably don't need to include ERROR: given that the message already includes ###!!! ASSERTION in it. >+ while (aAttributes && *aAttributes) >+ { >+ mAttributes.Append(','); >+ mAttributes.Append(*aAttributes); >+ ++aAttributes; >+ } >+ >+ if (!mAttributes.IsEmpty()) >+ mAttributes.Append(','); This is correct, but it took me a few seconds to realise that! >+ mAttributes = ','; >+ mAttributes.Append(aAttribute); >+ mAttributes.Append(','); I like this way of wrapping aAttribute in commas, as opposed to using Insert. >+ PRInt32 pos = mAttributes.Find(PromiseFlatCString(findAttribute), Nit: findAttribute is now a flat string, so there's nothing to promise. >+ mAttributes.Append(Substring(findAttribute, 1, findAttribute.Length() - 1)); Nit: Substring(findAttribute, 1) defaults to the rest of the string. >+ if (pos == 0) >+ mAttributes = StringTail(mAttributes, >+ mAttributes.Length() - findAttribute.Length() + 1); Nit: This ends up doing the same as the Cut below. >+ else >+ mAttributes.Cut(pos, findAttribute.Length() - 1); I think RemoveAttribute needs to clear mAttributes when you remove the last one. Maybe if you write if (mAttributes.Equals(findAttribute, nsCaseInsensitiveCStringComparator())) mAttributes.Truncate(); >+ if (mAttributes.IsEmpty()) >+ return NS_OK; You forgot to write your outparam. (Or you could just skip this check.)
Assignee | ||
Comment 12•12 years ago
|
||
(In reply to comment #11) > Comment on attachment 513430 [details] [diff] [review] > The fix v5 > > >+ NS_ASSERTION(mAttributes[0] == ',' && > >+ mAttributes[mAttributes.Length() - 1] == ',', > Nit: .First() and .Last() [this happens in a few places] The external API version (nsStringAPI.h) doesn't have any sign of a Last() function (although it does have First!).
Assignee | ||
Comment 13•12 years ago
|
||
Updated for latest comments.
Attachment #513430 -
Attachment is obsolete: true
Attachment #513782 -
Flags: review?(neil)
Attachment #513430 -
Flags: review?(neil)
Comment 14•12 years ago
|
||
(In reply to comment #12) >(In reply to comment #11) >>(From update of attachment 513430 [details] [diff] [review]) >>>+ NS_ASSERTION(mAttributes[0] == ',' && >>>+ mAttributes[mAttributes.Length() - 1] == ',', >>Nit: .First() and .Last() [this happens in a few places] >The external API version (nsStringAPI.h) doesn't have any sign of a Last() >function (although it does have First!). Ah yes, we added Last() to nsMsgUtils.h but I guess we can't use that here.
Comment 15•12 years ago
|
||
Comment on attachment 513782 [details] [diff] [review] The fix v6 >+ >+ NS_FREE_XPCOM_ALLOCATED_POINTER_ARRAY(valueCount, vals); ... >+ >+ // If mAttributes isn't empty, cut of the internally stored commas at start >+ // and end, and append to the path. Nit: whitespace on the blank lines As I typed the above nit, I just noticed that the comment I quoted should say "cut off" instead of "cut of".
Attachment #513782 -
Flags: review?(neil) → review+
Assignee | ||
Comment 16•12 years ago
|
||
Updated for Neil's nits.
Attachment #513782 -
Attachment is obsolete: true
Attachment #513879 -
Flags: superreview?(bienvenu)
Attachment #513879 -
Flags: review+
Comment 17•12 years ago
|
||
Comment on attachment 513879 [details] [diff] [review] The fix v7 sr=me, with some nits: tab? - attrs[aAttrCount] = 0; - } + PromiseFlatCString(aAttributes).get(), aSizeLimit)); don't need space between ** and attrs: + char ** attrs = nsnull; don't need ns_error when out of memory: + NS_ERROR("nsLDAPOperation::SearchExt: out of memory"); wow, this file is all over the place with its spaces :-) + char** attributes; + nsresult rv = aMessage->GetAttributes(&attrCount, &attributes); rv is an nsresult here, I think: nsLDAPURL::SetPathInternal(const nsCString &aPath) { - PRUint32 rv, count; + PRUint32 rv; this comment needs to move down a bit to be closer to the code it refers to: + // We store the string internally with comma before and after, so strip + // them off here. + if (mAttributes.IsEmpty()) I think the same comment about starting the string off with a ',' would be appropriate here too, since otherwise the code just looks wrong:-) : + if (aAttributes.IsEmpty()) + mAttributes.Truncate(); + else + { + if (aAttributes[0] != ',') + mAttributes = ','; lose the commented out line: - nsCStringArray mAttributes; // List of attributes + // nsTArray<nsCString> mAttributes; // List of attributes + nsCString mAttributes; a comma-separated...: + * @return an comma-separated list of attribute names
Attachment #513879 -
Flags: superreview?(bienvenu) → superreview+
Assignee | ||
Comment 18•12 years ago
|
||
(In reply to comment #17) > Comment on attachment 513879 [details] [diff] [review] > The fix v7 > > sr=me, with some nits: > > tab? > > - attrs[aAttrCount] = 0; > - } > + PromiseFlatCString(aAttributes).get(), aSizeLimit)); Nope, just a complicated diff. > wow, this file is all over the place with its spaces :-) Well, the function will be consistent with itself. > + char** attributes; > + nsresult rv = aMessage->GetAttributes(&attrCount, &attributes); > > rv is an nsresult here, I think: > > nsLDAPURL::SetPathInternal(const nsCString &aPath) > { > - PRUint32 rv, count; > + PRUint32 rv; ldap_url_parse returns an int, and we translate the error code further down in the function to a proper nsresult.
Assignee | ||
Comment 19•12 years ago
|
||
Checked in: http://hg.mozilla.org/comm-central/rev/e9817d0c0d02
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.3a3
Comment 20•12 years ago
|
||
(In reply to comment #18) > > rv is an nsresult here, I think: > > > > nsLDAPURL::SetPathInternal(const nsCString &aPath) > > { > > - PRUint32 rv, count; > > + PRUint32 rv; > > ldap_url_parse returns an int, and we translate the error code further down in > the function to a proper nsresult. Ah, I see, sorry - I got confused because we assign the nsresult return of nsLDAPURL::SetAttributeArray to rv, and rv is an unsigned int...
You need to log in
before you can comment on or make changes to this bug.
Description
•